GNOME Bugzilla – Bug 759120
chatView: Use symbolic arrows rather than ellipsis to indicate that status summaries are expandable
Last modified: 2016-02-08 22:50:11 UTC
Created attachment 316877 [details] mockup of the pan-end and pan-down in action on status summaries It would be more coherent with other GTK+ widgets elements if we used the pan-end[0] and pan-down symbolic arrows[0] instead of the current ellipsis "(...)". Attached mockup shows how it could look like. This could possibly be done by inserting a GtkArrow[2] widget into the text view instead. [0]: https://git.gnome.org/browse/adwaita-icon-theme/tree/Adwaita/scalable/actions/pan-down-symbolic.svg [1]: https://git.gnome.org/browse/adwaita-icon-theme/tree/Adwaita/scalable/actions/pan-end-symbolic.svg [2]: https://developer.gnome.org/gtk3/stable/GtkArrow.html
Created attachment 317262 [details] [review] chatVIew: Use symbolic arrows instead of ellipsis to git apply this patch, please apply the attached patch in bug 759032 first.
Created attachment 317263 [details] [review] chatView: Use symbolic arrows instead of ellipsis to git apply this patch, please apply the attached patch in bug 759032 first.
Review of attachment 317263 [details] [review]: Alternatives to inserting a widget: - use appropriate UTF-8 symbols - don't actually insert an arrow, just draw it with cairo ::: src/chatView.js @@ +845,3 @@ stats.push(ngettext("%d user left", "%d users left", this._statusCount.left).format(this._statusCount.left)); + this._insertStatusCompressedwithTags(buffer.get_iter_at_mark(headerMark), 1) that method is weird - better keep using the existing method for inserting the text, and add something new for the widget: let tags = [this._lookupTag('status'), headerTag]; let iter = buffer.get_iter_at_mark(headerMark); this._insertWithTags(iter, stats.join(", "), tags); this._insertWidgetWithTags(iter, headerTag._stack, tags); 2) the current code destroys and creates the widget every time a new status message arrives, which is really ineffective; better to add the arrow just once when creating the header, then make sure to not delete it on updates
Created attachment 317969 [details] [review] chatView: Use unicode arrows instead of ellipsis
Florian are you aware of this patch? :)
Review of attachment 317969 [details] [review]: It would be interesting to know what's more expensive - adding more tags, or iterating over the buffer to search for a particular tag (e.g. modify the header when the expansion state changes instead of toggling the visibility). In any case, you shouldn't use more tags than necessary: ::: src/chatView.js @@ +861,3 @@ let headerTagName = 'status-compressed' + this._state.lastStatusGroup; + let headerArrowRightTagName = 'status-arrow-right' + this._state.lastStatusGroup; + let headerArrowDownTagName = 'status-arrow-down' + this._state.lastStatusGroup; You don't need arrow-down, you can use the existing groupTag instead @@ +880,3 @@ groupTag.invisible = !groupTag.invisible; + headerArrowDownTag.invisible = !headerArrowDownTag.invisible; + headerArrowRightTag.invisible = !headerArrowRightTag.invisible; I prefer groupTag.bind_property('invisible', arrowTag, 'invisible', GObject.BindingFlags.INVERT_BOOLEAN); to make it explicit that the tag should be invisible when the other is visible @@ +917,3 @@ "%d users left", this._statusCount.left).format(this._statusCount.left)); + let iter = buffer.get_iter_at_mark(headerMark); + this._insertWithTags(iter, I'd add a tags variable as well: let tags = [this._lookupTag('status'), headerTag]; this._insertWithTags(iter, stats.join(', ') + ' ', tags); this._insertWithTags(iter, '\u25B6', tags.concat(arrowTag)); this._insertWithTags(iter, '\u25BC', tags.concat(groupTag));
(In reply to Florian Müllner from comment #6) > + let headerArrowRightTagName = 'status-arrow-right' + > this._state.lastStatusGroup; Also: arrow-right probably needs to be arrow-left in RTL locales.
Created attachment 320602 [details] [review] chatView: Use unicode arrows in status summaries Instead of ellipsis that we use currently to indicate that status summaries are expandable, use unicode arrows, which are more visually appealing.
Review of attachment 320602 [details] [review]: ::: src/chatView.js @@ +374,3 @@ let context = this.get_style_context(); + + this._textDirection = context.get_direction(); This method is deprecated. It's also a bit odd to use the direction from the style context outside of drawing code, see below @@ +953,3 @@ "%d users left", this._statusCount.left).format(this._statusCount.left)); + let iter = buffer.get_iter_at_mark(headerMark); + let tags = [this._lookupTag('status'), headerTag]; Odd grouping @@ +956,3 @@ + + // TODO: How do we update the arrow direction when text direction change? + if (this._textDirection == Gtk.TextDirection.LTR) { Use this.get_direction() instead. @@ +963,3 @@ + this._insertWithTags(iter, '\u25C0', tags.concat(headerArrowTag)); + this._insertWithTags(iter, '\u25BC', tags.concat(groupTag)); + this._insertWithTags(iter, stats.join(', ') + ' ', tags); Ah, tricky - the position of the arrow depends on two factors: - whether it's inserted before or after the text - the text direction of the text So if the text in stats is properly translated, you end up inserting the arrow on the right! Something like this is probably the best we can do: let headerArrow = this.get_direction() == Gtk.TextDirection.LTR ? '\u25B6' : '\u25C0';
(In reply to Florian Müllner from comment #9) > Review of attachment 320602 [details] [review] [review]: > > ::: src/chatView.js > @@ +963,3 @@ > + this._insertWithTags(iter, '\u25C0', > tags.concat(headerArrowTag)); > + this._insertWithTags(iter, '\u25BC', tags.concat(groupTag)); > + this._insertWithTags(iter, stats.join(', ') + ' ', tags); > > Ah, tricky - the position of the arrow depends on two factors: > - whether it's inserted before or after the text > - the text direction of the text I agree. If direction is RTL we insert in beginning ( Because we align left in that case our status summaries ) , for LTR at right ( We align right our status summaries). > So if the text in stats is properly translated, you end up inserting the > arrow on the right! err.. if text is transalted and supposed to be RTL, we insert arrow on left > > Something like this is probably the best we can do: > let headerArrow = this.get_direction() == Gtk.TextDirection.LTR ? '\u25B6' > : '\u25C0'; So we insert arrow always to the right?
(In reply to Kunaal Jain from comment #10) > (In reply to Florian Müllner from comment #9) > > Something like this is probably the best we can do: > > let headerArrow = this.get_direction() == Gtk.TextDirection.LTR ? '\u25B6' > > : '\u25C0'; > > So we insert arrow always to the right? No, we insert the arrow after the text - if the script is LTR (e.g. latin), that's on the right, but if the script is RTL (Arabic, Hebrew) it's on the left. To see what I mean, you can overwrite the real stats value with something like stats = ['המשתמשים 2 הצטרף']; But after thinking about it a bit more, there is actually something else we can do: let headerText = stats.join(', '); let baseDir = Pango.find_base_dir(headerText, -1); this._insertWithTags(iter, headerText, tags); this._insertWithTags(iter, baseDir == Pango.Direction.LTR ? '▶' : '◀', tags.concat(headerArrowTag)); this._insertWithTags(iter, '▼', tags.concat(groupTag)); That will pick the arrow based on the direction of the actual text instead of the general text direction of the widget - namely in an RTL locale, we'll only use the right-pointing arrow if the message is actually translated. I think using the widget's text direction should be good enough as we expect messages to be translated correctly(*) and it's consistent with GtkExpander, but if you want to do the above instead, that's fine with me as well. (*) in this particular case, the string was added this cycle, so it's not uncommon for locales to not have a translation for it yet - translations tend to speed up towards the end of the cycle when strings are less likely to be changed, thus avoiding unnecessary work
(In reply to Florian Müllner from comment #11) > That will pick the arrow based on the direction of the actual text instead > of the general text direction of the widget - namely in an RTL locale, we'll > only use the right-pointing arrow if the message is actually translated. I > think using the widget's text direction should be good enough as we expect > messages to be translated correctly(*) and it's consistent with GtkExpander, > but if you want to do the above instead, that's fine with me as well. Lets do it with Pango. Handles all the cases of widget's text direction as well as untranslated text direction. Out of curiosity, can you tell more about the consistency with GtkExpander? > (*) in this particular case, the string was added this cycle, so it's not > uncommon for locales to not have a translation for it yet - translations > tend to speed up towards the end of the cycle when strings are less likely > to be changed, thus avoiding unnecessary work Solves my concern about status compressed strings not been translated yet.
Created attachment 320659 [details] [review] chatView: Use unicode arrows in status summaries Instead of ellipsis that we use currently to indicate that status summaries are expandable, use unicode arrows, which are more visually appealing.
Review of attachment 320659 [details] [review]: (In reply to Kunaal Jain from comment #12) > Out of curiosity, can you tell more about the consistency with GtkExpander? I just meant that it uses a left/right arrow based solely on text direction, i.e. regardless of whether the label is actually in an RTL script or not.
Review of attachment 320659 [details] [review]: Pushed to master with commit 67d295a