967376 - Pull master password dialog out of GeckoPreferences

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect, P5)

Comment 1

[Alex Deaconu]

Hi, I am new to Fennec and I would like to start working on this, as a first issue. I have successfully build the project and I am using eclipse as an IDE. 

Thanks, 
Alex.

Comment 2

[Wesley Johnston (:wesj)]

Nice! Thanks!

All of Java's master password code is in:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/preferences/GeckoPreferences.java#696

I haven't looked into it much, but I think the right way to do this will be to create a MasterPasswordDialogFragment class that extends DialogFragment from Android:

http://developer.android.com/reference/android/support/v4/app/DialogFragment.html

and then implement as much as we can there. Alternatively, you can just create a generic MasterPasswordDialog class that doesn't extend anything and we can add some show() methods to it. All of these TextWatcher and EditText classes should be something we can pull out though:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/preferences/GeckoPreferences.java#632

Comment 3

[Alex Deaconu]

I will go the first option, creating a MasterPasswordDialogFragment.

Comment 4

[Alex Deaconu]

It seems the class http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/preferences/GeckoPreferences.java#696 extends the http://developer.android.com/reference/android/preference/PreferenceActivity.html which extends the Activity class instead of the FragmentActivity.

Currently the android support library does not contain a 'PreferenceActivity' class so in order to use the current implementation of 'PreferenceActivity' we will have to use the http://developer.android.com/reference/android/app/DialogFragment.html instead of the one from the the support library in order to be able to attach the 'MasterPasswordDialog' to the FragmentManger within the 'GeckoPreferences' class

Comment 5

[Alex Deaconu]

Attachment: Created MasterPasswordDialog fragment to encapsulate password handling logic from GeckoPreferences

- created MasterPasswordDialog fragment
- moved 'EmptyTextWatcher' and 'PasswordTextWatcher' to MasterPasswordDialog
- organized GeckoPreferences imports (removed unused imports)

Comment 6

[Wesley Johnston (:wesj)]

Comment on attachment 8411641 [details] [diff] [review]
Created MasterPasswordDialog fragment to encapsulate password handling logic from GeckoPreferences

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

Seems really good overall, but I'm worried about using the Fragment on older android versions (I was hoping we could sneak the compat libraries in and have things work :( )

::: mobile/android/base/preferences/GeckoPreferences.java
@@ +28,5 @@
>  
>  import android.app.Activity;
>  import android.app.AlertDialog;
>  import android.app.Dialog;
> +import android.app.DialogFragment;

Hmm.. so if I understand right, we can't use the support fragment in here because we're not in a PrefrenceFragment? I think this will crash on older devices, so we'll probably just need to fall back to extending a generic Dialog.

@@ +471,5 @@
>          return super.onOptionsItemSelected(item);
>      }
>  
> +    private final static int DIALOG_CREATE_MASTER_PASSWORD = 0;
> +    private final static int DIALOG_REMOVE_MASTER_PASSWORD = 1;

Move these into your new class. We can make them public (or class scoped) there.

