Closed Bug 968908 Opened 11 years ago Closed 11 years ago

Prompt.jsm: result from addCheckbox is a string true/false - should be a boolean

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 30

People

(Reporter: mkaply, Assigned: abhizuko)

References

Details

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

Attachments

(1 file, 4 obsolete files)

If you add a checkbox using addCheckbox, when you look at the result, it is either "true" or "false".

It should be an actual boolean, not a string.
Whiteboard: [lang=js][mentor=wesj]
Could I be assigned this bug?
(In reply to Mike Kaply (:mkaply) from comment #0)
> If you add a checkbox using addCheckbox, when you look at the result, it is
> either "true" or "false".
> 
> It should be an actual boolean, not a string.

Doesn't addCheckbox() return the modified prompt?
Assignee: nobody → abhizuko
Problem is I'm still unsure of what the actual fix would be.  Since the function addCheckBox returns an object (the prompt) via the _addInput method, I don't get why it would return a boolean, nor do I see it returning a string at any point.

On an unrelated note, since this is my first bug, could I get a bit of information about how to go about making the patch and getting it reviewed or should I just initiate a pull request if I've finished?
The issue is what happens after the .show, not the prompt.

The .show creates a Javascript object with the results of the user interacting with the prompt.

The issue that the checkbox result in that object is "true" or "false" not true or false.
OK.  Thanks.  I'll start work on it.
wesj can also offer some advice on how to approach this bug.
Flags: needinfo?(wjohnston)
I actually have a fix for this over in bug 942270, but since its tied up in such a large refactor, I'd be fine if you wanted to pull it out and land it separately here. First step is just to get builds going though: https://wiki.mozilla.org/Mobile/Fennec/Android

Once you have that done, we just need to modify our PromptInput's in java code to return objects instead of strings:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/prompts/PromptInput.java#373

Then you'll have to go around and update all of the implemented PromptInput objects to return something better. i.e. The PromptCheckbox (in the same file) can return Boolean.TRUE or Boolean.FALSE;
Flags: needinfo?(wjohnston)
OK.  I would change the return type of the function getValue() to boolean and then change the last line to return the Boolean object.  And I would do this for all such functions, yes?  

What procedure would I follow for creating a patch here?
(In reply to abhizuko from comment #8)
> OK.  I would change the return type of the function getValue() to boolean
> and then change the last line to return the Boolean object.  And I would do
> this for all such functions, yes?  

I've changed it for the getValue() function on line 147 in the class CheckboxInput.  As for the return of PromptInput's getValue() function on line 373, I'm unsure of what value to use.
Just to be clear, lets turn the return type to Object. We take these returns and throw them onto a JSON response in Java here:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/prompts/Prompt.java#197

If we pass Objects, the JSONObject should just do the right thing for us. You can just use the same "" (i.e. an empty String Object) for the default return value (we should probably make that abstract and pull these subclasses out into their own implementations at some point).

Once you've got that done, building, and tested, you just need to make it into a diff/patch and upload it to the bug. There's pretty detailed instructions here:

https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

You can mark me for the review when you upload the bug (just put :wesj in the review box).

Lots of reading there, ask away here or in #mobile on irc.mozilla.org if you've got questions.
Attached patch Changes return types to Object (obsolete) — Splinter Review
Attachment #8374167 - Flags: review?(wjohnston)
Comment on attachment 8374167 [details] [diff] [review]
Changes return types to Object

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

Nice! These are mostly nits, but need to be fixed :)

There's also two more classes in this same package that implement PromptInput, ColorPickerInput and IconGridInput. We should update them as well. If I'd added proper @Override annotations, they'd have told you that themselves :)

::: mobile/android/base/prompts/PromptInput.java
@@ -84,5 @@
>              mView = (View)input;
>              return mView;
>          }
>  
> -        public String getValue() {

Can you also add @Override annotations on these methods as well. All of them that are inheriting from PromptInput base class need it.

@@ +89,2 @@
>              EditText edit = (EditText)mView;
>              return edit.getText().toString();

We may not need this toString() calls anymore either.

@@ -255,5 @@
>              }
>              return super.getValue();
>          }
>      }
> -

Whoops. Leave this space :)

@@ +305,1 @@
>              return Integer.toString(spinner.getSelectedItemPosition());

We should return an Integer object here. i.e. return new Integer(spniner.getSelectedItemPosition()); You might just try returning the int. I'm not sure how much we can trust java's boxing/unboxing.
Attachment #8374167 - Flags: review?(wjohnston) → feedback+
Attached patch patch 2 (obsolete) — Splinter Review
Attachment #8374167 - Attachment is obsolete: true
Attachment #8374841 - Flags: review?(wjohnston)
Comment on attachment 8374841 [details] [diff] [review]
patch 2

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

Sorry. One last round. Looks good though. Still need to make these same changes in these two files :)

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/prompts/ColorPickerInput.java
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/prompts/IconGridInput.java
Attachment #8374841 - Flags: review?(wjohnston) → review-
Unsure whether you need me to consolidate this patch with the last one.
Attachment #8374946 - Flags: review?(wjohnston)
Attached patch messed up the previous patch... (obsolete) — Splinter Review
Attachment #8374964 - Flags: review?(wjohnston)
Attachment #8374964 - Attachment is patch: true
Attachment #8374964 - Attachment mime type: message/rfc822 → text/plain
Attachment #8374946 - Attachment is obsolete: true
Attachment #8374946 - Flags: review?(wjohnston)
Comment on attachment 8374964 [details] [diff] [review]
messed up the previous patch...

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

Great!
Attachment #8374964 - Flags: review?(wjohnston) → review+
Attachment #8374841 - Flags: review- → review+
So, is there anything else I need to do with this bug or is it closed?
Yeah, so we'll need one final patch that's ready for checkin (sorry forgot about that). The instructions here should help:

https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

But to summarize, you'll need to fold these two patches together. If you're using mercurial queues you can just apply the first patch and run:
hg qfold second-patch-name

You also need to make sure it has your name, and a commit message. If your .hgrc is setup, the name should set itself up (or you can run 'hg qref -U'. To add a commit message I use:

hg qref -m Bug 968908 - PromptInputs should return an Object not a String. r=wesj"

If you're not using mercurial or mercurial queues you may have to do something different. Your final patch should have a header like the one here: https://hg.mozilla.org/mozilla-central/raw-rev/a62bde1d6efe Then you can upload that patch here again, mark it r+, and add [checkin-needed] to the whiteboard at the top of the bug. Someone will come by and check it in and set the right bug flags for you.

Hope that helps :)
Attached patch final_patchSplinter Review
Attachment #8374841 - Attachment is obsolete: true
Attachment #8374964 - Attachment is obsolete: true
Attachment #8375401 - Flags: review+
Attachment #8375401 - Flags: checkin+
Comment on attachment 8375401 [details] [diff] [review]
final_patch

Please don't set flags if you don't understand what they're used for.
Attachment #8375401 - Flags: checkin+
https://hg.mozilla.org/integration/fx-team/rev/27a790bd9a90
Keywords: checkin-needed
Whiteboard: [lang=js][mentor=wesj] → [lang=js][mentor=wesj][fixed-in-fx-team]
Backed out for Android bustage.
https://hg.mozilla.org/integration/fx-team/rev/0fa443dcea16

https://tbpl.mozilla.org/php/getParsedLog.php?id=34631870&tree=Fx-Team
Whiteboard: [lang=js][mentor=wesj][fixed-in-fx-team] → [lang=js][mentor=wesj]
Sorry about that...
This was just a capitalization error. Fixed and relanded for you:
https://hg.mozilla.org/integration/fx-team/rev/15b7383d5712
https://hg.mozilla.org/mozilla-central/rev/15b7383d5712
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Depends on: 1016348
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: