feat(slides): add enhanced translation with source language selection and translate-all functionality #603
Conversation
Summary of ChangesHello @happygoluckycodeeditor, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly upgrades the Google Slides translation add-on by introducing an "enhanced" version. The primary goal is to facilitate quick and comprehensive translation of entire presentations, particularly useful in professional settings. It achieves this by offering a new sidebar interface that allows users to select both source and target languages (with an auto-detect option for source) and provides the choice to translate either selected elements or all text content across all slides, supporting a much broader range of languages. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an enhanced translation feature for Google Slides, including the ability to translate all slides at once and select a source language. The implementation is a good step forward, but there are a few important issues to address. A critical security vulnerability exists due to an outdated version of jQuery. Additionally, the 'Translate Selected' feature has a functional bug where it doesn't use the source language selector, and a critical bug that can cause the script to crash when translating text selected inside a table. My review includes suggestions to fix these issues.
| <span class="gray branding-text">Translate sample by Google</span> | ||
| </div> | ||
|
|
||
| <script src="//ajax.googleapis.com/ajax/libs/jquery/1.9.1/jquery.min.js"></script> |
There was a problem hiding this comment.
The version of jQuery being used (1.9.1) is outdated and has known security vulnerabilities, such as Cross-Site Scripting (XSS) (e.g., CVE-2020-11022, CVE-2015-9251). It is highly recommended to upgrade to a recent version of jQuery (e.g., 3.7.1) to mitigate these security risks.
| <script src="//ajax.googleapis.com/ajax/libs/jquery/1.9.1/jquery.min.js"></script> | |
| <script src="https://ajax.googleapis.com/ajax/libs/jquery/3.7.1/jquery.min.js"></script> |
| function translateSelectedElements(targetLanguage) { | ||
| // Get selected elements. | ||
| const selection = SlidesApp.getActivePresentation().getSelection(); | ||
| const selectionType = selection.getSelectionType(); | ||
| /** @type {GoogleAppsScript.Slides.TextRange[]} */ | ||
| let texts = []; | ||
| switch (selectionType) { | ||
| case SlidesApp.SelectionType.PAGE: | ||
| for (const page of selection.getPageRange().getPages()) { | ||
| texts = texts.concat(getElementTexts(page.getPageElements())); | ||
| } | ||
| break; | ||
| case SlidesApp.SelectionType.PAGE_ELEMENT: { | ||
| const pageElements = selection.getPageElementRange().getPageElements(); | ||
| texts = texts.concat(getElementTexts(pageElements)); | ||
| break; | ||
| } | ||
| case SlidesApp.SelectionType.TABLE_CELL: | ||
| for (const cell of selection.getTableCellRange().getTableCells()) { | ||
| texts.push(cell.getText()); | ||
| } | ||
| break; | ||
| case SlidesApp.SelectionType.TEXT: | ||
| for (const element of selection.getPageElementRange().getPageElements()) { | ||
| texts.push(element.asShape().getText()); | ||
| } | ||
| break; | ||
| } | ||
|
|
||
| // Translate all elements in-place. | ||
| for (const text of texts) { | ||
| text.setText( | ||
| LanguageApp.translate(text.asRenderedString(), "", targetLanguage), | ||
| ); | ||
| } | ||
|
|
||
| return texts.length; | ||
| } |
There was a problem hiding this comment.
The translateSelectedElements function has two issues:
- High: It does not accept a source language parameter, forcing it to use auto-detection. This is inconsistent with the new UI which allows source language selection.
- Critical: The code handling
SelectionType.TEXTassumes the selected text is always within aShape. If a user selects text within a table cell,element.asShape()will throw an error, crashing the script.
The suggested code below fixes both issues.
function translateSelectedElements(sourceLanguage, targetLanguage) {
// Get selected elements.
const selection = SlidesApp.getActivePresentation().getSelection();
const selectionType = selection.getSelectionType();
/** @type {GoogleAppsScript.Slides.TextRange[]} */
let texts = [];
switch (selectionType) {
case SlidesApp.SelectionType.PAGE:
for (const page of selection.getPageRange().getPages()) {
texts = texts.concat(getElementTexts(page.getPageElements()));
}
break;
case SlidesApp.SelectionType.PAGE_ELEMENT: {
const pageElements = selection.getPageElementRange().getPageElements();
texts = texts.concat(getElementTexts(pageElements));
break;
}
case SlidesApp.SelectionType.TABLE_CELL:
for (const cell of selection.getTableCellRange().getTableCells()) {
texts.push(cell.getText());
}
break;
case SlidesApp.SelectionType.TEXT:
for (const element of selection.getPageElementRange().getPageElements()) {
if (element.getPageElementType() == SlidesApp.PageElementType.SHAPE) {
texts.push(element.asShape().getText());
}
}
break;
}
// Translate all elements in-place.
for (const text of texts) {
text.setText(
LanguageApp.translate(text.asRenderedString(), sourceLanguage, targetLanguage),
);
}
return texts.length;
}| function runTranslation() { | ||
| this.disabled = true; | ||
| $('#error').text(''); | ||
| $('#success').text(''); | ||
| google.script.run | ||
| .withSuccessHandler((numTranslatedElements, element) =>{ | ||
| element.disabled = false; | ||
| if (numTranslatedElements === 0) { | ||
| $('#error').empty() | ||
| .append('Did you select elements to translate?') | ||
| .append('<br/>') | ||
| .append('Please select slides or individual elements.'); | ||
| } else { | ||
| $('#success').text('Successfully translated ' + numTranslatedElements + ' element(s)!'); | ||
| } | ||
| return false; | ||
| }) | ||
| .withFailureHandler((msg, element)=> { | ||
| element.disabled = false; | ||
| $('#error').text('Something went wrong. Please check the add-on logs.'); | ||
| return false; | ||
| }) | ||
| .withUserObject(this) | ||
| .translateSelectedElements($('input[name=dest]:checked').val()); | ||
| } |
There was a problem hiding this comment.
The runTranslation function for 'Translate Selected' does not use the selected source language from the UI. It calls translateSelectedElements with only the target language, which causes the backend to always use auto-detect for the source language. This is inconsistent with the 'Translate All Slides' functionality and doesn't use the full capability of the new UI. The function should be updated to retrieve and pass the selected source language.
function runTranslation() {
this.disabled = true;
$('#error').text('');
$('#success').text('');
const sourceLanguage = $('input[name=source]:checked').val();
const targetLanguage = $('input[name=dest]:checked').val();
google.script.run
.withSuccessHandler((numTranslatedElements, element) =>{
element.disabled = false;
if (numTranslatedElements === 0) {
$('#error').empty()
.append('Did you select elements to translate?')
.append('<br/>')
.append('Please select slides or individual elements.');
} else {
$('#success').text('Successfully translated ' + numTranslatedElements + ' element(s)!');
}
return false;
})
.withFailureHandler((msg, element)=> {
element.disabled = false;
$('#error').text('Something went wrong. Please check the add-on logs.');
return false;
})
.withUserObject(this)
.translateSelectedElements(sourceLanguage, targetLanguage);
}
|
Working on the feedback! Thank you very much! Be right back! :D |
|
@gemini-code-assist I have added/made the necessary changes. Can you have a look? Also removed the Auto-detect thing because it does not serve much purpose when the person doing the translate already knows the content of the slide and is just trying to change it to another language. |
There was a problem hiding this comment.
Code Review
This pull request introduces an excellent new feature for translating entire presentations and selecting source languages. The implementation is solid, but I've identified a few high-priority issues that should be addressed. These include improving user feedback on errors, ensuring text selection translation behaves as expected, and optimizing the translation process by batching API calls to enhance performance and avoid hitting service quotas. The suggested changes will make the feature more robust and user-friendly.
| .withFailureHandler((msg, element)=> { | ||
| element.disabled = false; | ||
| $('#error').text('Something went wrong. Please check the add-on logs.'); | ||
| return false; | ||
| }) |
There was a problem hiding this comment.
The failure handler for runTranslation currently shows a generic error message and ignores the specific error details (msg) returned from the server. This makes it difficult for users to understand what went wrong. To improve user experience, you should display the msg content in the error message, similar to how it's done in runAllTranslation's failure handler.
| .withFailureHandler((msg, element)=> { | |
| element.disabled = false; | |
| $('#error').text('Something went wrong. Please check the add-on logs.'); | |
| return false; | |
| }) | |
| .withFailureHandler((msg, element)=> { | |
| element.disabled = false; | |
| $('#error').text('Something went wrong: ' + msg); | |
| return false; | |
| }) |
| for (const text of texts) { | ||
| text.setText( | ||
| LanguageApp.translate( | ||
| text.asRenderedString(), | ||
| sourceLanguage, | ||
| targetLanguage, | ||
| ), | ||
| ); | ||
| } | ||
|
|
||
| return texts.length; |
There was a problem hiding this comment.
This function makes individual calls to LanguageApp.translate in a loop. This is inefficient and can lead to quota issues, especially when many elements are selected. It's much better to batch these calls. You can do this by collecting all text strings into an array, translating them with a single API call, and then updating the elements. This also allows you to return a more accurate count of translated elements.
// Translate all elements in-place.
const textsToTranslate = [];
const originalStrings = [];
for (const text of texts) {
const originalText = text.asRenderedString();
if (originalText.trim().length > 0) {
textsToTranslate.push(text);
originalStrings.push(originalText);
}
}
if (originalStrings.length > 0) {
const translatedStrings = LanguageApp.translate(
originalStrings,
sourceLanguage,
targetLanguage,
);
for (let i = 0; i < textsToTranslate.length; i++) {
textsToTranslate[i].setText(translatedStrings[i]);
}
}
return textsToTranslate.length;| function translateAllSlides(sourceLanguage, targetLanguage) { | ||
| // Get the active presentation and all slides | ||
| const presentation = SlidesApp.getActivePresentation(); | ||
| const slides = presentation.getSlides(); | ||
|
|
||
| let totalTranslated = 0; | ||
|
|
||
| // Loop through each slide | ||
| for (const slide of slides) { | ||
| // Get all text-containing elements from the slide | ||
| const texts = getElementTexts(slide.getPageElements()); | ||
|
|
||
| // Translate all text elements in-place | ||
| for (const text of texts) { | ||
| const originalText = text.asRenderedString(); | ||
| if (originalText.trim().length > 0) { | ||
| const translatedText = LanguageApp.translate( | ||
| originalText, | ||
| sourceLanguage, | ||
| targetLanguage, | ||
| ); | ||
| text.setText(translatedText); | ||
| totalTranslated++; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return totalTranslated; | ||
| } |
There was a problem hiding this comment.
The translateAllSlides function calls LanguageApp.translate for each text element individually. For presentations with many text elements, this is inefficient and can easily exceed Apps Script quotas for service calls. To improve performance and avoid quota issues, you should batch these calls. Collect all text strings into an array, make a single call to LanguageApp.translate, and then iterate through the results to update the text elements.
function translateAllSlides(sourceLanguage, targetLanguage) {
// Get the active presentation and all slides
const presentation = SlidesApp.getActivePresentation();
const slides = presentation.getSlides();
const textsToTranslate = [];
const originalStrings = [];
// Loop through each slide to gather all text elements
for (const slide of slides) {
const texts = getElementTexts(slide.getPageElements());
for (const text of texts) {
const originalText = text.asRenderedString();
if (originalText.trim().length > 0) {
textsToTranslate.push(text);
originalStrings.push(originalText);
}
}
}
if (originalStrings.length === 0) {
return 0;
}
// Translate all strings in a single batch call
const translatedStrings = LanguageApp.translate(originalStrings, sourceLanguage, targetLanguage);
// Update all text elements with translated content
for (let i = 0; i < textsToTranslate.length; i++) {
textsToTranslate[i].setText(translatedStrings[i]);
}
return textsToTranslate.length;
}
|
Thanks for the feedback. I am not sure if all of the suggestions are worth it.
P.S. Tested and reviewed by peers as well 
|
|
@jpoehnelt |
2 participants
Description
This contribution adds an enhanced translation feature for Google Slides that allows users to translate entire presentations with full control over source and target languages. Helps in quick translation of all slides.
The code is based on the already existing sample app.
Use-case: Usually used in office scenarios to translate lots of slides and its elements quickly in one-go.
New files added:
translateAllSlides.gs- Enhanced version withtranslateAllSlides()function to translate all slides at oncesidebarEnhanced.html- Improved UI with source language selection and expanded language supportKey improvements:
translateAllSlides()function to translate entire presentations in one clicktranslate.gsandsidebar.htmlremain unchanged (used in official documentation)Fixes # (N/A - this is a new feature addition)
Is it been tested?
Checklist