GNOME Bugzilla – Bug 760872
Use popover to change nick
Last modified: 2016-02-02 01:15:56 UTC
... and update the overall entry area style as discussed with Bastian and Lapo.
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.
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.
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.
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.
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.
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?
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?
Review of attachment 319399 [details] [review]: looks good to me.
(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.
(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.
(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.
(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?
(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.
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
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.
(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).
(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..
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.
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
(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!
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 ...
*** Bug 755598 has been marked as a duplicate of this bug. ***