884851 - Use an enum instead of a boolean for mIsAutoComplete in FormAssistPopup

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

See bug 877782 comment 3. I'm volunteering kats to be a mentor for this bug :)

Comment 1

[Michael Nares]

How do I assign this to myself?

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Do you have a fennec build environment set up yet? You can follow the instructions at https://wiki.mozilla.org/Mobile/Fennec/Android to set it up. Once you have that working it should be a fairly simple change to mobile/android/base/FormAssistPopup.java to turn mIsAutoComplete into an enum instead of a boolean and update references to it. Feel free to comment here, email me, or ping me on IRC if you have questions.

Comment 3

[Michael Nares]

I have now accessed the relevant class file.  If I've just changed it where it is declared as as a boolean, so it is now private enum mIsAutoComplete = true;, and all references within the class point to this, is this sufficient?

Also, how do I put my changes into a commit that can be re-accessed publicly by other developers, as opposed to me just saving the changes on my own computer?

Comment 4

[Kartikaya Gupta (email:[e-mail])]

(In reply to Michael Nares from comment #3)
> I have now accessed the relevant class file.  If I've just changed it where
> it is declared as as a boolean, so it is now private enum mIsAutoComplete =
> true;, and all references within the class point to this, is this sufficient?

Maybe. I'm not sure because I don't think "enum mIsAutoComplete = true;" would compile, it doesn't look like valid Java code. The idea here is to create an enum type with values like AutoComplete and ValidationMessage, which are the two types of popups supported, and then use that instead of the boolean.

> Also, how do I put my changes into a commit that can be re-accessed publicly
> by other developers, as opposed to me just saving the changes on my own
> computer?

See the page at https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch for details on submitting patches. Specifically the section on "Creating a Patch" has a link to a page that describes how you can create a patch and attach it to this bug for review.

Comment 5

[Michael Nares]

Attachment: New version of this class

When I try and add a patch on Mercurial I get:

abort: no repository found in '/Users/michael' (.hg not found)!

Here is an updated version of this class.  I'm not entirely sure if I have done this correctly, and would be grateful if someone could review this.

Comment 6

[:Margaret Leibovic]

(In reply to Michael Nares from comment #5)
> Created attachment 769494 [details]
> New version of this class
> 
> When I try and add a patch on Mercurial I get:
> 
> abort: no repository found in '/Users/michael' (.hg not found)!

It sounds like you're trying to run an hg command from outside your source directory. Can you navigate to your clone of mozilla-central and try running the command from in there?

> Here is an updated version of this class.  I'm not entirely sure if I have
> done this correctly, and would be grateful if someone could review this.

So, you'll need to upload a patch, not just a new version of the code. You should be able to follow the directions here:
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

If you have trouble, you can hop on IRC and join #mobile, and someone should be able to help you.

Comment 7

[Michael Nares]

Attachment: My attempt at a patch submitted to Kats for review.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 783107 [details] [diff] [review]
My attempt at a patch submitted to Kats for review.

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

This doesn't look like it will even compile. Did you successfully compile and run this code?

Comment 9

[Michael Nares]

No, would the best way to do that be to copy and paste the code into an IDE and try and get it to compile?

Comment 10

[Kartikaya Gupta (email:[e-mail])]

No, you should use the build instructions on the wiki page linked in comment 2.

Comment 11

[Michael Nares]

I've just tried doing make -f client.mk build_and_deploy from my laptop (but not connecting my phone in, just to see if it would compile), and I couldn't see any error messages apart from right at the end:

BUILDSTATUS TIERDIR_FINISH browser
BUILDSTATUS SUBTIER_FINISH app tools
BUILDSTATUS TIER_FINISH app
make -j4 -C obj-x86_64-apple-darwin12.4.0 fast-package
make[1]: *** No rule to make target `fast-package'.  Stop.
make: *** [fast-package] Error 2

Comment 12

[Michael Nares]

One other thing I can do is to run it on an Android device, i.e. do make -f client.mk build_and_deploy with my phone plugged in, but I thought it would be useful to let you know what happened without deploying it.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

It looks like you're building a desktop version of Firefox, not the Android version, so you probably don't have a proper mozconfig set up. Also the actual error message will appear higher up in the output. If you redirect the output to a file and search for the first instance "error:" that will point you to the actual error.

That being said, if you are having trouble building the code you should jump on our IRC channel (#mobile on irc.mozilla.org) and we can help you better there. It would be better to only have actual bug-related comments on this bug.

Comment 14

[Michael Nares]

I have now compiled the code using the correct mozconfig, and couldn't trigger any compilation errors.  Are you able to either explain to me where my code is wrong, or if you manage to trigger compilation errors can you show them to me?

Comment 15

[Kartikaya Gupta (email:[e-mail])]

You can see the first compilation error that your patch triggers here:

https://tbpl.mozilla.org/php/getParsedLog.php?id=27004414&tree=Try

Comment 16

[Michael Nares]

Attachment: New patch, this should now compile.

Comment 17

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 801750 [details] [diff] [review]
New patch, this should now compile.

The patch is empty :(

Comment 18

[Michael Nares]

Attachment: Patch for feedback.

I can see from looking at the patch that the indentation is all wrong.  What would be the best way of sorting this?  Am I best off looking again at the original FormAssistPopup.java?

Comment 19

[Michael Nares]

Or is the indentation etc now ok?

Comment 20

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 826134 [details] [diff] [review]
Patch for feedback.

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

So this patch is based on one of your previous modifications to FormAssistPopup. That is, if you look at the left side of the diff, you'll see a previous version of your code rather than the code we have in mozilla-central right now. So yes, it might be better to start with a fresh copy of FormAssistPopup.java and reapply your changes to that.

Comment 21

[Kartikaya Gupta (email:[e-mail])]

Also the indentation in your patch looks ok, but there's a lot of trailing whitespace which should be removed. If you click on the "review" link for the attachment you'll see the trailing whitespace highlighted in pink.

Comment 22

[Michael Nares]

Attachment: Updated patch.

Comment 23

[Michael Nares]

Attachment: Attempt at removing whitespace

Comment 24

[Michael Nares]

Attachment: Attempt at removing whitespace

Comment 25

[Kartikaya Gupta (email:[e-mail])]

This last patch removes the whitespace, but again it's not based on the latest code in mozilla-central; it's based on your previous changes. You need to combine the patches using hg qfold or similar.

Comment 26

[Michael Nares]

Attachment: Patch based on latest src with whitespace removed.

Comment 27

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8337415 [details] [diff] [review]
Patch based on latest src with whitespace removed.

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

::: mobile/android/base/FormAssistPopup.java
@@ +212,5 @@
>              // Bail if we can't get the correct dimensions for the popup.
>              Log.e(LOGTAG, "Error getting FormAssistPopup dimensions", e);
>              return false;
>          }
> +        mPopupType = AutoComplete;

You should only be setting mPopupType to AutoComplete if isAutoComplete is true.

@@ +257,5 @@
>          int popupLeft = left < 0 ? 0 : left;
>          FloatSize viewport = aMetrics.getSize();
> +        int popupHeight;
> +
> +        switch (mPopupType) {

While you have the general idea of what needs to happen here, the actual implementation needs to change heavily. You must ensure that the same code runs before and after your change for both types of popups. With your current patch a lot of the code that would run for both types of popups ends up inside the "case ValidationMessage" and so will not run for AutoComplete popups.

Comment 28

[Michael Nares]

Attachment: Enum patch attempt.

Comment 29

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8359402 [details] [diff] [review]
Enum patch attempt.

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

This is much better. Good to start with the simple change and then build on that. This looks almost perfect, just a couple of things as I note below.

::: mobile/android/base/FormAssistPopup.java
@@ +53,5 @@
> +    private enum PopupType {
> +        AUTOCOMPLETE,
> +        VALIDATIONMESSAGE;
> +    }
> +    PopupType mPopupType;

Make this private.

@@ +211,5 @@
>              // Bail if we can't get the correct dimensions for the popup.
>              Log.e(LOGTAG, "Error getting FormAssistPopup dimensions", e);
>              return false;
>          }
> +        mPopupType = isAutoComplete;

This doesn't compile. You need to do mPopupType = (isAutoComplete ? PopupType.AUTOCOMPLETE : PopupType.VALIDATIONMESSAGE); or something similar.

@@ +267,5 @@
>                      popupLeft = (int) (viewport.width - popupWidth);
>              }
>          }
>          int popupHeight;
> +        if ((mPopupType == PopupType.AUTOCOMPLETE))

Remove one pair of parentheses here

@@ +272,5 @@
>              popupHeight = sAutoCompleteRowHeight * mAutoCompleteList.getAdapter().getCount();
>          else
>              popupHeight = sValidationMessageHeight;
>          int popupTop = top + height;
> +        if ((mPopupType == PopupType.VALIDATIONMESSAGE)) {

Ditto

@@ +302,5 @@
>                      // Shrink to available space on top.
>                      popupTop = 0;
>                      popupHeight = top;
>                  }
> +                if ((mPopupType == PopupType.VALIDATIONMESSAGE)) {

Ditto

Comment 30

[Michael Nares]

Attachment: Enum patch attempt 21.01.2014

Comment 31

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8363060 [details] [diff] [review]
Enum patch attempt 21.01.2014

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

Awesome! We just need to update the commit message to include a description of what the patch is doing. Something like "Bug 884851 - Replace the mIsAutoComplete boolean with an enum. r=kats" should be good. If you can remove the whitespace I pointed out below at the same time and upload a revised patch we can get it landed in the tree.

::: mobile/android/base/FormAssistPopup.java
@@ +53,5 @@
> +    private enum PopupType {
> +        AUTOCOMPLETE,
> +        VALIDATIONMESSAGE;
> +    }
> +    

nit: there's still some whitespace on this line.

Comment 32

[Michael Nares]

Attachment: Patch with updated commit message and whitespace removed.

Comment 33

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8363076 [details] [diff] [review]
Patch with updated commit message and whitespace removed.

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

Beautiful!

Comment 34

[Kartikaya Gupta (email:[e-mail])]

I'll land this patch shortly.

Comment 35

[Kartikaya Gupta (email:[e-mail])]

Hm, something odd with the patch format since neither hg or git would apply it. But I applied it manually and landed:

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

Comment 36

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d1c8fd742642

Comment 37

[Kartikaya Gupta (email:[e-mail])]

Nice work, Michael! Glad to see this finally get into the tree - I really admire your persistence in sticking with this bug even with all the trouble you've had. Kudos :)