fix: proper localization when using GtkFileChooserNative #30888
Conversation
393863c to
1cde286
Compare
| } | ||
|
|
||
| const char* GetYesLabel() { | ||
| if (!gtk::GtkCheckVersion(4)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Wait, read the conditional backwards. It would be very surprising to me that you would need this still though.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
I see. Let me forward this to #gtk on IRC. Something seems off at the moment.
There was a problem hiding this comment.
What happens if you remove the version check conditionals?
There was a problem hiding this comment.
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.
cebcbc5 to
329a346
Compare
| // 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( |
There was a problem hiding this comment.
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.
|
I guess my only other comment is, does Electron not already have existing localization/internationalization infrastructure? Seems peculiar to piggy back on GTK's. |
329a346 to
9ab6ff1
Compare
|
@tristan957 no, we would typically snap to Chromium's approach for something like this. |
9ab6ff1 to
ed9e6cb
Compare
ed9e6cb to
5a46029
Compare
6c06d46 to
61caae9
Compare
61caae9 to
14bfa1b
Compare
zcbenz
left a comment
There was a problem hiding this comment.
I'm good with this PR since it aligns with Chromium's behavior.
|
Release Notes Persisted
|
|
I have automatically backported this PR to "14-x-y", please check out #31065 |
|
I have automatically backported this PR to "15-x-y", please check out #31066 |
|
I have automatically backported this PR to "16-x-y", please check out #31067 |
Description of Change
Closes #30247.
When passing
nullptrfor the text fields inGtkFileChooserNative, 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 (usinggettextto 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
npm testpassesRelease Notes
Notes: Fixed an issue where button labels in file choosers were improperly localized on Linux.