[PATCH] Threaded messagelist view patch rev 3

[Vladislav Bogdanov]

Hi all,

here is the next revision of threaded message list patch

Changes comparing to roundcubemail-threading-20090706.patch from
http://www.atomice.com/blog/?page_id=34:
1. Ported to 0.3-SVN-r2976
2. Implemented auto expand of threads with unread messages only.
3. Added GUI configuration option for autoexpand control (on, off, unread).
4. Autoexpansion works in both static and JS message lists.
5. Automatically change between 'unread children' and message icons in a
list (in parent message rows) when child message is marked read/unread.
6. Count messages, not threads in a message view pane.
7. Fixed navigation in a message view pane when threading is enabled.
8. Fixed setting folders with non-ASCII named to threaded mode.
9. Fixed incorrectly displayed rows (children of collapsed row) after
multi-level expand/collapse. Simplified expand logic.
10. Fixed expand indicator is not changing when expanding/collapsing
with keyboard.
11. Fixed plus key on a numeric keypad is not working (at least on linux
Fedora 11 + Firefox 3.5.2).
12. Added support for whole thread expand/collapse with
clicking/pressing hotkeys while holding the Control key.
13. Save expand state in env everywhere (after keypresses too).
14. Added russian translation.
15. Fixed indentation and braces style in JS to comply with RC coding style.

Changes to my previous public revision:

