Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[WIP] Translation: desktop-electron #3267

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
+112 −26
Open

[WIP] Translation: desktop-electron #3267

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
5ead846
Translation: desktop-electron
psybers Aug 15, 2024
a803e1e
add release note
psybers Aug 15, 2024
bec104d
fix lint
psybers Aug 15, 2024
dac930d
switch to bare i18n function
psybers Aug 15, 2024
4861a73
fix linter
psybers Aug 15, 2024
0f6c011
i18n -> i18next
psybers Aug 15, 2024
0394007
fix error
psybers Aug 15, 2024
4fd11dd
try another approach
psybers Aug 15, 2024
58e5ae5
fix path
psybers Aug 15, 2024
4823db3
cleanup
psybers Aug 15, 2024
f15ae02
Merge branch 'master' into translate-deskto-pelectron
psybers Aug 20, 2024
fe874be
Merge branch 'master' into translate-deskto-pelectron
psybers Aug 26, 2024
fe38531
Merge branch 'master' into translate-deskto-pelectron
psybers Sep 3, 2024
23a85b5
Merge branch 'master' into translate-deskto-pelectron
psybers Sep 11, 2024
54cd752
Merge branch 'master' into translate-deskto-pelectron
psybers Sep 19, 2024
844faf0
Merge branch 'master' into translate-deskto-pelectron
psybers Sep 20, 2024
651394f
Merge branch 'master' into translate-deskto-pelectron
psybers Sep 28, 2024
865d5cb
Merge branch 'master' into translate-deskto-pelectron
psybers Oct 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .eslintignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ packages/desktop-client/playwright-report/

packages/desktop-electron/client-build/
packages/desktop-electron/dist/
packages/desktop-electron/build/

packages/import-ynab4/**/node_modules/*

Expand Down
6 changes: 5 additions & 1 deletion packages/desktop-client/i18next-parser.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
module.exports = {
input: ['src/**/*.{js,jsx,ts,tsx}', '../loot-core/src/**/*.{js,jsx,ts,tsx}'],
input: [
'src/**/*.{js,jsx,ts,tsx}',
'../loot-core/src/**/*.{js,jsx,ts,tsx}',
'../desktop-electron/*.{js,ts}',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Include subdirectories in the input path for comprehensive localization

The current input path '../desktop-electron/*.{js,ts}' only includes JavaScript and TypeScript files located directly in the ../desktop-electron/ directory. To ensure that all relevant files, including those in subdirectories, are processed for translation keys, consider updating the pattern to include subdirectories.

Apply this diff to update the input path:

-      '../desktop-electron/*.{js,ts}',
+      '../desktop-electron/**/*.{js,ts}',
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
'../desktop-electron/*.{js,ts}',
'../desktop-electron/**/*.{js,ts}',

],
output: 'src/locale/$LOCALE.json',
locales: ['en'],
sort: true,
Expand Down
27 changes: 27 additions & 0 deletions packages/desktop-electron/i18n.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import i18n from 'i18next';
import backend from 'i18next-electron-fs-backend';

i18n.use(backend).init({
backend: {
loadPath: '../desktop-client/locale/{{lng}}.json',
addPath: '../desktop-client/locale/{{lng}}.missing.json',
contextBridgeApiKey: 'Actual',
},

// While we mark all strings for translations, one can test
// it by setting the language in localStorage to their choice.
// Set this to 'cimode' to see the exact keys without interpolation.
lng: 'en', // FIXME localStorage undefined
// lng: localStorage.getItem('language') || 'en',
Comment on lines +14 to +15
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cannot Access localStorage in the Main Process

localStorage is a Web API available in the renderer process, not in the main process of an Electron application. Attempting to access it in the main process results in undefined.

To resolve this, consider the following options:

  • Option 1: Use Electron's ipcMain and ipcRenderer modules to communicate the language preference from the renderer process to the main process.

  • Option 2: Store the language preference in a configuration file using Electron's app.getPath('userData') directory.

Here's how you might implement Option 1:

  1. In your renderer process, send the language preference to the main process:

    // renderer.js
const { ipcRenderer } = require('electron');
ipcRenderer.send('set-language', localStorage.getItem('language') || 'en');

  2. In the main process, listen for the language setting and initialize i18n accordingly:

    +import { ipcMain } from 'electron';

 let language = 'en'; // default language

+ipcMain.on('set-language', (event, lng) => {
+  language = lng;
+  i18n.changeLanguage(language);
+});

 i18n.use(backend).init({
    // ...
-   lng: 'en', // FIXME localStorage undefined
+   lng: language,
    // ...
 });


// allow keys to be phrases having `:`, `.`
nsSeparator: false,
keySeparator: false,
// do not load a fallback
fallbackLng: false,
interpolation: {
escapeValue: false,
},
});

