Load menu icons only when "menus-have-icons" is set

[lukefromdc]

Most panel menus excluding main menus. Duplicate behavior of now-deprecated code replaced by github.com/mate-desktop/mate-panel/commit/86701517e7d7cb3d2c08a40d76af97308f18902c

[TomaszGasior]

There are three problems with this merge request.

1. This code should be part of MATE's shared library, not part of panel or any MATE application. But this is only my private opinion.
2. It does not work dynamically like GtkImageMenuItem was. When I un-check menu icons in MATE apeearance settings I have to re-login or killall mate-panel to see effect.
3. See ss:
   . No, fixing this should not be part of theme.

[muktupavels]

If you allocate memory, you should also free it.

[lukefromdc]

I'm seeking to get the panel to respect icon preferences here. Thus I am modifying a function already existing in mate-panel, not refactoring mate-panel, caja, and everything else to move a function to mate-desktop. That kind of combined PR is time-consuming to test and we are a small team. I for one will not be writing such a PR. If we want this to work at all, we need to keep it simple. We also have apps like Pluma that are deliberately designed NOT to depend on mate-desktop.

[lukefromdc]

@muktupavels, I don't know offhand how to free a gsettings object, and I don't see anything being done to free them in other functions in MATE. I am wondering if this is why changes to this setting are not being immediately applied. I tried g_object_unref() but got no change in behavior.

[lukefromdc]

OK, I see g_object_unref being used in some places in MATE for gsettings objects, but left out in a LOT of others. Didn't cause any problems and might be one less memory leak, but still looking at the delayed application issue.

We've got another PR hung up behind the panel not detecting window manager changes, BTW.

[lukefromdc]

I found the dynamic application issue: panel_image_menu_item_new_from_icon is only run the FIRST time the panel context menu is opened, and thus any change in the gsettings value cannot be applied after the menu is first created, because it is never remade. We have had problems in many other places created by this kind of problem.

[lukefromdc]

The dynamic application problems are now fixed, and it should now cover every menuitem icon loaded by mate-panel except those in the main menu button or main menu bar. I assume we want to load those unconditionally.

This ended up being four commits affecting three files. I reordered them a bit and squashed it into two commits, one to apply the gsettings preference and the other to rebuild the affected menus each time they are shown.

Note that building menus anew when shown is slightly heavier code, but these menus are small and if this is not done issues arise. Not only does reusing menus prevent applying any gsettings preference that changes the menus until the panel is restarted, in another place I am all but certain it plays a role in #817 , which I will now take another crack at. EDIT: that issue may be in libwnck and thus out of our scope for patching.

[lukefromdc]

@TomaszGasior , would you please retest this? I don't know what you were referrring to concerning themes, but you should not find icon show/hide prefs applied dynamically

[muktupavels]

Also why do you create icon_settings for each menu item?

Why do you need to rebuild menus? You should not comment out assert, but should make sure that old menu is properly destroyed.

HINT: you can bind icon visibility to GSettings setting!

[muktupavels]

Every time when panel_menu_get is called you re-create menu now... What happens with menu or menus that was created previously?

[lukefromdc]

Keep in mind, I am NOT a university-trained programmer but am self-taught by experimenting with this stuff. The menus are rebuilt each time because otherwise the changes are not applied. To apply a change in the icon settings requires repacking the box that is in turn packed into the menu item. As for the fate of previously created menus" NO idea but no observed adverse effect. Variables involved are simply overwritten with new values in at least some cases though.

As for why icon settings is created and destroyed for each menu item, it works that way because that's where the code is that needs to read it.

"bind icon visibility to GSetting" I have exactly no idea how to do or even what you mean.

[lukefromdc]

Note that I have never found any other way to make a menu change its appearance other than to re-create it. Do you know a good way to destroy a menu when it is closed (popped down) which would presumably have the same effect? As for the assert, it caused the panel to exit and could not be used. With it commented out the panel worked fine, not sure why it was there in the first place since I did not get a segfault or similar issue with it gone.

[lukefromdc]

We have another option here if we don't want to remake menus: ditch the attempt to make this work dynamically, and document in mate-appearance-properties that mate-panel may need to be restarted after enabling or disabling icons in menus. That is lighter code as the menus are made only once, at the price of being a little less functional. Is that a significant difference on something like a Raspberry Pi? If so I would vote for not attempting to apply it dynamically at all here.

The origjnal report on this issue came from someone who really wants icons in menus gone, not user selectable, which at least two of us are absolutely refusing to consider for MATE. None the less, if the preference for icons or no icons in menus exists it should be made to work, so users can choose for themselves.

