After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 738726 - Missing separators in popover menus
Missing separators in popover menus
Status: RESOLVED FIXED
Product: gnome-boxes
Classification: Applications
Component: snapshots
3.14.x
Other Linux
: Normal normal
: --
Assigned To: GNOME Boxes maintainer(s)
GNOME Boxes maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2014-10-17 21:53 UTC by Daniel Korostil
Modified: 2016-09-20 08:15 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Standalone app that tries to reproduce the issue (946 bytes, application/x-gzip)
2014-10-29 15:42 UTC, Zeeshan Ali
  Details
theming: Remove .separator override (856 bytes, patch)
2015-03-02 12:50 UTC, Kalev Lember
committed Details | Review
actions popover: Put all items in a section (2.48 KB, patch)
2015-03-02 12:50 UTC, Kalev Lember
needs-work Details | Review
actions popover: Put all items in a section (2.24 KB, patch)
2015-03-02 13:40 UTC, Kalev Lember
committed Details | Review

Description Daniel Korostil 2014-10-17 21:53:37 UTC
There is different distance between items: Rename is closer to Revert than to Delete. See screenshot.
http://i.imgur.com/RfzQrEM.png
Comment 2 Lasse Schuirmann 2014-10-24 17:25:45 UTC
It looks really a bit weird in the screenshot. Could we bump up the distance of delete and the rest like in the mockup?
Comment 3 Lasse Schuirmann 2014-10-24 17:26:38 UTC
Or maybe put a seperator or so between the entries - we have those in the app menus usually, why not here?
Comment 4 Zeeshan Ali 2014-10-24 20:48:48 UTC
jimmac> zeenix: mclasen really doesn't like using whitespace for separation
<jimmac> zeenix: I hate the line separator
<jimmac> but use it for now
Comment 5 Zeeshan Ali 2014-10-29 15:39:56 UTC
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.
Comment 6 Zeeshan Ali 2014-10-29 15:42:49 UTC
Created attachment 289596 [details]
Standalone app that tries to reproduce the issue
Comment 7 Zeeshan Ali 2014-12-14 16:20:56 UTC
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
Comment 8 Matthias Clasen 2014-12-15 11:35:32 UTC
not sure if it is relevant here, but gtk only puts separators between toplevel sections in gmenus
Comment 9 Zeeshan Ali 2014-12-15 15:02:30 UTC
(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
Comment 10 Matthias Clasen 2015-02-12 18:24:37 UTC
You should try putting the first few items into a section too - everything should be in a section, afair.
Comment 11 Zeeshan Ali 2015-02-13 11:34:46 UTC
(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.
Comment 12 Zeeshan Ali 2015-02-13 11:39:02 UTC
(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.
Comment 13 Kalev Lember 2015-03-02 12:50:03 UTC
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.
Comment 14 Kalev Lember 2015-03-02 12:50:12 UTC
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.
Comment 15 Zeeshan Ali 2015-03-02 13:25:49 UTC
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;
Comment 16 Zeeshan Ali 2015-03-02 13:26:50 UTC
Review of attachment 298282 [details] [review]:

Patch itself is good but I see some invisible redundant libgd change.
Comment 17 Kalev Lember 2015-03-02 13:35:14 UTC
(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.
Comment 18 Kalev Lember 2015-03-02 13:40:41 UTC
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.
Comment 19 Zeeshan Ali 2015-03-02 13:51:49 UTC
(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.
Comment 20 Zeeshan Ali 2015-03-02 13:52:15 UTC
Review of attachment 298281 [details] [review]:

ack then
Comment 21 Zeeshan Ali 2015-03-02 13:52:32 UTC
Review of attachment 298291 [details] [review]:

ack
Comment 22 Kalev Lember 2015-03-02 14:27:12 UTC
Attachment 298281 [details] pushed as f2c3954 - theming: Remove .separator override
Attachment 298291 [details] pushed as 0f8a403 - actions popover: Put all items in a section