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 709395 - We should hide the Users-list button when on selection-mode
We should hide the Users-list button when on selection-mode
Status: RESOLVED FIXED
Product: polari
Classification: Applications
Component: general
unspecified
Other Linux
: Normal trivial
: ---
Assigned To: Polari maintainers
Polari maintainers
Depends on:
Blocks: 709896
 
 
Reported: 2013-10-03 21:54 UTC by Felipe Borges
Modified: 2013-10-14 17:33 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
it does the job. :) (2.08 KB, patch)
2013-10-03 21:57 UTC, Felipe Borges
needs-work Details | Review
Now it hides the user list too. (2.56 KB, patch)
2013-10-03 23:55 UTC, Felipe Borges
needs-work Details | Review
This time looking up at the user-list action to hide the user-list on selection-mode (2.34 KB, patch)
2013-10-14 01:21 UTC, Felipe Borges
reviewed Details | Review
mainWindow: Hide user-list while on selection-mode (2.39 KB, patch)
2013-10-14 13:02 UTC, Felipe Borges
committed Details | Review

Description Felipe Borges 2013-10-03 21:54:25 UTC
I think we should hide this button on Selection-Mode. It doesn't make sense to show the current channel's users list while managing/selecting the Rooms' list.
Comment 1 Felipe Borges 2013-10-03 21:57:18 UTC
Created attachment 256429 [details] [review]
it does the job. :)
Comment 2 Florian Müllner 2013-10-03 21:59:58 UTC
(In reply to comment #0)
> It doesn't make sense to show the current channel's users list 
> while managing/selecting the Rooms' list.

Yeah, makes sense to me - at least if we keep selection mode, which is not quite clear yet.
Comment 3 Florian Müllner 2013-10-03 22:08:19 UTC
Review of attachment 256429 [details] [review]:

Only a minor style comment about the code, but I don't think the behavior is quite right - if the user-list is visible when entering selection mode, it seems odd to me to hide the control button but not the sidebar itself.

::: src/mainWindow.js
@@ +212,3 @@
             this._titlebarLeft.get_style_context().add_class('selection-mode');
             this._titlebarRight.get_style_context().add_class('selection-mode');
+            this._showUserListButton.hide();

You can use

   this._showUserListButton.visible = !enabled;

above.
Comment 4 Felipe Borges 2013-10-03 23:54:45 UTC
(In reply to comment #2)
> (In reply to comment #0)
> > It doesn't make sense to show the current channel's users list 
> > while managing/selecting the Rooms' list.
> 
> Yeah, makes sense to me - at least if we keep selection mode, which is not
> quite clear yet.

Hmm... I didn't considered that.

(In reply to comment #3)
> Review of attachment 256429 [details] [review]:
> 
> Only a minor style comment about the code, but I don't think the behavior is
> quite right - if the user-list is visible when entering selection mode, it
> seems odd to me to hide the control button but not the sidebar itself.

oh, I missed that! Srry.

> 
> ::: src/mainWindow.js
> @@ +212,3 @@
>             
> this._titlebarLeft.get_style_context().add_class('selection-mode');
>             
> this._titlebarRight.get_style_context().add_class('selection-mode');
> +            this._showUserListButton.hide();
> 
> You can use
> 
>    this._showUserListButton.visible = !enabled;
> 
> above.

Clever! ;)
Comment 5 Felipe Borges 2013-10-03 23:55:28 UTC
Created attachment 256433 [details] [review]
Now it hides the user list too.
Comment 6 Florian Müllner 2013-10-04 21:02:45 UTC
Review of attachment 256433 [details] [review]:

::: src/mainWindow.js
@@ +216,3 @@
             this._titlebarRight.get_style_context().remove_class('selection-mode');
         }
+        

Trailing whitespace

@@ +218,3 @@
+        
+        this._showUserListButton.visible = !enabled;
+        this._userListRevealer.visible = !enabled;

Sorry, I still don't like this. Just hiding the titlebar button without transition is ok (the whole titlebar changes color without transition, so the abrupt button change is the least of worries there), but I don't think that applies to the sidebar which is a much larger UI element - in my opinion, sliding it out properly is much nicer.
With a small change I just pushed[0], this should be as easy as looking up the 'user-list' action and doing
  action.enabled = !enabled;

(also fwiw, I'd slightly prefer this code above the if{}else{} block where there's already similar code, but that's obviously nitpicking)

[0] https://git.gnome.org/browse/polari/commit?id=32f038df815d74f0fe
Comment 7 Felipe Borges 2013-10-14 01:21:53 UTC
Created attachment 257204 [details] [review]
This time looking up at the user-list action to hide the user-list on selection-mode

Thx Florian! :)
Comment 8 Florian Müllner 2013-10-14 07:58:33 UTC
Review of attachment 257204 [details] [review]:

Code looks good, thanks!

The commit message should at least contain a link to this bug (see for instance https://git.gnome.org/browse/polari/commit/?id=cb69f7d3a62ea83e15 for the preferred commit message format) - you might want to consider using git-bz[0] to attach patches, which will take care of appending the URL for you.

[0] http://blog.fishsoup.net/2008/11/16/git-bz-bugzilla-subcommand-for-git/
Comment 9 Felipe Borges 2013-10-14 13:02:06 UTC
Created attachment 257252 [details] [review]
mainWindow: Hide user-list while on selection-mode
Comment 10 Florian Müllner 2013-10-14 17:10:22 UTC
Review of attachment 257252 [details] [review]:

Looks good - do you have git access or should I push the patch for you?
Comment 11 Felipe Borges 2013-10-14 17:30:49 UTC
No, I don't.
Comment 12 Florian Müllner 2013-10-14 17:33:12 UTC
Attachment 257252 [details] pushed as 65513b2 - mainWindow: Hide user-list while on selection-mode