16. (Multiple) selection works fine with threaded view too now (based on
patch published by Nathan Kinkade). Selection list is altered
immediately on a select action (opposed to Nathan's patch). Nested
messages are added to selection list when collapsed parent is selected,
and removed from it on a thread expand. I ask everybody to test this
feature with both mouse and keyboard shortcuts.
17. Remove a bit of unused code.
18. Add only one div of a variable width to a messagelist row instead of
set of divs. See comments inline.
19. Make dragging work with threaded view (locate subject text differently).
20. Re-read messagelist portion after move/delete operation if threading
is enabled. See comments inline. (This does not fix a bug with
non-atomic move/delete operations I noted before). I'm not sure I did it
100% right because of that bug which makes testing nearly impossible in
my setup.

I think that code (except thread caching which I don't use and didn't
touch it at all, it is up to Chris) is a true beta quality now (based on
my testing) and is ready to be tested intensively in different setups.
Comments are welcome as usual.

Best,
Vladislav



 --- 8< --- detachments --- 8< ---
 The following attachments have been detached and are available for viewing.
  http://detached.gigo.com/rc/1h/WkvtBXFZ/roundcubemail-thread.patch
  http://detached.gigo.com/rc/1h/WkvtBXFZ/unread_children.png
 Only click these links if you trust the sender, as well as this message.
 --- 8< --- detachments --- 8< ---


_______________________________________________
List info: http://lists.roundcube.net/dev/

[A.L.E.C]

Vladislav Bogdanov wrote:

> I think that code (except thread caching which I don't use and didn't
> touch it at all, it is up to Chris) is a true beta quality now (based on
> my testing) and is ready to be tested intensively in different setups.
> Comments are welcome as usual.

Great, I plan to start including this into trunk in October.

--
Aleksander 'A.L.E.C' Machniak http://alec.pl gg:2275252
LAN Management System Developer http://lms.org.pl
Roundcube Webmail Project Developer http://roundcube.net
_______________________________________________
List info: http://lists.roundcube.net/dev/

On Tue, 22 Sep 2009 12:11:02 +0300, Vladislav Bogdanov
<bubble@...> wrote:
> Hi all,
>
> here is the next revision of threaded message list patch
>
> Changes comparing to roundcubemail-threading-20090706.patch from
> http://www.atomice.com/blog/?page_id=34:

<-- snip lots of changes -->

Looks good. Do you mind if I add your patch to my web page (with credit)?

> I think that code (except thread caching which I don't use and didn't
> touch it at all, it is up to Chris) is a true beta quality now (based on
> my testing) and is ready to be tested intensively in different setups.
> Comments are welcome as usual.

I found thread caching (and also message caching) just slows things down if
your IMAP server is running on the same machine as Roundcube, so I don't
use it either. It may be better if you use SQLite instead of MySQL.

Chris

_______________________________________________
List info: http://lists.roundcube.net/dev/

[Eric Appelt]

Hi Vladislav,

wooow sounds like good, many thanks for this great work.

I test it directly tomorrow.

Great thing!!!

_______________________________________________
List info: http://lists.roundcube.net/dev/

[Vladislav Bogdanov]

Chris January wrote:
>
> Looks good. Do you mind if I add your patch to my web page (with credit)?
>

Approved, I think it's a best place for it. I really hate code forking.
And this leads to a wider testing.

Please don't forget to publish (originally yours) icon separately too ;)
And please do not forget to credit Nathan Kinkade, that patch was very
useful.

> I found thread caching (and also message caching) just slows things down if
> your IMAP server is running on the same machine as Roundcube, so I don't
> use it either. It may be better if you use SQLite instead of MySQL.

It should be tested anyway if caching code exists in a trunk. Personally
I do not use caching too, cyrus is quick enough even on a neighbor host.

Vladislav
_______________________________________________
List info: http://lists.roundcube.net/dev/

[Vladislav Bogdanov]

Patch I sent originally mistakenly contains a bit of uncommented debug
code I placed there while catching that 'non-atomic deletion' bug.

Resending with this code commented.

Thanks to Eric Appelt for noticing that,

Vladislav



 --- 8< --- detachments --- 8< ---
 The following attachments have been detached and are available for viewing.
  http://detached.gigo.com/rc/NT/1MR2llPQ/roundcubemail-thread.patch
  http://detached.gigo.com/rc/NT/1MR2llPQ/unread_children.png
 Only click these links if you trust the sender, as well as this message.
 --- 8< --- detachments --- 8< ---


_______________________________________________
List info: http://lists.roundcube.net/dev/

[Jean-Baptiste Hétier-2]

On Tue, 22 Sep 2009 11:29:22 +0200, "A.L.E.C" <alec@...> wrote:
> Vladislav Bogdanov wrote:
>
>> I think that code (except thread caching which I don't use and didn't
>> touch it at all, it is up to Chris) is a true beta quality now (based
on
>> my testing) and is ready to be tested intensively in different setups.
>> Comments are welcome as usual.
>
> Great, I plan to start including this into trunk in October.

Is that still planned?  I'm really excited about threaded view coming to
Roundcube.
_______________________________________________
List info: http://lists.roundcube.net/dev/

[A.L.E.C]

Jean-Baptiste Hétier wrote:
>> Great, I plan to start including this into trunk in October.
>
> Is that still planned?  I'm really excited about threaded view coming to
> Roundcube.

Yes, but I was too busy. I'll start probably in next week. Do you guys
have any newer version of this patch?

--
Aleksander 'A.L.E.C' Machniak http://alec.pl gg:2275252
LAN Management System Developer http://lms.org.pl
Roundcube Webmail Project Developer http://roundcube.net
_______________________________________________
List info: http://lists.roundcube.net/dev/

[Vladislav Bogdanov]

A.L.E.C wrote:

> Yes, but I was too busy. I'll start probably in next week. Do you guys
> have any newer version of this patch?
>

Not yet.
I'm quite busy with other projects now, sorry.
Everything except rcube_imap.php should should apply clear still (at
least I hope). I tried to apply last patch week or two ago, and
everything applied fine except that file.
Anyway I think it would be better if you reorganize that code yourself,
does anybody else know that algorithm with headers better in its current
state? :)

