search

mate-desktop / marco

MATE default window manager
https://mate-desktop.org
GNU General Public License v2.0
196 stars 87 forks source link

Remove obsolete gdk_window_process_all_updates() #698

Open tormodvolden opened 2 years ago

tormodvolden commented 2 years ago

The first commit "Always include config.h before util.h" is independent but included here for simplicity.

I am not sure if meta_frames_repaint_frame() is useful at all at this point, or it might be excised as well. Just let me know.

tormodvolden commented 2 years ago

I added a third commit, also independent, but on the same topic of replacing deprecated functions.

raveit65 commented 2 years ago

Are you sure gdk_window_process_all_updates (); can simply be removed? See solution from metacity. https://gitlab.gnome.org/GNOME/metacity/-/commit/4613a803ea33f2f938f498d5ec272ca8f8852209

raveit65 commented 2 years ago

Marco has a keybinding to activate the window menu, alt+space. See mate-keybinding-properties. This is broken with your commit. Beside from this issue I am thinking it's better to use gtk_menu_popup_at_rect () to avoid that the menu opens somewhere on the screen when mouse-pointer isn't on window and using the keybinding. See metacity commit https://gitlab.gnome.org/GNOME/metacity/-/commit/37fa0d19f35a7fbff3638a6e6deb108500d41abf

tormodvolden commented 2 years ago

Are you sure gdk_window_process_all_updates (); can simply be removed?

No, not at all. This is just based on the gtk documentation, that goes along lines that the function is deprecated because doing this explicitly is not needed any longer because gtk takes care of this for you now. And from my cursory testing it seemed fine. However, there might be gtk bugs or we are doing something of a corner case so it is not automatic.

I find it poor form from metacity patching it this way without explanation or a reference to a gtk bug.

tormodvolden commented 2 years ago

Marco has a keybinding to activate the window menu, alt+space. See mate-keybinding-properties. This is broken with your commit.

Oops, I can confirm this. I didn't test this, wasn't aware of that shortcut, sorry about that.

Beside from this issue I am thinking it's better to use gtk_menu_popup_at_rect () to avoid that the menu opens somewhere on the screen when mouse-pointer isn't on window and using the keybinding.

Indeed, at_pointer is too simplistic in this case.

See metacity commit https://gitlab.gnome.org/GNOME/metacity/-/commit/37fa0d19f35a7fbff3638a6e6deb108500d41abf

I understand I should look there next time :) Unfortunately a lot of code is needed, whereas I optimistically thought we could simplify a lot. Can we just cherry-pick that commit instead maybe?

I was basically on a journey to get rid of all warnings before doing anymore else on the code. But I can live with warnings if we know why we need to stick with them. BTW I made a shot on getting rid of gtk_image_menu_item_set_image() as well, but I am stuck with the icons being aligned with the text of the other menu items instead of showing up in the left hand margin.

tormodvolden commented 2 years ago

I pushed the gtk_image_menu_item_set_image() work as well in case you have a chance to look at it and tell me what is wrong. It is pretty much based on the gtk documentation on how to replace the old function.

raveit65 commented 2 years ago

I understand I should look there next time :) Unfortunately a lot of code is needed, whereas I optimistically thought we could simplify a lot. Can we just cherry-pick that commit instead maybe?

If cherry-picking works , why not..... I pointed you to this commit to give you an idea

tormodvolden commented 2 years ago

If cherry-picking works , why not..... I pointed you to this commit to give you an idea

It will need some manual tweaking, I will give it a try later.

muktupavels commented 2 years ago

I find it poor form from metacity patching it this way without explanation or a reference to a gtk bug.

What explanation you want? That commit just silences deprecation warning. Function is deprecated but is not going to be removed from GTK 3. Also no idea why there would be reference to GTK bug.

Repaint was added in https://gitlab.gnome.org/GNOME/metacity/-/commit/912afb6e6bc029e4be99f2d145399c63a5a88a80, but it does not have explanation why it was needed and/or link to bug with more info. And https://gitlab.gnome.org/GNOME/metacity/-/commit/947e45d27dcacae98f97630e25cc976b98b400cb commit changed gdk_window_process_updates to gdk_window_process_all_updates. https://bugzilla.gnome.org/show_bug.cgi?id=141813

tormodvolden commented 2 years ago

Explanation about why sticking to deprecated function and silencing the warning. According to gtk docs the function is not needed any longer, so if metacity has found a reason they need it, they should file a bug or try fix it in gtk. Thanks for the extra pointers.

muktupavels commented 2 years ago

Explanation about why sticking to deprecated function and silencing the warning.

Because I like to build with -Werror and deprecated function is not problem until there will be plan to move to GTK 4.

According to gtk docs the function is not needed any longer, so if metacity has found a reason they need it, they should file a bug or try fix it in gtk. Thanks for the extra pointers.

Sure, it is not used by GTK anymore and normal applications does not need it. Deprecation commit - https://gitlab.gnome.org/GNOME/gtk/-/commit/6da8cbc87e5ff442301da9c78b2049e6fa06fee2.

But if you have checked comments in metacity/marco you would see that deprecated function is used to redraw immediately.

lukefromdc commented 2 years ago

I suspect anytime someone adds code to force an immediate redraw its to get around a problem. This is how we got (and fixed) things like panel applets that didn't resize with user-set changes in panel height/width until the panel was restarted or something else forced a resize.

tormodvolden commented 2 years ago

