528379 - Provide Cocoa prompt for select types.

Status: RESOLVED FIXED

(Camino Graveyard :: General, enhancement, P3)

Description

[Christopher Henderson]

Bug 306613 brought this to light. CocoaPromptService needs to provide an implementation of the select method so we can create a new boolean pref.

Firefox 3.5 has the options in a list box, but I'm leaning towards using an NSPopupButton.

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

[6:16pm] sauron: we should get an opinion from smorgan on the UI
[6:18pm] smorgan: Popup seems like it makes the most sense

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

P3 on the 2.1 list.

Comment 3

[Christopher Henderson]

Attachment: Fix v1.0

Provides a select prompt using code similar to the other prompt methods.

Comment 4

[Stuart Morgan]

Comment on attachment 427518 [details] [diff] [review]
Fix v1.0

>+- (BOOL)select:(NSWindow*)parent title:(NSString*)title text:(NSString*)text selectList:(NSArray*)selectList selectedIndex:(unsigned int*)selectedIndex;

Needs a method-level comment (for example, the only way to know that selectedIndex is both an input and output param right now is to read the whole implementation).

>+const int kPopupButtonHeight = 26;

In my testing, sizeToFit gives you the correct height, so you shouldn't have to hard-code a standard control size. Was that not the case for you on some OS version?

>+  [selectPopup setFont:[NSFont systemFontOfSize:[NSFont systemFontSize]]];

Why? If you want something other than a standard control size, you should use systemFontSizeForControlSize:, but if it's standard you should have the right size already.

>+  [selectPopup setBordered:YES];

Isn't this the default?

>+  [panel setInitialFirstResponder: selectPopup];

No space after :

>+  int result = [self runModalWindow:panel relativeToWindow:parent];
>+
>+  [panel close];

Is the close actually necessary? I would expect closing it to be the trigger for runModalWindow:relativeToWindow: to return.

>+  NSString* titleStr = [NSString stringWithPRUnichars:dialogTitle];
>+  NSString* textStr = [NSString stringWithPRUnichars:text];

Any reason not to inline these in the call below?

>+  unsigned int selIndex = 0;

Declare this down by nsresult.
But come to think of it... why is selectedIndex an input param to your helper (instead of just output) if the API you are implementing this for doesn't need that?

>+    *_retval = (PRBool)[controller select:[browserView nativeWindow]

The baby Jesus prefers we avoid casting between bool types, as unexpected truncation bugs make him cry. Instead, assign the result to a local BOOL, and then use a ?: to assign actual PRBool values to *_retval based on that variable.

Comment 5

[Christopher Henderson]

Attachment: Fix v1.1

Patch with review comments addressed. The panel won't close without telling it to, so we need to keep that line. selectedIndex is now just an output parameter.

Comment 6

[Stuart Morgan]

Comment on attachment 427905 [details] [diff] [review]
Fix v1.1

r=smorgan with two nits:

>+// -select: title: text: selectList: selectedIndex:

Method signatures don't have spaces in them, so if you are going to put it here it should be without spaces.

>+  //  get panel and display it

One space after //, not two, and it should be capitalized and have a period.

Comment 7

[Christopher Henderson]

Attachment: Fix v1.2

Fix with comments modified as suggested. Testing behaviour of this patch depends on the patch for Bug 306613 being landed or applied.

Comment 8

[Mike Pinkerton (not reading bugmail)]

+  unsigned int selIndex;
+  nsresult rv = NS_OK;

I know this gets set by the function call, but I'd still rather see it initialized. In addition, when documenting -select:title:text... you should mention that the value is always set on output just in case someone wants to try to rely on it not getting clobbered in certain instances.

sr=pink

Comment 9

[Christopher Henderson]

Attachment: Fix v1.3, for checkin

Patch with sr comments addressed, for checkin.

Comment 10

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Landed on cvs trunk and the test repo: http://hg.mozilla.org/users/alqahira_ardisson.org/camino-1.9.2-test/rev/43ec7cfeb77a

Comment 11

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

http://hg.mozilla.org/camino/rev/da72989847af

Comment 12

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Landed on the CAMINO_2_0_BRANCH.