GNOME Bugzilla – Bug 768907
chatView: Improve handling of initial pending messages
Last modified: 2016-07-19 00:30:20 UTC
See patches.
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.
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 ...
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.
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.
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.
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.
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.
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.
Review of attachment 331665 [details] [review]: looks good to me.
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?
Review of attachment 331667 [details] [review]: looks good to me.
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)
(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)
Review of attachment 331669 [details] [review]: mmm.....looks good to me (this one is complicated to understand though).
(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);
(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 ...
Review of attachment 331670 [details] [review]: looks good to me
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()?
Review of attachment 331672 [details] [review]: looks good to me.
(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..
(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
Review of attachment 331665 [details] [review]: looks good to me
Review of attachment 331666 [details] [review]: looks good to me
Review of attachment 331667 [details] [review]: looks good to me
Review of attachment 331668 [details] [review]: looks good to me
Review of attachment 331669 [details] [review]: looks good to me
Review of attachment 331671 [details] [review]: looks good to me
Review of attachment 331672 [details] [review]: looks good to me
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