[muktupavels]

On Thu, Jun 21, 2018 at 8:52 PM, lukefromdc ***@***.***> wrote: Keep in mind, I am NOT a university-trained programmer but am self-taught by experimenting with this stuff. The menus are rebuilt each time because otherwise the changes are not applied. To apply a change in the icon settings requires repacking the box that is in turn packed into the menu item. As for the fate of previously created menus" NO idea but no observed adverse effect. Variables involved are simply overwritten with new values in at least some cases though.

And? I comment to tell you that there are problems... I don't expect that you are professional who don't make mistakes. I do make mistakes myself... I just try to help... I strongly recommend you to read about `g_object_ref_sink`. In this case you are responsible to free created menu, but you simply overwrite existing menu/object with newly created menu. That is memory leak, no?

As for why icon settings is created and destroyed for each menu item, it works that way because that's where the code is that needs to read it.

Again, and? It is my opinion and everyone is free to ignore that, but I think that if you need some object accessed more than once you probably should create it somewhere else and simply pass it to function that needs it... In this case you are interested if icon should be shown, you could pass extra boolean parameter not create/destroy gsettings object for each menu item...

"bind icon visibility to GSetting" I have exactly no idea how to do or even what you mean.

Did you even tried to read documentation for GSettings? In short you can bind setting to object property. So in this case you can bind "visible" property of GtkWidget to GSettings `menus-have-icons` setting. When setting changes it will automatically update property for widget.

…

-- Alberts Muktupāvels

[muktupavels]

On Thu, Jun 21, 2018 at 8:54 PM, lukefromdc ***@***.***> wrote: Note that I have never found any other way to make a menu change its appearance other than to re-create it. Do you know a good way to destroy a menu when it is closed (popped down) which would presumably have the same effect? As for the assert, it caused the panel to exit and could not be used. With it commented out the panel worked fine, not sure why it was there in the first place since I did not get a segfault or similar issue with it gone.

Do you know why that assert was there? I would think / expect that developer who wrote it expected that this function would be called only once. So when this function is called `info->move_item` should be `NULL`. It worked for original author, you are breaking his expectation... If you are trying to call that function again, you should make sure that previously created menu and/or allocated memory is freed... If old menu would have been destroyed and that variable would have been reset to NULL then you would not get assert, no? I might have drink too much beer, so might be wrong, feel free to correct me. :)

[lukefromdc]

Are you saying that overwriting a menu with another menu leaks the old menu and leaves mutliple copies storied in RAM? I could start the process by resetting to NULL if so

[lukefromdc]

As for the origjnal author and assert, when I found nothing broke if the assert was removed, I assumed he ran into trouble with this on GTK2 and the original problem was now long gone

[muktupavels]

Check pd->menu = g_object_ref_sink (panel_context_menu_create (panel)); line! Really, read this:
https://developer.gnome.org/gobject/stable/gobject-The-Base-Object-Type.html#g-object-ref-sink

You can not simply set it to NULL... You need free allocated memory first and only then set it to NULL. Of course if you free old menu before creating new one you don't need to set it to NULL, just free and assign new value. In this case free is gtk_widget_destroy, don't remember, but you might use also g_object_unref...

I put asserts to make sure my expectations are met. For example if I know or expect that variable must be NULL when function is called I put assert. If I would expect that function could be called more then once then I would make sure that old value is freed first.

[TomaszGasior]

In my honest opinion… you can avoid problem by dropping menu items icons. As I wrote to you, maintaining good reimplementation of GtkImageMenuItem will be hard task…

[lukefromdc]

NO, NO, and HELL NO! @TomaszGasior, we are absolutely REFUSING to remove icons from menus. I have put a lot of work into making this user selectable but repeated demands to remove icons with no user ability to bring them back are tempting me to stop work.

I would revert a removal of icons locally, and if I was unable to do that I would refuse to install the "updated" version. I have trouble even using the menus without the icons, noticed that in testing.

We are not going to remove icons from menus, we are not going to remove icons from the desktop, any further discusion of such things is a waste of time and energy. Case closed.

[sc0w]

to make sure we have spaces in the left (the expected in all the gtk applications) you need to do the changes here with something like this:

if ((icon_name) && (g_settings_get_boolean (icon_settings, "menus-have-icons")))
        icon = gtk_image_new_from_icon_name (icon_name, GTK_ICON_SIZE_MENU);