Only few notes from me:
* We should probably combine threading with index_sort in a way that
index_sort is implicitly enabled when threading is on. This would
simplify code a bit and make some hunks unnecessary (I mean a so-called
'default' sorting mode).
* We should give user a possibility to switch between threaded and
non-threaded view from a message_list itself. I already wrote that an
ideal solution for me would be a tiny dropdown in the very left
message_list header position with options for threading, index_sort and
default sort (I mean sorting by columns) modes. One problem I see with
this is a 'date' sorting which is probably a kinda special case, it
should be enabled for all of three modes (FIXME), giving user an option
to 'reverse-sort' the list. I hope somebody can propose a better
solution for that 'reverse' listing.
Hmmm.... I think all other sorting options should be disabled when
either threading or index_sort are enabled, shouldn't they?

I see no problems with all the added JS code, and I'd say it is stable.

Only one quite big problem remains - that async deletion bug I wrote
before. It would be really cool if you fix it in one shot with threading
so we have finished threading solution.

Best,
Vladislav
_______________________________________________
List info: http://lists.roundcube.net/dev/

[A.L.E.C]

Vladislav Bogdanov wrote:

> * We should probably combine threading with index_sort in a way that
> index_sort is implicitly enabled when threading is on. This would
> simplify code a bit and make some hunks unnecessary (I mean a so-called
> 'default' sorting mode).
> * We should give user a possibility to switch between threaded and
> non-threaded view from a message_list itself. I already wrote that an
> ideal solution for me would be a tiny dropdown in the very left
> message_list header position with options for threading, index_sort and
> default sort (I mean sorting by columns) modes. One problem I see with
> this is a 'date' sorting which is probably a kinda special case, it
> should be enabled for all of three modes (FIXME), giving user an option
> to 'reverse-sort' the list. I hope somebody can propose a better
> solution for that 'reverse' listing.
> Hmmm.... I think all other sorting options should be disabled when
> either threading or index_sort are enabled, shouldn't they?

Yeah, something like that. I like to see a popup with coulmns, sorting
and grouping selection:

Columns (checkboxes):
   [x] Subject
   [x] Date
   [x] From/To
   [x] Size
   [ ] Attachment
   [ ] Flag
... other columns here
Sorting (radio buttons):
   (o) None (current index_sort)
   ( ) Date
   ( ) Arrival date (not implemented yet)
   ( ) Subject
   ( ) From/To
   ( ) Size
Order (radio buttons):
   (o) Ascending
   ( ) Descending
Grouping (radio buttons):
   (o) None
   ( ) Thread
   ( ) Group (or sth in future)

This could be a simple form or two-level menu. Any volunteers to
implement this?

 > Hmmm.... I think all other sorting options should be disabled when
 > either threading or index_sort are enabled, shouldn't they?

I think not. We can sort threads (root messages) by any field.

--
Aleksander 'A.L.E.C' Machniak http://alec.pl gg:2275252
LAN Management System Developer http://lms.org.pl
Roundcube Webmail Project Developer http://roundcube.net
_______________________________________________
List info: http://lists.roundcube.net/dev/

[Vladislav Bogdanov]

A.L.E.C wrote:

>
> Yeah, something like that. I like to see a popup with coulmns, sorting
> and grouping selection:
>
> Columns (checkboxes):
>    [x] Subject
>    [x] Date
>    [x] From/To
>    [x] Size
>    [ ] Attachment
>    [ ] Flag
> ... other columns here
> Sorting (radio buttons):
>    (o) None (current index_sort)
>    ( ) Date
>    ( ) Arrival date (not implemented yet)
>    ( ) Subject
>    ( ) From/To
>    ( ) Size
> Order (radio buttons):
>    (o) Ascending
>    ( ) Descending
> Grouping (radio buttons):
>    (o) None
>    ( ) Thread
>    ( ) Group (or sth in future)

This could be inspired by a contextmenu plugin implementation.
One more thought: There should be a 'Reset to default' option.
And, this sorting and grouping state should be a per-folder one...
So, may be one more option - 'Reset all to default' may be useful.

