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 760872 - Use popover to change nick
Use popover to change nick
Status: RESOLVED FIXED
Product: polari
Classification: Applications
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: Polari maintainers
Polari maintainers
: 755598 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2016-01-20 03:54 UTC by Florian Müllner
Modified: 2016-02-02 01:15 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
entryArea: Use template for entry area (16.16 KB, patch)
2016-01-20 03:55 UTC, Florian Müllner
committed Details | Review
entryArea: Use popover to change nick ... (16.39 KB, patch)
2016-01-20 03:55 UTC, Florian Müllner
none Details | Review
chatView: Sync maxNickChars with nick button (5.60 KB, patch)
2016-01-20 03:55 UTC, Florian Müllner
committed Details | Review
roomStack: Don't show entry area with placeholder (1.85 KB, patch)
2016-01-20 03:55 UTC, Florian Müllner
committed Details | Review
Screenshots showing the different whitespace options (63.19 KB, image/png)
2016-01-21 15:46 UTC, Florian Müllner
  Details
left-margin comparison (34.02 KB, image/png)
2016-01-21 18:21 UTC, Bastian Ilsø
  Details
entryArea: Use popover to change nick ... (14.95 KB, patch)
2016-01-21 23:29 UTC, Florian Müllner
committed Details | Review

Description Florian Müllner 2016-01-20 03:54:58 UTC
... and update the overall entry area style as discussed with Bastian and Lapo.
Comment 1 Florian Müllner 2016-01-20 03:55:02 UTC
Created attachment 319396 [details] [review]
entryArea: Use template for entry area

With paste confirmation, the entry area became more complex than the original
box with two entries. Now we are about to move nick changing to a popover,
so this looks like a good time to move the UI to a template.
Comment 2 Florian Müllner 2016-01-20 03:55:07 UTC
Created attachment 319397 [details] [review]
entryArea: Use popover to change nick ...

... and restyle the entry area to blend in with the chat log. This results
in an overall less heavy layout, removes the focus-rect weirdness of linked
entries, and last but not least makes Lapo happy.
Comment 3 Florian Müllner 2016-01-20 03:55:12 UTC
Created attachment 319398 [details] [review]
chatView: Sync maxNickChars with nick button

As the entry area now blends in with the chat log, both elements should
use the same width for nicks to properly align the chat entry with
messages in the log.
Comment 4 Florian Müllner 2016-01-20 03:55:17 UTC
Created attachment 319399 [details] [review]
roomStack: Don't show entry area with placeholder

With a visually distinct entry area, showing an insensitive entry with the
placeholder helped reducing the visual noise during transitions.
However now that the entry area appears as part of the chat log, it makes
more sense to not show an entry when there is no chat log.
Comment 5 Bastian Ilsø 2016-01-20 13:19:54 UTC
Review of attachment 319396 [details] [review]:

I noticed that the GtkStack's 'sensitive' property is set to false in our current code (line 79, entryArea.js), but this patch doesn't set that for the GtkStack in entry-area.ui. is that on purpose?

Otherwise looks good to me.
Comment 6 Bastian Ilsø 2016-01-20 13:25:20 UTC
Review of attachment 319397 [details] [review]:

very nice :) - I have some questions


