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 768907 - chatView: Improve handling of initial pending messages
chatView: Improve handling of initial pending messages
Status: RESOLVED FIXED
Product: polari
Classification: Applications
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: Polari maintainers
Polari maintainers
Depends on:
Blocks:
 
 
Reported: 2016-07-17 22:52 UTC by Florian Müllner
Modified: 2016-07-19 00:30 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
chatView: Make sure we insert something when fetching backlog (1.75 KB, patch)
2016-07-17 22:53 UTC, Florian Müllner
committed Details | Review
chatView: Store pending marks in a map (3.66 KB, patch)
2016-07-17 22:53 UTC, Florian Müllner
committed Details | Review
chatView: Split out _createMessage() method (3.27 KB, patch)
2016-07-17 22:53 UTC, Florian Müllner
committed Details | Review
chatView: Factor out _setIndicatorMark() method (2.53 KB, patch)
2016-07-17 22:53 UTC, Florian Müllner
committed Details | Review
chatView: Move handling of 'highlight' marks into insertMessage() (4.72 KB, patch)
2016-07-17 22:53 UTC, Florian Müllner
committed Details | Review
chatView: Store pending logs as messages (2.43 KB, patch)
2016-07-17 22:53 UTC, Florian Müllner
committed Details | Review
chatView: Split out helper function for clarity (2.63 KB, patch)
2016-07-17 22:53 UTC, Florian Müllner
committed Details | Review
chatView: Improve handling of initial pending messages (4.03 KB, patch)
2016-07-17 22:53 UTC, Florian Müllner
committed Details | Review

Description Florian Müllner 2016-07-17 22:52:57 UTC
See patches.
Comment 1 Florian Müllner 2016-07-17 22:53:02 UTC
Created attachment 331665 [details] [review]
chatView: Make sure we insert something when fetching backlog

When the user triggers loading of more logs, the expectation is that
indeed more logs will be inserted in the view. However in order to
only insert nicks once for consecutive messages from the same user,
we keep all log events from the same nick as the first one pending
until we fetch the next batch of log events. As a result we end up
not inserting anything when all events have the same sender. To
meet user expectation as well as maintain our grouping, just fetch
more backlogs immediately in case we didn't insert any logs.
Comment 2 Florian Müllner 2016-07-17 22:53:08 UTC
Created attachment 331666 [details] [review]
chatView: Store pending marks in a map

With the availability of the ECMA6 Map class in gjs, it's time to
phase out the old pattern of using objects as hash tables ...
Comment 3 Florian Müllner 2016-07-17 22:53:13 UTC
Created attachment 331667 [details] [review]
chatView: Split out _createMessage() method

As we display both Tp.Messages we send/receive and Tpl.Events we
get from logs, we use a small intermediate object that abstracts
the differences when passed to insertMessage(). Splitting the
current code to create those objects into a helper methods makes
it easier to keep the two code paths in sync, plus we'll soon
use it from more places.
Comment 4 Florian Müllner 2016-07-17 22:53:19 UTC
Created attachment 331668 [details] [review]
chatView: Factor out _setIndicatorMark() method

It's self-contained functionality, and having it separate improves
the readability of the calling code.
Comment 5 Florian Müllner 2016-07-17 22:53:23 UTC
Created attachment 331669 [details] [review]
chatView: Move handling of 'highlight' marks into insertMessage()

We pause autoscrolling while we have unread highlighted messages.
Currently we create the marks we use to implement this for messages
passed to _insertTpMessage(), which is fine as those are the only
ones that can have an unread state. However we'll soon insert
messages that were pending when we joined a room together with
log events (i.e. from _insertPendingLogs()), so moving the handling
of 'highlight' marks into _insertMessage() allows for sharing that
code.
Comment 6 Florian Müllner 2016-07-17 22:53:28 UTC
Created attachment 331670 [details] [review]
chatView: Store pending logs as messages

We are not using anything from log events besides the properties
we have in the data object we pass to insertMessage(), so we can
just create those immediately and store them instead of the log
events themselves. As we are about to insert pending Tp.Messages
together with logs, this means we can operate on a single data
type.
Comment 7 Florian Müllner 2016-07-17 22:53:33 UTC
Created attachment 331671 [details] [review]
chatView: Split out helper function for clarity

When we display consecutive messages from the same sender in the
chat log, we only insert the nick for the first one. This is trivial
for regular messages we append to the end, but a bit trickier for
log messages where we move backwards. To get the grouping right,
we don't insert all retrieved logs immediately, but only from the
first message that groups differently than the oldest message we
currently have from logs. This works fine, but having the code
directly in insertPendingLogs() distracts a bit from the main
functionality, plus a separate function can be simplified by
using returns instead of nesting if blocks.
Comment 8 Florian Müllner 2016-07-17 22:53:39 UTC
Created attachment 331672 [details] [review]
chatView: Improve handling of initial pending messages

