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 596000 - Add lightbox.js and use it in overview, alt-tab, and run dialog
Add lightbox.js and use it in overview, alt-tab, and run dialog
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2009-09-22 19:44 UTC by Dan Winship
Modified: 2009-09-22 21:24 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[lightbox] Add lightbox.js and use it in overview, alt-tab, and run dialog (16.81 KB, patch)
2009-09-22 19:44 UTC, Dan Winship
committed Details | Review

Description Dan Winship 2009-09-22 19:44:00 UTC
As mentioned in passing in other bugs, here's an abstraction for
highlighting particular actors in a group by using a dark translucent
actor to obscure the others
Comment 1 Dan Winship 2009-09-22 19:44:02 UTC
Created attachment 143746 [details] [review]
[lightbox] Add lightbox.js and use it in overview, alt-tab, and run dialog
Comment 2 Owen Taylor 2009-09-22 20:15:43 UTC
Review of attachment 143746 [details] [review]:

Like it! only minor style comments.

::: /dev/null
@@ +85,3 @@
+            // Somewhere else; insert it into the correct spot
+            let nextChild = this._children.indexOf(children[newChildIndex + 1]);
+            if (nextChild != -1)

Might be good to add a // paranoia comment to confirm the reader's guess that this shouldn't happen.

@@ +92,3 @@
+    _actorRemoved : function(container, child) {
+        let index = this._children.indexOf(child);
+        if (index != -1)

Also // paranoia, right?

@@ +121,3 @@
+            } else if (this._children[i] == this._highlighted) {
+                this._children[i].lower(below);
+                below = this._children[i];

You don't need to set below here, right?

@@ +130,3 @@
+
+    destroy : function() {
+        if (this._allocationChangedSignalId)

I don't like testing undefined property members for truth - I have a dream that somebody we can turn on a JS warning that will catch errors. I'd say either set it to null explicitly or test '=== undefined' here.

(Or maybe test '== null' here but I don't really like testing undefined members against null either for similar reasons.)
Comment 3 Dan Winship 2009-09-22 21:24:51 UTC
> You don't need to set below here, right?

right. leftover from an earlier state where I allowed highlighting
multiple actors at once, because I'd initially thought the overview did
that sometimes.

Attachment 143746 [details] pushed as 3734479 - [lightbox] Add lightbox.js and use it in overview, alt-tab, and run dialog