GNOME Bugzilla – Bug 738726
Missing separators in popover menus
Last modified: 2016-09-20 08:15:55 UTC
There is different distance between items: Rename is closer to Revert than to Delete. See screenshot. http://i.imgur.com/RfzQrEM.png
Thats per design: https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/boxes/wires/snapshots-branching.png
It looks really a bit weird in the screenshot. Could we bump up the distance of delete and the rest like in the mockup?
Or maybe put a seperator or so between the entries - we have those in the app menus usually, why not here?
jimmac> zeenix: mclasen really doesn't like using whitespace for separation <jimmac> zeenix: I hate the line separator <jimmac> but use it for now
I spent good many hours trying to find out where we tell gtk+ to insert space instead of separator but I failed to find it. :( I even transformed the relevant code into a standalone app to see if its coming from Gtk+ somehow but I get a separator in there too. Tim (who wrote snapshots code) doesn't recall doing anything special in this case either so I'm totally out of clues here.
Created attachment 289596 [details] Standalone app that tries to reproduce the issue
This issue exists in other popovers in Boxes too. This commit is a suspect but I don't see how it doesn't affect the standalone app in comment#6 then? https://mail.gnome.org/archives/commits-list/2014-April/msg08043.html
not sure if it is relevant here, but gtk only puts separators between toplevel sections in gmenus
(In reply to comment #8) > not sure if it is relevant here, but gtk only puts separators between toplevel > sections in gmenus Well, AFAICT ours are toplevel: https://git.gnome.org/browse/gnome-boxes/tree/src/actions-popover.vala#n32
You should try putting the first few items into a section too - everything should be in a section, afair.
(In reply to Matthias Clasen from comment #10) > You should try putting the first few items into a section too - everything > should be in a section, afair. Thanks. Although I recall trying that but I'll try again.
(In reply to Zeeshan Ali (Khattak) from comment #11) > (In reply to Matthias Clasen from comment #10) > > You should try putting the first few items into a section too - everything > > should be in a section, afair. > > Thanks. Although I recall trying that but I'll try again. Nope, it doesn't work. I'm willing to bet this has something to do with theme somehow cause as I said, I was unable to reproduce the issue in a standalone test app.
Created attachment 298281 [details] [review] theming: Remove .separator override This was getting in the way for separator theming in popover menus. It used to be needed for the MenuBox code that was removed in commit 770522a1ef59ce34aa5a3a08364665298f85a3c3.
Created attachment 298282 [details] [review] actions popover: Put all items in a section Create a separate section for the first few items as well. Even though not strictly neccessary, this clarifies the intent behind the sectioning.
Review of attachment 298281 [details] [review]: ::: data/gtk-style.css @@ -12,3 @@ - -GtkWidget-separator-height: 1; -} - Ah you found the culprit. While this fixes the issue, the separator is very hard to see so I think should rather modify this theme to set the fg color. color: @theme_fg_color;
Review of attachment 298282 [details] [review]: Patch itself is good but I see some invisible redundant libgd change.
(In reply to Zeeshan Ali (Khattak) from comment #15) > Ah you found the culprit. While this fixes the issue, the separator is very > hard to see so I think should rather modify this theme to set the fg color. > > color: @theme_fg_color; That would be an issue with the gtk+ dark theme and not a gnome-boxes bug. The same problem shows up in any other app with the dark theme. I think we should ask one of the designers to look at the separators in the gtk+ dark theme and make them lighter or something.
Created attachment 298291 [details] [review] actions popover: Put all items in a section Create a separate section for the first few items as well. Even though not strictly neccessary, this clarifies the intent behind the sectioning.
(In reply to Kalev Lember from comment #17) > (In reply to Zeeshan Ali (Khattak) from comment #15) > > Ah you found the culprit. While this fixes the issue, the separator is very > > hard to see so I think should rather modify this theme to set the fg color. > > > > color: @theme_fg_color; > > That would be an issue with the gtk+ dark theme and not a gnome-boxes bug. > The same problem shows up in any other app with the dark theme. > > I think we should ask one of the designers to look at the separators in the > gtk+ dark theme and make them lighter or something. Good point.
Review of attachment 298281 [details] [review]: ack then
Review of attachment 298291 [details] [review]: ack
Attachment 298281 [details] pushed as f2c3954 - theming: Remove .separator override Attachment 298291 [details] pushed as 0f8a403 - actions popover: Put all items in a section