GNOME Bugzilla – Bug 604118
Autohide sidebar
Last modified: 2010-01-09 22:40:16 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.
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.
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
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.
Created attachment 149436 [details] [review] formatting fixes obsoletes first patch (buzilla would not let me select the option)
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,
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 :)
oh, one more thing, this only applies cleanly after applying #604177
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,
(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) ?