In general I like your idea even more then mine ;)
It even adds missing control over what columns are displayed and adds
overall consistency to a whole UI.

>
> This could be a simple form or two-level menu. Any volunteers to
> implement this?

I prefer one-level. It is quicker to work with. And size of menu is
almost constant, so I see no need for a second level. That second level
is cool for a contextmenu, which has a (variable sized) list of folders
to move message to, but not for a sorting control I think.
Only 'Reset' commands could be moved to a second level, as they are not
of a frequent use.

Something like this:
Reset view -> This folder
              All folders

And one more note about possible implementation: clicks on columns
checkboxes shouldn't hide menu, while click on sorting and grouping
radios should.

>
>  > Hmmm.... I think all other sorting options should be disabled when
>  > either threading or index_sort are enabled, shouldn't they?
>
> I think not. We can sort threads (root messages) by any field.
>

Yep, you are right about threading. But what about index_sort? Should
click on any header disable it and switch to a 'sorted' mode? Ah, yes,
it should.

Best,
Vladislav
_______________________________________________
List info: http://lists.roundcube.net/dev/

[A.L.E.C]

Vladislav Bogdanov wrote:

> This could be inspired by a contextmenu plugin implementation.
> One more thought: There should be a 'Reset to default' option.
> And, this sorting and grouping state should be a per-folder one...
> So, may be one more option - 'Reset all to default' may be useful.
> In general I like your idea even more then mine ;)

We don't need two-levels until the menu height will be not too high.
Another step to make messages list complete will be columns order
changing using drag'n'drop (preferred) or using up/down buttons in the menu.

> And one more note about possible implementation: clicks on columns
> checkboxes shouldn't hide menu, while click on sorting and grouping
> radios should.

For sorting/grouping also shouldn't. I think we should add a "Save"
link/button at bottom, also "Reset" and "Reset all" (the last one with
user warning).

>>  > Hmmm.... I think all other sorting options should be disabled when
>>  > either threading or index_sort are enabled, shouldn't they?
>>
>> I think not. We can sort threads (root messages) by any field.
>>
>
> Yep, you are right about threading. But what about index_sort? Should
> click on any header disable it and switch to a 'sorted' mode? Ah, yes,
> it should.

Exectly. That's how it should working from the beginning, but we've got
no such sorting menu.

--
Aleksander 'A.L.E.C' Machniak http://alec.pl gg:2275252
LAN Management System Developer http://lms.org.pl
Roundcube Webmail Project Developer http://roundcube.net
_______________________________________________
List info: http://lists.roundcube.net/dev/

[Vladislav Bogdanov]

A.L.E.C wrote:
>
> We don't need two-levels until the menu height will be not too high.
> Another step to make messages list complete will be columns order
> changing using drag'n'drop (preferred) or using up/down buttons in the menu.

I'd prefer drag'n'drop in a message_list itself. This would be more
intuitive. But checkboxes in menu should follow order in which columns
appear in a message_list anyway.
And I'd move this functionality into a separate commit after main
features are mature. And, maybe I'd prefer to have this menu in a
separate (of threading) commit also. Just not to mess things.

>
>> And one more note about possible implementation: clicks on columns
>> checkboxes shouldn't hide menu, while click on sorting and grouping
>> radios should.
>
> For sorting/grouping also shouldn't. I think we should add a "Save"
> link/button at bottom, also "Reset" and "Reset all" (the last one with
> user warning).

+1

But I'd write 'Reset view' or 'Reset options' instead of 'Reset'.
'Reset' word alone may mean anything for an user... ;)

Vladislav

_______________________________________________
List info: http://lists.roundcube.net/dev/

[A.L.E.C]

Finally I've started to work on this. Just want to share an idea of
performace improvement. Now, we're fetching all messages in all threads
for current page. I think we should do this only when "Autoexpand/Expand
All" is enabled. Otherwise we should fetch headers for roots only and
fetch children headers on demand. Also the last operation could be
improved with SEARCH=INTHREAD use.

