Closed Bug 1412843 Opened 7 years ago Closed 7 years ago

Shortcut to switch through unread conversations

Categories

(Thunderbird :: Instant Messaging, enhancement)

54 Branch
enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 60.0

People

(Reporter: freaktechnik, Assigned: freaktechnik)

References

Details

Attachments

(1 file, 7 obsolete files)

Currently Ctrl + Up and Ctrl + Down let you navigate chat conversations. It would be nice if there was a similar shortcut to go to the next unread conversation. I'd propose Ctrl + Shift Up/Down, as that's currently unused and falls back to switching conversations. The relevant key handler code can be found in https://dxr.mozilla.org/comm-central/rev/c28b7379ca06000b0648ffc615d3344ff4dbabce/mail/components/im/content/chat-messenger-overlay.js#1125 I'll try to add a patch later today.
Severity: normal → enhancement
Attached patch bug1412843-v1.patch (obsolete) — Splinter Review
I haven't found any simple way to not traverse the whole list like it currently does. One way to at least make the endpoint faster would be to have a conditional end point based on the direction, so the Conversations group when going backward and the Online group when going forward. In a perfect world I'd like to only search within the current conversations, but if the selection is outside of them, the shortcut should still work, in my opinion. An alternative would be to not make shift jump to the next unread conversation when not inside the conversations group (as in, not an imconv is highlighted).
Attachment #8923777 - Flags: review?(nhnt11)
Those console.logs should of course be removed, sorry about that.
Attached patch bug1412843-v2.patch (obsolete) — Splinter Review
This is functionally the same as the previous patch, just cleaned up some console logging, obsolete code and code style. The design questions from the previous patch still apply.
Attachment #8923777 - Attachment is obsolete: true
Attachment #8923777 - Flags: review?(nhnt11)
Attachment #8923848 - Flags: review?(nhnt11)
Some ideas to reduce how many items you iterate over: - check for .conv on the node. This property should only exist for conversation items. - you can access the list of all conversation items as a JS array in document.getElementById("conversationsGroup").contacts
(In reply to Florian Quèze [:florian] [:flo] from comment #4) > Some ideas to reduce how many items you iterate over: > - check for .conv on the node. This property should only exist for > conversation items. > - you can access the list of all conversation items as a JS array in > document.getElementById("conversationsGroup").contacts The unreadCount should also only exist for conversations, from what I understand. Or I could check for the localName to be "imconv", like most of the rest of the code seems to do. The problem here is to move relative to the currently selected item. I don't see how the array would help there, I'd have to add code to decide if the current item is not a conversation, and in that case decide where to start, which is a lot more complicated for marginal benefits.
(In reply to Martin Giger [:freaktechnik] from comment #5) > The problem here is to move relative to the currently selected item. I don't > see how the array would help there, I'd have to add code to decide if the > current item is not a conversation, and in that case decide where to start, > which is a lot more complicated for marginal benefits. If you are at the bottom of a contact list that is several hundred contacts long, the benefit is not so marginal. What I'm suggesting is not a lot more complicated: - if the currently selected item is an imconv (checking the local name is OK), start from there - if the currently selected item is not an imconv, start with the first or last item of the array, depending on the direction you are moving in. Stop moving when you are no longer on a conversation item.
(In reply to Florian Quèze [:florian] [:flo] from comment #6) > If you are at the bottom of a contact list that is several hundred contacts > long, the benefit is not so marginal. Ah, correct, or if you search the next unread conv downward and there is none it will move through all items. It is especially bad since this will block the UI from what I understand. > - if the currently selected item is not an imconv, start with the first or > last item of the array, depending on the direction you are moving in. That makes me wonder, should the search "wrap around", so going down from a non-conv item starts from the top? Because currently it does not. If it doesn't wrap around it's more a decision of "are we above or below" conversations. With wrapping around that detection would not be needed. I've seen both behaviors in other apps.
(In reply to Martin Giger [:freaktechnik] from comment #7) > That makes me wonder, should the search "wrap around" Yes :-)
Attached patch bug1412843-v3.patch (obsolete) — Splinter Review
This revision adds wrap-around logic by searching using the array florian suggested. There are two things I'd like to improve: - Quick exit if there are no unread conversations (the total unread count is available somewhere in there, so that should be quite easy) - A better solution than findIndex, if possible. Switched to feedback, since I want to at least add the first item.
Attachment #8923848 - Attachment is obsolete: true
Attachment #8923848 - Flags: review?(nhnt11)
Attachment #8924522 - Flags: feedback?(nhnt11)
Attachment #8924522 - Flags: feedback?(florian)
After investigating a little further, the first point seems hard to do actually, since it the info that already exists is only mentions, not unread messages, those would have to be explicitly counted, which is not worth it. For the second point I've found out about listbox.getIndexOfItem() which seems to perform way better than an array search. However the current patch is broken when searching for conversations without having a conversation selected.
Attached patch bug1412843-v4.patch (obsolete) — Splinter Review
This fixes everything I mentioned earlier that is fixable. It iterates over every current conversation before giving up. It also turns out using the listbox index functions is much, much faster than using an array find(). Not sure if this still applies to a current c-c tip, I haven't rebased this due to an apparently broken tip.
Attachment #8924522 - Attachment is obsolete: true
Attachment #8924522 - Flags: feedback?(nhnt11)
Attachment #8924522 - Flags: feedback?(florian)
Attachment #8926371 - Flags: review?(nhnt11)
Comment on attachment 8926371 [details] [diff] [review] bug1412843-v4.patch Review of attachment 8926371 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch, and many apologies for the long wait for the review! Could you please investigate if we have tests around the conversations list? If we do, it'd be nice to add a test for _selectNextUnreadConversation. Having said that, I think for now, it's very realistic and low-cost to move the logical part of this code into its own function to make it easy to test (i.e. xpcshell) later, at least. Could you look into this, keeping in mind my review comments? Cheers! ::: mail/components/im/content/chat-messenger-overlay.js @@ +1084,5 @@ > _removeObservers: function() { > for (let topic of this._observedTopics) > imServices.obs.removeObserver(this, topic); > }, > + _selectNextUnreadConversation(direction, list) { nit: aDirection, aList @@ +1085,5 @@ > for (let topic of this._observedTopics) > imServices.obs.removeObserver(this, topic); > }, > + _selectNextUnreadConversation(direction, list) { > + const conversations = document.getElementById("conversationsGroup").contacts; Nit: please use let; there's nothing technically wrong with using const here but generally when I see const I tend to think we're defining a literal or something like that (though you can probably change my mind about this by finding precedent). @@ +1086,5 @@ > imServices.obs.removeObserver(this, topic); > }, > + _selectNextUnreadConversation(direction, list) { > + const conversations = document.getElementById("conversationsGroup").contacts; > + const convCount = conversations.length; Nit: let @@ +1089,5 @@ > + const conversations = document.getElementById("conversationsGroup").contacts; > + const convCount = conversations.length; > + if (!convCount) > + return; > + const next = (i) => (i + direction + convCount) % convCount; Nit: let Also, let's be less trusting of callers passing an appropriate value for direction. I propose that this function use positive direction by default, and accept an optional argument "aReverse", which will force negative direction for any truthy value. Something like this: _selectNextUnreadConversation(aList, aReverse) { let direction = aReverse ? -1 : 1; @@ +1098,5 @@ > + if (list.selectedItem.localName === "imconv") { > + start = next(list.selectedIndex - list.getIndexOfItem(conversations[0])); > + count = convCount - 1; > + } > + else if (direction < 0) { nit: } else if (... @@ +1103,5 @@ > + start = convCount - 1; > + } > + > + // Cycle through all conversations until we are at the start again. > + for (let i = start, j = 0; j < convCount; i = next(i), ++j) { We don't really need j in this loop. We could probably modify the for loop to work without j, but I think it'd be more readable with a do/while loop: let i = start; do { if (conversations[i].conv.unreadIncomingMessageCount) { list.selectedItem = conversations[i]; return; } i = next(i); } while (i != start); @@ +1154,5 @@ > if (!accelKeyPressed || > (aEvent.keyCode != aEvent.DOM_VK_DOWN && aEvent.keyCode != aEvent.DOM_VK_UP)) > return; > listbox._userSelecting = true; > + const direction = aEvent.keyCode == aEvent.DOM_VK_DOWN ? 1 : -1; Based on my comments about _selectNextUnreadConversation, let's change this to let reverse = aEvent.keyCode == aEvent.DOM_VK_UP;
Attachment #8926371 - Flags: review?(nhnt11) → review-
Attached patch bug1412843-v5.patch (obsolete) — Splinter Review
I've split the function to find the next unread conversation into two parts, one is independent of the actual markup and operates on an array of conversations. The other one interfaces between the DOM and that function. I don't know if there's an appropriate place to move it to yet (maybe it could even go into core?). The j in the loop is to avoid one iteration when there is a selected conversation, since start is the item before/after the selected one and we don't have to check the currently selected one. On my development machine that was a noticeable improvement in performance (though the fan still spins up to find the next conversation to focus...)
Attachment #8926371 - Attachment is obsolete: true
Attachment #8943584 - Flags: review?(nhnt11)
Attached patch bug1412843-v6.patch (obsolete) — Splinter Review
No more fan spinning, no more j.
Attachment #8943584 - Attachment is obsolete: true
Attachment #8943584 - Flags: review?(nhnt11)
Attachment #8943595 - Flags: review?(nhnt11)
Attached patch bug1412843-v6.1.patch (obsolete) — Splinter Review
Messed up the indents of the new next() implementation and noticed too late. Sorry for the spam!
Attachment #8943595 - Attachment is obsolete: true
Attachment #8943595 - Flags: review?(nhnt11)
Attachment #8943596 - Flags: review?(nhnt11)
Comment on attachment 8943596 [details] [diff] [review] bug1412843-v6.1.patch Review of attachment 8943596 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, this is very close! Just a few comments. ::: mail/components/im/content/chat-messenger-overlay.js @@ +1084,5 @@ > _removeObservers: function() { > for (let topic of this._observedTopics) > imServices.obs.removeObserver(this, topic); > }, > + //TODO move this function away from here and test it. nit: space after // @@ +1088,5 @@ > + //TODO move this function away from here and test it. > + _getNextUnreadConversation(aConversations, aCurrent, aReverse) { > + let convCount = aConversations.length; > + if (!convCount) > + return; nit: newline after this return @@ +1101,5 @@ > + }; > + > + // Find starting point > + let start = 0; > + let count = convCount; This is unused! :) @@ +1103,5 @@ > + // Find starting point > + let start = 0; > + let count = convCount; > + if (aCurrent) { > + start = next(aCurrent); Since we're doing arithmetic with aCurrent, let's also ensure aCurrent is an integer. If it's not, we can either get really angry and throw here, or we can reset it to -1 (since we are using it in the for loop). @@ +1104,5 @@ > + let start = 0; > + let count = convCount; > + if (aCurrent) { > + start = next(aCurrent); > + count = convCount - 1; unused @@ +1110,5 @@ > + start = convCount - 1; > + } > + > + // Cycle through all conversations until we are at the start again. > + for (let i = start; i !== start && i !== aCurrent; i = next(i)) { Are you sure this works? for loops check their conditionals immediately, no? I tested in scratchpad, and the following code results in no output: var start = 0; for (let i = start; i !== start; i++) { console.log(i); if (i > 2) break; } @@ +1114,5 @@ > + for (let i = start; i !== start && i !== aCurrent; i = next(i)) { > + // If there is a conversation with unread messages, select it. > + if (aConversations[i].unreadIncomingMessageCount) > + return i; > + } Let's make sure we return something in case there was no conversation with an unreadIncomingMessageCount. I suggest -1. @@ +1119,5 @@ > + }, > + _selectNextUnreadConversation(aReverse, aList) { > + let conversations = document.getElementById("conversationsGroup").contacts; > + if (!conversations.length) > + return; nit: newline after this return @@ +1125,5 @@ > + let current; > + if (aList.selectedItem.localName === "imconv") > + current = aList.selectedIndex - aList.getIndexOfItem(conversations[0]); > + let newIndex = this._getNextUnreadConversation(rawConversations, current, aReverse); > + if (newIndex) This should be newIndex !== -1 (or otherwise default return value of _getNextUnreadConversation). newIndex will be falsey if it's 0, which is a value we care about.
Attachment #8943596 - Flags: review?(nhnt11)
If we want to get contract levels of safety we should also check that aConversations is an array, and that aCurrent is positive but not bigger than the array length. Other than that feedback should be addressed.
Attachment #8943596 - Attachment is obsolete: true
Attachment #8943671 - Flags: review?(nhnt11)
Comment on attachment 8943671 [details] [diff] [review] bug1412843-v7.patch Review of attachment 8943671 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!
Attachment #8943671 - Flags: review?(nhnt11) → review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/c9eeebf68c0147796f4f3cb3e611c370359fd35b Add keyboard shortcut to cycle through unread channels. r=nhnt11
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 60.0
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: