search

mate-desktop / mate-panel

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

wncklet: center preview windows when grouping is enabled #1344

Closed balazs-endresz closed 1 year ago

balazs-endresz commented 1 year ago

When grouping is enabled all tasklist buttons are siblings of each other but the grouped ones shouldn't contribute to the width. These always have 1px height and width and x/y is -1 only when the group is not open. I'm not sure if there's a better way to distinguish these.

Also, annoyingly when grouping is enabled and the mouse is moved from left to right, and you stop over the very first pixel of the next button the preview will be positioned incorrectly above the previous button. This doesn't happen for every button, and I haven't been able to figure out why. I tried to detect the hover state to get around this with GTK_STATE_FLAG_PRELIGHT but gtk_widget_get_state_flags (children->data); returns a value of GTK_STATE_FLAG_CHECKED for every button all the time.

balazs-endresz commented 1 year ago

Grouped items didn't have previews before either. I've checked with an earlier version to be sure. There's also a comment referring to this: https://github.com/mate-desktop/mate-panel/blob/43837da1f4c25438381a6efb26620c4e7baf5598/applets/wncklet/window-list.c#L424

lukefromdc commented 1 year ago

I just managed to confirm the 1px/wrong button issue

lukefromdc commented 1 year ago

We still get a preview opening over the previous button if that one is grouped windows and the mouse is on the first px of the next button some of the time, but not all of the time.

lukefromdc commented 1 year ago

With two window buttons on the panel, first one being a group I was able to invoke misalignment about half the time. THis is in a bottom panel, left panel seems OK. Worst issue was with putting the pointer in the top or bottom corner of the next button, but this was not the only time it was misaligned

lukefromdc commented 1 year ago

Also is seems (from presumably pre-existing code) that grouped windows always are listed first in the taskbar window list, so we are always dealing with the grouped window button first case. That suggests using greater of width of button/width of previous button as the offset in the case of a button creating a preview after a grouped window.

lukefromdc commented 1 year ago

Looks like part of the problem may for some reason using if (alloc.width == 1 || alloc.height == 1) as the test case (line 369), perhaps it sometimes comes up as zero? I switched to if (alloc.width < 2 || alloc.height < 2) and the misalignment now occurred only in putting the mouse in the top or bottom corner of the next button after a grouped button and only with no extra space on the part of the panel usable by the window list

balazs-endresz commented 1 year ago

Changing it to < 2 didn't help for me and I've already tried adding some offsets earlier. But I think I know what the problem is now: the buttons are not ordered correctly in gtk instpector, and neither do we iterate over them according to their actual position here. This seems to be a problem only with grouping because the grouped buttons are added at the end of the list. Also, each button can vary in width by a few pixels.

lukefromdc commented 1 year ago

We probably have different screen resolutions, preview width settings (200px here), CPU speeds etc. Lots of things can act different

lukefromdc commented 1 year ago

What about creating some kind of dummy preview for grouped windows so the space is filled? That might be the only thing that works

balazs-endresz commented 1 year ago

Adding previews within groups is not something I want address here, perhaps in another PR later.

I don't see how adding dummy previews would help either.

But I did just rewrite this to be based on the correct button offset ordering, which fixed the off by one issue in grouped mode, with some exceptions:

  1. You need to add/remove a window after toggling grouped mode, but it will work fine afterwards.
  2. Grouped mode now consistently has an off by one issue with 2x scaling for every button. Adding a constant offset in either direction didn't help. Maybe scaling needs to be factored in at a few more places, I'm not sure.

(All the list manipulation code is from chatgpt, I don't know if there's a more concise way to do those things.)

balazs-endresz commented 1 year ago

Actually the off by one issue is present with 2x scaling with both the first and second commit, and in grouped/non-grouped mode too.

Would it be maybe better if we just used the mouse position when scaling is enabled? That will be still more accurate than before (because of the changes in the previous PR).

balazs-endresz commented 1 year ago

if we just used the mouse position when scaling is enabled

When I do that with 2x scaling then I can see that the preview doesn't move at all when going back and forth 1px between two buttons. So this seems to me like a more general scaling related issue, and probably not something we can fix here. If wncklib passed the current button widget in the enter-notify event then all this would be a lot easier. I've seen a wncklib ticket a while ago asking for exactly that but they decided not to do it. Although that didn't mention the scaling issue, but still I suspect they wouldn't want to add another event just for this.

