66456 - Checkbox state doesn't persist in Replace dialog

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Blake Ross]

The states of the checkboxes in the replace dialog don't persist, for the same 
reason they didn't in the find dialog until I fixed that...

Unfortunately, my find dialog cleanup came too late, and replaceDialog.js 
(which uses a lot of its code) needs similar cleanup.

Comment 1

[Blake Ross]

Attachment: [patch] cleanup

Comment 2

[Blake Ross]

kin, sr?

Comment 3

[timeless]

r=timeless for the code, but I request that the mime type for javascript follow 
the recommendations current in bug 65428

Comment 4

[kinmoz]

Blake, thanks for fixing these files! As you can probably tell, I was in a hurry 
to get the backend replace code done, and needed a dialog to test it, so I just 
used the finddialog code as a starting point and modified it to suit my needs.

Some questions/comments:

1. I suppose the change from using SetAttribute("value", "foo") to obj.value = 
"foo" is due to the revelation that SetAttribute actually sets the default value 
for the attribute? Btw, I like your code it's much more compact. :-)
 
2. You changed the button from "Close" to "Cancel" ... but "Cancel" implies that 
any changes you have made will automatically be undone when it is pressed. This 
is not the case with this dialog.

3. I noticed you removed the check for a null gFindComponent in onLoad() I 
suppose this is because QueryInterface should throw an exception if something 
goes wrong? Shouldn't we wrap the GetService and QueryInterface calls with a 
try/catch so we can close the dialog if something goes wrong?

Also, I'm not sure if we need to get [e-mail] to approve these changes 
since he is module owner for this code.

Comment 5

[Blake Ross]

Hey Kin,

1. They're actually the same.  Value is a property of <textfield/> per its 
binding 
(http://lxr.mozilla.org/seamonkey/source/xpfe/global/resources/content/xulBindin
gs.xml#299) so we can use this notation.  It's really just syntatic sugar, and 
should be the same as using setAttribute/removeAttribute (but as you pointed 
out, it makes things cleaner and easier.

2. Yeah, you're right. I just changed it to match our Find dialog and that of 
Windows.  We could leave it Close, but we'll need to change the Find dialog's 
then (for consistency).  cc'ing Matthew

3. This is based on the idea that retrieval of the Find component should almost 
never fail (if it does, things are terrible wrong in the world), and as such 
throwing an exception would be much more useful for figuring out the problem 
than a human/user-readable alert (which wasn't localizable, anyways).  We're 
starting to do this in other places as well.

By the way, these are basically the same changes as for Find, which were 
already approved (but by ben, iirc).  Hope this helps.

Comment 6

[kinmoz]

sr=[e-mail]

Comment 7

[Matthew T (active 1999-2002)]

No window should ever have a button labelled `Close'. Where such a button exists 
in other apps (Microsoft frequently makes this mistake, for example), in about 70 
percent of cases the wrong kind of window is being used and the button shouldn't 
be present at all, and in the other 30 percent of cases the button should be 
renamed to `Cancel' or `OK'.

Which is the case here depends on how the Find/Replace window is used. If it can 
be kept open to carry out a number of Find/Replace operations one after the 
other, and it needs to be explicitly closed when finished, then it should be a 
document-/utility-type window (closable, minimizable, not modal), and should not 
have an extra `Close' button.

If, however, it is intended for one-time entering of search/replace data, and 
menu/toolbar items (with their own keyboard shortcuts) are then used for Find 
Next, Replace Next, etc (this is my preferred option), then it should be a dialog 
window (not closable, not minimizable, modal), and the button should be labelled 
`Cancel'. If `Cancel' is chosen, changes made to the dialog (such as
checking/unchecking checkboxes) should *not* persist.

If in doubt, e-mail me a screenshot. (Sorry, I'll be away from Bugzilla for a 
while, so attaching a screenshot here won't attract my attention.)

Forwards-compatibility note: Now, before a large number of users has seen it, 
might be a good time to rename the feature `Find/Change'; since in the future it 
will be able to Change style/capitalization/whatever of the found (possibly 
regexp) text, rather than just Replacing it with some other text.

Comment 8

[Blake Ross]

Fix was checked in.