163946 - cursoring through thread pane tries to load most messages

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[David :Bienvenu]

Because of the fix for bug 159477, you can no longer easily scroll through the
thread pane - messages get selected and loading, making this very herky jerky.

Comment 1

[David :Bienvenu]

Attachment: patch to tree to add settable attribute

Comment 2

[David :Bienvenu]

Attachment: restore 500 millisecond timer for thread pane

set _selectDelay attribute for thread pane tree.

Comment 3

[David :Bienvenu]

I don't know if this is the right way to do this, since I'm not a big xul/xml
hacker - it seems to work. The thread pane definitely needs a longer timeout,
especially if you're using imap on a slower connection. Can I get some reviews
or advice? thx.

Comment 4

[Brendan Eich [:brendan]]

Great, now we have the problem that people will blame XUL or XPCOM for this
500ms delay, as already happened (don't ask).  More grist for certain anti-XUL,
anti-Mozilla-mail mills.

I repeat that time is not relevant (see
http://bugzilla.mozilla.org/show_bug.cgi?id=159477#c15).  Can we not elaborate
the tree widget programming model in a compatible way, without imposing a > .5
second delay on loading a message that one *does* want to load?

/be

Comment 5

[David :Bienvenu]

Seth points out that this patch undoes blake's patch, because it doesn't allow
the default of 50 milliseconds, so that's wrong.

Brendan, time does matter - we can't try to load messages every 50 milliseconds;
there's no way around that. We could reduce the cases where time matters to the
case where we haven't gotten the vk_up event, so that, for example, hitting the
down arrow once would load the message as close to instantly as possible after
getting the vk_up event, but holding an arrow key down would not cause any
selects/message loads until a certain amount of time had elapsed, to handle the
edge case.

Comment 6

[Brendan Eich [:brendan]]

David, that (using up key or repeat sensing) is exactly what I proposed in
http://bugzilla.mozilla.org/show_bug.cgi?id=159477#c15.  Time doesn't matter
means there's no good delay value, not that you should be able to load messages
instantly.  I'm arguing against the use of a delay to discriminate between two
time-uncoordinated logic states (one where the key has been released, and one
where it is still held).

Note from http://bugzilla.mozilla.org/show_bug.cgi?id=159477#c15 the edge cases
at top and bottom of the tree.  I don't know enough about the xul tree widget to
propose a backward-compatible extension, but maybe just the presence of an
onselectrepeat attribute could be used?

/be

Comment 7

[David :Bienvenu]

Heh, I think I've already amply demonstrated my ignorance of the tree xml :-)
I'm adding Jan Varga to the cc list - maybe he can bail me out. I'm a little
fuzzy on what we can and cannot change to be backward compatible.

I understand what you're saying about time not mattering, in the global sense.
But I'm not sure that we have enough information in tree.xml not to use time.
From what I can see, tree.xml only has keypress events, and doesn't seem to know
about key up or key down, or repeating events. We'd need to make sure that the
time before repeating kicks in to be less than the time we wait before we decide
to select a message, or else we'd always select the next message.

Comment 8

[Johnny Stenback (:jst)]

At least on windows, there is indeed a time delay between selecting an item in a
tree view using the up/down arrow keys and the "selected" action taking place.
Try in in the folder pane in Windows Explorer, if you click on a folder it's
immediately selected, but if I move up/down the tree Explorer waits ~.5 seconds
before displaying the folder contents. It is *not* dependent on wether or not
the key is released between moving from folder to folder. I don't know if the
length of the delay on Windows depends on the CPU speed or not (I hope not), but
there's an obvious delay and anthing else seems very wrong to me.

Comment 9

[Brendan Eich [:brendan]]

jst, that's what hyatt said when we were talking about this the other month. 
But why is a half-second delay the right answer?  Even if it's a good answer for
the case where the key is *held*, why shouldn't I see the mail message right
away if I release the key?

/be

Comment 10

[David :Bienvenu]

some more datapoints - 100 milliseconds is not enough of a delay for mail on my
1.4GHz machine (if we're force to customize this for mail somehow). We can get
key up and down events in tree.xml - the trick is controlling the tree selection
so that it will highlight lines w/o sending a select event and thus loading the
message. There is a way to suppress events, but it's getting a bit complicated -
I'm not sure what will happen when I unsupress events and try to select the
currently selected message...

Comment 11

[Johnny Stenback (:jst)]

Brendan, I'm not against having an email message displayed immediately if you
release the key after holding the down arrow key for a while, but as it is right
now I can not move from the first message to the third message in my inbox w/o
marking my second message unread. Maybe if I train my fingers to be really fast
I could hold the down key just long enough to move only over the first message
and then release it, but that's really frekin hard to do :-), not to mention
that immediately when you press the down key (and hold it), the message below
the one you have selected is marked read.

.5 seconds is long enough that you can hit a key and then hit it again within
that time, and it's short enough to not be too annoyingly long from the time you
stop hitting the key until you see the message you stopped at. I don't know that
it's the perfect answer, but it's workable.

What I do know is that what we have now sucks, a lot, and something needs to
change. What we had before bug 159477 was fixed was *way* better when you want
to navigate your thread pane by pressing your arrow keys repeatedly.

Comment 12

[Brendan Eich [:brendan]]

jst: nothing should be marked read (or shown) while the arrow key is held down
and the mail user agent is moving selection up or down.  If selection stops,
then maybe after .5 seconds the message should be shown and marked read even if
the key is still held.  But don't get me wrong: I never advocated
showing/marking-read a message that you wanted to skip by holding down an arrow
key while focused in the threadpane.

This all seems solvable -- is there something making it harder than it should be
to fix?  I should go study tree's syntax and semantics before blathering further.

/be

Comment 13

[David :Bienvenu]

I have it kind of working right now, at least for the up arrow key.

Comment 14

[David :Bienvenu]

Attachment: proposed fix for up and down arrow keys

OK, this fix makes it work as we've described for the up and down arrow keys -
when we cursor up to the top, it selects the top line w/o waiting for the up
event, and similarly when we cursor to the button, we select the last line w/o
waiting for the up event. We probably need to do something similar for the page
up and page down keys. Since I've never hacked xml before, I'm probably not
doing things in the best way but it does seem to work (no matter what anyone
says, I think this architecture is pretty cool :-) )

Comment 15

[David Hyatt]

Why not just restore the higher delay?

Comment 16

[Brendan Eich [:brendan]]

hyatt: the high delay is fine if it's only a deadman for when to show the
message and mark it read while you still have the arrow key held down.  If you
release the key, the message should bloody well show right away.  Maybe that's
an easier fix to make, at a lower level in the code?

/be

Comment 17

[David :Bienvenu]

as it is now, the lower level selection code doesn't get the individual
keypresses - it just gets selection events...and I don't think you want to mess
with the code that's giving the key events to tree.xml

Comment 18

[Johnny Stenback (:jst)]

Brendan, after thinking about this some more and experimenting more with MS
Windows Explorer I must say that I disagree with your last comment. In the
Windows Explorer, if you hold down the down key to move down in the folder tree
view and then release the key, there's a ~.5s delay before that folder is shown,
and this is actually really nice IMO. Say you're holding the down arrow key for
a while going down through unread email and you want to stop at a specific
message in the middle of other unread messages, if there's no delay for
downloading and marking the message where the cursor stops when you release the
key it'll be downloaded and read right away, before you have a chance to correct
the selection by pushing the up or down key a few times to select the exact
message you wanted to select.

I must say I'm with hyatt on this one, let's just put back the longer delay, at
least for the thread pane in mailnews. I never found that delay anything but
helpful, I never saw the delay harmful or annoying, but I now find the lack of
the delay extremely annoying in every case where I realize it's missing.

Comment 19

[Brendan Eich [:brendan]]

Well, we all thought it was a bug, and so did people inside netscape.com who
were boosting an alternative mail user agent that (unless I'm badly mistaken
about their complaint) doesn't have this delay.  Someone check that alterna-MUA
out and tell me if it too delays .5 seconds when you arrow up and release the
key while focused on the last message in the threadpane.

/be

Comment 20

[Blake Ross]

There seem to be two different actions described here, quickly arrowing up or
down (i.e., hitting the key multiple times), or holding down and then releasing
the arrow key.

For the former action, Windows seems to only have a delay sometimes, where it's
needed.  In Windows Explorer (on XP), rapidly hitting down in the folder pane
won't load each folder, but rapidly hitting down in the right (contents) pane,
the statusbar is updated each time you select an item.

For the latter action, I can confirm jst's findings that there's a delay after
you release the arrow key, after having held it down.  But this same delay is
not present in Outlook Express.

We definitely shouldn't just restore the longer delay for all trees.  That
sucked.  You arrow keyed down in bookmarks and waited half a second for the
statusbar to update.  We should add the delay back to the thread pane, or try to
pursue the solution brendan is advocating.

Comment 21

[Johnny Stenback (:jst)]

Comment on attachment 96239 [details] [diff] [review]
restore 500 millisecond timer for thread pane

sr=jst, let's get this one checked in! Please!

Comment 22

[Johnny Stenback (:jst)]

Ok, so the proposed fix to restore the 500ms delay for the thread pane doesn't
work, I don't see any change with that diff.

Would someone who knows the tree view code please step in and help us here, if I
don't see any help in this bug soon I'll back out the fix for bug 159477 until
someone comes up with a fix for this bug.

Blake, would you please have a look at this.

Comment 23

[Jan Varga [:janv]]

It can't work because the getter in tree.xml is not completely implemented:
<property name="_selectDelay" onget="return 50;"/>

My 2 cents.

Comment 24

[David :Bienvenu]

you need both of the first two patches for anything to work. But the first patch
doesn't have the correct default. I'll attach a patch that allows just the
thread pane to override the 50 millisecond default.

Comment 25

[David :Bienvenu]

it would be: <property name="_selectDelay" onget="return 500;"/>

I was trying my patch that makes the delay be overrideable by the individual
tree's, but it no longer seems to work - the delay is always 0. I think it
worked before, but I can't swear to it.

      <property name="_selectDelay" 
                onset="this.setAttribute('selectDelay', val);"
                onget="if (this.hasAttribute('selectDelay') return
this.getAttribute('selectDelay') else return 500;"/>

and making threadPane.xul look like this:
<tree id="threadTree" flex="1" enableColumnDrag="true" _selectDelay="500"
class="plain" disableKeyNavigation="true"

Comment 26

[David :Bienvenu]

Hyatt says this should work.  I'm at a loss as to why it doesn't, and I don't
have the cycles to try to figure it out, which is a shame. I think our choices
are to have someone more familiar with xbl/the outliner figure it out, or go
back to the 500 millisecond delay for all trees.

Comment 27

[Blake Ross]

bienvenu: It looks like you have a typo in comment 25.  You have a _selectDelay
attribute on the tree but the _selectDelay getters and setters use selectDelay
(no underscore).

Comment 28

[David :Bienvenu]

Blake, originally I had everything as _selectDelay and that didn't work, so I
tried  changing the name of the attribute (or the getter/setter, I don't
remember). I'll go back and change everything back, but I don't think that worked.

Comment 29

[David :Bienvenu]

Attachment: proposed fix, in conjuction with 96239

this seems to work almost all of the time. Sometimes, the next message gets
loaded because there's a delay before the auto-repeat kicks in, but I suspect
that's been that way all along. So I need r/sr for this patch, and 96239. thx.

Comment 30

[Johnny Stenback (:jst)]

Comment on attachment 98617 [details] [diff] [review]
proposed fix, in conjuction with 96239

+		 onget="return (this.hasAttribute('_selectDelay')) ?
this.getAttribute('_selectDelay') : 500;"/>

Writing code like this in the above way seems to be the norm in mozilla, so I'm
cool with doing the above, but I bet this would be significantly faster:

+		 onget="var d = this.getAttribute('_selectDelay'); return (d &&
d.length) ? d : 500;"/>

Either way, sr=jst

Comment 31

[David :Bienvenu]

Heh, sorry, I'd never write C++ code like that, but xml is such a strange land -
also, FYI, I think the reason the previous patch didn't work was that I was
missing a ')' in the if statement, and for some reason it just silently failed.

I agree that this code should be as fast as possible. I can change it.

Comment 32

[Johnny Stenback (:jst)]

Nominating for mozilla1.2a, this month+ old regression makes mailnews pretty
much unusable (w/o a lot of pain that I've been feeling since this regression
was introduced) if you're primarily using the keyboard to navigate through
messages in mailnews.

Comment 33

[Blake Ross]

Comment on attachment 98617 [details] [diff] [review]
proposed fix, in conjuction with 96239

r=blake

Comment 34

[Brendan Eich [:brendan]]

Comment on attachment 98617 [details] [diff] [review]
proposed fix, in conjuction with 96239

>Index: tree.xml
>===================================================================
>RCS file: /cvsroot/mozilla/xpfe/global/resources/content/bindings/tree.xml,v
>retrieving revision 1.21
>diff -u -w -r1.21 tree.xml
>--- tree.xml	28 Jul 2002 21:45:06 -0000	1.21
>+++ tree.xml	10 Sep 2002 19:58:07 -0000
>@@ -88,7 +88,9 @@
>                 onset="if (val) this.setAttribute('disableKeyNavigation', 'true');
>                        else this.removeAttribute('disableKeyNavigation'); return val;"/>
>       
>-      <property name="_selectDelay" onget="return 50;"/>
>+      <property name="_selectDelay" 
>+                onset="this.setAttribute('_selectDelay', val);"
>+                onget="return (this.hasAttribute('_selectDelay')) ? this.getAttribute('_selectDelay') : 500;"/>

Really, you just want this, I think:

+		 onget="return this.getAttribute('_selectDelay') || 500;"/>

There's no need to test (d && d.length) to exclude an empty string -- just d
would work (for jst's suggestion).  But || and && in JS work as in Perl -- they
convert their first operand to boolean, and short-circuit evaluation as in C,
*but they preserve the type and value of whichever operand decides the outcome
of the logical connective*.  Ain't JS fun?

/be

Comment 35

[Asa Dotzler [:asa]]

Comment on attachment 98617 [details] [diff] [review]
proposed fix, in conjuction with 96239

a=asa (on behalf of drivers) for checkin to 1.2a

Comment 36

[Asa Dotzler [:asa]]

Comment on attachment 96239 [details] [diff] [review]
restore 500 millisecond timer for thread pane

a=asa (on behalf of drivers) for checkin to 1.2a

Comment 37

[David :Bienvenu]

heh, that is cool. I'll go with Brendan's suggestion. It works fine. I'll check
it in.

Comment 38

[David :Bienvenu]

fix checked into the trunk.

Comment 39

[Johnny Stenback (:jst)]

Wooohohohohohhhoooooo.

/me dances around in his cube with a big smile on his face :-)

Comment 40

[Blake Ross]

gah, no. This patch is wrong.  We reviewers should be shot!

Your patch falls back on 500 if there's no attr, but it should be 50. That's the
bug we were fixing in the first place.

Comment 41

[David :Bienvenu]

geez, you're right. sorry.

Comment 42

[Blake Ross]

I just checked in the fix.

Comment 43

[laurel]

OK using nov01 commercial trunk: win98, linux rh6.2, mac OS 10.2
No longer herky jerky!