Skip to content
This repository has been archived by the owner on Mar 3, 2023. It is now read-only.

Fix/not update menu with i18n (#22594) #22597

Merged
merged 20 commits into from
Jul 8, 2021

Conversation

juggernautjp
Copy link
Contributor

Identify the Bug

Issue #22594

Description of the Change

In regard to MenuManager.add(items) in atom/src/menu-manager.coffee.

Atom Core and atom-i18n package use the same label member of the argument items,
which is mentioned above.
After atom-i18n localize menus and submenus, Atom Core can not update them correctly.

So, I added the class member id to the argument items, which means a member of class MenuItem in Electron.
After atom-i18n localize label, Atom Core use id (instead of label, which is current implementation) to update menus and submenus.

Modified files

  1. atom/src/menu-helpers.js
  2. atom/src/menu-manager.coffee
  3. atom/src/reopen-project-menu-manager.js
  4. atom/src/main-process/application-menu.js

Alternate Designs

I used sublabel instead of id. But strings of sublabel appear under the strings of label.

Possible Drawbacks

  1. The menus, which are created from menus/*.cson, of Atom Core and other packages are not updated correctly.
  2. I have not yet create specs files, because I need to study how to write specs.
  3. I tested on only Windows. This fix should be verified on Linux and macOS.

Verification Process

I imported atom-i18n package, and set locale to Japanese.

  1. The menubar has "ファイル(F)" and no "File" menu, and "Reopen Project" menu has correct projects.
  2. The "ヘルプ(H)" menu has "アップデートを確認", which means "Check for Update".

Release Notes

When Atom is localized with the atom-i18n package, the "File" menu is no longer added to the menu bar and "Check for Update" is localized and displayed in the "Help" menu on Windows.

"Check for Update" and "Reopen Project" menu dose not work.
The fixed bug is that "Reopen Project" menu has no project.
@icecream17
Copy link
Contributor

icecream17 commented Jun 15, 2021

Edit: ohhhh I get it now.

This pr adds "id" for identification instead of label
The issue is that i18n does stuff with label, which breaks atom.

And I see that label is not redundant with id, they're different concepts even though the tests show that they're usually the same...

FIx bug of atom/spec/*menu*.js, that are not tested at 1st Pull Request.
@juggernautjp
Copy link
Contributor Author

1. Atom Internationalization (i18n)

For globally used applications, such as VS Code and Atom editor, it is very important that they are localized in their own languages. In Japan, VS Code is almost all localized, so it has become more popular than Atom editor, last year.
The Atom Editor can be localized with the atom-i18n package, but it cannot localize menus or some strings that appear in menus. That's because those strings are hard-coded into the Atom Core's source code.

For internationalization of Atom, the label item of Menu should not be used in the merge/unmerge, etc.
Atom Core programmers should use id instead of label for Menu manipulation, in order for atom-i18n package to be able to localize their strings.

2. Fix the bug of CI error

I fixed the bug of the following CI bugs, and pass the CI test.

  1. [error] Tests for core-render-process in G:/Downloads/atom/spec/menu-manager-spec.js. failed.
  2. [error] Tests for core-render-process in G:/Downloads/atom/spec/context-menu-manager-spec.js. failed.

Change spec/{context-,}-menu-manager.spec.js
src/menu-manager.coffee Outdated Show resolved Hide resolved
src/menu-manager.coffee Outdated Show resolved Hide resolved
src/menu-manager.coffee Outdated Show resolved Hide resolved
src/menu-helpers.js Outdated Show resolved Hide resolved
src/menu-helpers.js Outdated Show resolved Hide resolved
src/menu-helpers.js Outdated Show resolved Hide resolved
juggernautjp and others added 6 commits June 25, 2021 06:45
Co-authored-by: Sadick <sadickjunior@gmail.com>
Co-authored-by: Sadick <sadickjunior@gmail.com>
Co-authored-by: Sadick <sadickjunior@gmail.com>
Co-authored-by: Sadick <sadickjunior@gmail.com>
Co-authored-by: Sadick <sadickjunior@gmail.com>
Copy link
Contributor Author

@juggernautjp juggernautjp left a comment

Choose a reason for hiding this comment

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

I agree.

src/context-menu-manager.coffee Outdated Show resolved Hide resolved
src/main-process/application-menu.js Outdated Show resolved Hide resolved
src/main-process/application-menu.js Outdated Show resolved Hide resolved
src/main-process/application-menu.js Outdated Show resolved Hide resolved
src/main-process/application-menu.js Outdated Show resolved Hide resolved
src/menu-manager.coffee Outdated Show resolved Hide resolved
src/menu-manager.coffee Outdated Show resolved Hide resolved
juggernautjp and others added 7 commits July 4, 2021 04:02
Co-authored-by: Sadick <sadickjunior@gmail.com>
Co-authored-by: Sadick <sadickjunior@gmail.com>
Co-authored-by: Sadick <sadickjunior@gmail.com>
Co-authored-by: Sadick <sadickjunior@gmail.com>
Co-authored-by: Sadick <sadickjunior@gmail.com>
Co-authored-by: Sadick <sadickjunior@gmail.com>
Co-authored-by: Sadick <sadickjunior@gmail.com>
@icecream17
Copy link
Contributor

Note: I commented in a resolved suggestion - don't know how to make it show up:
#22597 (comment)

Copy link
Contributor

@sadick254 sadick254 left a comment

Choose a reason for hiding this comment

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

Thank you for opening this PR and making the changes requested. 🙇

@sadick254 sadick254 merged commit 5676dfc into atom:master Jul 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants