741728 - Previous conversations should be collapsed by default

Status: RESOLVED FIXED

(Thunderbird :: Instant Messaging, defect)

Description

[Jb Piacentino]

The "previous conversation" pane currently occupies whole or part of the right column. In many instances, and particularly for day to day conversations, it does not add useful information and rather 'pollutes' the visual aspect.
I would like to hide this list in a collapsable pane. The expected behaviour would be:
- Previous conversation pane is collapsed by default
- Keep the expanded/collapsed status accross all current conversations
- Expand the previous conversations pane if a conversation is displayed as a result of a click on a search result

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Attachment: bwinton's wireframe

Comment 2

[Mike Conley (:mconley) (:⚙️)]

I looked at the wireframe, refreshed myself on how XUL Treeviews work (that's the element that allows twisties, like the address book selector), and examined the chat log viewer code.

I believe this work would require an overhaul in how logs are displayed, since (I believe) the wireframe implies that logs in a particular time range ("Yesterday", for example) would be concatenated together. As it stands, the log viewer isn't really set up to do that.

That's pretty invasive work for something on beta. I think we should punt on that request (although, please correct me if I'm wrong about what the wireframe is asking for).

Jb's request for having the log list be collapsible is do-able, and likely far-less invasive.

Comment 3

[Florian Quèze [:florian]]

Maybe just replacing the date with "today" and "yesterday" for the first conversations would make things a tiny bit more user friendly without requiring a tree view?

Comment 4

[Mike Conley (:mconley) (:⚙️)]