Because I like to build with -Werror and deprecated function is not problem until there will be plan to move to GTK 4.

I like -Werror too :) That's why I got so keen on getting rid of the deprecated functions.

But if you have checked comments in metacity/marco you would see that deprecated function is used to redraw immediately.

Indeed there are comment about "repainting everything", but I assumed they were from a time when these functions were needed (for any application). I wasn't aware that the "frame clock" mentioned in the gtk deprecation commit would not apply to window managers in particular. So I thought those comments were obsolete too. In the case of metacity it would be good to mention* directly in the anti-deprecation wrapper why the functions are still needed (nothing was mentioned in the commit introducing it), a small comment like from one above "not used by GTK anymore and normal applications does not need it, however we still need it to get immediate redraw because...". Out of curiosity, what makes the window manager different than any other application when it comes to having widgets redrawn? I don't expect anyone to provide a long explanation for me on this, but if it can be said in a few words... When you say immediately, you mean it cannot wait for the next frame clock tick, or does this mechanism not work in this context?

*) EDIT(2) I checked out latest metacity git and there is a (short) comment about repainting but it is from before the deprecation and the comment was not updated.

muktupavels commented 2 years ago

Why would I mention that functions are still needed if I don't know that? Again, I just disabled warning! I had no time and have no time now to investigate why it was needed back then and if it is still needed today!

Check where and when deprecated function are used!

meta_frames_repaint_frame is used only when window is resized: https://github.com/mate-desktop/marco/blob/5d48375212d816bb9e4d23efdc155ec77dbe927d/src/core/frame.c#L394-L400

meta_frames_push_delay_exposes is used in effects.c.

Mutter did stop using that function - https://gitlab.gnome.org/GNOME/mutter/-/commit/05353c1f7ed9c8c2d0095b92750b8bbee331a4c0. But metacity nor macro has frame synchronization. One more difference is that both still can be used as non compositing window managers.

And I think metacity did not have compositor when gdk_window_process_updates was added to redraw frame immediately.

tormodvolden commented 2 years ago

Why would I mention that functions are still needed if I don't know that? Again, I just disabled warning! I had no time and have no time now to investigate why it was needed back then and if it is still needed today!

That clarifies the situation, thanks for the frank answer. I originally assumed you knew something more and had just skipped commenting about it. Well, it could be a useful comment that you don't know whether it is needed or not also... Anyway I have been testing window resizing with and without gdk_window_process_all_updates() and it doesn't seem to be needed here, but this might vary between drivers and setups. (Ubuntu Mate 20.04 on Intel IGP, dual screen with QHD.) I understand this is a difficult topic and to really get it right the window manager or compositor should drive the frame clock based on vsync signaling from the graphics driver.

raveit65 commented 2 years ago

Is there anything we should merge from PR after discussion?

raveit65 commented 2 years ago

@vkareh Can you please do a review?

tormodvolden commented 2 years ago

The first commit is still valid. The second is controversial (if it works don't fix it). The third should be replaced by adapting the metacity commit. The fourth is still broken, and any hints to why would be welcome.

raveit65 commented 2 years ago

Ok, i will start testing commit 1 and 2. I think we can drop the forth commit. We have a lot of that warnings in other repos too. When we ever switch to gtk4 than we have to decide drop all this menu icons, as this isn't supported any more. Maybe you can try to update commit 3 from metacity?

raveit65 commented 2 years ago 
[rave@mother marco]$ git branch 
  1.18
  1.20
  1.22
  1.24
  1.26
  master
* tormodvolden-process_update
[rave@mother marco]$ git pull https://github.com/tormodvolden/marco.git process_update
From https://github.com/tormodvolden/marco
 * branch              process_update -> FETCH_HEAD
fatal: Not possible to fast-forward, aborting.
[rave@mother marco]$

Can you please rebase your fork with our master?

lukefromdc commented 2 years ago

Note that if wr ever do move to GTK 4 we just have to manually code for packing icons into menuitems, as GTK won't have a function handling that any more. I think I did some of that in packing icons into the xrandr-applet menu and a few other places but not at my desktop now so can't look that up.

This will be used in so many places we might want to write it once and put it in mate-desktop to be called as needed from there instead of directly frim GTK.

tormodvolden commented 2 years ago 
[rave@mother marco]$ git pull https://github.com/tormodvolden/marco.git process_update

If you use pull you will try to merge the whole branch. Instead just fetch the remote branch and cherry-pick i.e. my 1st and 2nd commit (you can also use their hashes):

git fetch https://github.com/tormodvolden/marco.git process_update
git cherry-pick FETCH_HEAD~3
git cherry-pick FETCH_HEAD~2
tormodvolden commented 2 years ago

I rebased my branch anyway, it went cleanly with git rebase v1.26.0 process_update --onto master

I will look at the metacity commit (instead of commit 3) later.

tormodvolden commented 2 years ago

I filed #709 for the adaption of the metacity commit (instead of commit 3).

lukefromdc commented 2 years ago

Note that on hidpi displays/window-scaling-factor=2, https://github.com/mate-desktop/marco/pull/709 puts the window menu out of position to the right and too far down, looking like it is exactly twice as far right and exactly twice as far down from the top left corner as it should. This implies the window scaling factor may be getting applied twice in calculating the menu's position.

raveit65 commented 2 years ago

Commits 1 and 2 seems to run fine, can you please remove non working commits?