When a room has pending messages when we join, we currently
simply insert them into the log. While simple, there are
two issues with that approach:

 - we will always display the nick for the first pending
   message, so grouping will be wrong if the last message
   from the logs was from the same nick
 - if the message was already seen by the logger, we will
   duplicate it in the chat log (e.g. when mission-control
   restarts us after a crash)

Address those issues by caching pending messages and only
inserting them with the first batch of (de-duplicated) logs
instead of immediately.
Comment 9 Bastian Ilsø 2016-07-18 20:25:25 UTC
Review of attachment 331665 [details] [review]:

looks good to me.
Comment 10 Bastian Ilsø 2016-07-18 20:36:39 UTC
Review of attachment 331666 [details] [review]:

looks good to me, just one question:

::: src/chatView.js
@@ +588,2 @@
             this._autoscroll = false;
+            let mark = [...this._pending.values()].shift();

the "..." looks like a typo to me?
Comment 11 Bastian Ilsø 2016-07-18 20:40:59 UTC
Review of attachment 331667 [details] [review]:

looks good to me.
Comment 12 Bastian Ilsø 2016-07-18 20:55:31 UTC
Review of attachment 331668 [details] [review]:

it came to my mind that the name "indicator-line" feels very generic (especially since we also have 'loading indicator' which is not related). It's the line we use to show that there are new messages right? 'newmessage-line' might describe it better maybe. I guess it could be practical if the 'indicator-line' was chosen in case our use of such as line changes though.

::: src/chatView.js
@@ +779,3 @@
+    _setIndicatorMark: function(iter) {
+        let lineStart = iter.copy();
+        lineStart.set_line_offset(0);

are these two lines the same as:

let lineStart = this._view.buffer.get_start_iter();

?

@@ +792,3 @@
+        }
+
+        let [start, end] = this._getLineIters(iter);

if the code is supposed to be equivalent to the old code, then I think it should be 

 let [start, end] = this._getLineIters(lineStart);

