Closed Bug 954293 Opened 10 years ago Closed 10 years ago

Add reading position marker line to conversations

Categories

(Instantbird Graveyard :: Conversation, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

References

Details

Attachments

(3 files, 13 obsolete files)

*** Original post on bio 860 at 2011-06-27 17:56:00 UTC ***

Most IRC clients have the following useful feature: After a certain idle time has passed, a red line across the chat window marks the position up to which the chat has been read by the user. This allows one to find where one left off easily when returning to a conversation.
*** Original post on bio 860 at 2011-06-27 17:58:20 UTC ***

Definitely wanted!
Status: UNCONFIRMED → NEW
Ever confirmed: true
*** Original post on bio 860 at 2011-07-20 16:56:34 UTC ***

It should also appear whenever a tab is put into the background, so when you switch back to a conversation you can easily see where you left off.
Summary: Add reading position marker line to (IRC) chat window → Add reading position marker line to conversations
*** Original post on bio 860 by Andreas Schleifer <webmaster AT segaja.de> at 2012-01-26 09:44:00 UTC ***

I would like to see these features aswell.
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 860 as attmnt 1154 at 2012-02-06 10:45:00 UTC ***

Thanks to clokep for the original code in setUnreadRuler (lifted from his add-on) :)

There should be a follow-up patch which adds the unread ruler as a section to section scroll, but I thought this had better be OK first.
Attachment #8352899 - Flags: review?(florian)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 860 as attmnt 1155 at 2012-02-06 10:59:00 UTC ***

Fix diff error.
Attachment #8352900 - Flags: review?(florian)
Comment on attachment 8352899 [details] [diff] [review]
Patch

*** Original change on bio 860 attmnt 1154 at 2012-02-06 10:59:46 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352899 - Attachment is obsolete: true
Attachment #8352899 - Flags: review?(florian)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 860 as attmnt 1156 at 2012-02-06 12:42:00 UTC ***

This is clearer and fixes a possible edge case.
Attachment #8352901 - Flags: review?(florian)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 860 as attmnt 1157 at 2012-02-06 12:44:00 UTC ***

Forgot to obsolete the previous patch.
Attachment #8352902 - Flags: review?(florian)
Comment on attachment 8352900 [details] [diff] [review]
Patch

*** Original change on bio 860 attmnt 1155 at 2012-02-06 12:44:57 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352900 - Attachment is obsolete: true
Attachment #8352900 - Flags: review?(florian)
Comment on attachment 8352901 [details] [diff] [review]
Patch

*** Original change on bio 860 attmnt 1156 at 2012-02-06 12:44:57 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352901 - Attachment is obsolete: true
Attachment #8352901 - Flags: review?(florian)
*** Original post on bio 860 at 2012-02-06 12:48:03 UTC ***

(In reply to comment #4)
> Created attachment 8352899 [details] [diff] [review] (bio-attmnt 1154) [details]
> Thanks to clokep for the original code in setUnreadRuler (lifted from his
> add-on) :)
Glad my code was useful to you!

The issue with this code, however, is that the message style can't really handle it: it's kind of a hack. I think a bit more work would need to be done in order for it to properly know where the last read point was; hopefully someone with more knowledge about that code and the "spec" can add pointers here. :)
*** Original post on bio 860 at 2012-02-06 12:52:07 UTC ***

(In reply to comment #8)
> The issue with this code, however, is that the message style can't really
> handle it: it's kind of a hack. I think a bit more work would need to be done
> in order for it to properly know where the last read point was; hopefully
> someone with more knowledge about that code and the "spec" can add pointers
> here. :)

I'm not entirely sure what you mean, apart from the fact that the ruler is outside the message style. But this seems right to me as you don't want the ruler to be logged, for example. Note the logic of when to place a ruler is a bit different here from your add-on.
*** Original post on bio 860 at 2012-02-06 13:03:55 UTC ***

However, the <hr> tag should probably be assigned a class so that message styles can style it if they want to.
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 860 as attmnt 1158 at 2012-02-06 14:16:00 UTC ***

