search

vslavik / poedit

Translations editor for Mac, Windows and Unix
https://poedit.net
MIT License
1.77k stars 275 forks source link

Recent files menu item icons are not displayed under wxGTK #753

Closed super7ramp closed 2 years ago

super7ramp commented 2 years ago

Environment

Bug

Icons of items in "Recent files" menu are not displayed under wxGTK.

The following message is printed twice for each time in "Recent files" menu when Poedit is launched from command line [^1].

(net.poedit.Poedit:29444): GLib-GObject-WARNING **: 21:04:22.662: invalid cast from 'GtkMenuItem' to 'GtkImageMenuItem'

(net.poedit.Poedit:29444): Gtk-CRITICAL **: 21:04:22.662: gtk_image_menu_item_set_image: assertion 'GTK_IS_IMAGE_MENU_ITEM (image_menu_item)' failed

I believe this is due to the current usage of wxMenuItem::SetBitmap in MyHistory::DoAddFile; method is called after the item has been appended to the menu:

            auto item = menu->Append(wxID_FILE1 + n, wxString::Format("&%d %s", n + 1, menuEntry));
            item->SetHelp(fn.GetFullPath());
            item->SetBitmap(m_icons_cache->get_small(fn.GetExt()));

This is not compliant with the documentation:

SetBitmap() must be called before the item is appended to the menu, i.e. appending the item without a bitmap and setting one later is not guaranteed to work. But the bitmap can be changed or reset later if it had been set up initially.

I guess it's OK for wxMSW to set the bitmap after the item has been appended but it doesn't work with wxGTK [^2].

Proposed patch

The following patch fixes the issue:

diff --git a/src/recent_files.cpp b/src/recent_files.cpp
index 9f0114afe..c0501e2bd 100644
--- a/src/recent_files.cpp
+++ b/src/recent_files.cpp
@@ -406,9 +406,10 @@ protected:
             // we need to quote '&' characters which are used for mnemonics
             menuEntry.Replace("&", "&&");

-            auto item = menu->Append(wxID_FILE1 + n, wxString::Format("&%d %s", n + 1, menuEntry));
+            auto item = new wxMenuItem(menu, wxID_FILE1 + n, wxString::Format("&%d %s", n + 1, menuEntry));
             item->SetHelp(fn.GetFullPath());
             item->SetBitmap(m_icons_cache->get_small(fn.GetExt()));
+            menu->Append(item);
         }

         file_icons_ptr m_icons_cache;

Although it reveals that the sizing of the icon is wrong (under wxGTK):

poedit-recent-files-icon-sizing

The following patch fixes the sizing issue but is a bit awkward - plus I doubt the result would look nice for HiDPI monitors:

diff --git a/src/recent_files.cpp b/src/recent_files.cpp
index c0501e2bd..a9a607f3e 100644
--- a/src/recent_files.cpp
+++ b/src/recent_files.cpp
@@ -118,7 +118,12 @@ class file_icons
 public:
     file_icons()
     {
+#ifdef __WXGTK__
+        // wxGTK doesn't provide metric for wxSYS_SMALLICON_X
+        m_iconSize[icon_small] = PX(16);
+#else
         m_iconSize[icon_small] = wxSystemSettings::GetMetric(wxSYS_SMALLICON_X);
+#endif
         m_iconSize[icon_large] = wxSystemSettings::GetMetric(wxSYS_ICON_X);
     }

@@ -155,6 +160,7 @@ private:
     wxBitmap create_bitmap(const wxIconLocation& loc, icon_size size)
     {
         wxString fullname = loc.GetFileName();
+        int desiredSize = m_iconSize[size];
 #ifdef __WXMSW__
         if (loc.GetIndex())
         {
@@ -162,10 +168,18 @@ private:
             fullname << ';' << loc.GetIndex();
         }
 #endif
-        wxIcon icon(fullname, wxBITMAP_TYPE_ICO, m_iconSize[size], m_iconSize[size]);
+        wxIcon icon(fullname, wxBITMAP_TYPE_ICO, desiredSize, desiredSize);
         if (!icon.IsOk())
             icon.LoadFile(fullname, wxBITMAP_TYPE_ICO);
-
+#ifndef __WXMSW__
+        // There is no guarantee that the desired size given at icon construction has been taken into account - only
+        // wxMSW seems to use it
+        if (icon.GetWidth() != desiredSize || icon.GetHeight() != desiredSize)
+        {
+            wxImage image = icon.ConvertToImage();
+            return image.Scale(desiredSize, desiredSize);
+        }
+#endif
         return icon;
     }

Steps to reproduce

  1. Configure GTK in order to display images in menu by appending gtk-menu-images=1 to ~/.config/gtk-3.0/settings.ini [^3].
  2. Open Poedit
  3. Open a po file (e.g. this one)
  4. Close Poedit
  5. Open Poedit from command line
  6. Open Recent Files menu

Note: If you have already opened po file in the past and your "Recent files" menu is already populated, then you can skip steps 2 to 4.

Actual result

Note: The assertion error is printed even if gtk-menu-images=0.

Expected Result

[^1]: The assertion error is raised with wxGTK >= 3.1.6 and is due to changes of https://github.com/wxWidgets/wxWidgets/pull/22087.

