Add reading position marker line to conversations

RESOLVED FIXED in Thunderbird 38.0

Status

defect
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: Mook, Assigned: aleth)

Tracking

({relnote})

unspecified
Thunderbird 38.0
Bug Flags:
in-moztrap +

Thunderbird Tracking Flags

(thunderbird31-, thunderbird38+ fixed)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

7 years ago
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.)
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
(Reporter)

Comment 2

6 years ago
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.)
Attachment #778882 - Flags: review?(florian)
(Reporter)

Comment 3

6 years ago
Posted patch without the extra hr crap (obsolete) — Splinter Review
Take out the silly hr css rules (and the things that made them necessary)
Attachment #778882 - Attachment is obsolete: true
Attachment #778882 - Flags: review?(florian)
Attachment #778883 - Flags: review?(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 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.
Attachment #778883 - Flags: review?(florian) → review-
Posted patch updated patch (obsolete) — Splinter Review
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.
Attachment #8414427 - Flags: review?(aleth)
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.
(Assignee)

Comment 8

5 years ago
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.)
Attachment #8414427 - Flags: review?(aleth) → review-
(Assignee)

Updated

5 years ago
Attachment #778883 - Attachment is obsolete: true
(Assignee)

Comment 9

5 years ago
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.
This is nothing I can do with my low experience. :(
Too late for 31 & not tracking features.
(Assignee)

Comment 12

4 years ago
By popular demand, I'll try to get this in for 38.
Assignee: nobody → aleth
Status: NEW → ASSIGNED
(Assignee)

Updated

4 years ago
(Assignee)

Comment 13

4 years ago
Posted patch portunread.diff (obsolete) — Splinter Review
The code in imconversation.xml is ported, the rest is not.
Attachment #8414427 - Attachment is obsolete: true
Attachment #8565202 - Flags: review?(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).
Attachment #8565202 - Flags: review?(florian) → review-
(Assignee)

Comment 15

4 years ago
Posted patch portunread.diff v2 (obsolete) — Splinter Review
(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.
Attachment #8565202 - Attachment is obsolete: true
Attachment #8565658 - Flags: review?(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.
Attachment #8565658 - Flags: review?(florian) → review+
(Assignee)

Comment 17

4 years ago
Modified commented-out code as requested.
Attachment #8565658 - Attachment is obsolete: true
Attachment #8566179 - Flags: review+
(Assignee)

Updated

4 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 38.0
(Assignee)

Updated

4 years ago
Keywords: relnote
(correction) In Moztrap https://moztrap.mozilla.org/manage/case/16266/ thanks to aleth for the steps
You need to log in before you can comment on or make changes to this bug.