--
Aleksander 'A.L.E.C' Machniak http://alec.pl gg:2275252
LAN Management System Developer http://lms.org.pl
Roundcube Webmail Project Developer http://roundcube.net
_______________________________________________
List info: http://lists.roundcube.net/dev/

[Jean-Baptiste Hétier-2]

On Fri, 06 Nov 2009 14:22:36 +0100, "A.L.E.C" <alec@...> wrote:
> Finally I've started to work on this. Just want to share an idea of
> performace improvement. Now, we're fetching all messages in all threads
> for current page. I think we should do this only when "Autoexpand/Expand

> All" is enabled. Otherwise we should fetch headers for roots only and
> fetch children headers on demand. Also the last operation could be
> improved with SEARCH=INTHREAD use.

It's a good idea.  I think that displaying the number of messages in a
thread is an important feature as well though.  Could you do that without
fetching all messages?
_______________________________________________
List info: http://lists.roundcube.net/dev/

[A.L.E.C]

Jean-Baptiste Hétier wrote:

> It's a good idea.  I think that displaying the number of messages in a
> thread is an important feature as well though.

It's not important for me.

> Could you do that without fetching all messages?

Of course, THREAD returns message ids for roots and its children, so
number of messages in a thread is not a problem.

--
Aleksander 'A.L.E.C' Machniak http://alec.pl gg:2275252
LAN Management System Developer http://lms.org.pl
Roundcube Webmail Project Developer http://roundcube.net
_______________________________________________
List info: http://lists.roundcube.net/dev/

[Jean-Baptiste Hétier-2]

On Fri, 06 Nov 2009 14:53:18 +0100, "A.L.E.C" <alec@...> wrote:
> Jean-Baptiste Hétier wrote:
>> Could you do that without fetching all messages?
>
> Of course, THREAD returns message ids for roots and its children, so
> number of messages in a thread is not a problem.

Cool!

Great to read that you're working on that by the way!
_______________________________________________
List info: http://lists.roundcube.net/dev/

[A.L.E.C]

Jean-Baptiste Hétier wrote:
> Cool!

Also, what you think abut such feature.... I like to implement the whole
thread expanding/collapsing with one click. I think we can do this like
in Thunderbird. I don't like to click every child to expand the whole
thread. The expand/collapse icon should be displayed only for a root
message. Currently it's displayed for root and every child with children.

--
Aleksander 'A.L.E.C' Machniak http://alec.pl gg:2275252
LAN Management System Developer http://lms.org.pl
Roundcube Webmail Project Developer http://roundcube.net
_______________________________________________
List info: http://lists.roundcube.net/dev/

[Vladislav Bogdanov]

Hi all,

sorry for delay

A.L.E.C wrote:

> Also, what you think abut such feature.... I like to implement the whole
> thread expanding/collapsing with one click. I think we can do this like

Ctrl-Left_Click on an expando icon already does this. This should be
mentioned somewhere in a help.

> in Thunderbird. I don't like to click every child to expand the whole
> thread. The expand/collapse icon should be displayed only for a root
> message. Currently it's displayed for root and every child with children.

I like current behavior much more than thunderbirds one.

-1


Vladislav
_______________________________________________
List info: http://lists.roundcube.net/dev/

[Vladislav Bogdanov]

A.L.E.C wrote:
> Finally I've started to work on this. Just want to share an idea of
> performace improvement. Now, we're fetching all messages in all threads
> for current page. I think we should do this only when "Autoexpand/Expand
> All" is enabled. Otherwise we should fetch headers for roots only and
> fetch children headers on demand. Also the last operation could be

+1

> improved with SEARCH=INTHREAD use.
>

_______________________________________________
List info: http://lists.roundcube.net/dev/