?
(because that is the iter which has been set_line_offset to 0)
Comment 13 Florian Müllner 2016-07-18 20:58:05 UTC
(In reply to Bastian Ilsø from comment #10)
>              this._autoscroll = false;
> +            let mark = [...this._pending.values()].shift();
> 
> the "..." looks like a typo to me?

It's not, it's called the spread operator:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_operator

(Now that I looked up the page, it should allow to make the splice() call in the last patch slightly nicer as well)
Comment 14 Bastian Ilsø 2016-07-18 21:06:48 UTC
Review of attachment 331669 [details] [review]:

mmm.....looks good to me (this one is complicated to understand though).
Comment 15 Florian Müllner 2016-07-18 21:08:41 UTC
(In reply to Bastian Ilsø from comment #12)
> Review of attachment 331668 [details] [review] [review]:
> 
> it came to my mind that the name "indicator-line" feels very generic
> (especially since we also have 'loading indicator' which is not related).
> It's the line we use to show that there are new messages right?
> 'newmessage-line' might describe it better maybe. I guess it could be
> practical if the 'indicator-line' was chosen in case our use of such as line
> changes though.
> 
> ::: src/chatView.js
> @@ +779,3 @@
> +    _setIndicatorMark: function(iter) {
> +        let lineStart = iter.copy();
> +        lineStart.set_line_offset(0);
> 
> are these two lines the same as:
> 
> let lineStart = this._view.buffer.get_start_iter();

No. get_start_iter() returns an iter to the start of the buffer, i.e. the first character of the first line.

We want to apply a tag to the entire line iter points to (not necessarily the first), so we set the line start to 0 (i.e. the first character of the line it's pointing to). The copy is just to not change the passed in iter as side-effect of calling the function - that'd be expected for a method that inserts some text, but just changing the character position within the existing text is a bit random ...



> if the code is supposed to be equivalent to the old code, then I think it
> should be 
> 
>  let [start, end] = this._getLineIters(lineStart);
> 
> ?
> (because that is the iter which has been set_line_offset to 0)

It may be cleaner, but not necessary - iter and lineStart refer to the same line by definition, so the returned [start, end] values are the same by definition. We could even do something like:

let [, end] = this._getLineIters(iter);
buffer.apply_tag(..., lineStart, end);
Comment 16 Florian Müllner 2016-07-18 21:10:41 UTC
(In reply to Bastian Ilsø from comment #14)
> mmm.....looks good to me (this one is complicated to understand though).

What exactly is unclear? It's mostly moving one bit of code elsewhere ...
Comment 17 Bastian Ilsø 2016-07-18 21:11:42 UTC
Review of attachment 331670 [details] [review]:

looks good to me
Comment 18 Bastian Ilsø 2016-07-18 21:20:16 UTC
Review of attachment 331671 [details] [review]:

looks good to me, one question:

::: src/chatView.js
@@ +501,3 @@
+    _getReadyLogs: function() {
+        if (this._logWalker.is_end())
+            return this._pendingLogs.splice(0);

you use "return this._pendingLogs.splice(0);" here, why not use  return []; ? or asked in a different way..

@@ +508,3 @@
+            if (this._pendingLogs[i].nick != nick ||
+                this._pendingLogs[i].messageType != type)
+                return this._pendingLogs.splice(i);

..if i = 0, then it's "return this._pendingLogs.splice(0);", is that equivalent to this._logWalker.is_end()?
Comment 19 Bastian Ilsø 2016-07-18 21:31:22 UTC
Review of attachment 331672 [details] [review]:

looks good to me.
Comment 20 Bastian Ilsø 2016-07-18 21:42:04 UTC
(In reply to Florian Müllner from comment #16)
> (In reply to Bastian Ilsø from comment #14)
> > mmm.....looks good to me (this one is complicated to understand though).
> 
> What exactly is unclear? It's mostly moving one bit of code elsewhere ...

looking at it again helped, nevermind me..
Comment 21 Florian Müllner 2016-07-18 21:44:53 UTC
(In reply to Bastian Ilsø from comment #18)
> ::: src/chatView.js
> @@ +501,3 @@
> +    _getReadyLogs: function() {
> +        if (this._logWalker.is_end())
> +            return this._pendingLogs.splice(0);
> 
> you use "return this._pendingLogs.splice(0);" here, why not use  return [];
> ?

That's something very different. array.splice(index) will remove elements from array starting from index to the end (without further parameters that is). splice() also returns the elements that were removed, so

  this._pendingLogs.splice(0);

clears all elements from this._pendingLogs and returns them. In a way, it's the exact opposite of return [] :-)



> @@ +508,3 @@
> +            if (this._pendingLogs[i].nick != nick ||
> +                this._pendingLogs[i].messageType != type)
> +                return this._pendingLogs.splice(i);
> 
> ..if i = 0, then it's "return this._pendingLogs.splice(0);", is that
> equivalent to this._logWalker.is_end()?

If pos could be 0, that would be the case. However we search the position of the first element that has a different sender or message type from the element at index 0, so by definition pos will at least be 1.

The purpose of the method is to return the logs that are ready to be inserted. We want to keep the grouping by nick, e.g.:

  fmuellner: this was fun
             too little sleep, but fun

When we go backward, we don't know who sent the message *before* the oldest log message we currently got, so if we just went ahead and inserted it anyway, we could end up with:

  fmuellner: this was fun
  fmuellner: too little sleep, but fun

So what the function does is the following:
 - if there are no more log events, return all events we have
   (and reset the log cache to [])
 - find the position of a message that starts a new group and
   return all newer log messages if found (and drop those
   from the cache)
 - if not found (i.e. all cached log messages are from the same
   sender), we return no events and don't touch the array; we'll
   have to fetch more events from the logger in that case
Comment 22 Rares Visalom 2016-07-18 21:49:40 UTC
Review of attachment 331665 [details] [review]:

looks good to me
Comment 23 Rares Visalom 2016-07-18 21:56:43 UTC
Review of attachment 331666 [details] [review]:

looks good to me
Comment 24 Rares Visalom 2016-07-18 22:06:41 UTC
Review of attachment 331667 [details] [review]:

looks good to me
Comment 25 Rares Visalom 2016-07-18 22:17:11 UTC
Review of attachment 331668 [details] [review]:

looks good to me
Comment 26 Rares Visalom 2016-07-18 23:34:30 UTC
Review of attachment 331669 [details] [review]:

looks good to me
Comment 27 Rares Visalom 2016-07-18 23:39:30 UTC
Review of attachment 331670 [details] [review]:

looks good to me
Comment 28 Rares Visalom 2016-07-18 23:48:06 UTC
Review of attachment 331671 [details] [review]:

looks good to me
Comment 29 Rares Visalom 2016-07-19 00:10:58 UTC
Review of attachment 331672 [details] [review]:

looks good to me
Comment 30 Florian Müllner 2016-07-19 00:29:48 UTC
Attachment 331665 [details] pushed as 579be70 - chatView: Make sure we insert something when fetching backlog
Attachment 331666 [details] pushed as 5633573 - chatView: Store pending marks in a map
Attachment 331667 [details] pushed as f0559c3 - chatView: Split out _createMessage() method
Attachment 331668 [details] pushed as f7b3ead - chatView: Factor out _setIndicatorMark() method
Attachment 331669 [details] pushed as cfe392c - chatView: Move handling of 'highlight' marks into insertMessage()
Attachment 331670 [details] pushed as 0a7f508 - chatView: Store pending logs as messages
Attachment 331671 [details] pushed as 2ed8ad3 - chatView: Split out helper function for clarity
Attachment 331672 [details] pushed as 1349979 - chatView: Improve handling of initial pending messages