export default i18n;

Check warning on line 27 in packages/desktop-electron/i18n.ts

View workflow job for this annotation

GitHub Actions / lint 

Prefer named exports
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer Named Exports Over Default Export

Using named exports can improve maintainability and enable easier refactoring and code navigation.

Change the default export to a named export:

-export default i18n;
+export { i18n };

And update any import statements accordingly:

-import i18n from './i18n';
+import { i18n } from './i18n';
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export default i18n;
export { i18n };
Tools
GitHub Check: lint

[warning] 27-27:
Prefer named exports

18 changes: 13 additions & 5 deletions packages/desktop-electron/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,15 @@ import {
import isDev from 'electron-is-dev';
import promiseRetry from 'promise-retry';

import i18n from './i18n';
import { getMenu } from './menu';
import {
get as getWindowState,
listen as listenToWindowState,
} from './window-state';

import './security';
const backend = require('i18next-electron-fs-backend');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use ES6 import instead of require for module consistency

To maintain consistency and leverage TypeScript's features, consider replacing require with an import statement. This ensures better type checking and aligns with modern ES6 module syntax.

Apply this diff:

-const backend = require('i18next-electron-fs-backend');
+import backend from 'i18next-electron-fs-backend';

If i18next-electron-fs-backend does not have a default export or if there are compatibility issues, you might need to adjust the import:

-const backend = require('i18next-electron-fs-backend');
+import * as backend from 'i18next-electron-fs-backend';

Ensure that your tsconfig.json has "esModuleInterop": true if you're using default imports with CommonJS modules.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const backend = require('i18next-electron-fs-backend');
import backend from 'i18next-electron-fs-backend';


Module.globalPaths.push(__dirname + '/..');

Expand Down Expand Up @@ -111,6 +113,8 @@ async function createWindow() {

win.setBackgroundColor('#E8ECF0');

backend.mainBindings(ipcMain, win, fs);

if (isDev) {
win.webContents.openDevTools();
}
Expand Down Expand Up @@ -184,23 +188,25 @@ function isExternalUrl(url: string) {
function updateMenu(budgetId?: string) {
const isBudgetOpen = !!budgetId;
const menu = getMenu(isDev, createWindow, budgetId);
const file = menu.items.filter(item => item.label === 'File')[0];
const file = menu.items.filter(item => item.label === i18n.t('File'))[0];
const fileItems = file.submenu?.items || [];
fileItems
.filter(item => item.label === 'Load Backup...')
.filter(item => item.label === i18n.t('Load Backup...'))
Comment on lines +191 to +194
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid using translated labels for menu item identification

Using i18n.t('File') and i18n.t('Load Backup...') to filter menu items can lead to issues when the application's language changes, as the labels will vary. This can make menu item identification unreliable. Instead, assign unique identifiers or use the role property for menu items to ensure consistent identification regardless of the language.

Suggested refactor:

When creating the menu items, add an id property:

{
  label: i18n.t('File'),
  id: 'menu-file',
  submenu: [
    {
      label: i18n.t('Load Backup...'),
      id: 'menu-load-backup',
      // ...
    },
    // ...
  ],
},

Then, update your filtering logic:

-const file = menu.items.filter(item => item.label === i18n.t('File'))[0];
+const file = menu.getMenuItemById('menu-file');

.forEach(item => {
item.enabled = isBudgetOpen;
});

const tools = menu.items.filter(item => item.label === 'Tools')[0];
const tools = menu.items.filter(item => item.label === i18n.t('Tools'))[0];
tools.submenu?.items.forEach(item => {
Comment on lines +199 to 200
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid using translated labels for menu item identification

Filtering menu items using i18n.t('Tools') can cause identification issues when the language changes. Assign unique identifiers to menu items to ensure reliable access regardless of the current language setting.

Suggested refactor:

Assign an id to the "Tools" menu item:

{
  label: i18n.t('Tools'),
  id: 'menu-tools',
  // ...
},

Update the reference in your code:

-const tools = menu.items.filter(item => item.label === i18n.t('Tools'))[0];
+const tools = menu.getMenuItemById('menu-tools');

item.enabled = isBudgetOpen;
});

const edit = menu.items.filter(item => item.label === 'Edit')[0];
const edit = menu.items.filter(item => item.label === i18n.t('Edit'))[0];
const editItems = edit.submenu?.items || [];
editItems
.filter(item => item.label === 'Undo' || item.label === 'Redo')
.filter(
item => item.label === i18n.t('Undo') || item.label === i18n.t('Redo'),
)
Comment on lines +204 to +209
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid using translated labels for menu item identification

Using i18n.t('Edit'), i18n.t('Undo'), and i18n.t('Redo') for filtering can become unreliable with different languages. Assign unique identifiers to these menu items to maintain consistent behavior.

Suggested refactor:

Assign ids to the relevant menu items:

{
  label: i18n.t('Edit'),
  id: 'menu-edit',
  submenu: [
    {
      label: i18n.t('Undo'),
      id: 'menu-undo',
      // ...
    },
    {
      label: i18n.t('Redo'),
      id: 'menu-redo',
      // ...
    },
    // ...
  ],
},

Update your filtering logic:

-const edit = menu.items.filter(item => item.label === i18n.t('Edit'))[0];
+const edit = menu.getMenuItemById('menu-edit');

- editItems.filter(
-   item => item.label === i18n.t('Undo') || item.label === i18n.t('Redo'),
- )
+ editItems.filter(
+   item => item.id === 'menu-undo' || item.id === 'menu-redo',
+ )

.map(item => (item.enabled = isBudgetOpen));

if (process.platform === 'win32') {
Expand Down Expand Up @@ -280,6 +286,8 @@ app.on('window-all-closed', () => {
// On macOS, closing all windows shouldn't exit the process
if (process.platform !== 'darwin') {
app.quit();
} else {
backend.clearMainBindings(ipcMain);
}
});

Expand Down
42 changes: 22 additions & 20 deletions packages/desktop-electron/menu.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,19 @@ import {
shell,
} from 'electron';

import i18n from './i18n';

export function getMenu(
isDev: boolean,
createWindow: () => Promise<void>,
budgetId: string | undefined = undefined,
) {
const template: MenuItemConstructorOptions[] = [
{
label: 'File',
label: i18n.t('File'),
submenu: [
{
label: 'Load Backup...',
label: i18n.t('Load Backup...'),
enabled: false,
click(_item, focusedWindow) {
if (focusedWindow && budgetId) {
Expand All @@ -32,7 +34,7 @@ export function getMenu(
type: 'separator',
},
{
label: 'Manage files...',
label: i18n.t('Manage files...'),
accelerator: 'CmdOrCtrl+O',
click(_item, focusedWindow) {
if (focusedWindow) {
Expand All @@ -52,10 +54,10 @@ export function getMenu(
],
},
{
label: 'Edit',
label: i18n.t('Edit'),
submenu: [
{
label: 'Undo',
label: i18n.t('Undo'),
enabled: false,
accelerator: 'CmdOrCtrl+Z',
click: function (_menuItem, focusedWin) {
Expand All @@ -68,7 +70,7 @@ export function getMenu(
},
},
{
label: 'Redo',
label: i18n.t('Redo'),
enabled: false,
accelerator: 'Shift+CmdOrCtrl+Z',
click: function (_menuItem, focusedWin) {
Expand Down Expand Up @@ -104,19 +106,19 @@ export function getMenu(
],
},
{
label: 'View',
label: i18n.t('View'),
submenu: [
isDev
? {
label: 'Reload',
label: i18n.t('Reload'),
accelerator: 'CmdOrCtrl+R',
click(_item, focusedWindow) {
if (focusedWindow) focusedWindow.reload();
},
}
: { label: 'hidden', visible: false },
{
label: 'Toggle Developer Tools',
label: i18n.t('Toggle Developer Tools'),
accelerator:
process.platform === 'darwin' ? 'Alt+Command+I' : 'Ctrl+Shift+I',
click(_item, focusedWindow) {
Expand Down Expand Up @@ -146,10 +148,10 @@ export function getMenu(
],
},
{
label: 'Tools',
label: i18n.t('Tools'),
submenu: [
{
label: 'Find schedules',
label: i18n.t('Find schedules'),
enabled: false,
click: function (_menuItem, focusedWin) {
if (focusedWin) {
Expand All @@ -173,7 +175,7 @@ export function getMenu(
role: 'help',
submenu: [
{
label: 'Keyboard Shortcuts Reference',
label: i18n.t('Keyboard Shortcuts Reference'),
accelerator: '?',
enabled: !!budgetId,
click: function (_menuItem, focusedWin) {
Expand All @@ -188,7 +190,7 @@ export function getMenu(
type: 'separator',
},
{
label: 'Learn More',
label: i18n.t('Learn More'),
click() {
shell.openExternal('https://actualbudget.org/docs/');
},
Expand All @@ -204,7 +206,7 @@ export function getMenu(
submenu: [
isDev
? {
label: 'Screenshot',
label: i18n.t('Screenshot'),
click() {
ipcMain.emit('screenshot');
},
Expand Down Expand Up @@ -238,13 +240,13 @@ export function getMenu(
],
});
// Edit menu.
const editIdx = template.findIndex(t => t.label === 'Edit');
const editIdx = template.findIndex(t => t.label === i18n.t('Edit'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential Issue with Using Translated Labels in findIndex

Using i18n.t('Edit') within template.findIndex may lead to issues if the translation of 'Edit' changes or differs across languages. This could result in editIdx being -1, causing a runtime error when accessing template[editIdx].

Consider assigning a unique identifier to the 'Edit' menu item to reliably locate it, regardless of language.

Apply this diff to fix the issue:

 {
   label: i18n.t('Edit'),
+  id: 'edit-menu',
   submenu: [
     // submenu items...
   ],
 },
 // Later in the code, find the 'Edit' menu using the id
-const editIdx = template.findIndex(t => t.label === i18n.t('Edit'));
+const editIdx = template.findIndex(t => t.id === 'edit-menu');

Committable suggestion was skipped due to low confidence.

(template[editIdx].submenu as MenuItemConstructorOptions[]).push(
{
type: 'separator',
},
{
label: 'Speech',
label: i18n.t('Speech'),
submenu: [
{
role: 'startSpeaking',
Expand All @@ -259,24 +261,24 @@ export function getMenu(
const windowIdx = template.findIndex(t => t.role === 'window');
template[windowIdx].submenu = [
{
label: 'Close',
label: i18n.t('Close'),
accelerator: 'CmdOrCtrl+W',
role: 'close',
},
{
label: 'Minimize',
label: i18n.t('Minimize'),
accelerator: 'CmdOrCtrl+M',
role: 'minimize',
},
{
label: 'Zoom',
label: i18n.t('Zoom'),
role: 'zoom',
},
{
type: 'separator',
},
{
label: 'Bring All to Front',
label: i18n.t('Bring All to Front'),
role: 'front',
},
];
Expand Down
4 changes: 4 additions & 0 deletions packages/desktop-electron/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,9 @@
"dependencies": {
"electron-is-dev": "2.0.0",
"electron-log": "4.4.8",
"i18next": "^23.12.6",
"i18next-electron-fs-backend": "^3.0.2",
"lodash": "^4.17.21",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider optimizing lodash usage

Adding "lodash" introduces a large dependency that can increase the application's size. If only a few utility functions are needed, consider importing specific modules (e.g., "lodash.merge") or using native JavaScript alternatives to minimize the bundle size.

"loot-core": "*",
"node-fetch": "^2.7.0",
"promise-retry": "^2.0.1"
Expand All @@ -98,6 +101,7 @@
"@electron/notarize": "2.4.0",
"@electron/rebuild": "3.6.0",
"@types/copyfiles": "^2",
"@types/lodash": "^4",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Align TypeScript definitions with lodash version

The @types/lodash package is specified with "^4". To ensure compatibility and prevent potential type mismatches, consider specifying the exact version to match the "lodash" version "^4.17.21".

Apply this diff to update the @types/lodash version:

     "@types/copyfiles": "^2",
-    "@types/lodash": "^4",
+    "@types/lodash": "^4.17.21",
     "copyfiles": "^2.4.1",

Committable suggestion was skipped due to low confidence.

"copyfiles": "^2.4.1",
"cross-env": "^7.0.3",
"electron": "30.0.6",
Expand Down
4 changes: 4 additions & 0 deletions packages/desktop-electron/preload.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import {
SaveFileDialogPayload,
} from './index';

const backend = require('i18next-electron-fs-backend');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use ES6 import instead of CommonJS require

For consistency and better TypeScript integration, consider importing i18next-electron-fs-backend using ES6 import syntax. This enhances type checking and aligns the code with modern standards.

Suggested change:

-const backend = require('i18next-electron-fs-backend');
+import backend from 'i18next-electron-fs-backend';

Ensure that the module exports support this syntax, and adjust the import statement if necessary.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const backend = require('i18next-electron-fs-backend');
import backend from 'i18next-electron-fs-backend';


const { version: VERSION, isDev: IS_DEV }: GetBootstrapDataPayload =
ipcRenderer.sendSync('get-bootstrap-data');

Expand All @@ -14,6 +16,8 @@ contextBridge.exposeInMainWorld('Actual', {
ACTUAL_VERSION: VERSION,
logToTerminal: console.log,

i18nextElectronBackend: backend.preloadBindings(ipcRenderer, process),

ipcConnect: (
func: (payload: {
on: IpcRenderer['on'];
Expand Down
6 changes: 6 additions & 0 deletions upcoming-release-notes/3267.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
category: Enhancements
authors: [psybers]
---

Support translations in desktop-electron.
30 changes: 30 additions & 0 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8412,12 +8412,16 @@ __metadata:
"@electron/notarize": "npm:2.4.0"
"@electron/rebuild": "npm:3.6.0"
"@types/copyfiles": "npm:^2"
"@types/lodash": "npm:^4"
copyfiles: "npm:^2.4.1"
cross-env: "npm:^7.0.3"
electron: "npm:30.0.6"
electron-builder: "npm:24.13.3"
electron-is-dev: "npm:2.0.0"
electron-log: "npm:4.4.8"
i18next: "npm:^23.12.6"
i18next-electron-fs-backend: "npm:^3.0.2"
lodash: "npm:^4.17.21"
loot-core: "npm:*"
node-fetch: "npm:^2.7.0"
promise-retry: "npm:^2.0.1"
Expand Down Expand Up @@ -10982,6 +10986,16 @@ __metadata:
languageName: node
linkType: hard

"i18next-electron-fs-backend@npm:^3.0.2":
version: 3.0.2
resolution: "i18next-electron-fs-backend@npm:3.0.2"
dependencies:
lodash.clonedeep: "npm:^4.5.0"
lodash.merge: "npm:^4.6.2"
checksum: 10/43227421724382ba3cea544e03d0ca3d159c73dad57abc85ba216fd6ae425b7f5cd1937205123e75d405b4db2488f654fbf6214145d2970316cd43572f789cbd
languageName: node
linkType: hard

"i18next-parser@npm:^9.0.0":
version: 9.0.1
resolution: "i18next-parser@npm:9.0.1"
Expand Down Expand Up @@ -11027,6 +11041,15 @@ __metadata:
languageName: node
linkType: hard

"i18next@npm:^23.12.6":
version: 23.12.6
resolution: "i18next@npm:23.12.6"
dependencies:
"@babel/runtime": "npm:^7.23.2"
checksum: 10/6bb7faa2f1645dd7fc8d04c485707221edcf14911756d94faa8f5b7ffb5507995e00dca01f3c516ced9e986effb0073b06e9c55c2d762d400841cc557002abb1
languageName: node
linkType: hard

"iconv-corefoundation@npm:^1.1.7":
version: 1.1.7
resolution: "iconv-corefoundation@npm:1.1.7"
Expand Down Expand Up @@ -13042,6 +13065,13 @@ __metadata:
languageName: node
linkType: hard

"lodash.clonedeep@npm:^4.5.0":
version: 4.5.0
resolution: "lodash.clonedeep@npm:4.5.0"
checksum: 10/957ed243f84ba6791d4992d5c222ffffca339a3b79dbe81d2eaf0c90504160b500641c5a0f56e27630030b18b8e971ea10b44f928a977d5ced3c8948841b555f
languageName: node
linkType: hard

"lodash.debounce@npm:^4.0.8":
version: 4.0.8
resolution: "lodash.debounce@npm:4.0.8"
Expand Down
Loading