Anyway, I guess the main question is: shall I push some changes to use the mouse position (instead of centering) when scaling is enabled? That should be still less bad than the random positioning it used to have before, and not as jarring as the occasional off by one issue.

lukefromdc commented 1 year ago

That would be OK by me, I would advise documenting this in the code in case we get future bug reports about it.

On 12/26/2022 at 2:12 PM, "Balazs Endresz" @.***> wrote:

if we just used the mouse position when scaling is enabled

When I do that with 2x scaling then I can see that the preview doesn't move at all when going back and forth 1px between two buttons. So this seems to me like a more general scaling related issue, and probably not something we can fix here. If wncklib passed the current button widget in the enter-notify event then all this would be a lot easier. I've seen a wncklib ticket a while ago asking for exactly that but they decided not to do it. Although that didn't mention the scaling issue, but still I suspect they wouldn't want to add another event just for this.

Anyway, I guess the main question is: shall I push some changes to use the mouse position (instead of centering) when scaling is enabled? That should be still less bad than the random positioning it used to have before, and not as jarring as the occasional off by one issue.

-- Reply to this email directly or view it on GitHub: https://github.com/mate-desktop/mate-panel/pull/1344#issuecomment- 1365214503 You are receiving this because you commented.

Message ID: <mate-desktop/mate- @.***>

lukefromdc commented 1 year ago

Also note it works fine for me with 2s scaling so long as grouping windows is NOT enabled, so if you also get this result, we can limit use of the mouse position to the case of 2x (greater than 1x in case we ever have fractional scaling) window scaling AND grouping enabled.If you are getting previews over the wrong button WITHOUT scaling greater than 1x even with window grouping disabled then I would recommend using the mouse position whenever window scaling > 1x

On 12/26/2022 at 2:12 PM, "Balazs Endresz" @.***> wrote:

if we just used the mouse position when scaling is enabled

When I do that with 2x scaling then I can see that the preview doesn't move at all when going back and forth 1px between two buttons. So this seems to me like a more general scaling related issue, and probably not something we can fix here. If wncklib passed the current button widget in the enter-notify event then all this would be a lot easier. I've seen a wncklib ticket a while ago asking for exactly that but they decided not to do it. Although that didn't mention the scaling issue, but still I suspect they wouldn't want to add another event just for this.

Anyway, I guess the main question is: shall I push some changes to use the mouse position (instead of centering) when scaling is enabled? That should be still less bad than the random positioning it used to have before, and not as jarring as the occasional off by one issue.

-- Reply to this email directly or view it on GitHub: https://github.com/mate-desktop/mate-panel/pull/1344#issuecomment- 1365214503 You are receiving this because you commented.

Message ID: <mate-desktop/mate- @.***>

balazs-endresz commented 1 year ago

I checked again and I still got the scaling issue in non-grouped mode as well (both in a VM and on my laptop, and after opening a new window), so just disabled centering for scaling in general. I've added more comments and some minor formatting tweaks, so this might just be ready now.

lukefromdc commented 1 year ago

That works, centering works with window-scaling=1 and is disabled with window-scaling=2 on account of being buggy. More work w the scaling code in the applet probably needed if this is to be made to work with scaling

lukefromdc commented 1 year ago

Merged, the "off by one" bug could not be allowed to reach any future release

muktupavels commented 1 year ago

Merged, the "off by one" bug could not be allowed to reach any future release

Maybe you should not rush with merging in first place? There is one more thing that should be fixed in previous merge request...

lukefromdc commented 1 year ago

What is the "one more thing" you have identified?

Everyone, please mark any "do not merge quickly" PR's with "WIP" as otherwise we have no way whatsoever of knowing your intentions. Note that worst case we can revert if this turns out to be a problem.

We have a small team, not enough testers, and a lot of PR's sit for a long time without ever being reviewed at all. We can ALWAYS use more help with reviewing these, as that brings in more test cases and more eyes on code with different types of experience

In this particular PR, we are simply excluding the prior change from HiDPI (window-scaling=2) cases