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

fix: proper localization when using GtkFileChooserNative #30888

Merged
merged 2 commits into from Sep 22, 2021
Merged

fix: proper localization when using GtkFileChooserNative #30888

merged 2 commits into from Sep 22, 2021

Conversation

codebytere
Copy link
Member

Description of Change

Closes #30247.

When passing nullptr for the text fields in GtkFileChooserNative, GTK+ will provide default, localized buttons.

GTK+ 4.0 also removes the stock items for dialog buttons, with the idea that devs would instead use _("_Cancel") _("_Open"), etc directly (using gettext to pull the appropriate translated strings from the translations that ship with GTK+).

This updates our logic accordingly.

Tested with https://gist.github.com/Kilian/3de5f404d90d9b12ccaec817fdc4d6da.

Checklist

Release Notes

Notes: Fixed an issue where button labels in file choosers were improperly localized on Linux.

@codebytere codebytere added semver/patch backwards-compatible bug fixes target/14-x-y labels Sep 8, 2021
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Sep 8, 2021
@codebytere codebytere mentioned this pull request Sep 8, 2021
Closed
3 tasks
tristan957
tristan957 reviewed Sep 8, 2021
View reviewed changes
shell/browser/ui/gtk_util.cc
}

const char* GetYesLabel() {
if (!gtk::GtkCheckVersion(4))
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you need this conditional. GTK 4 follows the same i18n usage as GTK 3, so the GtkGettext() should in theory be enough. Hard to know for sure since Electron doesn't link against GTK4.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, read the conditional backwards. It would be very surprising to me that you would need this still though.

Copy link
Member Author

@codebytere codebytere Sep 8, 2021 •
edited

Choose a reason for hiding this comment

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

@tristan957 you could be right - i followed Chromium's approach here since it's been battle-tested more but am happy to reduce the code here if it's not strictly necessary for proper behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Let me forward this to #gtk on IRC. Something seems off at the moment.

Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if you remove the version check conditionals?

Copy link
Contributor

@TingPing TingPing Sep 9, 2021 •
edited

Choose a reason for hiding this comment

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

Stock icons have been deprecated for nearly a decade. I agree with @tristan957; Everything becomes cleaner removing this fallback path, for an older version where it was already deprecated.

deepak1556
deepak1556 reviewed Sep 8, 2021
View reviewed changes
shell/browser/ui/file_dialog_gtk.cc Outdated Show resolved Hide resolved
shell/browser/ui/gtk_util.cc Show resolved Hide resolved
nornagon
nornagon approved these changes Sep 8, 2021
View reviewed changes
Copy link
Member

@nornagon nornagon left a comment

Choose a reason for hiding this comment

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

seems legit

@codebytere codebytere force-pushed the fix-localized-translations branch 2 times, most recently from cebcbc5 to 329a346 Compare September 8, 2021 22:23
tristan957
tristan957 reviewed Sep 8, 2021
View reviewed changes
shell/browser/ui/gtk_util.cc
// https://source.chromium.org/chromium/chromium/src/+/main:ui/gtk/select_file_dialog_impl_gtk.cc;l=43-74

const char* GettextPackage() {
static base::NoDestructor<std::string> gettext_package(
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like you're (using that liberally since its chromium code), reaching into GTK's own gettext package. As far as I know, GTK makes no guarantees about its internal translated strings, so this seems pretty dangerous. I guess its fine to risk it for GTK3 since it is essentially frozen for the rest of eternity, but this could break at any moment really.

@tristan957
Copy link
Contributor

CC @vchernin and @squalou

@tristan957
Copy link
Contributor

I guess my only other comment is, does Electron not already have existing localization/internationalization infrastructure? Seems peculiar to piggy back on GTK's.

@codebytere
Copy link
Member Author

@tristan957 no, we would typically snap to Chromium's approach for something like this.

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Sep 9, 2021
@tscheims1 tscheims1 mentioned this pull request Sep 17, 2021
Closed
5 tasks
@codebytere codebytere force-pushed the fix-localized-translations branch 3 times, most recently from 6c06d46 to 61caae9 Compare September 21, 2021 13:51
zcbenz
zcbenz approved these changes Sep 21, 2021
View reviewed changes
Copy link
Member

@zcbenz zcbenz left a comment

Choose a reason for hiding this comment

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

I'm good with this PR since it aligns with Chromium's behavior.

@jkleinsc jkleinsc merged commit 38b810b into main Sep 22, 2021
@jkleinsc jkleinsc deleted the fix-localized-translations branch September 22, 2021 18:12
@release-clerk
Copy link

release-clerk bot commented Sep 22, 2021

Release Notes Persisted

Fixed an issue where button labels in file choosers were improperly localized on Linux.

This was referenced Sep 22, 2021
Merged
Merged
@trop
Copy link
Contributor

trop bot commented Sep 22, 2021

I have automatically backported this PR to "14-x-y", please check out #31065

@trop trop bot mentioned this pull request Sep 22, 2021
Merged
@trop
Copy link
Contributor

trop bot commented Sep 22, 2021

I have automatically backported this PR to "15-x-y", please check out #31066

@trop
Copy link
Contributor

trop bot commented Sep 22, 2021

I have automatically backported this PR to "16-x-y", please check out #31067

@commit-lint commit-lint bot mentioned this pull request Dec 13, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TingPing TingPing TingPing left review comments

@deepak1556 deepak1556 deepak1556 left review comments

@tristan957 tristan957 tristan957 left review comments

@nornagon nornagon nornagon approved these changes

@zcbenz zcbenz zcbenz approved these changes

@MarshallOfSound MarshallOfSound Awaiting requested review from MarshallOfSound

Assignees
No one assigned
Labels
semver/patch backwards-compatible bug fixes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: Button labels in File choosers are untranslated on Linux in e14
7 participants
@codebytere @tristan957 @nornagon @zcbenz @TingPing @deepak1556 @jkleinsc