403749 - Enter/Return in Join Channel dialog doesn't join channel

Status: RESOLVED FIXED

(Other Applications :: ChatZilla, defect)

Description

[James Ross]

I'm pretty sure this is a regression; hitting Enter/Return with the focus in the search box should join the selected channel in the list (which might be the custom one for what the user has entered or an existing channel). The Join button is indicated as the default at this time, and it should act like it.

I'm sure it used to work - this might be a trunk Gecko/XUL regression or it might be my reworked code for the dialog, I don't know yet.

Comment 1

[:Gijs (he/him)]

Doesn't work anymore on Fx 2.0.0.9 either, so I'm guessing it's a regression on our end.

Comment 2

[:Gijs (he/him)]

Actually, it doesn't work in .78.1 or .72, so I bet it never really did work. :-(

Comment 3

[:Gijs (he/him)]

Er, meant .73. Whatever. Nuking regression keyword.

Comment 4

[:Gijs (he/him)]

Attachment: Patch

This ought to work. I also nuked 4 strict warnings, so the patch is a bit convoluted, sorry about that.

Comment 5

[James Ross]

Comment on attachment 288640 [details] [diff] [review]
Patch

That doesn't look right at all. It's supposed to join the channel selected in the list, not what you've typed in the box.

I don't know why you need the sortColumn check, but that is wrong.

And I'm sure you didn't mean just "channelText" at the end.

Comment 6

[:Gijs (he/him)]

(In reply to comment #5)
> (From update of attachment 288640 [details] [diff] [review])
> That doesn't look right at all. It's supposed to join the channel selected in
> the list, not what you've typed in the box.

Uh, yeah. Thanks for the cluestick. The reason I didn't do this (it would be easier, just calling joinChannel) is that it didn't work so well, it made me join "#". Not sure how that happened, figured it was the create channel row with the filter being lazy, or something.

> 
> I don't know why you need the sortColumn check, but that is wrong.

Because it was undefined at some point. Got a warning in my error console.

> And I'm sure you didn't mean just "channelText" at the end.
> 

Yeah, I meant opData.channelText. Good catch.

Comment 7

[:Gijs (he/him)]

(In reply to comment #5)
> (From update of attachment 288640 [details] [diff] [review])
> That doesn't look right at all. It's supposed to join the channel selected in
> the list, not what you've typed in the box.

Actually, I think it *is* right. If I've focused the box, there are two cases that I can see:
1) the channel exists and is listed in /list, in which case after the filtering runs, this will be the selected item and we Should join this, and if we use the text typed in the box, will do so.
2) the channel does not exist or is not listed in /list, in which case after the filtering runs, we will select the "create channel" row.


If you select another channel in the userlist and hit return, the onRowcommand should take care of going to that precise channel.

Am I missing something?

Comment 8

[James Ross]

You're completely missing that the user can change the selected item in the list - and is in fact expected to do so. The dialog is designed to operate for the most part without any focus changing; you type something in, it searches, you can use up/down arrows to change the selected item, and (this bug) you hit enter to join whatever you picked.

Use the list selection or it's wrong.

Comment 9

[:Gijs (he/him)]

OK. Thanks for yet another cluestick. Here's a different scenario:

you type something in, and then you hit enter immediately expecting it to join the channel you typed (particularly if it exists already), then the behaviour if we're using the list selection will be incorrect as it takes half a second for the filtering to kick in and select the channel you typed.


A possible way to work around this is to set a flag after filtering a channel (processOpFilterStop) and remove it onkeypress for anything which changes the value of the textbox (and check it when processing the return key). The problem I see with this approach is race conditions: if I type something at time N, the filter starts running at N+500ms, and I type something else at N+750ms, the filter finishes running at N+800ms, then the flag will be wrong. Or am I missing something again? :-(

Comment 10

[James Ross]

Pressing <Enter> in the textbox causes the filter to start immediately and skip the pause. As the first thing OP_FILTER does is to set up the "create channel" row to match the input, unhide it and select it, I think things should be fine. (The only proviso is that our <Enter> code runs after the textbox's.)

Comment 11

[:Gijs (he/him)]

Attachment: Better Patch

This fixes two strict warnings and the original issue of this bug. Finally. :-)

Comment 12

[James Ross]

Comment on attachment 302317 [details] [diff] [review]
Better Patch

Two issues:
1. equalsObject doesn't check properties on o2 that aren't on o1 (do the for..in both ways to fix).
2. The stored data for the filter doesn't include the "search topics" checkbox. This is why I'm giving review-.

The rest looks OK.

Comment 13

[:Gijs (he/him)]

Attachment: Patch testing o1 + o2, saving searchOptions

Comment 14

[James Ross]

Comment on attachment 302672 [details] [diff] [review]
Patch testing o1 + o2, saving searchOptions

>+    for (p in o1)

Nit: |var| on the first one.

Comment 15

[:Gijs (he/him)]

Checked in. I also fixed the for (p in pattern) loop in matchObject to use a var ;-)

--> FIXED.

Checking in mozilla/extensions/irc/js/lib/utils.js;
/cvsroot/mozilla/extensions/irc/js/lib/utils.js,v  <--  utils.js
new revision: 1.70; previous revision: 1.69
done
Checking in mozilla/extensions/irc/xul/content/channels.js;
/cvsroot/mozilla/extensions/irc/xul/content/channels.js,v  <--  channels.js
new revision: 1.7; previous revision: 1.6
done