Following up https://github.com/geany/geany/pull/3593#issuecomment-1757592993, should we do something about it on our end? Basically the solution is to use `edit-clear-symbolic` icon-name instead of `GTK_STOCK_CLEAR`, which is for most of it easy, for some a bit more involved.
* is it our problem? * well, no, but many distro ship Adwaita by default, so we suffer from it * is it specific to Debian Bookworm? * can we do something about it? is it easy? * we can, using icon names and `edit-clear-symbolic` for example. * most of the time, it's easy. Some of out API make other cases less easy.
@elextr @eht16 @techee opinions?
Subset of a possible fix: ```diff diff --git a/src/build.c b/src/build.c index 56ec9d613..3d94a683f 100644 --- a/src/build.c +++ b/src/build.c @@ -1898,7 +1898,7 @@ static RowWidgets *build_add_dialog_row(GeanyDocument *doc, GtkTable *table, gui GTK_FILL | GTK_EXPAND, entry_x_padding, entry_y_padding); } column++; - clearicon = gtk_image_new_from_stock(GTK_STOCK_CLEAR, GTK_ICON_SIZE_MENU); + clearicon = gtk_image_new_from_icon_name("edit-clear-symbolic", GTK_ICON_SIZE_MENU); clear = gtk_button_new(); gtk_button_set_image(GTK_BUTTON(clear), clearicon); g_signal_connect(clear, "clicked", G_CALLBACK(on_clear_dialog_row), roww); @@ -1995,7 +1995,7 @@ GtkWidget *build_commands_table(GeanyDocument *doc, GeanyBuildSource dst, BuildT } gtk_table_attach(table, fields->fileregex, DC_ENTRIES + 1, DC_CLEAR, row, row + 1, GTK_FILL, GTK_FILL | GTK_EXPAND, entry_x_padding, entry_y_padding); - clearicon = gtk_image_new_from_stock(GTK_STOCK_CLEAR, GTK_ICON_SIZE_MENU); + clearicon = gtk_image_new_from_icon_name("edit-clear-symbolic", GTK_ICON_SIZE_MENU); clear = gtk_button_new(); gtk_button_set_image(GTK_BUTTON(clear), clearicon); g_signal_connect_swapped(clear, "clicked", @@ -2029,7 +2029,7 @@ GtkWidget *build_commands_table(GeanyDocument *doc, GeanyBuildSource dst, BuildT } gtk_table_attach(table, fields->nonfileregex, DC_ENTRIES + 1, DC_CLEAR, row, row + 1, GTK_FILL, GTK_FILL | GTK_EXPAND, entry_x_padding, entry_y_padding); - clearicon = gtk_image_new_from_stock(GTK_STOCK_CLEAR, GTK_ICON_SIZE_MENU); + clearicon = gtk_image_new_from_icon_name("edit-clear-symbolic", GTK_ICON_SIZE_MENU); clear = gtk_button_new(); gtk_button_set_image(GTK_BUTTON(clear), clearicon); g_signal_connect_swapped(clear, "clicked", diff --git a/src/notebook.c b/src/notebook.c index ec00d3e96..df6f44f9d 100644 --- a/src/notebook.c +++ b/src/notebook.c @@ -746,7 +746,7 @@ gint notebook_new_tab(GeanyDocument *this) gtk_button_set_focus_on_click(GTK_BUTTON(btn), FALSE); gtk_widget_set_name(btn, "geany-close-tab-button");
- image = gtk_image_new_from_stock(GTK_STOCK_CLOSE, GTK_ICON_SIZE_MENU); + image = gtk_image_new_from_icon_name("window-close-symbolic", GTK_ICON_SIZE_MENU); gtk_container_add(GTK_CONTAINER(btn), image);
align = gtk_alignment_new(1.0, 0.5, 0.0, 0.0); diff --git a/src/ui_utils.c b/src/ui_utils.c index 34b26bc62..602d87080 100644 --- a/src/ui_utils.c +++ b/src/ui_utils.c @@ -1601,7 +1601,7 @@ static void entry_clear_icon_release_cb(GtkEntry *entry, gint icon_pos, GEANY_API_SYMBOL void ui_entry_add_clear_icon(GtkEntry *entry) { - g_object_set(entry, "secondary-icon-stock", GTK_STOCK_CLEAR, + g_object_set(entry, "secondary-icon-name", "edit-clear-symbolic", "secondary-icon-activatable", TRUE, NULL); g_signal_connect(entry, "icon-release", G_CALLBACK(entry_clear_icon_release_cb), NULL); } ```
IIUC in theory GTK icons should fall back on the non-symbolic version if the symbolic one is missing. Not sure if it plays any better if there's the same "one is there but not in the correct size" issue though.
Regardless of whether it is an Adwaita icon theme bug (I just tried with other icon themes and apart from the Adwaita one, all work well), I think if the patch is as simple as you propose (possibly covering more instances), we should do it - otherwise we might get flooded with bug reports regarding this problem and we might have to make a new release because of it anyway.
In fact, `gtk_image_new_from_stock()` is deprecated and we should migrate to `gtk_image_new_from_icon_name()` anyway so we can even call it a small step in the right direction.
otherwise we might get flooded with bug reports regarding this problem and we might have to make a new release because of it anyway.
Yeah that's kind of my point, whomever fault it is, some of it will land on us. And it still might, as if you install Geany from Debian under GNOME in Bookworm it will show up.
FWIW, I reported it to Debian -- not that I am very hopeful, but if nobody says anything, nothing will happen anyway -- see https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1054168
Note that unstable doesn't seem affected, because they have a newer version of the Adwaita icon theme which doesn't have the problematic icons anymore. So which distro is affected is gonna be a guessing game -- or rather, testing :)
In fact, `gtk_image_new_from_stock()` is deprecated and we should migrate to `gtk_image_new_from_icon_name()` anyway so we can even call it a small step in the right direction.
Yeah in theory it's a good thing to move away from the GtkStock API, so it's indeed not a pad change per se. If it's not in 2.0, I'll submit something similar for 2.1 anyway.
Great, that you have it reported on Debian. And good to know that newer versions of the theme do not expose this bug. Debian users could probably even install the Sid package (judged from the direct dependencies which should be possible).
Back to Geany: I fully agree with what @techee said. It's not unnecessary work as we have to do anyway in the future (if we don't find a way to burn GTK4 before...) and it prevents users from having a bad first impression of Geany and/or the update to the new version.
Might there be any risk that other themes may not work well with the changed icon references? I guess this is hard to answer; the icon loading mechanism are big mystique to me and so I'm just wondering how much we could break with this change.
Might there be any risk that other themes may not work well with the changed icon references? I guess this is hard to answer; the icon loading mechanism are big mystique to me and so I'm just wondering how much we could break with this change.
Well, I'm not 100% sure, but I think yes, there is a small risk: IIUC GTK falls back to and from symbolic icons, but *only if the requested version does not exist in the theme **at any size*** -- hence the bug we have, and it disappearing if you remove the offending icons. But this also means that a theme that has symbolic icons but only at some sizes that don't fit our needs would start breaking if we ask for the symbolic version.
I'll make some experiments though, because icon fallback rules is a tiny bit mystique to me as well.
Might there be any risk that other themes may not work well with the changed icon references? I guess this is hard to answer; the icon loading mechanism are big mystique to me and so I'm just wondering how much we could break with this change.
Just speculating but `gtk_image_new_from_stock()` has been deprecated since GTK 3.10 and we now require GTK 3.24 so we shouldn't run into the risk that Geany is started on some outdated distribution with an icon theme not supporting the names used by `gtk_image_new_from_icon_name()`. It will probably be the other way round - that themes support the `-symbolic` names and neglect the old naming like in the case of Adwaita.
@techee symbolic icons has nothing to do with using icon names rather than stock items, apart that stock items don't use symbolic icons.
See #3614
At least GTK seems to have a test for these icon names so I think their presence is kind of required:
https://gitlab.gnome.org/GNOME/gtk/-/blob/main/testsuite/gtk/check-icon-name...
@techee Ah yeah those names are common indeed. And cool, if they are required by GTK itself, it's indeed very likely to be present, or at least work fine if not (e.g. falling back on the non-symbolic version) :+1:
I'm gonna postpone that as discussed in #3614, although it might come back to bite us.
Or we mention it somewhere in the announcement email? @eht16 :laughing:
Or we mention it somewhere in the announcement email? @eht16 😆
It's not new to 2.0 though. I just tried Geany 1.38 packaged with Debian Bookworm and the issue is there too - yet, nobody is complaining. So maybe people use another icon theme or older distributions (or maybe we are the only people left still using Geany :-).
Or we mention it somewhere in the announcement email? @eht16 😆
Done.
github-comments@lists.geany.org