Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove obsolete gdk_window_process_all_updates() #698

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

tormodvolden
Copy link
Contributor

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
Copy link
Contributor Author

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

@raveit65
Copy link
Member

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
Copy link
Member

raveit65 commented Oct 16, 2021

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor Author

tormodvolden commented Oct 18, 2021

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
Copy link
Contributor

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:

marco/src/core/frame.c

Lines 394 to 400 in 5d48375

/* If we're interactively resizing the frame, repaint
* it immediately so we don't start to lag.
*/
if (frame->window->display->grab_window ==
frame->window)
meta_ui_repaint_frame (frame->window->screen->ui,
frame->xwindow);

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
Copy link
Contributor Author

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
Copy link
Member

Is there anything we should merge from PR after discussion?

@raveit65
Copy link
Member

@vkareh
Can you please do a review?

@tormodvolden
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

[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
Copy link
Member

lukefromdc commented Dec 12, 2021 via email

@tormodvolden
Copy link
Contributor Author

[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

util.h references WITH_VERBOSE_MODE

Signed-off-by: Tormod Volden <[email protected]>
gdk_window_process_all_updates() was deprecated in gtk 3.22
which is the required minimal version for building marco.

Signed-off-by: Tormod Volden <[email protected]>
gtk_menu_popup_at_pointer() saves us the helper functions for
positioning and scaling.

gtk_menu_popup_at_pointer() was introduced in gtk 3.22 and
gtk_menu_popup() was deprecated at the same time.

Signed-off-by: Tormod Volden <[email protected]>
@tormodvolden
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@lukefromdc
Copy link
Member

lukefromdc commented Dec 14, 2021

Note that on hidpi displays/window-scaling-factor=2, #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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants