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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mike Kaply [:mkaply]]

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.

Comment 1

[p0ff]

Could I be assigned this bug?

Comment 2

[p0ff]

(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?

Comment 3

[p0ff]

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?

Comment 4

[Mike Kaply [:mkaply]]

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.

Comment 5

[p0ff]

OK.  Thanks.  I'll start work on it.

Comment 6

[:Margaret Leibovic]

wesj can also offer some advice on how to approach this bug.

Comment 7

[Wesley Johnston (:wesj)]

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;

Comment 8

[p0ff]

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?

Comment 9

[p0ff]

(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.

Comment 10

[Wesley Johnston (:wesj)]

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.

Comment 11

[p0ff]

Attachment: Changes return types to Object

Comment 12

[Wesley Johnston (:wesj)]

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.

Comment 13

[p0ff]

Attachment: patch 2

Comment 14

[Wesley Johnston (:wesj)]

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

Comment 15

[p0ff]

Attachment: Changed ColorPickerInput and IconGridInput

Unsure whether you need me to consolidate this patch with the last one.

Comment 16

[p0ff]

Attachment: messed up the previous patch...

Comment 17

[Wesley Johnston (:wesj)]

Comment on attachment 8374964 [details] [diff] [review]
messed up the previous patch...

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

Great!

Comment 18

[p0ff]

So, is there anything else I need to do with this bug or is it closed?

Comment 19

[Wesley Johnston (:wesj)]

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 :)

Comment 20

[p0ff]

Attachment: final_patch

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8375401 [details] [diff] [review]
final_patch

Please don't set flags if you don't understand what they're used for.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/27a790bd9a90

Comment 23

[Ryan VanderMeulen [:RyanVM]]

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

Comment 24

[p0ff]

Sorry about that...

Comment 25

[Wesley Johnston (:wesj)]

This was just a capitalization error. Fixed and relanded for you:
https://hg.mozilla.org/integration/fx-team/rev/15b7383d5712

Comment 26

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/15b7383d5712