mate-desktop / mate-panel

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

wnck-pager: update workspace switcher aspect ratio #1417

Closed raveit65 closed 1 year ago

raveit65 commented 1 year ago

This fixes https://github.com/mate-desktop/mate-panel/issues/1230 and is needed because of an update in libwnck https://gitlab.gnome.org/GNOME/libwnck/-/commit/3456b747b6381f17d48629dd8fdd4d511e739b10

In fedora i could workaround this issue with reverting the libwnck commit. But it seems that xfce users had problems with the xfce-panel because of reverted libwnck commit. See https://gitlab.xfce.org/xfce/xfce4-panel/-/issues/630#note_80841 and https://bugzilla.redhat.com/show_bug.cgi?id=2242944 I offered a test RPM without the mentioned patch to confirm the xfce issue. But we should really try to fix the issue with wnck-pager from our side after 2 years. It is urgent!

As a downside this PR overwrites https://github.com/mate-desktop/mate-panel/pull/806 which fixed 2 issues. I can't reproduce https://github.com/mate-desktop/mate-panel/issues/799 with PR anymore, but unfortunately https://github.com/mate-desktop/mate-panel/issues/797 still exists with PR.

@mate-desktop/core-team Please help to fix https://github.com/mate-desktop/mate-panel/issues/797 on top of this PR If we find no solution the only option is to use PR plus disable applet in-process build for fedora and all other distributions which use libwnck >= 40.

raveit65 commented 1 year ago

I built 1.26 branch with this PR and i got same results when testing previous issues. I am unable to reproduce https://github.com/mate-desktop/mate-panel/issues/799 with un-expanded panel (tested bottom panel), but windows-list applet issue with un-expanded panel still exists https://github.com/mate-desktop/mate-panel/issues/797.

raveit65 commented 1 year ago

When building the panel out-of-process issue https://github.com/mate-desktop/mate-panel/issues/797 doesn't exists \o/ If i have to remove https://src.fedoraproject.org/rpms/libwnck3/blob/rawhide/f/libwnck_0001-Revert-pager-do-not-change-workspace-size-from-size_.patch from fedora libwnck3 RPM i will switch mate-panel back to out-of-process build, ..............good bye wayland.

lukefromdc commented 1 year ago

Using return GTK_SIZE_REQUEST_CONSTANT_SIZE; unconditionally in mate_panel_applet_get_request_mode makes the unexpanded panel work again, but brings back the switcher aspect ratio problem. If we could look for MATE_PANEL_APPLET_EXPAND_MAJOR (used only by the window list) and apply that for that condition, we should have a fix however hacky

lukefromdc commented 1 year ago

Also, on restarting the panel, the window list on a nonexpanded in-process panel appears for a fraction of a second before disappearing when the panel is rerendered, possibly for the first time.

cwendling commented 1 year ago

This only fixes the workspace selector aspect ration in-process, right?

For the non-expanded issue, there seem to be a problem in the panel->packed path of panel_widget_size_allocate(). If you comment this out:

diff --git a/mate-panel/panel-widget.c b/mate-panel/panel-widget.c
index 665fdfab..95016be1 100644
--- a/mate-panel/panel-widget.c
+++ b/mate-panel/panel-widget.c
@@ -1514,7 +1514,7 @@ panel_widget_size_allocate(GtkWidget *widget, GtkAllocation *allocation)
    else
        panel->size = allocation->height;