+          // Remove node that indicates where the next message should be
+          // inserted, if it exists.
+          let insert = doc.getElementById("insert");
+          if (insert)
+            insert.parentNode.removeChild(insert);

Removed this, which was inherited from clokep's add-on. It seems a bit hacky and I don't understand why it was necessary. It certainly seems to work well without it.

(In reply to comment #10)
> However, the <hr> tag should probably be assigned a class so that message
> styles can style it if they want to.

On the other hand, styling the <hr> tag would be enough, so introducing a class shouldn't be necessary really.
Attachment #8352903 - Flags: review?(florian)
Comment on attachment 8352902 [details] [diff] [review]
Patch

*** Original change on bio 860 attmnt 1157 at 2012-02-06 14:16:39 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352902 - Attachment is obsolete: true
Attachment #8352902 - Flags: review?(florian)
Comment on attachment 8352903 [details] [diff] [review]
Patch

*** Original change on bio 860 attmnt 1158 at 2012-02-06 14:43:36 UTC ***

Discovered a problem.
Attachment #8352903 - Flags: review?(florian)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 860 as attmnt 1159 at 2012-02-06 15:02:00 UTC ***

Under some circumstances, the ruler would 'move downwards' through system messages as the unread flag is not set for those. This became visible when the selected tab did not have focus.

System messages are now treated consistently everywhere with section scroll etc I believe (i.e. don't treat them as "real messages").
Attachment #8352904 - Flags: review?(florian)
Comment on attachment 8352903 [details] [diff] [review]
Patch

*** Original change on bio 860 attmnt 1158 at 2012-02-06 15:02:30 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352903 - Attachment is obsolete: true
*** Original post on bio 860 at 2012-02-06 15:22:34 UTC ***

(In reply to comment #11)
> Created attachment 8352903 [details] [diff] [review] (bio-attmnt 1158) [details]
> Patch
> 
> +          // Remove node that indicates where the next message should be
> +          // inserted, if it exists.
> +          let insert = doc.getElementById("insert");
> +          if (insert)
> +            insert.parentNode.removeChild(insert);
> 
> Removed this, which was inherited from clokep's add-on. It seems a bit hacky
> and I don't understand why it was necessary. It certainly seems to work well
> without it.
These essentially do what your call to this.removeUnreadRuler(); (two lines below that) does. It ensures there is only ONE unread marker.

> (In reply to comment #10)
> > However, the <hr> tag should probably be assigned a class so that message
> > styles can style it if they want to.
> 
> On the other hand, styling the <hr> tag would be enough, so introducing a class
> shouldn't be necessary really.

It already has an ID ("last-viewed-ruler"), so we don't need to introduce a class. The message style just needs to add some rules about how to handle hr#last-viewed-ruler.
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 860 as attmnt 1160 at 2012-02-06 23:11:00 UTC ***

No functional changes, just removed some duplication, which also makes the logic more obvious.
Attachment #8352905 - Flags: review?(florian)
Comment on attachment 8352904 [details] [diff] [review]
Patch

*** Original change on bio 860 attmnt 1159 at 2012-02-06 23:11:27 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352904 - Attachment is obsolete: true
Attachment #8352904 - Flags: review?(florian)
Comment on attachment 8352905 [details] [diff] [review]
Patch

*** Original change on bio 860 attmnt 1160 at 2012-02-07 01:44:37 UTC ***

This looks good. I do wonder if setUnreadFlag should be moved further down where it's used and if the other statements can just be nested like:
        if (this.tab && aMsg.incoming && !aMsg.system &&
            (!this.tab.selected || !document.hasFocus())) {
          if (!this.tab.hasAttribute("unread") && this._isAfterFirstRealMessage)
            this.browser.setUnreadRuler();
          if (conv.isChat && aMsg.containsNick)
            this.tab.setAttribute("attention", "true");
          this.tab.setAttribute("unread", "true");

I'm not broken up about it either way though. flo, any opinions?
Attachment #8352905 - Flags: review?(florian) → review+
*** Original post on bio 860 at 2012-02-07 01:47:30 UTC ***

(In reply to comment #16)
You can't move it that far down (at least not just like that) as the ruler needs to be inserted before the message itself.
*** Original post on bio 860 at 2012-02-08 01:06:57 UTC ***

Consider the following a WIP, just putting this here fwiw so it can be discussed:


Here's what I think the design goals are:
1 - something unobtrusive but easy to spot when you scroll to it (eventually, by section scroll) that marks the first unread message
2 - that doesn't get in your way or distract when you don't need it
3 - that works OK with most message styles as default but can be tweaked by these to look really good.

Unobtrusive because there is no big distinction between read and unread messages in an active conversation (the software can't actually know what's been read, it's just "new arrivals"). This means setting 'read' messages to look like context messages, or making unread messages look drastically different (yet another message type?) seems too much. 

tl;dr: It's like a bookmark.


(Which is why I was a bit surprised by proposals to animate the background as they would seem to violate 2) unless it is done in a very subtle way. I probably didn't understand the idea properly ;) )


Issues with the present patch:

--- Inserting a ruler breaks bubbles in two.

Personally I don't think this is a bad thing. After all, in a situation where a ruler appears, there was definitely a gap in one's focus - why not reflect this by starting a new bubble?

Technically, the big advantage is that it is quite clean to place something between two message blocks, while figuring out where to "overlay" something is much harder. 

I've played around with position:absolute divs with a border (hr's can't carry position:absolute), inserted above or below or inside a message, and it seems impossible to get it into the right spot for both within bubbles and between bubbles unless one distinguishes between Content and NextContent messages, and even then I have no idea how one would do the right thing without being really message-style specific. (Also position:absolute seems much more hackish to me than a dynamically inserted ruler...)

A ruler between messages can be styled to look OK for practically any message style (or display:none if desired). 

I haven't come up with a good alternative yet that would work out of the box for most message styles. My current suspicion is that without breaking 'bubbles', it's impossible to offer something 'default'.

** The only negative I can see is that if a particular message style would prefer to do things without splitting bubbles, it can't.


--- Should maybe place an attribute ("first-unread" or something) on the first unread message, so the message style can do something more elaborate with that.


--- Lacks default styling, and styling in the default message styles. 


Maybe if there was a really good, acceptable-to-everyone idea for how things should look in Bubbles it would be clearer where to take this patch.


Also, I don't think this should be an add-on as being able to (section) scroll quickly to the first new message is core functionality that I think is really missing from IB.
*** Original post on bio 860 as attmnt 1163 at 2012-02-08 23:06:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
*** Original post on bio 860 at 2012-02-09 08:41:17 UTC ***

(In reply to comment #18)

> (Which is why I was a bit surprised by proposals to animate the background as
> they would seem to violate 2) unless it is done in a very subtle way. I
> probably didn't understand the idea properly ;) )

I imagined it was meant like the new messages in Facebooks webchat/message system that start with a light yellow background and fade to white. It's not distracting (at this place) at all in my opinion.

> Issues with the present patch:
> 
> --- Inserting a ruler breaks bubbles in two.
> 
> Personally I don't think this is a bad thing. After all, in a situation where a
> ruler appears, there was definitely a gap in one's focus - why not reflect this
> by starting a new bubble?

I don't see a real problem here either. 

(In reply to comment #19)
> Created attachment 8352908 [details] (bio-attmnt 1163) [details]
> Screenshot: Bubbles with simple styled hr

Looks good :)
*** Original post on bio 860 at 2012-02-11 21:28:41 UTC ***

I have some code which glues split bubbles back together again when the ruler is removed. I am not sure if that is better though. (Maybe if one added a transition animation of some sort?)
Attached patch WIP with rejoining bubbles (obsolete) — Splinter Review
*** Original post on bio 860 as attmnt 1169 at 2012-02-12 02:02:00 UTC ***

Here's the code with the rejoining bubbles - it works but comes at a cost (because of the way the message html works). I added a little CSS transition which makes it look smoother in the cases where it is visible. 

Not submitting this as a patch as some aspects of the code could be nicer, but ready to be tested.
Attached patch WIP with rejoining bubbles (obsolete) — Splinter Review
*** Original post on bio 860 as attmnt 1170 at 2012-02-12 02:21:00 UTC ***

Don't affect /chat/modules.
Comment on attachment 8352915 [details] [diff] [review]
WIP with rejoining bubbles

*** Original change on bio 860 attmnt 1169 at 2012-02-12 02:21:25 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352915 - Attachment is obsolete: true
*** Original post on bio 860 at 2012-02-12 15:09:46 UTC ***

Another possibility: Allow a message style to disable the insertion of a ruler (if that is chosen as the default behaviour) altogether via an info.plist key.
*** Original post on bio 860 at 2012-02-13 23:15:45 UTC ***

(In reply to comment #20)
> (In reply to comment #18)
> 
> > (Which is why I was a bit surprised by proposals to animate the background as
> > they would seem to violate 2) unless it is done in a very subtle way. I
> > probably didn't understand the idea properly ;) )
> 
> I imagined it was meant like the new messages in Facebooks webchat/message
> system that start with a light yellow background and fade to white. It's not
> distracting (at this place) at all in my opinion.

Something like this would be complementary, not a replacement, as it is an animation that would be displayed at the moment when new messages arrive. After the animation is 'finished', you would still need a way to tell which is the first "unread" message.

So for the purposes of this bug, the only alternative I can think of to some kind of ruler would be to add a way for "unread messages" to be styled differently (e.g. a different coloured background or shade of grey behind the bubbles) if the message style so desires.

To do this, one would have to attach an attribute to all new messages which is then removed when the 'ruler' (in the present setup) is removed. A message style could do what it liked with this info.

Splitting bubbles (if possibly temporarily) as in the current patches would probably make this doable without major JS I think.

Caveat: You would then be in the situation where this "new message" background would be the one you would see most often while actively having a conversation or chat, if it didn't fade away completely after some time period. If it does fade away, you still need to display a ruler of some sort.
*** Original post on bio 860 at 2012-02-13 23:47:47 UTC ***

(In reply to comment #25)
 
> So for the purposes of this bug, the only alternative I can think of to some
> kind of ruler would be to add a way for "unread messages" to be styled
> differently (e.g. a different coloured background or shade of grey behind the
> bubbles) if the message style so desires.
>
> To do this, one would have to attach an attribute to all new messages which is
> then removed when the 'ruler' (in the present setup) is removed. A message
> style could do what it liked with this info.

You can try something like this rule here to style all sibling elements following the marker with a slightly darker background for example:

#last-viewed-ruler ~ * {
  background-color: rgba(0, 0, 0, 0.1);
}

I'm interested to hear if this works :)
*** Original post on bio 860 at 2012-02-13 23:59:56 UTC ***

(In reply to comment #26)
> You can try something like this rule here to style all sibling elements
> following the marker with a slightly darker background for example:
> 
> #last-viewed-ruler ~ * {
>   background-color: rgba(0, 0, 0, 0.1);
> }
> 
> I'm interested to hear if this works :)

Works with the addition of an !important, which one could probably get rid of somehow...

Currently, when applied to Bubbles, replaces the previous background color of the bubble rather than darken the background. But it seems (maybe with additional nesting) this would indeed be enough to allow message styles to theme what follows the ruler differently without requiring an explicit attribute.
*** Original post on bio 860 at 2012-02-14 00:21:29 UTC ***

(In reply to comment #26)
> #last-viewed-ruler ~ * {
>   background-color: rgba(0, 0, 0, 0.1);
> }
> 
> I'm interested to hear if this works :)

Yup, this looks great, especially as the message style can use a much more specific rule than * :)
Attached file Styling example
*** Original post on bio 860 as attmnt 1171 at 2012-02-14 00:32:00 UTC ***

Appending this example by Mic as it demonstrates some possibilities for message styles... all at once :P
*** Original post on bio 860 at 2012-02-14 10:39:47 UTC ***

(In reply to comment #25)
> (In reply to comment #20)
> > (In reply to comment #18)
> > 
> > > (Which is why I was a bit surprised by proposals to animate the background as
> > > they would seem to violate 2) unless it is done in a very subtle way. I
> > > probably didn't understand the idea properly ;) )
> > 
> > I imagined it was meant like the new messages in Facebooks webchat/message
> > system that start with a light yellow background and fade to white. It's not
> > distracting (at this place) at all in my opinion.
> 
> Something like this would be complementary, not a replacement, as it is an
> animation that would be displayed at the moment when new messages arrive. After
> the animation is 'finished', you would still need a way to tell which is the
> first "unread" message.

The animation would not start when the messages arrive but when they are marked as read.
*** Original post on bio 860 at 2012-02-14 12:14:53 UTC ***

(In reply to comment #30)
> > Something like this would be complementary, not a replacement, as it is an
> > animation that would be displayed at the moment when new messages arrive. After
> > the animation is 'finished', you would still need a way to tell which is the
> > first "unread" message.
> 
> The animation would not start when the messages arrive but when they are marked
> as read.

That's a possible variation (and could I think be easily implemented by setting an attribute on the ruler when the unread flag is removed from a tab, which the message style can transition on). 

It is still complementary however, as you still want to be able to find the first 'unread' message after this animation is over.
*** Original post on bio 860 at 2012-02-14 12:24:10 UTC ***

Again, the goal here is to come up with a patch that supports being themed in all desired ways by the message styles.
Comment on attachment 8352916 [details] [diff] [review]
WIP with rejoining bubbles

*** Original change on bio 860 attmnt 1170 at 2012-03-22 21:33:01 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352916 - Flags: review-
*** Original post on bio 860 at 2012-03-22 21:33:01 UTC ***

Comment on attachment 8352916 [details] [diff] [review] (bio-attmnt 1170)
WIP with rejoining bubbles

For a WIP it seems quite good. Rejoining bubbles is difficult!

The code here seems to make dangerous assumptions about message themes.

[1] That next messages are always child nodes of the first message. This won't be the case for a theme built like this:
content.html: <p>...</p><div id="insert"/>
nextContent.html: <p class="next">...</p><div id="insert"/>
(We would have such a theme if we decided that in Simple we want the timestamps of the next messages to be in gray instead of black.)

[2] That all the DOM nodes related to a message are a child of a single node.
A theme like this doesn't respect this assumption:
content.html: <span class="username">%username%</span>%message<br/>

[3] That the insert node has a previous sibling.
A theme like this doesn't respect this assumption:
content.html: <div>...<div class="nextMessages"><div id="insert"/></div></div>
nextContent.html: ...

Warning: I haven't outlined the place in the code making this third assumption as I found this late while writing the comment.

I had to do an almost complete review of the patch to find this so I'll also give you some nits to fix:

>diff -u /instantbird/content/tabbrowser.xml ./tabbrowser.xml
>--- /instantbird/content/tabbrowser.xml 2012-02-06 11:29:07.000000000 +0100
>+++ ./tabbrowser.xml  2012-02-06 11:30:01.000000000 +0100
>@@ -2071,8 +2071,10 @@
>         if (event.attrName != "selected")
>           return;
>
>-        if (event.attrChange == event.REMOVAL)
>+        if (event.attrChange == event.REMOVAL) {
>           this.linkedConversation.removeAttribute("selected");

Add a comment here explaining that we remove the unread marker when the conversation is no longer visible rather than when it's marked as read so that the user doesn't notice the ruler disappearing.

>+          this.linkedBrowser.removeUnreadRuler();
>+        }
>         else
>           this.linkedConversation.setAttribute("selected", event.newValue);
>        ]]>
>diff -u /chat/content/convbrowser.xml ./convbrowser.xml
>--- /chat/content/convbrowser.xml	2012-02-06 11:30:53.000000000 +0100
>+++ ./convbrowser.xml 2012-02-12 03:16:59.000000000 +0100
>@@ -291,9 +290,10 @@
>       <method name="appendMessage">
>         <parameter name="aMsg"/>
>         <parameter name="aContext"/>
>+        <parameter name="aFirstUnread"/>
>         <body>
>           <![CDATA[
>-            this._pendingMessages.push({msg: aMsg, context: aContext});
>+            this._pendingMessages.push({msg: aMsg, context: aContext, firstUnread: aFirstUnread});

nit: This line seems > 80 chars, break it.

>             if (this._messageDisplayPending)
>               return;
>             this._messageDisplayPending = true;
>@@ -351,6 +348,7 @@
>         <parameter name="aMsg"/>
>         <parameter name="aContext"/>
>         <parameter name="aNoAutoScroll"/>
>+        <parameter name="aFirstUnread"/>
>         <body>
>           <![CDATA[
>             let doc = this.contentDocument;
>@@ -386,11 +384,38 @@
>
>             aMsg.message = cleanupImMarkup(doc, msg.replace(/\r?\n/g, "<br/>"),
>                                            null, this._textModifiers);
>+
>             let next = (aContext == this._lastMessageIsContext || aMsg.system) &&
>                        isNextMessage(this.theme, aMsg, this._lastMessage);
>-            let html = getHTMLForMessage(aMsg, this.theme, next, aContext);
>-            let body = doc.getElementsByTagName("body")[0];
>-            let newElt = insertHTMLForMessage(aMsg, html, doc, next);
>+            let newElt;
>+            if (next && aFirstUnread) {
>+              // If there wasn't an unread ruler, this would be a Next message.
>+              // Therefore, save that version for later.
>+              let html = getHTMLForMessage(aMsg, this.theme, next, aContext);
>+              let ruler = doc.getElementById("last-viewed-ruler");
>+              ruler.aHTML = html;

aHTML doesn't seem like a good name.

>+              ruler.aMsg = aMsg;
>+
>+              let marker = doc.createElement("div");
>+              marker.id = "insert-before-ruler";
>+              let insert = doc.getElementById("insert");
>+              if (insert)
>+                ruler.marker = insert.parentNode.insertBefore(marker, insert);

This is too complicated. If you set next to false, the theme code will never use the "insert" node, it will just remove it if it exists before inserting a new message, so you don't need to create a new insert-before-ruler node, you can just reuse this one and change the id. Or just .removeAttribute("id"), as it seems you don't use that id at all.

>+
>+              next = false;
>+              html = getHTMLForMessage(aMsg, this.theme, next, aContext);
>+              newElt = insertHTMLForMessage(aMsg, html, doc, next);

To not assume [2] I think you need to change the insertHTMLForMessage function to return all the inserted node, and keep this array in a property of the ruler.

>@@ -405,6 +430,74 @@
>         </body>
>       </method>
>
>+      <method name="setUnreadRuler">
>+        <body>
>+        <![CDATA[
>+          let doc = this.contentDocument;

Add a comment here explaining why you need to remove the ruler before adding one (you explained on IRC that it's for cases where a new message appears in a window that has lost focus but may still be visible).

>+          this.removeUnreadRuler();
>+          let ruler = doc.createElement("hr");
>+          ruler.id = "last-viewed-ruler";
>+
>+          // default
>+          //ruler.style.margin = "0";
>+          //ruler.style.width = "100%";
>+          //ruler.style.border = "solid";
>+          //ruler.style.borderWidth = "1px 0 0 0";
>+          //ruler.style.color = "DarkRed";

This should be in http://lxr.instantbird.org/instantbird/source/chat/themes/conv.css

>+
>+          // for Bubbles
>+          ruler.style.width = "100%";
>+          ruler.style.height = "2px";
>+          ruler.style.margin = "1em 0";
>+          ruler.style.border = "none";
>+          ruler.style.background = "#ddd";
>+          ruler.style.backgroundImage = "-moz-linear-gradient(center top, rgb(211,211,211) 50%, rgb(255,255,255) 50%)";
>+          ruler.style.transition = "all 0.5s";

I think you know where this could be :).

>+      <method name="removeUnreadRuler">
>+        <body>
>+        <![CDATA[
>+          let doc = this.contentDocument;
>+          let ruler = doc.getElementById("last-viewed-ruler");
>+          if (ruler) {

nit: Return early if !ruler.

>+            // Trigger transition.
>+            ruler.style.height = "0px";
>+            ruler.style.margin = "0";

I'm not sure this transition will actually be visible as you remove that node.

>+
>+            // Rejoin split message blocks if possible.
>+            if (ruler.marker) {
>+              // Add first message following the ruler as a Next type message.
>+              let range = doc.createRange();
>+              let parent = ruler.marker.parentNode;
>+              range.selectNode(parent);
>+              let documentFragment = range.createContextualFragment(ruler.aHTML);
>+              parent.insertBefore(documentFragment, ruler.marker);
>+
>+              // Remove any insert node this added.
>+              let insert = doc.getElementById("insert");
>+              if (insert)
>+                insert.parentNode.removeChild(insert);

Errr... really? So to unsplit the bubble of the first unread message, you force-split the bubble of the last unread message by removing its insert node?

I think you need to change the id of the insert node before inserting the new fragment, then remove potential insert node (like you do) and finally restore the id of the previous insert node.


>+              // Move remaining messages from the message block following the ruler.
>+              let moveTo = ruler.marker;
>+              while (ruler.moveFrom.nextSibling)
>+                moveTo = ruler.marker.parentNode.insertBefore(ruler.moveFrom.nextSibling, moveTo.nextSibling);

I think this will perform poorly if [1] isn't respected.

2 possible work-arounds:
- avoid the loop if ruler.moveFrom.parentNode.id == "Chat"
- (more complicated) for each node you are about to move, find which message it is part of, and call isNextMessage.

>+              // Delete surplus message block.
>+              ruler.followingElt.parentNode.removeChild(ruler.followingElt);

This will let duplicated content stay in the document if [1] isn't respected by the theme.
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 860 as attmnt 1273 at 2012-03-28 17:50:00 UTC ***

Thanks for the very helpful review!

This patch has so far survived what message styles I have thrown at it.

Whether it's due to the rewrite of the message moving code or not, the transition in the WIP does not appear noticeable, so I took it out.

For the default styling for the ruler, I went with zero margins (so it should not break the layout of the message style) and DarkRed (so it is visible both for white and dark message styles - I thought Grey at first, but that might blend in too much). Does that seem right?
Attachment #8353026 - Flags: review?(florian)
Comment on attachment 8352905 [details] [diff] [review]
Patch

*** Original change on bio 860 attmnt 1160 at 2012-03-28 17:50:07 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352905 - Attachment is obsolete: true
Comment on attachment 8352916 [details] [diff] [review]
WIP with rejoining bubbles

*** Original change on bio 860 attmnt 1170 at 2012-03-28 17:50:07 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352916 - Attachment is obsolete: true
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 860 as attmnt 1274 at 2012-03-28 18:49:00 UTC ***

Forgot a nit fix from the previous review ;)

Also changed the default style to Red and style: dashed.
Attachment #8353027 - Flags: review?(florian)
Comment on attachment 8353026 [details] [diff] [review]
Patch

*** Original change on bio 860 attmnt 1273 at 2012-03-28 18:49:05 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353026 - Attachment is obsolete: true
Attachment #8353026 - Flags: review?(florian)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 860 as attmnt 1275 at 2012-03-28 20:55:00 UTC ***

Two more little nits.
Attachment #8353028 - Flags: review?(florian)
Comment on attachment 8353027 [details] [diff] [review]
Patch

*** Original change on bio 860 attmnt 1274 at 2012-03-28 20:55:55 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353027 - Attachment is obsolete: true
Attachment #8353027 - Flags: review?(florian)
Blocks: 954791
Comment on attachment 8353028 [details] [diff] [review]
Patch

*** Original change on bio 860 attmnt 1275 at 2012-04-12 20:39:02 UTC ***

>diff --git a/chrome/chat/content/chat/convbrowser.xml b/chrome/chat/content/chat/convbrowser.xml

>+            let actualinsert = doc.getElementById("insert");

Nit: actualInsert

>+            if (actualinsert)
>+              actualinsert.id = "actual-insert";
>+
>+            // Add first message following the ruler as a Next type message.
>+            let range = doc.createRange();
>+            let moveToParent = moveTo.parentNode;
>+            range.selectNode(moveToParent);
>+            let documentFragment = range.createContextualFragment(ruler.nextMsgHtml);
>+            moveToParent.insertBefore(documentFragment, moveTo);
>+
>+            // Remove any insert node this added and restore existing one.
>+            let insert = doc.getElementById("insert");
>+            if (insert)
>+              insert.parentNode.removeChild(insert);

It seems you wanted to write:
if (insert) {
  moveToParent.removeChild(moveTo);
  moveTo = insert;
  moveToParent = moveTo.parentNode;
}

>+            if (actualinsert)
>+              actualinsert.id = "insert";

And this should be move to...

>+
>+            // Move remaining messages from the message block following the ruler.
>+            let nextmessagesStart = doc.getElementById("nextmessages-start");
>+            if (nextmessagesStart) {
>+              range = doc.createRange();
>+              range.setStartAfter(nextmessagesStart);
>+              range.setEndBefore(doc.getElementById("nextmessages-end"));
>+              moveToParent.insertBefore(range.extractContents(), moveTo);
>+            }
>+            moveToParent.removeChild(moveTo);

... here.

>diff --git a/chrome/instantbird/skin/classic/instantbird/messages/bubbles/main.css b/chrome/instantbird/skin/classic/instantbird/messages/bubbles/main.css
>index bc985e7..304a6c0 100644
>--- a/chrome/instantbird/skin/classic/instantbird/messages/bubbles/main.css
>+++ b/chrome/instantbird/skin/classic/instantbird/messages/bubbles/main.css
>@@ -151,16 +151,23 @@ p.event {
>   white-space: normal;
> }
> 
> p *:-moz-any-link img {
>   margin-bottom: 1px;
>   border-bottom: solid 1px;
> }
> 
>+#Chat>#unread-ruler {
>+  margin: 1em 0;
>+  height: 2px;
>+  width: 100%;
>+  border: none;
>+  background-image: -moz-linear-gradient(center top, rgb(211,211,211) 50%, rgb(255,255,255) 50%);
>+}

It seems you don't need the width: 100% as it's already in conv.css.

Rather than using #Chat>, I think I would prefer !important on the margin and border rules.

Looks reasonable otherwise.
Attachment #8353028 - Flags: review?(florian) → review-
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 860 as attmnt 1333 at 2012-04-12 21:58:00 UTC ***

Fixed review comments.

(In reply to comment #37)
> It seems you wanted to write:
> if (insert) {
>   moveToParent.removeChild(moveTo);
>   moveTo = insert;
>   moveToParent = moveTo.parentNode;
> }

Yes, this would matter if a message style nests its next messages.
Attachment #8353086 - Flags: review?(florian)
Comment on attachment 8353028 [details] [diff] [review]
Patch

*** Original change on bio 860 attmnt 1275 at 2012-04-12 21:58:07 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353028 - Attachment is obsolete: true
Attached patch PatchSplinter Review
*** Original post on bio 860 as attmnt 1335 at 2012-04-12 22:10:00 UTC ***

- Changed incorrect comment.
Attachment #8353088 - Flags: review?(florian)
Comment on attachment 8353086 [details] [diff] [review]
Patch

*** Original change on bio 860 attmnt 1333 at 2012-04-12 22:10:58 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353086 - Attachment is obsolete: true
Attachment #8353086 - Flags: review?(florian)
Comment on attachment 8353088 [details] [diff] [review]
Patch

*** Original change on bio 860 attmnt 1335 at 2012-04-12 22:51:36 UTC ***

Looks ready to be tested on nightlies; thanks for working on this easy-looking-yet-hard-to-fix frequent enhancement request!
Attachment #8353088 - Flags: review?(florian) → review+
*** Original post on bio 860 at 2012-04-13 00:11:27 UTC ***

Committed as http://hg.instantbird.org/instantbird/rev/3d7c0b70b6a1

Can't wait to try this out in tomorrow's nightly!
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → 1.2
You need to log in before you can comment on or make changes to this bug.