GNOME Bugzilla – Bug 722869
Use GtkPopover for search popup
Last modified: 2014-02-11 12:19:19 UTC
Now that Gtk+ has this awesome widget: http://blogs.gnome.org/mclasen/2014/01/22/and-now-popovers/ I think it makes sense to put our search popup into this widget.
Yeah either a popover or a modal (I have no real preference here, just noting that that would be another possibility).
The Popover always points at something, tho? Should the searcPopup point?
(In reply to comment #2) > The Popover always points at something, tho? Should the searcPopup point? The search entry of course :)
I think we will run into the problem of no transparency over Clutter. I'll attach two picture to demonstrate, one with the searchPopup as a GtkPopover over mapView and one where I didn't add the mapView and instead have a blue background.
Created attachment 267398 [details] SearchPopup as GtkPopover over mapView
Created attachment 267399 [details] SearchPopup as GtkPopover over blue window
Created attachment 267401 [details] SearchPopup as GtkPopover over mapView with margin 0 The GtkPopover css in Adwaita sets a margin of 10, we can have our own css and set margin to 0px. Then it can look something like this.
(In reply to comment #7) > Created an attachment (id=267401) [details] > SearchPopup as GtkPopover over mapView with margin 0 > > The GtkPopover css in Adwaita sets a margin of 10, we can have our own css and > set margin to 0px. > Then it can look something like this. This looks ok.
Created attachment 268286 [details] [review] searchPopup: make GtkStack the top-level widget It is bad practice to add children that can not scroll to a GtkScrolledWindow. This patch makes the GtkStack the top-level widget of the search-popup ui file. With this patch the GtkTreeView will be the only child of the GtkScrolledWindow. This change also fixes a buglet where the spinner would not be centered in the visible popup when scrollbars where visible.
Created attachment 268287 [details] [review] searchPopup: Convert to GtkPopover
Ok so the first patch is not really needed for the second. But I found it while doing the patch so I appended it here. The first one also seem to solve the bug with the missing scrollbars :) At least it is not reproducible anymore.
Review of attachment 268286 [details] [review]: This looks good to me.
Review of attachment 268287 [details] [review]: Looks good! Thanks for doing this. ::: src/mainWindow.js @@ +46,3 @@ const _WINDOW_MIN_HEIGHT = 500; +const _PLACE_ICON_SIZE = 24; Perhaps a separate patch? But yeah, 24px looks much better than 20px. ::: src/searchPopup.js @@ +58,3 @@ visible: true }); + this.get_style_context().add_class('maps-popover'); I wish we had templates so this could go into the ui-file instead.
Review of attachment 268287 [details] [review]: ::: src/mainWindow.js @@ +46,3 @@ const _WINDOW_MIN_HEIGHT = 500; +const _PLACE_ICON_SIZE = 24; Yeah, if this gets accepted-commit I'll push without this change. Thanks for review.
Zeeshan: what do you think? :)
Review of attachment 268287 [details] [review]: looks good otherwise ::: data/gnome-maps.css @@ +7,3 @@ } +.maps-popover { why the name change?
Review of attachment 268286 [details] [review]: ack
Review of attachment 268287 [details] [review]: ::: data/gnome-maps.css @@ +7,3 @@ } +.maps-popover { The idea was that we might want popovers on other places and this should be the style for them. If we want something that is unique for the search-popup then we can create a class for that. You want it in a separate patch? Or not at all? :)
Review of attachment 268287 [details] [review]: ::: data/gnome-maps.css @@ +7,3 @@ } +.maps-popover { Ah ok. Separate patch will give you opportunity to put the justification in the commit log so it would be better but no strong feelings in this case. In any case, feel free to push directly now.
Attachment 268286 [details] pushed as 2c533ed - searchPopup: make GtkStack the top-level widget Attachment 268287 [details] pushed as dbe68f2 - searchPopup: Convert to GtkPopover