[^2]: It looks like one could replace menuItem = gtk_menu_item_new_with_label(""); at src/gtk/menu.cpp#L1037 by menuItem = gtk_image_menu_item_new_with_label(""); in order not to require the bitmap to be set before the item is appended to the menu. However, as GtkImageMenuItem is deprecated and as wxMenuItem::SetBitmap documents the usage I haven't investigated further.

[^3]: As described in wxMenuItem::SetBitmap documentation, default GTK behaviour has been not to show any images in menu for some time - which would explain why this issue hasn't been spotted earlier:

> Notice that GTK+ uses a global setting called `gtk-menu-images` to determine if the images should be shown in the menus at all. If it is off (which is the case in e.g. Gnome 2.28 by default), no images will be shown, consistently with the native behaviour.
vslavik commented 2 years ago

Thanks for the investigation! Feel free to create such well-researched, patch-heavy issues directly as draft PRs if it works for you.

The following patch fixes the sizing issue but is a bit awkward - plus I doubt the result would look nice for HiDPI monitors:

It's not going to support moving the window to another display, but should be fine for single-resolution setup. That's what the PX() macro does.

I agree it's awkward hack, but I don't think we can do any better yet — we'd need to extend wxIconLocation with support for wxBitmapBundle to allow representing differently sized icons.

We could also - and it's no less of a hack - manipulate fullname to get desired size. There's going to be either "scalable" or "256x256" or such in the path in your case, and we could replace it with "16x16" or "32x32". I tried looking into that, but apparently wxGTK won't even retrieve the icon in the version I have on the distro I have.

Notice that GTK+ uses a global setting called gtk-menu-images to determine if the images should be shown in the menus at all. If it is off (which is the case in e.g. Gnome 2.28 by default), no images will be shown, consistently with the native behaviour.

The bigger question, before applying these patches, is what should the right behavior be here? Should Poedit be using icons or not? gtk-menu-images and GtkImageMenuItem are deprecated and the guideline is to decide in application code on showing icons or not:

You should consider using icons in menu items only sparingly, and for “objects” (or “nouns”) elements only, like bookmarks, files, and links; “actions” (or “verbs”) should not have icons.

So we can use icons for files, the question is whether we should, and whether we should bypass gtk-menu-images (as would arguably be truly native GTK+3 behavior).

super7ramp commented 2 years ago

Feel free to create such well-researched, patch-heavy issues directly as draft PRs if it works for you.

Thanks, noted.

(Although here, I might have been too eager to show patches: As you rightly pointed out, it should be decided first whether these icons should be displayed or not on GTK >=3.)

The bigger question, before applying these patches, is what should the right behavior be here? Should Poedit be using icons or not?

I agree that's the important question.

Here are some arguments for both possible answers:

and whether we should bypass gtk-menu-images (as would arguably be truly native GTK+3 behavior).

I tend to think that letting the application choose the right behaviour and bypass deprecated gtk-menu-images would be the way to go with GTK >=3. With GTK4, Gtk.ImageMenuItem doesn't exist anymore so it would be inevitable.

we'd need to extend wxIconLocation with support for wxBitmapBundle to allow representing differently sized icons.

I've been playing with wxBitmapBundle and it looks better than resizing the image:

With icon resizing With wxBitmapBundle

Although:

vslavik commented 2 years ago

I guess it's OK for wxMSW to set the bitmap after the item has been appended but it doesn't work with wxGTK 2.

It was actually broken on wxMSW too, just in a more subtle and less deterministic way.

Here are some arguments for both possible answers:

Sorry for being unclear: what I'm after is knowing what the right thing is in GNOME's point of view. And I know, the first answer would be "all of this UI is completely wrong" - modern GNOME apps don't have recent files menu, they don't have menu mostly and the HIG doesn't talk about such mundane things.

But pragmatically, I don't have the resources to redo Poedit's UI on Linux to be like GNOME. Given that, I'm not sure how to approach the recent files menu.

The only "official" argument - for "no icons" - that I could find is gedit. It has a toolbar button for opening files and clicking the downarrow on it shows a list of recently edited files - as plain list, with no icons (and not looking like any other popup menu either). I didn't have any luck finding other GNOME apps with recent files.

I did also check GIMP and Inkscape and they both have Open Recent menu and use icons.

I tend to think that letting the application choose the right behaviour and bypass deprecated gtk-menu-images would be the way to go with GTK >=3. With GTK4, Gtk.ImageMenuItem doesn't exist anymore so it would be inevitable.

It's GtkImageMenuItem that is deprecated (as an artifact of dealing of too-icon-rich-menus of the past), not the concept of using icons. So yes, the question extends to whether it's worth doing anything for GTK4 or not.

super7ramp commented 2 years ago

I didn't have any luck finding other GNOME apps with recent files.

Here are some other examples I could find.

Also there are some applications without a recent files menu but with a recent file access on their welcome screen only. It might be an alternative given that Poedit has a welcome screen and RecentFilesCtrl that plays this role on non-wxGTK platforms.

vslavik commented 2 years ago

With icons: Glade (UI Designer) Vinagre (Remote Desktop Viewer)

Thanks. These two projects are sufficiently GNOME-associated, I think, to consider them as relevant. And if they use icons, so should we. I applied your changes until there's a better way with wx.

Thanks a lot!