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 604118 - Autohide sidebar
Autohide sidebar
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2009-12-08 21:45 UTC by Siegfried Gevatter (RainCT)
Modified: 2010-01-09 22:40 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch updated so that it applies cleanly against the latest code (5.97 KB, patch)
2009-12-08 21:45 UTC, Siegfried Gevatter (RainCT)
needs-work Details | Review
formatting fixes (5.85 KB, patch)
2009-12-09 15:10 UTC, Florian Scandella
accepted-commit_now Details | Review
formatting fixes + cancel timeout properly (6.03 KB, patch)
2009-12-17 20:22 UTC, Florian Scandella
reviewed Details | Review

Description Siegfried Gevatter (RainCT) 2009-12-08 21:45:40 UTC
Created attachment 149372 [details] [review]
Patch updated so that it applies cleanly against the latest code

I'm opening this bug to discuss the patch for adding a gconf option for the sidebar to hide automatically when in collapsed mode, which was send to the gnome-shell ML by Florian Scandella.
Comment 1 Siegfried Gevatter (RainCT) 2009-12-08 22:42:14 UTC
Thanks for your patch. At first I wasn't sure whether it really makes sense to have three different states (hidden, collapsed and expanded), but I think now that I've been playing with it I like the automatic hiding :). Anyway, below are a few comments on your patch:

* js/ui/:
 - If, when the panel is hidden, you put the mouse on the "recent documents" widget, as soon as you move it out again that widget hides immediately (while the others, as they are supposed to do, stay until the timeout happens and then hide slowly with the animation).
 - Consider using the _autohideChanged function in _init to avoid having the get_bool/set_reactive/_hide code twice and to have it all together.
 - Then there are several style issues:
  -- Spaces are missing in several spaces; eg. in "if(" between the "if" and the parentheses, in "ANIMATION_TIME/2" in both sides of the slash, "(actor,event)" after the comma, etc.
  -- Use "} else {" on a single line instead of having the "}" alone.
  -- Stuff like "if(this._hidden)\n {\t this._restore();\n }" is usually written just as "if(this._hidden)\n\t this._restore()".

* data/gnome-shell.schemas:
 - There's a typo in the short description: "autmatically" -> "automatically".

Another suggestion I'd like to make is implementing a more intelligent hiding, like Docky provides (under the name "intellihide"). In case you haven't seen it, it is like autohide but only enabled when it'd cover the currently active window.
Comment 2 Dan Winship 2009-12-08 23:05:39 UTC
FYI, it's not at all clear what is going to happen with the sidebar at this point. while you're welcome to hack on it, it's possible that it will just go away entirely
Comment 3 Florian Scandella 2009-12-09 00:38:45 UTC
hi, thx for the comments .. i like the idea with intellihide, i have to look into it.

i know there are some kind of timing problems. basically i would need a way to cancel a pending timeout, but i found none. maybe someone with better knowledge of javascript or glib could give me a hint there.
Comment 4 Florian Scandella 2009-12-09 15:10:11 UTC
Created attachment 149436 [details] [review]
formatting fixes

obsoletes first patch (buzilla would not let me select the option)
Comment 5 Colin Walters 2009-12-17 14:44:41 UTC
Review of attachment 149436 [details] [review]:

Thanks for the patch!  Overall looks very good, just had a few small comments.  Marking this as ACN; do you have a GNOME account?

::: js/ui/sidebar.js
@@ +168,3 @@
     },
+        if ( !this._expanded ) {
+    _hide: function() {

No spaces around conditionals.

@@ +198,3 @@
+        if (this._hidden)
+            this._restore();
+        this._wantsToHide = false;

Rather than having _wantsToHide, a cleaner and more efficient way to do this is to keep track of the integer ID returned by Mainloop.timeout_add_seconds, then you can do:

if (this._hideTimeoutId != 0) {
  Mainloop.source_remove(this._hideTimeoutId);
  this._hideTimeoutId = 0;
}

This ensures the timeout won't run if it's been queued, rather than the shell waking up, checking a boolean, and returning.

@@ +205,3 @@
+        if (!this._expanded) {
+            this._wantsToHide = true;
+            Mainloop.timeout_add(2000,Lang.bind(this,this._hideTimeoutFunc));

timeout_add_seconds(2,
Comment 6 Florian Scandella 2009-12-17 20:22:55 UTC
Created attachment 149935 [details] [review]
formatting fixes +  cancel timeout properly

heh.. i this was what i was looking for, i didn't realize i could cancel the timeout this way. autohides works much better now :)

i hope the formating issues are resolved (i think my editor is playing a game with me introducing random whitespaces at end of lines, etc).

gnome account? you mean bugzilla? yes :)
Comment 7 Florian Scandella 2009-12-17 20:25:09 UTC
oh, one more thing, this only applies cleanly after applying #604177
Comment 8 Colin Walters 2010-01-08 21:35:02 UTC
Review of attachment 149935 [details] [review]:

Some comments below:

I also took the liberty of fixing the awful looking text with some strategic calls to Math.floor() in this patch.

::: js/ui/sidebar.js
@@ +177,3 @@
+            this._hidden = true;
+        if (!this._expanded) {
+    _hide: function() {

"this' won't do what you expect here.  You need to use Lang.bind, like:

onComplete: Lang.bind(this, function () { this.actor... })

@@ +188,3 @@
+
+            // Updated the strut/stage area after the animation completes
+            Tweener.addTween(this, { time: WidgetBox.ANIMATION_TIME/2,

We use spaces around operators, like

WidgetBox.ANIMATION_TIME / 2,
Comment 9 Florian Scandella 2010-01-09 22:40:16 UTC
(In reply to comment #8)
> Review of attachment 149935 [details] [review]:
> 
> Some comments below:
> 
> I also took the liberty of fixing the awful looking text with some strategic
> calls to Math.floor() in this patch.
> 
> ::: js/ui/sidebar.js
> @@ +177,3 @@
> +            this._hidden = true;
> +        if (!this._expanded) {
> +    _hide: function() {
> 
> "this' won't do what you expect here.  You need to use Lang.bind, like:
> 
> onComplete: Lang.bind(this, function () { this.actor... })
> 

i don't get it .. first of all it does work, secondly i copied it from _collapse/_expand ...  can you please explain what you meant (or why the onComplete is ok in _collapse  but not in _hide) ?