I'm a bit concerned about the large margin we end up having to the left (http://i.imgur.com/QC1AzvS.png). I tried reducing it to the half which solves it, except the button doesn't have enough air then (http://i.imgur.com/iXPRRPI.png). If Lapo thinks it's okay, though, I won't mind - but the button is not visible most of the time, so I'm more concerned about the chatview left margin than the button left margin.
 
The right side of the entry and the right margin in the chatview doesn't seem to align (http://i.imgur.com/5cLvgIV.png). perhaps something we want as well?

I observed that writing a long nick seem to misalign entry and chatview text, but once you send text with that long nick, the chatview text and entry aligns up again. Not sure if it's something worth adressing.

Maybe it makes sense to show a spinner in place of document-edit-symbolic while we are waiting for the "external" nick to change (e.g until telepathy sends the notification that rupert changed nick to rupertfoo)? or maybe that's the ugly workaround you mentioned in Bug 710396?

::: data/resources/entry-area.ui
@@ +83,3 @@
+                  <object class="GtkImage" id="editImage">
+                    <property name="can_focus">False</property>
+                <child>

When trying this out, the document-edit-symbol didn't exist because i used the adwaita-icon-theme from 3.18 so I had a broken image here. Does that mean we need to bump the requirements to be larger than Gtk+ 3.16 in the ui file then or?

::: src/entryArea.js
@@ +87,1 @@
+        this._nickLabel.set_state_flags(Gtk.StateFlags.LINK, false);

I did not quite understand what this does, but I noticed we restore and save the context whenever we deal with state flags in chatView. Do we need to do the same here?
Comment 7 Bastian Ilsø 2016-01-20 13:30:25 UTC
Review of attachment 319398 [details] [review]:

::: src/entryArea.js
@@ +286,3 @@
                            : this._room ? this._room.account.nickname : '';
 
+        this._nickLabel.width_chars = Math.max(nick.length, this._maxNickChars);

Does this mean we have a maximum nickname length of 8 characters in place?

I tried testing some certain long nicks and managed to make the margin with nicks visually jarring (fx http://i.imgur.com/juYKbs8.png ). I was wondering if it makes sense to estimate a max-width and ellipsize extreme cases - but perhaps we should wait with that until we have context popovers (bug 760853).

Do you know if we have any error handling in case the user sets a name above the maximum nickname length irc handles?
Comment 8 Bastian Ilsø 2016-01-20 13:30:38 UTC
Review of attachment 319399 [details] [review]:

looks good to me.
Comment 9 Florian Müllner 2016-01-20 13:36:42 UTC
(In reply to Bastian Ilsø from comment #5)
> Review of attachment 319396 [details] [review] [review]:
> 
> I noticed that the GtkStack's 'sensitive' property is set to false in our
> current code (line 79, entryArea.js), but this patch doesn't set that for
> the GtkStack in entry-area.ui. is that on purpose?

Yes. In the patch, 'sensitive' is set in RoomStack, which already handles sensitivity changes.
Comment 10 Florian Müllner 2016-01-20 14:03:23 UTC
(In reply to Bastian Ilsø from comment #6)
> Review of attachment 319397 [details] [review] [review]:
> I'm a bit concerned about the large margin we end up having to the left
> (http://i.imgur.com/QC1AzvS.png).

Yeah, I agree. I went with the values Lapo was asking for last week, but am open to tweaking. Though the biggest contributor there is clearly the (most of the time) invisible icon (plus spacing between label and icon) - maybe we should consider dropping that again, the current padding should be fine after that.


> The right side of the entry and the right margin in the chatview doesn't
> seem to align (http://i.imgur.com/5cLvgIV.png). perhaps something we want as
> well?

Ah, good point - is it better to bump up the right margin in the entry or reduce it in the chat log?


> I observed that writing a long nick seem to misalign entry and chatview
> text, but once you send text with that long nick, the chatview text and
> entry aligns up again. Not sure if it's something worth adressing.

Mmmh, yeah - we can definitively reduce that time by handling nick changes, so we update the length when the change happens on the server rather than when the first message is send.

 
> Maybe it makes sense to show a spinner in place of document-edit-symbolic
> while we are waiting for the "external" nick to change (e.g until telepathy
> sends the notification that rupert changed nick to rupertfoo)? or maybe
> that's the ugly workaround you mentioned in Bug 710396?

There goes the idea of dropping the icon :-(


> ::: data/resources/entry-area.ui
> @@ +83,3 @@
> +                  <object class="GtkImage" id="editImage">
> +                    <property name="can_focus">False</property>
> +                <child>
> 
> When trying this out, the document-edit-symbol didn't exist because i used
> the adwaita-icon-theme from 3.18 so I had a broken image here. Does that
> mean we need to bump the requirements to be larger than Gtk+ 3.16 in the ui
> file then or?

The GTK+ version in the .ui file doesn't have anything to do with adwaita-icon-theme though. We'll actually need unstable GTK+ at runtime (for the 'CssName' in RoomListHeader), but that's not something we can put in the .ui file (we can ask for "3.20", but then glade will complain that GTK+ master is too old).


> ::: src/entryArea.js
> @@ +87,1 @@
> +        this._nickLabel.set_state_flags(Gtk.StateFlags.LINK, false);
> 
> I did not quite understand what this does, but I noticed we restore and save
> the context whenever we deal with state flags in chatView. Do we need to do
> the same here?

No, this is a flag we are adding permanently to the widget. We tell GTK+ that the label is a link, which is a lie, but makes sure the 'active' color is consistent with the one in the chat log.
Comment 11 Florian Müllner 2016-01-20 14:13:41 UTC
(In reply to Bastian Ilsø from comment #7)
> Review of attachment 319398 [details] [review] [review]:
> 
> ::: src/entryArea.js
> @@ +286,3 @@
>                             : this._room ? this._room.account.nickname : '';
>  
> +        this._nickLabel.width_chars = Math.max(nick.length,
> this._maxNickChars);
> 
> Does this mean we have a maximum nickname length of 8 characters in place?

No, it's a minimum length of 8 characters. "max nick chars" == "the length of the longest nick we've seen for this room".


> I tried testing some certain long nicks and managed to make the margin with
> nicks visually jarring (fx http://i.imgur.com/juYKbs8.png ). I was wondering
> if it makes sense to estimate a max-width and ellipsize extreme cases

Maybe. It's already the case for the chat log though (and if the overly long nick is your own, the nick entry as well), so best handled separately IMHO.


> Do you know if we have any error handling in case the user sets a name above
> the maximum nickname length irc handles?

Not really. The server will either truncate the requested nick or not change the nick at all, and we'll pick that up and update the nick accordingly.
Comment 12 Bastian Ilsø 2016-01-21 00:11:18 UTC
(In reply to Florian Müllner from comment #10)
> (In reply to Bastian Ilsø from comment #6)
> > Review of attachment 319397 [details] [review] [review] [review]:
> > I'm a bit concerned about the large margin we end up having to the left
> > (http://i.imgur.com/QC1AzvS.png).
> 
> Yeah, I agree. I went with the values Lapo was asking for last week, but am
> open to tweaking. 

after playing a bit more around with it, I think halfing is a the best compromise ie:

.polari-nick-button {
    padding-left: 4px;
    padding-right: 4px;
}

To box1 in entry-area.ui add:
<property name="margin-left">2</property>

in chatView.js set
const MARGIN = 6; 

(hmm, I see a 1px misalignment with these values, but can't spot the error).

> Though the biggest contributor there is clearly the (most
> of the time) invisible icon (plus spacing between label and icon) - maybe we
> should consider dropping that again, the current padding should be fine
> after that.

This is the problem for larger nicks. but, yea worth a shot I think.


> Ah, good point - is it better to bump up the right margin in the entry or
> reduce it in the chat log?

I think the ideal would be, if we have equal whitespace on the left and the right of the chat log (like now) and then bump up the right margin in the entry to fit.
 

> > I observed that writing a long nick seem to misalign entry and chatview
> > text, but once you send text with that long nick, the chatview text and
> > entry aligns up again. Not sure if it's something worth adressing.
> 
> Mmmh, yeah - we can definitively reduce that time by handling nick changes,
> so we update the length when the change happens on the server rather than
> when the first message is send.

> > Maybe it makes sense to show a spinner in place of document-edit-symbolic
> > while we are waiting for the "external" nick to change[..]
> 
> There goes the idea of dropping the icon :-(

hmm, maybe we could show the spinner instead of the nick while we are waiting?


(In reply to Florian Müllner from comment #11)
> Maybe. It's already the case for the chat log though (and if the overly long
> nick is your own, the nick entry as well), so best handled separately IMHO.

Handled separately how?
Comment 13 Florian Müllner 2016-01-21 15:42:48 UTC
(In reply to Bastian Ilsø from comment #12)
> after playing a bit more around with it, I think halfing is a the best
> compromise ie:

You mean "halfing" - cutting down 12px from 48px is a quarter ;-)

Which is still good of course, but I'm not quite convinced that it's cutting off whitespace in the right place - most of it is reducing the outer margin (to the current value of 6px again), but that's actually a place where a little more padding doesn't hurt IMHO, not least because it reduces the overlap of overlay scrollbars with the content. The big issues are really the 34px of whitespace we add after the *longest* nick - reducing that to 30px as you propose is still a lot, and disconnects messages in the chat log from the corresponding nick. So maybe it's really best to drop the icon - it's only visible when the button is clearly recognizable as a button, and even if users don't guess its function, I cannot think of any possible actions that would scare them away from just trying it.

One issue we get when dropping the icon though is an imbalance between left and right padding in the button ...


> > > Maybe it makes sense to show a spinner in place of document-edit-symbolic
> > > while we are waiting for the "external" nick to change[..]
> > 
> > There goes the idea of dropping the icon :-(
> 
> hmm, maybe we could show the spinner instead of the nick while we are
> waiting?

Oh, I guess we could change the button label to the requested nick and overlay a translucent spinner?


> (In reply to Florian Müllner from comment #11)
> > Maybe. It's already the case for the chat log though (and if the overly long
> > nick is your own, the nick entry as well), so best handled separately IMHO.
> 
> Handled separately how?

Separate patch/bug. It's not as trivial as setting a couple of properties, it involves implementing our own ellipsization in the TextView.
Comment 14 Florian Müllner 2016-01-21 15:46:37 UTC
Created attachment 319507 [details]
Screenshots showing the different whitespace options

Here's a screenshot with the different options discussed (all with and without the popover:
 (1) current patch set with Lapo's suggested values
 (2) your proposal
 (3) Lapo's values without icon
Comment 15 Bastian Ilsø 2016-01-21 18:21:00 UTC
Created attachment 319517 [details]
left-margin comparison

(In reply to Florian Müllner from comment #13)
> [..]
> Which is still good of course, but I'm not quite convinced that it's cutting
> off whitespace in the right place - most of it is reducing the outer margin
> (to the current value of 6px again), but that's actually a place where a
> little more padding doesn't hurt IMHO, not least because it reduces the
> overlap of overlay scrollbars with the content.

My concern with the padding is that IMO it makes all nicknames in the chatview appear to weight towards the center. I attached a comparison. But if I'm the only one seeing this, it may just be me. :-)


> The big issues are really
> the 34px of whitespace we add after the *longest* nick - reducing that to
> 30px as you propose is still a lot, and disconnects messages in the chat log
> from the corresponding nick. 
> [..]
> One issue we get when dropping the icon though is an imbalance between left
> and right padding in the button ...

Maybe we could do something like:

(*) If the difference between largest nickname in chatview and the user's nickname is above ~40px, display the display-edit-icon to create balance between left and right padding (ie. there should be space enough to insert it without introducing additional padding).

(*) If the difference is less, don't display it - the nickname takes care of the balancing on its own.
 
Ultimately I think ellipsizing longer nicknames (defined by a beauty value) is the best way to solve this issue, though..


> > hmm, maybe we could show the spinner instead of the nick while we are
> > waiting?
> 
> Oh, I guess we could change the button label to the requested nick and
> overlay a translucent spinner?

I was thinking of putting the button label and the spinner in a stack and switch between the two, but your idea could work too. :-)

 
> Separate patch/bug. It's not as trivial as setting a couple of properties,
> it involves implementing our own ellipsization in the TextView.

ah ok. I think it could definitely be useful to solve our spacing issues.
Comment 16 Florian Müllner 2016-01-21 20:06:31 UTC
(In reply to Bastian Ilsø from comment #15)
> My concern with the padding is that IMO it makes all nicknames in the
> chatview appear to weight towards the center. I attached a comparison. But
> if I'm the only one seeing this, it may just be me. :-)

Mmh, I'm not sure whether it's an issue or whether it's simply unfamiliar. On the other hand, the extra padding does really help on the right IMHO, and the nick button doesn't look cramped in.


> Maybe we could do something like:
> 
> (*) If the difference between largest nickname in chatview and the user's
> nickname is above ~40px, display the display-edit-icon to create balance
> between left and right padding (ie. there should be space enough to insert
> it without introducing additional padding).
> 
> (*) If the difference is less, don't display it - the nickname takes care of
> the balancing on its own.

Dunno. I've considered using the padding for the icon if it fits, and otherwise append it as in the patch, but that makes syncing with the chat view rather complex - I'm just not sure it's worth it.

The above suggestion is easier to implement, but sounds pretty confusing to me: The button will have an icon in some rooms, but not others (depending on the nicks who were talking on the channel). Users are bound to look for some meaning then which just isn't there. ("Oh, that icon wasn't there before, what happened?" is a fair question, and "Someone with a significantly longer nick just wrote a message" a very non-obvious answer ...)


> Ultimately I think ellipsizing longer nicknames (defined by a beauty value)
> is the best way to solve this issue, though..

To be honest, I'm not sure we really need this that badly - servers usually put a more-or-less reasonable restriction on the length already (GNOME: 15 chars, Freenode: 16 chars).
Comment 17 Bastian Ilsø 2016-01-21 22:42:37 UTC
(In reply to Florian Müllner from comment #16)
> (In reply to Bastian Ilsø from comment #15)
> > My concern with the padding is that IMO it makes all nicknames in the
> > chatview appear to weight towards the center. I attached a comparison. But
> > if I'm the only one seeing this, it may just be me. :-)
> 
> Mmh, I'm not sure whether it's an issue or whether it's simply unfamiliar.

ah ok, probably.


> The above suggestion is easier to implement, but sounds pretty confusing to
> me: The button will have an icon in some rooms, but not others (depending on
> the nicks who were talking on the channel). Users are bound to look for some
> meaning then which just isn't there. ("Oh, that icon wasn't there before,
> what happened?" is a fair question, and "Someone with a significantly longer
> nick just wrote a message" a very non-obvious answer ...)

I'm in the drop-the-icon boat now.


> > Ultimately I think ellipsizing longer nicknames (defined by a beauty value)
> > is the best way to solve this issue, though..
> 
> To be honest, I'm not sure we really need this that badly - servers usually
> put a more-or-less reasonable restriction on the length already (GNOME: 15
> chars, Freenode: 16 chars).

I measure a 85px difference between "av" and "bastianilso1994". You say it requires custom widgets though, so I'm less keen..
Comment 18 Florian Müllner 2016-01-21 23:29:13 UTC
Created attachment 319531 [details] [review]
entryArea: Use popover to change nick ...

(In reply to Bastian Ilsø from comment #17)
> I'm in the drop-the-icon boat now.

OK, updating the corresponding patch then.


> > To be honest, I'm not sure we really need this that badly - servers usually
> > put a more-or-less reasonable restriction on the length already (GNOME: 15
> > chars, Freenode: 16 chars).
> 
> I measure a 85px difference between "av" and "bastianilso1994".

Well, you are not suggesting to use "b…", are you? The absolute minimum IMHO is 9 characters (as mentioned in rfc2812) + ellipsis. The difference between 10 and 15 characters is much less impressive :-)


> You say it requires custom widgets though, so I'm less keen..

No, I'm saying it requires custom ellipsization code. We'd need to determine whether or not a nick should be ellipsized (either based on character count or the pixel extents of the layout (the latter is trickier, but more correct)), then insert the truncated nick + ellipsis into the buffer.
Comment 19 Bastian Ilsø 2016-01-22 12:12:14 UTC
Review of attachment 319531 [details] [review]:

(In reply to Florian Müllner from comment #18)
> Created attachment 319531 [details] [review] [review]
> entryArea: Use popover to change nick ...
Two style nits but otherwise looks good to me.


> Well, you are not suggesting to use "b…", are you? The absolute minimum IMHO
> is 9 characters (as mentioned in rfc2812) + ellipsis. The difference between
> 10 and 15 characters is much less impressive :-)

true, then its just around 32px, like removing the icon. :-)


> No, I'm saying it requires custom ellipsization code. We'd need to determine
> whether or not a nick should be ellipsized (either based on character count
> or the pixel extents of the layout (the latter is trickier, but more
> correct)), then insert the truncated nick + ellipsis into the buffer.

ah ok, thanks.

::: src/entryArea.js
@@ +230,2 @@
     _setNick: function(nick) {
+        this._nickLabel.width_chars = Math.max(nick.length, ChatView.MAX_NICK_CHARS)

missing ; ?

@@ +265,3 @@
                            : this._room ? this._room.account.nickname : '';
 
+        this._nickLabel.width_chars = Math.max(nick.length, ChatView.MAX_NICK_CHARS)

missing ; too
Comment 20 Florian Müllner 2016-01-22 12:40:20 UTC
(In reply to Bastian Ilsø from comment #19)
> missing ; ?

Ugh, fixed that up in the "sync maxChars" patch and left it in here - thanks for the catch!
Comment 21 Florian Müllner 2016-01-28 16:45:58 UTC
Attachment 319396 [details] pushed as 9edeae9 - entryArea: Use template for entry area
Attachment 319398 [details] pushed as 6108cda - chatView: Sync maxNickChars with nick button
Attachment 319399 [details] pushed as 3f9eea2 - roomStack: Don't show entry area with placeholder
Attachment 319531 [details] pushed as 2357406 - entryArea: Use popover to change nick ...

Agreed with Bastian on IRC to land this now ...
Comment 22 Florian Müllner 2016-02-02 01:15:56 UTC
*** Bug 755598 has been marked as a duplicate of this bug. ***