(In reply to Florian Quèze [:florian] [:flo] from comment #3)
> Maybe just replacing the date with "today" and "yesterday" for the first
> conversations would make things a tiny bit more user friendly without
> requiring a tree view?

I think the aim is to reduce the number of things being listed by default, as opposed to making the list more homogeneous.

bwinton and I were just discussing this, and we may have come up with a do-able solution - here's what we're thinking:

v Today
   2:16 PM
   11:41 AM
   10:15 AM
   9:47 AM
   9:45 AM
   9:42 AM
   9:33 AM
   9:22 AM
> Yesterday
   3:45 PM
   3:36 PM
   10:10 AM
> Last Week
> Two Weeks Ago
> 04/01/2012 - 19/07/2012

Comment 5

[Mike Conley (:mconley) (:⚙️)]

er, that Yesterday should have a "v" in front of it, of course.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP patch 1

Florian / Blake:

How do you feel about the direction this is heading?

-Mike

Comment 7

[Jb Piacentino]

(In reply to Mike Conley (:mconley) from comment #4)
> (In reply to Florian Quèze [:florian] [:flo] from comment #3)
> > Maybe just replacing the date with "today" and "yesterday" for the first
> > conversations would make things a tiny bit more user friendly without
> > requiring a tree view?
> 
> I think the aim is to reduce the number of things being listed by default,
> as opposed to making the list more homogeneous.
> 
> bwinton and I were just discussing this, and we may have come up with a
> do-able solution - here's what we're thinking:
> 
> v Today
>    2:16 PM
>    11:41 AM
>    10:15 AM
>    9:47 AM
>    9:45 AM
>    9:42 AM
>    9:33 AM
>    9:22 AM
> > Yesterday
>    3:45 PM
>    3:36 PM
>    10:10 AM
> > Last Week
> > Two Weeks Ago
> > 04/01/2012 - 19/07/2012

That is totally fine for v1, AFAIC.

Comment 8

[Jim Porter (:squib)]

Attachment: History++'s UI

This sounds an awful lot like the UI for History++, a history viewer for Miranda IM. I've attached a screenshot so you can see how it's organized.

Comment 9

[Florian Quèze [:florian]]

Comment on attachment 648847 [details] [diff] [review]
WIP patch 1

About the UI: Even though I'm not really a fan of the UI this creates (for example because more clicks will be needed to select something), I still think this is a huge improvement compared to the awful list we currently have; and I think it correctly addresses Jb's concerns about that list.

About the code:
It seems what remains to do is:
- handle automatic selection of a log. This is needed in 2 cases:
 * when selecting a contact, so that the most recent log is displayed instead of a place holder in the middle pane
 * when displaying a search result, so that the selected log in the right pane matches the conversation that we are displaying.
This automatic log selection requires finding the log in the list that's no longer linear, and opening the correct container.
- get rid of the old listbox (with Mike's WIP, it is still used for some code related to the 2 cases mentioned above), and theme the tree like the old listbox. This means mostly getting rid of the padding, margins and borders that a tree has by default.
- cleaning up the code. I noticed a few things that could be simplified, a few variables that aren't really useful, ... That was expected for a WIP though :)

Comment 10

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 648847 [details] [diff] [review]
WIP patch 1

I like it.  f=me.

The one thing I would add, in addition to Florian's notes, and only if it's fairly easy to do, would be to try and combine some of the the logs that are a little further in the past.  Like, in http://dl.dropbox.com/u/2301433/Screenshots/shortTimes.png I'm pretty sure I can't remember the difference between something that happened at 2:14 on the 2nd vs. something that happened at 2:40.

Thanks,
Blake.

Comment 11

[Florian Quèze [:florian]]

Attachment: WIP2

Done some cleanup, removed the old listbox, fixed the CSS, and prepared the code to handle automatic selections (there's an obvious FIXME where more work is needed for this).

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #10)

> The one thing I would add, in addition to Florian's notes, and only if it's
> fairly easy to do, would be to try and combine some of the the logs that are
> a little further in the past.

I'm not sure of how difficult it would be, but I'm pretty sure it would be risky enough that I wouldn't want to request approval for beta on a patch doing that.

Comment 12

[Florian Quèze [:florian]]

Attachment: Patch v3

Ready for review!

Comment 13

[Florian Quèze [:florian]]

Comment on attachment 650735 [details] [diff] [review]
Patch v3

>diff --git a/mail/themes/qute/mail/chat-aero.css b/mail/themes/qute/mail/chat-aero.css

> 
>-  #logList > listitem,
>   #nicklist > listitem {
>     border-width: 1px !important;
>     outline: none !important;
>   }

I removed the #logList > listitem selector here because it obviously won't have any effect any more, but if the intended theming here wasn't the default aero theming of a tree, someone will need to polish this for Windows aero.

Comment 14

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 650735 [details] [diff] [review]
Patch v3

UI-Review:
- Collapsing the log pane entirely leaves some of the first line showing on Mac.  (Fine on Windows.)
- Searching seemed _really_ slow on Windows, but that might be my setup.
- I can't compile under Linux, so it might be totally broken there.  :(

ui-r=me with that first one fixed, and the second one fixed if someone can reproduce it.

Now on to the review.  :)

>+++ b/mail/components/im/content/chat-messenger-overlay.js
>@@ -1002,9 +1009,124 @@ var chatHandler = {
>+chatLogTreeView.prototype = {
>+    let groups = {
>+      today: [],
>+      yesterday: [],
>+      lastWeek: [],
>+      twoWeeksAgo: [],
>+      other: []

I'm a little concerned that we're using in-code names as keys in a msgBundle.  A note here that says "These need to map to things in whatever.properties." would make me feel a little better…

>+++ b/mail/components/im/content/chat-messenger-overlay.xul
>@@ -259,17 +260,24 @@
>               <splitter id="logsSplitter" class="conv-chat"/>

I think we might also want 'collapse="after"' on this.  Or something better if we can arrange it.  But that might be able to wait for a different patch.

>+++ b/mail/themes/qute/mail/chat-aero.css
>@@ -151,17 +151,16 @@
>-  #logList > listitem,

Should this be converted to "#logTree > something"?

>   #nicklist > listitem {
>     border-width: 1px !important;
>     outline: none !important;

Other than those, I like it.  r=me.

Thanks,
Blake.

Comment 15

[Florian Quèze [:florian]]

Attachment: Patch v4

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #14)
> Comment on attachment 650735 [details] [diff] [review]
> Patch v3
> 
> UI-Review:
> - Collapsing the log pane entirely leaves some of the first line showing on
> Mac.  (Fine on Windows.)

I think this is fixed by adding the 'collapse="after"' you suggested in your code review.

> >+++ b/mail/components/im/content/chat-messenger-overlay.js
> >@@ -1002,9 +1009,124 @@ var chatHandler = {
> >+chatLogTreeView.prototype = {
> >+    let groups = {
> >+      today: [],
> >+      yesterday: [],
> >+      lastWeek: [],
> >+      twoWeeksAgo: [],
> >+      other: []
> 
> I'm a little concerned that we're using in-code names as keys in a
> msgBundle.  A note here that says "These need to map to things in
> whatever.properties." would make me feel a little better…

Added a comment.

> >+++ b/mail/themes/qute/mail/chat-aero.css
> >@@ -151,17 +151,16 @@
> >-  #logList > listitem,
> 
> Should this be converted to "#logTree > something"?

No, because the content of the tree doesn't actually have DOM nodes, so it's not possible to style trees in this way.

Someone may want to play with the magic CSS pseuso selectors described at https://developer.mozilla.org/en-US/docs/XUL_Tutorial/Styling_a_Tree though.


This new version of the patch also fixes the JS warnings that were caused by the usage of the jsTreeView.

Comment 16

[Florian Quèze [:florian]]

Attachment: Patch v5 (r+ui-r=bwinton)

[Approval Request Comment]
User impact if declined: This has been Jb's pet bug for the IM feature for months, so I think he would be really disappointed if it didn't make Tb15.
Risk to taking this patch: moderate (but in addition to the behavior change, it fixes a few annoying bugs, so I think we should take this).


Blake requested a screenshot on Linux.

Here's the IRC conversation we just had on #maildev:
17:59:25 - Paenglab: florian, bwinton: Screenshot on Linux https://dl.dropbox.com/u/23792533/screenshots/Linux-conv.png
17:59:51 - florian: Paenglab: thanks!
18:01:12 - bwinton: florian: ui-r=me. Although, I wonder if we should have <more recent date> - <less recent date>…
18:01:25 - bwinton: Since we're presenting them in reverse order…
18:02:19 - florian: bwinton: that would make sense I think
18:03:09 - bwinton: florian: Make it so. ;)

The only difference with Patch v4 is:
-          groupName = formatDate(fromDate) + " - " + groupName;
+          groupName += " - " + formatDate(fromDate);

Comment 17

[Mike Conley (:mconley) (:⚙️)]

comm-central: https://hg.mozilla.org/comm-central/rev/aee35a027d81
comm-aurora: https://hg.mozilla.org/releases/comm-aurora/rev/d725db4fdfdb
comm-beta: https://hg.mozilla.org/releases/comm-beta/rev/8ad0a8d8ffc9