Closed Bug 959742 Opened 6 years ago Closed 6 years ago

Prompt.jsm: selecting single choice option closes prompt.

Categories

(Firefox for Android :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30

People

(Reporter: jdover, Assigned: wesj)

References

Details

(Whiteboard: [mentor=wesj][lang=java])

Attachments

(1 file, 2 obsolete files)

As mentioned in bug 953272, prompts should allow the user to confirm their choice before dismissing the prompt for single choice lists.
Only if there are buttons.

If there are no buttons, the choice should simply cause it to proceed.

This is the "select from a list and continue" model.
We also have an "icongrid" input type I'd like to do this for too. That one will be a bit more work though.
Whiteboard: [lang=java] → [mentor=wesj][lang=java]
I asked two people to file this at once. Since I'm assigning to Josh, I'll dup the other one to his bug :)
Assignee: nobody → jdover
Duplicate of this bug: 959418
Blocks: 942270
Attached patch Patch (obsolete) — Splinter Review
I needed something like this for the Context quickshare as well, so I did it this way.

This adds listeners to normal PromptInput types and closes the dialog if their value changes. It also hooks the same code up for multiselect lists (and single select ones). All my test pages seem happy with this.
Attachment #8376561 - Flags: review?(bnicholson)
Comment on attachment 8376561 [details] [diff] [review]
Patch

Review of attachment 8376561 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/prompts/Prompt.java
@@ +358,5 @@
>          mSelected[position] = !mSelected[position];
> +
> +        // If there are no buttons on this dialog, then we take selecting an item as a sign to close
> +        // the dialog. Note that means it will be hard to select multiple things in this list, but
> +        // given there is no way to confirm+close the dialog, it seems reasonable.

I'm not sure I understand the second sentence. By "hard to select", do you mean "cannot select"? Do our Android prompts currently support selecting multiple items? If so, won't this break them?

@@ +455,5 @@
>          show(title, text, menuitems, multiple);
>      }
>  
> +    // Called when the prompt inputs on the dialog change
> +    public void onChange(PromptInput input, String value) {

You're not even using value here -- can we get rid of it? Or are you using this in a later patch?

::: mobile/android/base/prompts/PromptInput.java
@@ +49,4 @@
>      protected View mView;
>      public static final String LOGTAG = "GeckoPromptInput";
>  
> +    public interface Listener {

Please follow the naming convention that describes the listener's purpose (e.g., OnChangeListener).
Comment on attachment 8376561 [details] [diff] [review]
Patch

Review of attachment 8376561 [details] [diff] [review]:
-----------------------------------------------------------------

This patch looks mostly fine to me, but I'm curious about the point of the onChange String value as mentioned before, so setting r- just to get this out of my queue while I wait for the answer.

::: mobile/android/base/prompts/Prompt.java
@@ +358,5 @@
>          mSelected[position] = !mSelected[position];
> +
> +        // If there are no buttons on this dialog, then we take selecting an item as a sign to close
> +        // the dialog. Note that means it will be hard to select multiple things in this list, but
> +        // given there is no way to confirm+close the dialog, it seems reasonable.

OK, I understand this comment now -- you can't have a multi-select prompt with no buttons since there's no way to submit the prompt after you've made the selections. Can you change "it will be hard to select" to "we cannot select"?
Attachment #8376561 - Flags: review?(bnicholson) → review-
Attached patch Patch (obsolete) — Splinter Review
In case you can't tell, I'm not a big fan of Android's naming conventions, and I really don't think we should blindly follow them. That said, I don't care that much :)
Attachment #8376561 - Attachment is obsolete: true
Attachment #8381595 - Flags: review?(bnicholson)
Comment on attachment 8381595 [details] [diff] [review]
Patch

Review of attachment 8381595 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Wesley Johnston (:wesj) from comment #8)
> Created attachment 8381595 [details] [diff] [review]
> Patch
> 
> In case you can't tell, I'm not a big fan of Android's naming conventions,
> and I really don't think we should blindly follow them.

I'm not sure whether this is officially an Android convention or not -- I don't see this mentioned anywhere on https://source.android.com/source/code-style.html. From what I've seen, though, it is our own convention that our event listeners are usually of the form On<action> (see [1], [2], [3], etc).

I'd be fine with changing our convention if we have good reason to, but this is probably something worth bringing up to the mailing list or meetings instead of individual bugs. IMO, the most important rule is consistency, and it's hard to be consistent with conventions if people aren't aware of them!

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/widget/GeckoActionProvider.java#31
[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/widget/DoorHanger.java#82
[3] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/BrowserSearch.java#130
Attachment #8381595 - Flags: review?(bnicholson) → review+
whoops. lost a line in here. backed out. will reland after a local build:
https://hg.mozilla.org/integration/fx-team/rev/977b4d18c0e9
Had to push a follow-up due to the backout also being busted.
https://hg.mozilla.org/integration/fx-team/rev/37a363478881
Thanks. That's what I get for fixing while I'm backing out :)

back in. Looks fine here.
https://hg.mozilla.org/integration/fx-team/rev/e82cb49c7a8d
Assignee: jdover → wjohnston
Attached patch Patch v2Splinter Review
Sorry to send you through this again. The problem with the last patch is that I was using one listener for both Single select lists and button clicks (its the same interface). closeIfNoButtons() obviously doesn't work if you're clicking the buttons (the dialog closes but we never sent a response).

This adds a separate (anonymous) listener for the single select case, and just force closes for the buttons.
Attachment #8381595 - Attachment is obsolete: true
Attachment #8383437 - Flags: review?(bnicholson)
Flags: needinfo?(wjohnston)
Comment on attachment 8383437 [details] [diff] [review]
Patch v2

Review of attachment 8383437 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/prompts/Prompt.java
@@ +266,5 @@
>          mAdapter = new PromptListAdapter(mContext, R.layout.select_dialog_singlechoice, listItems);
> +        builder.setSingleChoiceItems(mAdapter, mAdapter.getSelectedIndex(), new DialogInterface.OnClickListener() {
> +            @Override
> +            public void onClick(DialogInterface dialog, int which) {
> +                ThreadUtils.assertOnUiThread();

This assertion might be a bit more useful inside closeIfNoButtons(). It's pretty clear here that we'll be on the UI thread since this is a callback directly from Android's dialog. It's less clear we're on the UI thread inside closeIfNoButtons(), requiring us to look at its callers to verify.

@@ +397,5 @@
>  
>      /* Called any time we're closing the dialog to cleanup and notify listeners that the dialog
>       * is closing.
>       */
> +    public void closeDialog(int which) {

public -> private

@@ +460,5 @@
>          show(title, text, menuitems, choiceMode);
>      }
>  
> +    // Called when the prompt inputs on the dialog change
> +    public void onChange(PromptInput input) {

@Override
Attachment #8383437 - Flags: review?(bnicholson) → review+
https://hg.mozilla.org/mozilla-central/rev/f89196ce3d3e
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
You need to log in before you can comment on or make changes to this bug.