-   if (panel->packed) {
+   if (FALSE /*panel->packed*/) {
        /* we're assuming the order is the same as the one that was
         * in size_request() */
        int applet_using_hint_index = 0;

then it mostly works -- yet I don't know the actual implications. But it's clearly not perfect, as it'll allocate take the expected room for the applets, not taking into account their position: e.g. if you put the tasklist on the far right of an unexpanded panel, the panel will be sized as if it was visible, but it's actually basically invisible because it has 0 room before the end of the panel.

I expect the actual issue being a problem computing the minimal size given to the applet when allocating the child, although it seems to be computing the size correctly when it sizes the panel.

If we could look for MATE_PANEL_APPLET_EXPAND_MAJOR (used only by the window list) and apply that for that condition, we should have a fix however hacky

That would indeed be a gross hack, but well, I don't think we're pass that just yet. Unfortunately, it doesn't seem to work; if I apply the following it doesn't help (possibly it's not set early enough):

diff --git a/libmate-panel-applet/mate-panel-applet.c b/libmate-panel-applet/mate-panel-applet.c
index 0830569f..5c604fe6 100644
--- a/libmate-panel-applet/mate-panel-applet.c
+++ b/libmate-panel-applet/mate-panel-applet.c
@@ -1138,7 +1138,7 @@ mate_panel_applet_get_request_mode (GtkWidget *widget)

    priv = mate_panel_applet_get_instance_private (applet);

-   if (priv->out_of_process)
+   if (priv->out_of_process || priv->flags & MATE_PANEL_APPLET_EXPAND_MAJOR)
        return GTK_SIZE_REQUEST_CONSTANT_SIZE;

    orientation = mate_panel_applet_get_orient (applet);

Another workaround could be try the intermediate wrapper for the desktop switcher that Alberts pasted somewhere in one of those reference issues. I didn't try it, but if he says it works, I'm pretty sure it does; and it's a more localized hack.

Of course, the best fix would be to figure out where the layout code is messing up… but that code is far from trivial.

raveit65 commented 1 year ago

This only fixes the workspace selector aspect ration in-process, right?

Yes, this is your code from https://github.com/mate-desktop/libmatewnck/pull/4#issuecomment-1651392278

lukefromdc commented 1 year ago

Just tested using

-   if (panel->packed) {
+   if (FALSE /*panel->packed*/) {

and yes, that gives me both a (semi-) functional window list on a non-expanded panel (looking at it right now) and the proper workspace aspect ratio. unfortunately, if I started opening a lot of new windows to use more space after putting the classic menu to the right of the window list to use up some space, the space needed for the buttons was allocated right of the classic menu applet, and the entire window list moved right too, compressing existing buttons and pulling the whole panel end to the right too.

I doublechecked that in the nonexpanded panel, the "packed" case is what's normally used by removing that block entirely, again getting the above results with only the "not packed" case available in the code. This confirms that whatever is allocating zero space to show the buttons after expanding the panel to take them is in or affected by that block of code.

Note the comment in the code:

        /* we're assuming the order is the same as the one that was
         * in size_request() */
lukefromdc commented 1 year ago

This block starting at line 1537 in panel-widget.c I just proved to be the problem

                if (ad->expand_major && ad->size_hints) {
                    int width = panel->applets_using_hint[applet_using_hint_index].size;
                    applet_using_hint_index++;
                    challoc.width = MIN (width, allocation->width - i);
                }

challoc.width is somehow coming up as zero, if I replaceMIN (width, allocation->width - i); with something like 700, I get quite normal window list behavior on that nonexpanded panel, other widgets don't get pushed off it, and the window buttons compress when space becomes limited for them.

EDIT: I have proven width to be the offending variable, for some reason int width = panel->applets_using_hint[applet_using_hint_index].size; is returning zero if I manually set width to 750 for test purposes only, again the buttons behave normally: getting allocated 750 px, but since space is limited the window list applet shrinks them down to fit.

If I replace MIN with MAX I get huge window buttons, if I use challoc.width = width the buttons disappear if I use challoc.width = (allocation->width - i); I again get overexpanded buttons pushing all else off the panel

This is the horizontal panel case only of course, and is restricted to the window list on a packed panel. We need to find out why

lukefromdc commented 1 year ago

Also note that you left the additional g_print debugging statements in this PR.

These are good for debugging but log spam for users if they get to git master or to a release.

I used a g_message statement to prove that width is coming up zero but had to remove the g_print statements to be able to find the output when running mate-panel --replace in terminal

lukefromdc commented 1 year ago

I added my own debugging code to the block in question to see what we are getting back. Making the block in question

                if (ad->expand_major && ad->size_hints) {
                    int width = panel->applets_using_hint[applet_using_hint_index].size;
                    applet_using_hint_index++;
                    challoc.width = MIN (width, allocation->width - i);
                    g_message("width =");
                    g_message("%i", width);
                    g_message("allocation->width - i =");
                    g_message("%i", (allocation->width - i));
                }
and running with a second, non-expanded top panel with the window list and the classic menu to the right of it, I got this result w 8 window buttons in the bottom panel's window list (also used in this test):

Message: 00:04:29.881: width = Message: 00:04:29.881: 11 Message: 00:04:29.881: allocation->width - i = Message: 00:04:29.881: 502 Message: 00:04:29.947: width = Message: 00:04:29.947: 11 Message: 00:04:29.947: allocation->width - i = Message: 00:04:29.947: 502 Message: 00:04:30.102: width = Message: 00:04:30.102: 0 Message: 00:04:30.102: allocation->width - i = Message: 00:04:30.102: 502 Message: 00:04:30.122: width = Message: 00:04:30.122: 0 Message: 00:04:30.122: allocation->width - i = Message: 00:04:30.122: 502 Message: 00:04:30.374: width = Message: 00:04:30.374: 1348 Message: 00:04:30.374: allocation->width - i = Message: 00:04:30.374: 1599 Message: 00:04:30.391: width = Message: 00:04:30.391: 0 Message: 00:04:30.391: allocation->width - i = Message: 00:04:30.391: 1599 Message: 00:04:30.408: width = Message: 00:04:30.408: 0 Message: 00:04:30.408: allocation->width - i = Message: 00:04:30.408: 1599 Message: 00:04:34.675: width = Message: 00:04:34.675: 0 Message: 00:04:34.675: allocation->width - i = Message: 00:04:34.675: 1599 Message: 00:04:34.703: width = Message: 00:04:34.703: 0 Message: 00:04:34.703: allocation->width - i = Message: 00:04:34.703: 1599 Message: 00:05:14.986: width = Message: 00:05:14.986: 0 Message: 00:05:14.986: allocation->width - i = Message: 00:05:14.986: 1599 Message: 00:05:15.012: width = Message: 00:05:15.012: 0 Message: 00:05:15.012: allocation->width - i = Message: 00:05:15.012: 1599

raveit65 commented 1 year ago

I dropped the debug code blog.

raveit65 commented 1 year ago

I found another issue when building mate-panel in-process and wnck-pager commit from PR. Last icon (ltr) in na-tray is cropped :/ With this commit: na-tray-cropped Without this commit: na-tray-normal

It doesn't matter which icon is at last position (ltr). An out-of-process panel build is not affected by this problem.

raveit65 commented 1 year ago

Another workaround could be try the intermediate wrapper for the desktop switcher that Alberts pasted somewhere in one of those reference issues. I didn't try it, but if he says it works, I'm pretty sure it does; and it's a more localized hack.

You mean that post? https://github.com/mate-desktop/mate-panel/issues/1230#issuecomment-1046235088

lukefromdc commented 1 year ago

I can confirm the issue with the tray. I looked at the wrapper, only the core of the suggested code is written there. Looks like whatever we do must be limited to the switcher only

lukefromdc commented 1 year ago

Some of the g_print debug code is still in the latest commits, line 1453 in panel-widget.c is one example. Also I am assuming all of applet_debug_name is only used for this

raveit65 commented 1 year ago

Done, i removed the rest of the debug code.

raveit65 commented 1 year ago

And all of applet_debug_name is removed.

lukefromdc commented 1 year ago

Thanks,I will be playing with this over the next few days trying to find a fix that works

lukefromdc commented 1 year ago

In playing with this, I've noticed something interesting: while the last icon in the tray always gets its right edge cut off, the seemingly random cutting (partial rendering in reduced space) of other icons such as the force-quit and inhibit buttons on restarting the panel is entirely stopped by this. Testing in x11 gives the results decribed above:

Testing in Wayland was interesting: The last icon in the tray (an indicator both here and in x11 on my setup) was cut as in X11, but the unexpanded panel showed the window list buttons! Instead of the panel expanding to take more of them, if more than one button was to show when the panel started text was ellipsized, and the space taken by those initial buttons was all that the applet would ever get, but all of the window list buttons showed up. The backend code is totally different of course, so querying the applet's size gets different results. Also the nonexpanded panel always renders on the left side of a wayland screen

raveit65 commented 1 year ago

Well, you changed the code for windows-list applet (wayland) for yourself in summer as far as i can remember.

2 xfce user did confirm that the xfce-panel crash was gone with vanilla libwnck in fedora https://bugzilla.redhat.com/show_bug.cgi?id=2242944#c5 So i have to remove https://src.fedoraproject.org/rpms/libwnck3/blob/rawhide/f/libwnck_0001-Revert-pager-do-not-change-workspace-size-from-size_.patch from fedora rpm. I will switch mate-panel to out-of-process build for the moment.

lukefromdc commented 1 year ago

Oh yeah-the wayland window list has to be complteley differenr becsuse libwnck is X11 only. Thankfully wlroots supports gtk-layer-shell which handles passing the window coordinates and name between the compositor and the panel.

Some of those icon cutting issues date all the way back to GNOME 2. If necessaey we could see distros building everything out-of-process and specialty repos (e.g old sryle Ubuntu PPA's) offering Wayland builds.

The x11 codepath is long enough that this PR mostly working in wayland only narrows things down a little. The tray icon cutting may or may not be the same issue.

raveit65 commented 1 year ago

I can provide an in-process build for wayland testers at fedora coprs cloud https://copr.fedorainfracloud.org/coprs/ But i hope that issues are fixed before a 1.28 release. @mate-desktop/core-team @cwendling @vkareh @yetist @zhuyaliang Please help....

lukefromdc commented 1 year ago

Given just how long we have had applet cutting issues (and GNOME 2 before us) I am wondering if there is anyone left who fully understands the applet code and these two applets.

cwendling commented 1 year ago

@raveit65 @lukefromdc can you give #1420 a try? It's based on the hack suggested by Alberts, and it seems to work a lot better than what we have here, as well as being contained in the pager applet.

It's definitely a hack, but I'm more comfortable with a working, low-impact hack than a proper fix that doesn't really work and has unknown implications :slightly_smiling_face:

lukefromdc commented 1 year ago

I just confirmed that the changes in mate-panel-applet.c in this are themselves sufficient to kill the window list in a nonexpanded panel, while the changes in panel-widget.c are needed to fix the switcher

lukefromdc commented 1 year ago

Better fix merged in

1421