@@ +788,5 @@
> +    /**
> +     * DialogFragment used for inserting/removing the master password
> +     *
> +     */
> +    public static class MasterPasswordDialog extends DialogFragment {

Lets put this in its own file. You just have to add it in alphabetical order in moz.build and 'hg add' it to your patch.

@@ +796,5 @@
> +        private static final String TAG = "MasterPasswordDialog";
> +            
> +        private int type = -1;
> +
> +        private class PasswordTextWatcher implements TextWatcher {

A short little description of what this does would be nice. i.e. "Checks to make sure that both inputs have the same text. Disables the dialogs positive button if they don't."

@@ +799,5 @@
> +
> +        private class PasswordTextWatcher implements TextWatcher {
> +            EditText input1 = null;
> +            EditText input2 = null;
> +            AlertDialog dialog = null;

Make these final and don't set them to null here.

@@ +801,5 @@
> +            EditText input1 = null;
> +            EditText input2 = null;
> +            AlertDialog dialog = null;
> +
> +            PasswordTextWatcher(EditText aInput1, EditText aInput2, AlertDialog aDialog) {

Don't bother with the 'a' prefixes on these parameters.

@@ +826,5 @@
> +        }
> +
> +        private class EmptyTextWatcher implements TextWatcher {
> +            EditText input = null;
> +            AlertDialog dialog = null;

Same here.

@@ +836,5 @@
> +
> +            @Override
> +            public void afterTextChanged(Editable s) {
> +                if (this.dialog == null)
> +                    return;

Braces. I know you're just moving code. Its a good time to clean it up :)

@@ +862,5 @@
> +		 *            <li>{@link GeckoPreferences#DIALOG_REMOVE_MASTER_PASSWORD}
> +		 *            in order to display a dialog with 1 edit field for typing
> +		 *            the password to be removed
> +		 *            </ul>
> +		 * @return a {@link DialogFragment}

I think you've got a mixture of tabs and spaces in here (we use spaces). Its good to turn on whitespace in your editor just to make it obvious what's going on. There's also some trailing whitespace (the review tool makes that bright red :))

@@ +892,5 @@
> +            
> +            AlertDialog.Builder builder = new AlertDialog.Builder(context);
> +            LinearLayout linearLayout = new LinearLayout(context);
> +            linearLayout.setOrientation(LinearLayout.VERTICAL);
> +            AlertDialog dialog = null;

We shouldn't need to use this builder I think... Can we override onCreateView like the example here: https://developer.android.com/reference/android/app/DialogFragment.html

@@ +906,5 @@
> +    					.setView((View) linearLayout)
> +    					.setPositiveButton(R.string.button_ok,
> +    							new DialogInterface.OnClickListener() {
> +    								@Override
> +    								public void onClick(DialogInterface dialog, int which) {

Can we use PrefsHelper.setPref(name, value) here? I think this was written before it existed.

@@ +951,5 @@
> +                       .setView((View) linearLayout)
> +                       .setPositiveButton(R.string.button_ok, new DialogInterface.OnClickListener() {  
> +                            @Override
> +                            public void onClick(DialogInterface dialog, int which) {
> +                                PrefsHelper.setPref(GeckoPreferences.PREFS_MP_ENABLED, input.getText().toString());

Hmm.. but we use it here... I wonder if there was some undocumented reason it didn't work above.

@@ +976,5 @@
> +            
> +            return dialog;
> +        }
> +        
> +        private EditText getTextBox(int aHintText) {

Lets rename this createPasswordBox

Comment 7

[Alex Deaconu]

Yes unfortunatelly, at this moment at least, we can't use the support dialog fragment because it needs to be attached to the support FragmentManager (http://developer.android.com/reference/android/support/v4/app/FragmentManager.html), which can be accessed though a class extending FragmentActivity.

Currently the GeckoPrefences activity extends PrefenceActivity which utimately extends Activity and has access to the standard API's FragmentManager (http://developer.android.com/reference/android/app/FragmentManager.html).

A solution would be waiting for the support library to come with a PrefenceActivity which would extend FragmentActivity, but there is no guarrantee this will happen in the near future, so I don't think we should focus on it too much :).

As the patch implementation uses http://developer.android.com/reference/android/app/DialogFragment.html it would require to increase to project's minimum SDK to 11, because otherwise I AGREE with you that would cause backwards compatibility issues. In order to limit the impact on the overall project the generic dialog implementation would be best.  

We can make a new generic class extending Dialog and we will can still override Activit.onCreateDialog and use the deprecated the Activity.showDialog to show it. 

I was thinking besides the generic dialog, we could also extend a http://developer.android.com/reference/android/app/DialogFragment.html class and use in the .onPreferenceChange, http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/preferences/GeckoPreferences.java#584, if the API level >=11 else just use call the Activity.showDialog dialog.

Besides that we should determine out how generic the dialog should be. Is it to be used only for creating/removing the master password or should it be used through the entire project for displaying dialogs to the user. I would need this info in order to know how to handle the layout generating part.

Thanks for the feedback. Part of the code was existing code and just moved into the MasterPasswordDialog class, but now I am more confident in trying to improve it, if it is the case of course.

Comment 8

[Nick Alexander :nalexander [he/him]]

I was looking for good bugs to recommend to a new contributor, and it looks like Alex's questions above fell through the cracks.  Alex, nice to see you in another bug!  wesj, what's next for this ticket?

Comment 9

[Alex Deaconu]

After surfing in the code I stumbbled upon 
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/background/preferences/PreferenceManagerCompat.java#33 

It uses reflection in order to access framework's Activity class methods, but it is currently used the http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/background/preferences/PreferenceFragment.java

It might be tricky to use reflection but it could suite our porpose to attach the support DialogFragment to the current 
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/preferences/GeckoPreferences.java#632

This would involve though changing the access modifiers of some methods in the 'PreferenceManagerCompat', as they are currently set to 'default'.

Comment 10

[Wesley Johnston (:wesj)]

Sorry I missed this going through bug mail:

(In reply to Alex Deaconu from comment #7)
> A solution would be waiting for the support library to come with a
> PrefenceActivity which would extend FragmentActivity, but there is no
Yeah, I don't want to wait :)

> We can make a new generic class extending Dialog and we will can still
> override Activit.onCreateDialog and use the deprecated the
> Activity.showDialog to show it. 

This is what I'd like to do here.
 
> I was thinking besides the generic dialog, we could also extend a
> http://developer.android.com/reference/android/app/DialogFragment.html class
> and use in the .onPreferenceChange,
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/
> preferences/GeckoPreferences.java#584, if the API level >=11 else just use
> call the Activity.showDialog dialog.

I wouldn't mind doing something like this, but I'd almost say just file a separate bug to do it in there if you want.
 
> Besides that we should determine out how generic the dialog should be. Is it
> to be used only for creating/removing the master password or should it be
> used through the entire project for displaying dialogs to the user. I would
> need this info in order to know how to handle the layout generating part.

We have some generic code for dialogs in mobile/android/base/prompt. TBH, I would have used that here originally except that it doesn't provide form validation and doesn't work in the Preference activity. At this point all of that code has grown far beyond what I originally wrote it for. Maybe this is a good place to start exploring a v2 system (but I'd still start by writing this one really specific use case out, and we can try to generalize from there in a separate bug). Or maybe you could look at adding some better form validation there. The truth is, the Android system for these is pretty good though. People writing Java code tend to just use those. People in JS are stuck with my system (or passing messages to java that say "Open this specific dialog").
 
> Thanks for the feedback. Part of the code was existing code and just moved
> into the MasterPasswordDialog class, but now I am more confident in trying
> to improve it, if it is the case of course.

Yep! Feel free to file bugs for any problems or just potential fixes you see and we can definitely try and make this better.

Comment 11

[Firefox Bug Husbandry Bot]

Re-triaging per https://bugzilla.mozilla.org/show_bug.cgi?id=1473195

Needinfo :susheel if you think this bug should be re-triaged.

Comment 12

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.