179050 - Clean up timed textbox binding and use it in mailnews and addressbook

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Jan Varga [:janv]]

Comment 1

[[e-mail]]

Also the editor image properties dialog.

Comment 2

[[e-mail]]

Attachment: My take on the textbox.xml changes

Comment 3

[Jan Varga [:janv]]

Neil, your patch looks good, although it should call this.select() in fireCommand()
Please check its behaviour to match quick search in mailnews and addressbook
This bug is also about reusing the timed textbox in mailnews and addressbook
But I'm ok with cleaning the binding first.
We can patch mailnews and addressbook later.

Comment 4

[[e-mail]]

Actually it should call this.select() in the keypress handler :-P

I'm practically done on my RLE decoder so I'll attack this patch next.

Comment 5

[[e-mail]]

Attachment: Fix addressbook and mail quick search (phew!)

Jan, I now see why the clear button wasn't a quick fix...

Comment 6

[[e-mail]]

Attachment: bitrot

Comment 7

[Jan Varga [:janv]]

Neil, your patch looks pretty good, although I think there are changes which
belong to other bug.

One nit:
I would not remove |onfocus="this.select()"|

Comment 8

[[e-mail]]

Attachment: Really updated for bitrot :-)

Sorry Jan, I've just got too many MailNews patches ;-)

Comment 9

[[e-mail]]

Attachment: Fixed Jan's nit

I see Jan's point now - Bug 178520 was hiding the problem.

Comment 10

[Jan Varga [:janv]]

note that there were some changes which affected this patch

Comment 11

[[e-mail]]

Attachment: Updated for bitrot

Comment 12

[Jan Varga [:janv]]

Comment on attachment 116769 [details] [diff] [review]
Updated for bitrot

Looks pretty good, just one thing.
I'm not sure if value setter should fire command event. I have a feeling that
we don't do that elsewhere.
Jag, what do you think ?
If we get a consensus on that r=varga

Comment 13

[(not reading, please use [e-mail] instead)]

note that shliang was in this code twice recently for bug #128503,
making mail and addressbook quick search behave the same.
(see her checkins on 2/27 and the follow up for a regression on 3/04)

how does that work affect your patch?

don't get me wrong, I'll all for code cleanup.
but will this fix have a noticable on the end user?
If not, consider pushing this to 1.5 alpha.

I'd much rather spend cycles on something that directly affects the end user.

Comment 14

[[e-mail]]

Attachment: bookmarks manager renaming bitrot

Comment 15

[Jan Varga [:janv]]

Comment on attachment 119450 [details] [diff] [review]
bookmarks manager renaming bitrot

r=varga with the change I already mentioned.
In my opinion, the value setter shouldn't fire command events. Note that you
need to fix callers too.

Comment 16

[[e-mail]]

I finally found a "regression" from those command events you mentioned - it
breaks my proposed fix to another bug, so I'll have to rewrite this anyway...

Comment 17

[[e-mail]]

Attachment: Address Jan's nit

I also tried out using an nsITimer component to make the code cleaner.

Comment 18

[Jan Varga [:janv]]

+        <setter>
+          this.inputField.value = val;
+          if (this._timer)
+            this._timer.cancel();
+          this.doCommand();
+          return val;
+        </setter>

Well, I'm not sure if you fixed my nit. It seems that the value setter still
fires command events.

Comment 19

[[e-mail]]

Attachment: D'oh! :-[

Well, at least I managed to fix the callers :-)

Comment 20

[Jan Varga [:janv]]

Well, sorry for saying it, but I don't like that nsITimerCallback in JS code
I believe jag would be more comfortable with the standard setTimeout function too.
Other than that r=varga

Comment 21

[[e-mail]]

I've just noticed that a textbox context menu generates command events :-(

Comment 22

[[e-mail]]

Attachment: Now that it's partly checked in...

Comment 23

[Jan Varga [:janv]]

Comment on attachment 125425 [details] [diff] [review]
Now that it's partly checked in...

r=varga
since I reviewed this multiple times and the only thing I objected to is
already in

Comment 24

[[e-mail]]

Attachment: Remove extra line

Jan: I'm sorry about that, I did mean to take that out again :-[

also I asked shuehan for review because Seth mentioned it.

Comment 25

[Jan Varga [:janv]]

no problem, thanks for fixing it

Comment 26

[[e-mail]]

*** Bug 106705 has been marked as a duplicate of this bug. ***

Comment 27

[Mike Cowperthwaite]

Neil, is this patch still viable?

Comment 28

[Mike Cowperthwaite]

I would like to see this patch checked in, particularly if bug 191813 comment 3 
is correct and the patch addresses that bug as well.

Comment 29

[[e-mail]]

Attachment: Updated

I've updated the patch for bitrot, and also applied the changes to
Thunderbird's address book (its main mail quick search is no longer suitable).
Unfortunately for Mike I have no idea if it still appears to fix bug 191813 or
not.

Comment 30

[Asa Dotzler [:asa]]

mscott, is this something you (we) want for 1.1? If so, can you review and if
not, can you set the blocking 1.1 flag to minus? Thanks.

Comment 31

[Scott MacGregor]

Let's go after this for 1.9, I don't think we need to get it in for the 1.5
releases and I haven't had a chance to look at this yet. 

Comment 32

[Scott MacGregor]

Comment on attachment 125425 [details] [diff] [review]
Now that it's partly checked in...

this patch is obsolete, there's a newer one in the bug.

Comment 33

[Scott MacGregor]

Comment on attachment 177669 [details] [diff] [review]
Updated

part of this patch is obsolete, in particular the changes to msgViewPickerOverlay.js.

I hope that change isn't going to break switching to a virtual folder:

http://lxr.mozilla.org/mozilla/source/mailnews/extensions/mailviews/resources/content/msgViewPickerOverlay.js#107

Comment 34

[[e-mail]]

Attachment: 3pane patch

I checked in the addrbook changes from the previous patch, and simplfied the changes for this patch to avoid problems with virtual folders.

Comment 35

[Matt Seitz]

Bug 286367, Comment 11 says fixing this bug will also fix bug 286367.  I tried adding Bug 286367 to the "blocks" field, but I don't have permssion to set that field.  

Would someone add Bug 286367 to the "blocks" field?

Comment 36

[[e-mail]]

Unfortunately Quick Search has changed sufficiently in the interim that I'm no longer in a position to make claims regarding bug 286367, and even if I was I could only fix SeaMonkey's Quick Search, not Thunderbird's.

Comment 37

[Matt Seitz]

> Unfortunately Quick Search has changed sufficiently in the interim that I'm no
> longer in a position to make claims regarding bug 286367, 

Does this mean that fixing this bug may not fix bug 286367?

>and even if I was I
> could only fix SeaMonkey's Quick Search, not Thunderbird's.

Bug 286367 is a Seamonkey bug, not a Thunderbird bug.  So, if fixing this bug fixes Seamonkey, that is sufficient to resolve Bug 286367.

Comment 38

[[e-mail]]

Comment on attachment 203792 [details] [diff] [review]
3pane patch

Not sure why I didn't request sr, maybe the r+ bugmail got lost :-/

Comment 39

[[e-mail]]

Fix checked in to the trunk. I did one test that I know triggers bug 286367 and this patch definitely fixes that case, but I didn't test it exhaustively.

Comment 40

[David :Bienvenu]

I didn't have much luck porting this to Thunderbird - searches stopped happening, so I must have screwed something up...

Comment 41

[[e-mail]]

Fix checked in to the branch.