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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3f76436
fix: proper localization when using GtkFileChooserNative
codebytere Sep 8, 2021
14bfa1b
fix: iwyu
codebytere Sep 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
28 changes: 15 additions & 13 deletions shell/browser/ui/file_dialog_gtk.cc
View file
Open in desktop
Expand Up @@ -5,6 +5,7 @@
#include <gmodule.h>

#include <memory>
#include <string>

#include "base/callback.h"
#include "base/files/file_util.h"
Expand Down Expand Up @@ -131,24 +132,25 @@ class FileChooserDialog {
: parent_(
static_cast<electron::NativeWindowViews*>(settings.parent_window)),
filters_(settings.filters) {
const char* confirm_text = gtk_util::kOkLabel;

if (!settings.button_label.empty())
confirm_text = settings.button_label.c_str();
else if (action == GTK_FILE_CHOOSER_ACTION_SAVE)
confirm_text = gtk_util::kSaveLabel;
else if (action == GTK_FILE_CHOOSER_ACTION_OPEN)
confirm_text = gtk_util::kOpenLabel;

InitGtkFileChooserNativeSupport();

auto label = settings.button_label;

if (*supports_gtk_file_chooser_native) {
dialog_ = GTK_FILE_CHOOSER(
dl_gtk_file_chooser_native_new(settings.title.c_str(), NULL, action,
confirm_text, gtk_util::kCancelLabel));
dialog_ = GTK_FILE_CHOOSER(dl_gtk_file_chooser_native_new(
settings.title.c_str(), NULL, action,
label.empty() ? nullptr : label.c_str(), nullptr));
} else {
const char* confirm_text = gtk_util::GetOkLabel();
if (!label.empty())
confirm_text = label.c_str();
else if (action == GTK_FILE_CHOOSER_ACTION_SAVE)
confirm_text = gtk_util::GetSaveLabel();
codebytere marked this conversation as resolved.
Show resolved Hide resolved
else if (action == GTK_FILE_CHOOSER_ACTION_OPEN)
confirm_text = gtk_util::GetOpenLabel();

dialog_ = GTK_FILE_CHOOSER(gtk_file_chooser_dialog_new(
settings.title.c_str(), NULL, action, gtk_util::kCancelLabel,
settings.title.c_str(), NULL, action, gtk_util::GetCancelLabel(),
GTK_RESPONSE_CANCEL, confirm_text, GTK_RESPONSE_ACCEPT, NULL));
}

Expand Down
83 changes: 59 additions & 24 deletions shell/browser/ui/gtk_util.cc
View file
Open in desktop
Expand Up @@ -8,36 +8,71 @@
#include <gtk/gtk.h>
#include <stdint.h>

#include <string>

#include "base/no_destructor.h"
#include "base/strings/string_number_conversions.h"
#include "third_party/skia/include/core/SkBitmap.h"
#include "third_party/skia/include/core/SkColor.h"
#include "third_party/skia/include/core/SkUnPreMultiply.h"
#include "ui/gtk/gtk_compat.h" // nogncheck

namespace gtk_util {

// Copied from L40-L55 in
// https://cs.chromium.org/chromium/src/chrome/browser/ui/libgtkui/select_file_dialog_impl_gtk.cc
#if GTK_CHECK_VERSION(3, 90, 0)
// GTK stock items have been deprecated. The docs say to switch to using the
// strings "_Open", etc. However this breaks i18n. We could supply our own
// internationalized strings, but the "_" in these strings is significant: it's
// the keyboard shortcut to select these actions. TODO: Provide
// internationalized strings when GTK provides support for it.
const char* const kCancelLabel = "_Cancel";
const char* const kNoLabel = "_No";
const char* const kOkLabel = "_OK";
const char* const kOpenLabel = "_Open";
const char* const kSaveLabel = "_Save";
const char* const kYesLabel = "_Yes";
#else
G_GNUC_BEGIN_IGNORE_DEPRECATIONS
const char* const kCancelLabel = GTK_STOCK_CANCEL;
const char* const kNoLabel = GTK_STOCK_NO;
const char* const kOkLabel = GTK_STOCK_OK;
const char* const kOpenLabel = GTK_STOCK_OPEN;
const char* const kSaveLabel = GTK_STOCK_SAVE;
const char* const kYesLabel = GTK_STOCK_YES;
G_GNUC_END_IGNORE_DEPRECATIONS
#endif
// The following utilities are pulled from
// https://source.chromium.org/chromium/chromium/src/+/main:ui/gtk/select_file_dialog_impl_gtk.cc;l=43-74

const char* GettextPackage() {
codebytere marked this conversation as resolved.
Show resolved Hide resolved
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.

"gtk" + base::NumberToString(gtk::GtkVersion().components()[0]) + "0");
return gettext_package->c_str();
}

const char* GtkGettext(const char* str) {
return g_dgettext(GettextPackage(), str);
}

const char* GetCancelLabel() {
if (!gtk::GtkCheckVersion(4))
return "gtk-cancel"; // In GTK3, this is GTK_STOCK_CANCEL.
static const char* cancel = GtkGettext("_Cancel");
return cancel;
}

const char* GetOpenLabel() {
if (!gtk::GtkCheckVersion(4))
return "gtk-open"; // In GTK3, this is GTK_STOCK_OPEN.
static const char* open = GtkGettext("_Open");
return open;
}

const char* GetSaveLabel() {
if (!gtk::GtkCheckVersion(4))
return "gtk-save"; // In GTK3, this is GTK_STOCK_SAVE.
static const char* save = GtkGettext("_Save");
return save;
}

const char* GetOkLabel() {
if (!gtk::GtkCheckVersion(4))
return "gtk-ok"; // In GTK3, this is GTK_STOCK_OK.
static const char* ok = GtkGettext("_Ok");
return ok;
}

const char* GetNoLabel() {
if (!gtk::GtkCheckVersion(4))
return "gtk-no"; // In GTK3, this is GTK_STOCK_NO.
static const char* no = GtkGettext("_No");
return no;
}

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.

return "gtk-yes"; // In GTK3, this is GTK_STOCK_YES.
static const char* yes = GtkGettext("_Yes");
return yes;
}

GdkPixbuf* GdkPixbufFromSkBitmap(const SkBitmap& bitmap) {
if (bitmap.isNull())
Expand Down
17 changes: 9 additions & 8 deletions shell/browser/ui/gtk_util.h
View file
Open in desktop
Expand Up @@ -11,14 +11,15 @@ class SkBitmap;

namespace gtk_util {

/* These are `const char*` rather than the project-preferred `const char[]`
because they must fit the type of an external dependency */
extern const char* const kCancelLabel;
extern const char* const kNoLabel;
extern const char* const kOkLabel;
extern const char* const kOpenLabel;
extern const char* const kSaveLabel;
extern const char* const kYesLabel;
const char* GettextPackage();
const char* GtkGettext(const char* str);

const char* GetCancelLabel();
const char* GetOpenLabel();
const char* GetSaveLabel();
const char* GetOkLabel();
const char* GetNoLabel();
const char* GetYesLabel();

// Convert and copy a SkBitmap to a GdkPixbuf. NOTE: this uses BGRAToRGBA, so
// it is an expensive operation. The returned GdkPixbuf will have a refcount of
Expand Down
8 changes: 4 additions & 4 deletions shell/browser/ui/message_box_gtk.cc
View file
Open in desktop
Expand Up @@ -145,13 +145,13 @@ class GtkMessageBox : public NativeWindowObserver {
const char* TranslateToStock(int id, const std::string& text) {
const std::string lower = base::ToLowerASCII(text);
if (lower == "cancel")
return gtk_util::kCancelLabel;
return gtk_util::GetCancelLabel();
if (lower == "no")
return gtk_util::kNoLabel;
return gtk_util::GetNoLabel();
if (lower == "ok")
return gtk_util::kOkLabel;
return gtk_util::GetOkLabel();
if (lower == "yes")
return gtk_util::kYesLabel;
return gtk_util::GetYesLabel();
return text.c_str();
}

Expand Down