GNOME Bugzilla – Bug 773631
Stop using Gtk.Menu.popup()
Last modified: 2017-01-16 09:28:12 UTC
Gtk.Menu.popup() has been deprecated, see: https://developer.gnome.org/gtk3/unstable/GtkMenu.html#gtk-menu-popup We use it in one location, in contextMenu.js This usage should probably be replaced with: Gtk.Menu.popup_at_pointer() taking the triggering event as the one parameter.
There would be a Fedora and GNOME conference in Peru this Saturday, I'd love if you can keep this bug on hold since I want to use it in a talk on how to start contributing to GNOME and I'm gonna use GNOME Maps as the app to contribute, this newcomer bug is a perfect fit to go through Gtk+ docs, do little code changes and demonstrate how to submit the patch-set. Thanks!
Cool!
Created attachment 340821 [details] [review] contextMenu: Stop using Gtk.Menu.popup() I've used popup_at_rect instead, since the event triggered in our case is a clutter event and I couldn't find a way to create a GDK event from it. For the same reason, you will see a warning in the console telling that there's no trigger event for the menu popup. I'm not sure if that could cause a problem in some corner case, need to investigate how gtk_menu_popup_internal exactly use this event data.
Created attachment 343196 [details] [review] contextMenu: Stop using Gtk.Menu.popup() Instead of using the deprecated popup() function, use popup_at_pointer() and connect to the Gtk embed widget instead of the Clutter actor.
Created attachment 343419 [details] [review] contextMenu: Stop using Gtk.Menu.popup() Instead of using the deprecated popup() function, use popup_at_pointer() and connect to the Gtk embed widget instead of the Clutter actor.
Worked fine for me under Wayland when I built as a Flatpak.
Review of attachment 343419 [details] [review]: The code looks good except the smallest of nit. So regarding testing, have we tested this on HiDPI /and/ LoDPI and also on X11 and Wayland? I don't have a JHBuild set up right now unfortunately. ::: src/contextMenu.js @@ +74,3 @@ + _onButtonReleaseEvent: function(widget, event) { + let [_, button] = event.get_button(); + let [_,x, y] = event.get_coords(); [_,x, y] → [_, x, y] (add a space)
(In reply to Mattias Bengtsson from comment #7) > Review of attachment 343419 [details] [review] [review]: > > The code looks good except the smallest of nit. > > So regarding testing, have we tested this on HiDPI /and/ LoDPI and also on > X11 and Wayland? I'm running on a 2560×1440 desktop display (with no scaling) on X11 (Nvidia) and it works fine. Andreas tested it on Wayland (and I _think_ he has a HiDPI laptop, if I remember correctly). > > I don't have a JHBuild set up right now unfortunately. > > ::: src/contextMenu.js > @@ +74,3 @@ > + _onButtonReleaseEvent: function(widget, event) { > + let [_, button] = event.get_button(); > + let [_,x, y] = event.get_coords(); > > [_,x, y] → [_, x, y] (add a space) Sure
Created attachment 343472 [details] [review] contextMenu: Stop using Gtk.Menu.popup() Instead of using the deprecated popup() function, use popup_at_pointer() and connect to the Gtk embed widget instead of the Clutter actor.
Review of attachment 343472 [details] [review]: Nice! Merge away! :)
Attachment 343472 [details] pushed as 6970e59 - contextMenu: Stop using Gtk.Menu.popup()
(In reply to Mattias Bengtsson from comment #10) > Review of attachment 343472 [details] [review] [review]: > > Nice! Merge away! :) Thanks! :)
(In reply to Marcus Lundblad from comment #8) > (In reply to Mattias Bengtsson from comment #7) > > Review of attachment 343419 [details] [review] [review] [review]: > > > > The code looks good except the smallest of nit. > > > > So regarding testing, have we tested this on HiDPI /and/ LoDPI and also on > > X11 and Wayland? > > I'm running on a 2560×1440 desktop display (with no scaling) on X11 (Nvidia) > and it works fine. > Andreas tested it on Wayland (and I _think_ he has a HiDPI laptop, if I > remember correctly). I have a hidpi laptop in the house, but I didn't test the patch on it. I can try master on the laptop today though.