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 707801 - Fix issues with submenus and separators hiding/showing when they aren't supposed to
Fix issues with submenus and separators hiding/showing when they aren't suppo...
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
: 708244 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2013-09-09 20:03 UTC by Jasper St. Pierre (not reading bugmail)
Modified: 2013-09-17 15:04 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Revert "popupMenu: Make the section invisible if it has no visible children" (1.29 KB, patch)
2013-09-09 20:03 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
popupMenu: Fix algorithm for separator visibility to work with sections (2.16 KB, patch)
2013-09-09 20:03 UTC, Jasper St. Pierre (not reading bugmail)
needs-work Details | Review
popupMenu: Fix algorithm for separator visibility to work with sections (2.18 KB, patch)
2013-09-10 13:31 UTC, Jasper St. Pierre (not reading bugmail)
reviewed Details | Review
popupMenu: Fix algorithm for separator visibility to work with sections (2.18 KB, patch)
2013-09-10 13:53 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review

Description Jasper St. Pierre (not reading bugmail) 2013-09-09 20:03:16 UTC
The quick hack fix we did to hide rogue separators in the lock screen was incorrect
and made some sections show when they weren't supposed to. Revert that and fix the
separator visiblity logic.
Comment 1 Jasper St. Pierre (not reading bugmail) 2013-09-09 20:03:18 UTC
Created attachment 254516 [details] [review]
Revert "popupMenu: Make the section invisible if it has no visible children"

This reverts commit 5a0ac6c2ac5686c679988c2ca922ed758d2b19f1.
Comment 2 Jasper St. Pierre (not reading bugmail) 2013-09-09 20:03:21 UTC
Created attachment 254517 [details] [review]
popupMenu: Fix algorithm for separator visibility to work with sections

Before, separators naively checked whether their siblings were visible
using actor visibility. However, if section actors are visible but have
no visible children, this will fail. Special-case separators when doing
visiblity checks.
Comment 3 Giovanni Campagna 2013-09-10 07:33:41 UTC
Review of attachment 254516 [details] [review]:

You hadn't reverted this yet?
Comment 4 Giovanni Campagna 2013-09-10 07:35:11 UTC
Review of attachment 254517 [details] [review]:

::: js/ui/popupMenu.js
@@ +38,3 @@
+function isPopupMenuItemVisible(child) {
+    if (child._delegate instanceof PopupMenuSection)
+        return !child._delegate.isEmpty();

The section can be empty, or can be not empty and invisible.
In both cases you need to hide the separator.
Comment 5 Jasper St. Pierre (not reading bugmail) 2013-09-10 13:31:36 UTC
Created attachment 254599 [details] [review]
popupMenu: Fix algorithm for separator visibility to work with sections

Before, separators naively checked whether their siblings were visible
using actor visibility. However, if section actors are visible but have
no visible children, this will fail. Special-case separators when doing
visiblity checks.
Comment 6 Giovanni Campagna 2013-09-10 13:49:52 UTC
Review of attachment 254599 [details] [review]:

::: js/ui/popupMenu.js
@@ +38,3 @@
+function isPopupMenuItemVisible(child) {
+    if (child._delegate instanceof PopupMenuSection)
+        if (!child._delegate.isEmpty())

Isn't the logic reversed here?
I mean, an empty section is like an invisible section, not the other way around.
Comment 7 Jasper St. Pierre (not reading bugmail) 2013-09-10 13:53:52 UTC
Created attachment 254601 [details] [review]
popupMenu: Fix algorithm for separator visibility to work with sections

Before, separators naively checked whether their siblings were visible
using actor visibility. However, if section actors are visible but have
no visible children, this will fail. Special-case separators when doing
visiblity checks.



I noticed that in testing, and thought I squashed before reattaching. Guess not.
Comment 8 Giovanni Campagna 2013-09-10 14:08:33 UTC
Review of attachment 254601 [details] [review]:

Looks good to me.
Comment 9 Jasper St. Pierre (not reading bugmail) 2013-09-10 14:24:08 UTC
Attachment 254516 [details] pushed as fd9401c - Revert "popupMenu: Make the section invisible if it has no visible children"
Attachment 254601 [details] pushed as 660f0fe - popupMenu: Fix algorithm for separator visibility to work with sections
Comment 10 Jasper St. Pierre (not reading bugmail) 2013-09-17 15:04:36 UTC
*** Bug 708244 has been marked as a duplicate of this bug. ***