else
        icon = gtk_image_new ();

and...

if ((gicon) && (g_settings_get_boolean (icon_settings, "menus-have-icons")))
        icon = gtk_image_new_from_icon_name (icon_name, GTK_ICON_SIZE_MENU);
else
        icon = gtk_image_new ();

and I think no more changes are needed, if the user needs to restart the panel to apply the changes in the gsettings key, that is the expected behavior for me

and other question, I am not sure if we will have problems with dbus with several clicks (remember mate-desktop/mate-terminal#233)

[lukefromdc]

Icons are staying in menus, its just a question of keeping deprecated code (and permanently swearing off GTK4) or reimplementing the deprecated items to use them to the best of our ability. There is no condition under which they are going to be removed. Just because GNOME doesn't use them does not me we are obilgated to drop them too. The user experience is just as important as code quality, and some of us find menus without icons much harder to use

[TomaszGasior]

@lukefromdc You are misunderstanding my words. You are think I am against MATE, GNOME-like friend. I wanted to say that lack of menu items icons is better than ugly and weak reimplementation of deprecated GTK component. And maybe insetad of wasting time to small 16x16 pxs icons you can move your energy to something more important in MATE, Wayland support for example.
My (small) participation in this discussion and whole MATE desktop now is closed too. Sorry, if someone was hurt by my words.

My last word in this topic: keep in mind that users of other environments may not have MATE"s gsettings schema installed. You should check existence of it when you are reading directly from MATE's gsettings, otherwise errors will be printed in logs. Maybe this problem is not related to mate-panel but it is for pluma, atril and other MATE apps, where you probably copy&paste this poor reimplementation of GtkImageMenuItem.

[muktupavels]

gtk_image_clear should work without rebuilding menu... And GTK3 will not remove deprecated things... And going to GTK4 probably will have bigger problems then icons in menus... :P

[lukefromdc]

I will try gtk_image_clear if I can find a way to run it from the places this function is called. The orignal problem in getting a response to changing the gsettings value is that the entire code to build the menu was run only once, so if gtk_image_clear is in that code it won't be run until the panel is restarted.

[lukefromdc]

As for the need for icons in menus, they are so important to me that I cannot accept removing them no matter what the benefits. Keep in mind, we have been blocking all replacements for deprecated code that remove icons anywhere unless the icon loading is reimplemented. GTK3 won't remove these items, and we can choose never to support GTK4 if necessary.

Thus this becomes a choice between keeping deprecated code and reimplementing deprecated code. Meanwhile I am wasting time reminding you that icons are not coming out of menus when I could be trying better reimplementation ideas.

[muktupavels]

Well you can just hide all images, no? By using simple g_settings_bind call when menu item is created. You can even provide your own binding function and do whatever you want:
https://developer.gnome.org/gio/stable/GSettings.html#g-settings-bind-with-mapping

[lukefromdc]

right now I am experimenting with two different options. In both cases the rebuilding of menus is scrapped and those files reverted to master. If I use Albert's suggestion to bind visibility to gsettings, the result is good dynamic application of the gsettings preference(no need to restart the panel) but when the icons are not shown the space for them is removed, as the allocated space of a non-visible GtkWidget is zero. Alternately, I can use the same code as I've used in Caja where either an empty image or the icon is loaded. That gives a consistant space with or without icons being loaded, but a change is not applied until the panel is restarted. Got a bit more space when empty by packing the icon into a box, trying to set a minimum size on the box without having to resort to cssproviders, as one would then be loaded on every menuitem and that is heavy.

Trying to find a way to preserve the space and apply changes immediately

[raveit65]

@TomaszGasior
Please stop to post your personal opinions about using icons or not here.
This is a technical topic, feel free to discuss this in you own report.

[TomaszGasior]

@raveit65 You didn't understand what I said.

[lukefromdc]

Finally found a way to make it work and force-pushed.

First bind the icon's visiblity to "menus-have-icons" though that sets a width and height of zero when the widget(the icon) is not visible. Because of that, pack the icon into a box of its own, and set a 16px min-width on that with css added to the existing global mate-panel.css file and a style class on the icons.

This applies changes in "menus-have-icons" immediately, and so long as the menus are are showing 16px icons will keep the same spacing between the label and the left edge as with or without the icons. The space would grow for a larger icon but will never shrink below this.

Results this time around consistant with what I got in Caja with mate-desktop/caja@76c2a2a but code was more complex due to these menus not being remade each time they are popped up. The point here is to make the gsettings preference behave as users expect, and to render the menus the way they are supposed to be rendered when the icons are not shown by user choice.

[raveit65]

Works well.

[sc0w]

for some reason, there is no space in the left here: (hardware, internet and network ...)

EDIT: it works fine with mate-panel 1.20.1

EDIT2: culprit commit 8670151

[lukefromdc]

The issue with the "preferences" item occurs both with this PR and also with mate-panel 1.21.1 as released. Thus, in between those was another menitem deprecation fix, affecting main menus which this PR did not touch. Also link to the control center in the "system" menu still gets its icon, I will have to find that one.

[muktupavels]

I still don't get why you need to create multiple GSettings objects. If you have 100 menu items then you create 100 GSettings objects... Why? You need only one, no?

These extra / uneeded objects will cause more memory usage, also menu creation time will be slower for no reason. Just because computers these days are fast and has huge memories does not mean that you should ignore these things.

If you don't want pass extra parameter to your helper functions you could do something like this (untested ofcourse):

static GSettings icon_settings = NULL;

static void
ensure_icon_settings (void)
{
  if (icon_settings != NULL)
    return;

  icon_settings = g_settings_new ("org.mate.interface");

  panel_cleanup_register (panel_cleanup_unref_and_nullify,
&icon_settings);
}

Then in helper functions call ensure_icon_settings before using g_settings_bind. Now GSettings object should be created once and destroyed when panel will exit.

[lukefromdc]

@muktupavels , your change works. I had to do a little tuning on it but once I changed the initial declaration of icon_prefs from gsettings to *gsettings like in the previous code and added panel-cleanup.h to the includes it worked.

I then matched the indents to the rest of the file, gave you credit for the basic code, and pushed it. I figure you probably don't have a mate build environment set up so you know what works on gnome-panel but can't directly test on mate-panel.

Anyway, these hints are quite helpful. Out of curiosity, how much RAM does a typical boolean gsettings object take? The boolean variable itself would need only one byte (can't use just one bit), but there's the rest of the object to consider. Multiply by the worst case for maximum number of menuitems and that's the RAM savings.

Many guides to programming promote limiting the scope of variables and avoiding globals, but in this case that advice seems to be wrong.

EDIT: I would think that as the menu items are created with the previous code, the multiple gsettings objects would be created, used, and then destroyed one at a time unless the g_object_unref didn't end up actually freeing them. More code to run and CPU usage on opening a menu but at least not all of them in RAM at once.

[muktupavels]

@muktupavels , your change works. I had to do a little tuning on it but once I changed the initial declaration of icon_prefs from gsettings to *gsettings like in the previous code and added panel-cleanup.h to the includes it worked.

I did not expect that it will work by simple copy&paste, I just gave idea...

I then matched the indents to the rest of the file, gave you credit for the basic code, and pushed it. I figure you probably don't have a mate build environment set up so you know what works on gnome-panel but can't directly test on mate-panel.

Not only I don't have MATE build environment, I also don't have any MATE package/module installed... :P I don't know if this works in gnome-panel... We don't have icons in normal/standard menus and in menu-bar / main-menu applets icons are always visible. Also in gnome-panel I don't use GtkBox to replace GtkImageMenuItem, it simply does not give you same result... I have GpImageMenuItem that is supposed to give same/similar result.

Anyway, these hints are quite helpful. Out of curiosity, how much RAM does a typical boolean gsettings object take? The boolean variable itself would need only one byte (can't use just one bit), but there's the rest of the object to consider. Multiply by the worst case for maximum number of menuitems and that's the RAM savings.

I don't know, but clearly 100 objects require more memory than 1 object. If you are interested, then just measure memory usage and/or time needed to create new objects.

Many guides to programming promote limiting the scope of variables and avoiding globals, but in this case that advice seems to be wrong.

I prefer to not use globals and in this case it is limited only to this file. If you would want then you could avoid this... This is not only option. :)

EDIT: I would think that as the menu items are created with the previous code, the multiple gsettings objects would be created, used, and then destroyed one at a time unless the g_object_unref didn't end up actually freeing them. More code to run and CPU usage on opening a menu but at least not all of them in RAM at once.

And how bindings works? If that object would have been destroyed then how would you get notifications that setting has changed? Yes, you dropped your own reference, but in this case it is not destroyed because there is extra reference added creating binding. From documentation:

Note that the lifecycle of the binding is tied to object

In this case it means that there is extra ref added:
https://gitlab.gnome.org/GNOME/glib/blob/master/gio/gsettings.c#L2862