760762 - Add reading position marker line to conversations

Status: RESOLVED FIXED

(Thunderbird :: Instant Messaging, defect)

Description

[:Mook]

Basically, it would be nice to port Instantbird bug 860 over, so that there can be a line indicating where the last read message was.  This is mainly useful for IRC, but probably also useful for other protocols.

(It's not in c-c build 20120601030203 linux/x86_64.)

Comment 1

[Patrick Cloke [:clokep]]

Note that the chat/ portions of this already exist in c-c. The changes in instantbird/ need to be ported to the UI bits of Thunderbird.

Additionally, there's a few follow up bugs to refine this behavior:
https://bugzilla.instantbird.org/show_bug.cgi?id=1415
https://bugzilla.instantbird.org/show_bug.cgi?id=1380

Comment 2

[:Mook]

Attachment: chat: add unread position marker

This copies over the relevant instantbird code.
The CSS change is because the <hr> is otherwise display: none (and yes, I needed that !important. sadfaces.)

Comment 3

[:Mook]

Attachment: without the extra hr crap

Take out the silly hr css rules (and the things that made them necessary)

Comment 4

[Florian Quèze [:florian]]

Comment on attachment 778883 [details] [diff] [review]
without the extra hr crap

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

You can simplify Footer.html a bit more. I think the rest is OK, but I would like aleth to have a look too as he knows the unread ruler code much better.

::: mail/components/im/messages/Footer.html
@@ +27,3 @@
>      }
>      P_list[i].style.display = "none";
>  

Doesn't this look like it could be simplified to:
for (var i = 1; i < nbEvents - 1; ++i)
  P_list[i].style.display = "none";

@@ +50,3 @@
>      }
>      P_list[i].style.display = "block";
>  

Same here.

@@ +58,3 @@
>      }
>      P_list[i].style.display = "none";
>  

and here.

Comment 5

[Florian Quèze [:florian]]

Comment on attachment 778883 [details] [diff] [review]
without the extra hr crap

Removing the review request as we decided on IRC to finish first bug 896216 which removes the code discussed in comment 4.

Comment 6

[Richard Marti (:Paenglab)]

Attachment: updated patch

This is a updated patch without the hr changes which are already fixed.

I let mook as author as he made the original patch and I only fixed the indentation of a code block.

I tested it and it still works.

Comment 7

[Richard Marti (:Paenglab)]

I set tracking TB 31 as I think this should go to TB 31. The code is the same as in IB and thus good tested.

Comment 8

[aleth [:aleth]]

Comment on attachment 8414427 [details] [diff] [review]
updated patch

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

Thanks for tackling this!

At first glance, this is missing some code to deal with switching between conversations, e.g. this method http://mxr.mozilla.org/comm-central/source/im/content/conversation.xml#1539 which is called by events handled in tabbrowser.xml. The TB equivalent is probably switching to/from conversations in chat-messenger-overlay.js, *and* switching between tabs. (What we capture here are events by which we can estimate that the user has read the messages previously marked as unread.)

Comment 9

[aleth [:aleth]]

What also needs to be ported is code supporting section scroll (which allows scrolling to the unread ruler and to session start rulers) both via keyboard and gestures. Again, the backend for this is already there in /chat.

Comment 10

[Richard Marti (:Paenglab)]

This is nothing I can do with my low experience. :(

Comment 11

[Mark Banner (:standard8)]

Too late for 31 & not tracking features.

Comment 12

[aleth [:aleth]]

By popular demand, I'll try to get this in for 38.

Comment 13

[aleth [:aleth]]

Attachment: portunread.diff

The code in imconversation.xml is ported, the rest is not.

Comment 14

[Florian Quèze [:florian]]

Comment on attachment 8565202 [details] [diff] [review]
portunread.diff

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

::: mail/components/im/content/chat-messenger-overlay.js
@@ +125,5 @@
>      else
>        document.getElementById("contactlistbox").selectedItem = item;
>    },
> +  _onWindowActivated() {
> +    let tabmail = document.getElementById('tabmail');

Why is tabmail in single quotes throughout the patch?

@@ +859,5 @@
>    _updateFocus: function() {
>      let focusId = this._placeHolderButtonId || "contactlistbox";
>      document.getElementById(focusId).focus();
>    },
> +  _onTabStateChange(aIsActive) {

I find this method slightly confusing. Wouldn't it be clearer if it was _getActiveConvView, and returned null in the cases where the current function returns early?

@@ +875,5 @@
> +    else
> +      convView.switchingAwayFromPanel();
> +  },
> +  _onTabActivated() {
> +    chatHandler._onTabStateChange(true);

then this would do:
  let convView = chatHandler._getActiveConvView();
  if (convView)
    convView.switchingToPanel();

::: mail/components/im/content/imconv.xml
@@ +39,5 @@
> +          for (let mutation of aMutations) {
> +            let attr = mutation.attributeName;
> +            if (attr != "selected" || !this.convView || !this.convView.loaded)
> +              return;
> +            if (this.hasAttribute(attr))

use "selected" instead of attr here, and inline mutation.attributeName in the test above.

Btw, why do we even need to test the attribute name? Do you not trust the attributeFilter?

::: mail/components/im/content/imconversation.xml
@@ +206,5 @@
>  
>  /*
> +        if (isUnreadMessage && (!aMsg.conversation.isChat || aMsg.containsNick)) {
> +          this._lastPing = aMsg.who;
> +          this._lastPingTime = aMsg.time;

These 2 variables only exist in im/

@@ +208,5 @@
> +        if (isUnreadMessage && (!aMsg.conversation.isChat || aMsg.containsNick)) {
> +          this._lastPing = aMsg.who;
> +          this._lastPingTime = aMsg.time;
> +          if (Services.prefs.getBoolPref("messenger.options.getAttentionOnNewMessages") &&
> +              Interruptions.requestInterrupt("new-text", aMsg, "get-attention"))

The interruptions manager only exists in im/.

@@ +1104,5 @@
> +        if (this._visibleTimer)
> +          return;
> +
> +        // Start a timer to detect if the tab has been visible to the
> +        // user for long enough to actually be seen (as opposed to the

I wonder why we need a timer for this, rather than just saving a timestamp value to compare later.

@@ +1107,5 @@
> +        // Start a timer to detect if the tab has been visible to the
> +        // user for long enough to actually be seen (as opposed to the
> +        // tab only being visible "accidentally in passing").
> +        delete this._wasVisible;
> +        this._visibleTimer = setTimeout(function() {

If you want to keep it as close to the im/ version as possible ignore this comment, but this really looks like it wants to be an arrow function rather than a .bind(this).

Comment 15

[aleth [:aleth]]

Attachment: portunread.diff v2

(In reply to Florian Quèze [:florian] [:flo] from comment #14)
> ::: mail/components/im/content/imconv.xml
> @@ +39,5 @@
> > +          for (let mutation of aMutations) {
> > +            let attr = mutation.attributeName;
> Btw, why do we even need to test the attribute name? Do you not trust the
> attributeFilter?

An earlier version of the patch observed more attributes, so this was dead code.

> ::: mail/components/im/content/imconversation.xml
> @@ +206,5 @@
> >  
> >  /*
> > +        if (isUnreadMessage && (!aMsg.conversation.isChat || aMsg.containsNick)) {
> > +          this._lastPing = aMsg.who;
> > +          this._lastPingTime = aMsg.time;
> 
> These 2 variables only exist in im/
> The interruptions manager only exists in im/.

This is currently commented out code in TB. I merely updated it to keep this function in sync with conversation.xml, to ease future ports.
 
> @@ +1104,5 @@
> > +        // Start a timer to detect if the tab has been visible to the
> > +        // user for long enough to actually be seen (as opposed to the
> 
> I wonder why we need a timer for this, rather than just saving a timestamp
> value to compare later.

IB could indeed get by without it, but TB needs it as it has to call this.tab.update.
 
> @@ +1107,5 @@
> > +        this._visibleTimer = setTimeout(function() {
> 
> If you want to keep it as close to the im/ version as possible ignore this
> comment, but this really looks like it wants to be an arrow function rather
> than a .bind(this).

Right, I kept it to minimize introducing differences. Any changes I made compared with IB are marked by comments.

Comment 16

[Florian Quèze [:florian]]

Comment on attachment 8565658 [details] [diff] [review]
portunread.diff v2

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

::: mail/components/im/content/imconversation.xml
@@ +208,5 @@
> +        if (isUnreadMessage && (!aMsg.conversation.isChat || aMsg.containsNick)) {
> +          this._lastPing = aMsg.who;
> +          this._lastPingTime = aMsg.time;
> +          if (Services.prefs.getBoolPref("messenger.options.getAttentionOnNewMessages") &&
> +              Interruptions.requestInterrupt("new-text", aMsg, "get-attention"))

If you update this piece of TB dead code, I think you should adapt it to match what it would be in TB if it wasn't commented out, so the 2 comments I had about this block in comment 14 still apply.

Comment 17

[aleth [:aleth]]

Attachment: portunread.diff v3

Modified commented-out code as requested.

Comment 18

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/3d6e9bd67cd9

Comment 19

[Wayne Mery (:wsmwk)]

In Moztrap https://moztrap.mozilla.org/manage/case/16267/

Comment 20

[Wayne Mery (:wsmwk)]

(correction) In Moztrap https://moztrap.mozilla.org/manage/case/16266/ thanks to aleth for the steps