mate-desktop / mate-panel

MATE panel
https://mate-desktop.org
GNU General Public License v2.0
185 stars 118 forks source link

Fix crashes caused by MateImageMenuItem vs GtkImageMenuItem conflict #1434

Closed raveit65 closed 8 months ago

raveit65 commented 9 months ago

This reverts commit 675f72ff302409dcbfb1991cf56fd09a294039bf.

First fix for https://github.com/mate-desktop/mate-panel/issues/1433

raveit65 commented 9 months ago

Are you sure? I asked for that here https://github.com/mate-desktop/mate-panel/issues/1433#issuecomment-1970773847 I think https://github.com/mate-desktop/mate-panel/commit/ebb3a795a351ca600eb539ed2bba013a6533d749 is independent from GType issue.

lukefromdc commented 9 months ago

Keeping https://github.com/mate-desktop/mate-panel/commit/ebb3a795a351ca600eb539ed2bba013a6533d749 by itself doesn't actually USE MateImageMenuItem for the offending compact menu items, thus the missing icons on the compact menu. What https://github.com/mate-desktop/mate-panel/commit/675f72ff302409dcbfb1991cf56fd09a294039bf actually did was to redefine these menu items to use our code, without it nothing is used for these two icons.

raveit65 commented 9 months ago

My results are different. I only need to revert this commit to fix the crash, which you never couldn't reproduce. In this post https://github.com/mate-desktop/mate-panel/issues/1433#issuecomment-1970664924 @cwendling did only speak about reverting this commit. Anyway, feel free to close this PR if you think it is wrong. As is said before, i fixed this already for fedora........so i have no rush and can wait.

lukefromdc commented 9 months ago

The other commit is not the reason for the crash but rather the reason for the missing icons when used by itself. I am not going to close this now, because I think a proper technical fix keeping our replacement function is possible when used w the mate-desktop change. I will have to drill down into that, hopefully tonight.

lukefromdc commented 9 months ago

Pretty sure the mission icons were ONLY in the compact menu BTW

raveit65 commented 9 months ago

Which missing icons? I have no idea you're talking about without a report about missing icons. I don't see any missing icons with using https://github.com/mate-desktop/mate-panel/commit/ebb3a795a351ca600eb539ed2bba013a6533d749

lukefromdc commented 9 months ago

Places and System menu items in compact menu below Missing_Icons

lukefromdc commented 9 months ago

OK, I will work on this and get it fixed right

lukefromdc commented 9 months ago

I just found that https://github.com/mate-desktop/mate-panel/commit/675f72ff302409dcbfb1991cf56fd09a294039bf was incomplete, in panel-menu-items.h we are still using gtk_image_menu_item and not mate_image_menu_item, I am surprised this ever worked, anywhere. Preparing a PR to fix that, will test against my menuitem drag to panel crash in the compact menuy

lukefromdc commented 9 months ago

Reopening for further development. Plan is to pack the icons into those two menuitems within the panel code instead of relying on MateImageMenuItem, as a workaround for the fact that MateImageMenuItem is declared as "final," cannot be subclassed, and thus won't work as a replacement for GtkImageMenuItem in this particular case.

lukefromdc commented 9 months ago

In testing, this shows the "Places" and "System" icons in the compact menu and does not crash on dragging a program icon from the compace menu to the panel. Needs a mate-panel --replace test vs 1.28.0 release on a system where that crashes

lukefromdc commented 9 months ago

I had to copy some code from menu.c to make this work as a strictly static (local to one file) function. This and using deprecated GtkImageMenuItem are a workaround for 1.28 since we should not change library dependencies. When we move on to the 1.29 development branch we can revert this, not declare MateImageMenuItem as final in mate-desktop, and use MateImageMenuItem in these two places where it has to be subclassed but now cannot be.

If others want, I can mark the places in panel-menu-items.c and the two places in panel-menu-items.h where we use GtkImageMenuItem as FIXME, and same for the two functions added to panel-menu-items.c to support this case.

lukefromdc commented 8 months ago

We can close this in favor of https://github.com/mate-desktop/mate-desktop/pull/603 and a panel PR to replace GtkImageMenuItem with MateImageMenuItem and include libmate-desktop/mate-image-menu-item.h in mate-panel/panel-menu-items.h if we decide to allow a breaking change just after the brand-new 1.28 release, or we can keep this fix for 1.28, bump versions, and use those for 1.29 to avoid a breaking change in 1.28.0. Deeming this not my choice to make

zhuyaliang commented 8 months ago

If we accept it https://github.com/mate-desktop/mate-desktop/pull/603 We can continue to use MATE_TYPE_IMAGE_MENU_ITEM.No need to reverts commit

cwendling commented 8 months ago

As said in https://github.com/mate-desktop/mate-desktop/pull/603#issuecomment-1980629626, I don't really like how this is growing, and would rather merge https://github.com/mate-desktop/mate-desktop/pull/603 and depend on it (that is, update the dependency and use the right type as the parents in the structure).

raveit65 commented 8 months ago

Can be closed or not?

lukefromdc commented 8 months ago

Fine with closing this so long as we are OK with depending on a new mate-desktop version