228017 - UI Consistency: "make default search engine" dialog's button choice is counter-intuitive

Status: RESOLVED FIXED

(SeaMonkey :: Search, defect)

Description

[Chris Andreae]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5) Gecko/20031007
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5) Gecko/20031007

When choosing a non-default search engine from the sidebar search feature, the
user is presented with a dialog "Change default search engine?" with text "Would
you like to make %s your default search engine?".  This is a yes/no question,
and the user should be presented with a yes/no response.  Instead, this dialog
has the "OK/Cancel" button choice.  The meaning of OK/Cancel in this context is
unclear, and will potentially confuse novice users.

Reproducible: Always

Steps to Reproduce:
1. Choose non-default search engine in sidebar search feature.
Actual Results:  
User presented with OK/Cancel

Expected Results:  
User presented with Yes/No

Comment 1

[Andreas Kunz]

Attachment: patch

Fixes what this bug report is about.

Comment 2

[Andreas Kunz]

Comment on attachment 145805 [details] [diff] [review]
patch

Asking Neil for review (or wontfix or whatever).

Bug 115852 is about why this dialog is generally bad, but I don't have the time
to fix that one.

Comment 3

[Andreas Kunz]

Comment on attachment 145805 [details] [diff] [review]
patch

Wait a minute, yes/no logic is reversed...

Comment 4

[Andreas Kunz]

Attachment: correct patch

Stupid me, confirmEx returns the pressed button, of course.
This patch does it correctly (also, when the dialog close box in the upper
right corner is pressed, also "1" for "No" is returned, as one would expect).

Comment 5

[Andreas Kunz]

Comment on attachment 145808 [details] [diff] [review]
correct patch

Requesting r= from Neil (see comment #2).

(You would not rather like a hack like:
change = !promptSvc.confirmEx([...]
or something similar confusing boolean and int vars, would you?)

Comment 6

[[e-mail]]

Comment on attachment 145808 [details] [diff] [review]
correct patch

No, your test is fine, given the state of the rest of the code in that function
...

Comment 7

[Andreas Kunz]

Comment on attachment 145808 [details] [diff] [review]
correct patch

Requesting sr= from alecf.

Comment 8

[Alec Flett]

Comment on attachment 145808 [details] [diff] [review]
correct patch

ugh, I think this should be &, not + - obviously it doesn't make a difference
in this case, but this is a bitmask, right?
sr=alecf with & or with a good explanation as to why you're using +

Comment 9

[Andreas Kunz]

alecf:
My "good reason" is that ConfirmEx seems to be only - or at least in the vast
mayority of cases - used in this manner ("+").
I just asked lxr and the first ten occurrences (all over the codebase) all had a
"+", none a "&".
I think I don't need to list all of them here, just for starters:
http://lxr.mozilla.org/seamonkey/source/xpfe/browser/resources/content/navigator.js#2301
http://lxr.mozilla.org/seamonkey/source/editor/ui/composer/content/ComposerCommands.js#2101
http://lxr.mozilla.org/seamonkey/source/xpfe/components/winhooks/nsWindowsHooks.cpp#447
http://lxr.mozilla.org/seamonkey/source/mailnews/base/prefs/resources/content/AccountWizard.js#178

After thinking about it, It's clear that "&" makes more sense, but I'm not able
to find anybody using it like this, so maybe it should be left as it is, for
consistency. (I don't remember, is consistency with bad things good?)
What do you think?

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

I checked in as-is for consistency with the rest of the code...
(you two did mean | when you said &, right?)

Checking in xpfe/components/search/resources/search-panel.js;
/cvsroot/mozilla/xpfe/components/search/resources/search-panel.js,v  <-- 
search-panel.js
new revision: 1.83; previous revision: 1.82
done