1412843 - Shortcut to switch through unread conversations

Status: RESOLVED FIXED

(Thunderbird :: Instant Messaging, enhancement)

Description

[Martin Giger [:freaktechnik]]

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.

Comment 1

[Martin Giger [:freaktechnik]]

Attachment: bug1412843-v1.patch

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).

Comment 2

[Martin Giger [:freaktechnik]]

Those console.logs should of course be removed, sorry about that.

Comment 3

[Martin Giger [:freaktechnik]]

Attachment: bug1412843-v2.patch

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.

Comment 4

[Florian Quèze [:florian]]

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

Comment 5

[Martin Giger [:freaktechnik]]

(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.

Comment 6

[Florian Quèze [:florian]]

(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.

Comment 7

[Martin Giger [:freaktechnik]]

(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.

Comment 8

[Florian Quèze [:florian]]

(In reply to Martin Giger [:freaktechnik] from comment #7)

> That makes me wonder, should the search "wrap around"

Yes :-)

Comment 9

[Martin Giger [:freaktechnik]]

Attachment: bug1412843-v3.patch

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.

Comment 10

[Martin Giger [:freaktechnik]]

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.

Comment 11

[Martin Giger [:freaktechnik]]

Attachment: bug1412843-v4.patch

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.

Comment 12

[(inactive) Nihanth Subramanya [: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;

Comment 13

[Martin Giger [:freaktechnik]]

Attachment: bug1412843-v5.patch

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...)

Comment 14

[Martin Giger [:freaktechnik]]

Attachment: bug1412843-v6.patch

No more fan spinning, no more j.

Comment 15

[Martin Giger [:freaktechnik]]

Attachment: bug1412843-v6.1.patch

Messed up the indents of the new next() implementation and noticed too late. Sorry for the spam!

Comment 16

[(inactive) Nihanth Subramanya [: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.

Comment 17

[Martin Giger [:freaktechnik]]

Attachment: bug1412843-v7.patch

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.

Comment 18

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8943671 [details] [diff] [review]
bug1412843-v7.patch

Review of attachment 8943671 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 19

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/c9eeebf68c0147796f4f3cb3e611c370359fd35b
Add keyboard shortcut to cycle through unread channels. r=nhnt11