1144807 - Move enabled state variables to resources in OverlayDialogButton

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

OverlayDialogButton sets several variables based on the enabled/disabled state [1]:

99         label.setEnabled(isEnabled);
100         if (isEnabled) {
101             label.setText(enabledLabel);
102             icon.setImageDrawable(enabledIcon);
103             super.setOnClickListener(enabledOnClickListener);

There are several cleanups I would like that would greatly reduce the complexity of the OverlayDialogButton class:
  * The icon can be handled with a StateListDrawable (i.e. selector) - this can be specified as a custom attribute on the OverlayDialogButton's declaration.
  * The OnClickListener can be one for both enabled and disabled that checks the appropriate state and branches
  * The text can be updated in @Override setEnabled - one of the enabled/disabled text can be specified as a custom attribute, the other as android:text

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/overlays/ui/OverlayDialogButton.java?rev=1f9b1f477588#98

Comment 1

[pinjiz]

Hi Michael,

I'd like to work on this bug. What I did so far:

- Cleaned up the code in OverlayDialogButton a bit
--- Moved the code in init() into the constructer that takes all three arguments
--- Adapted the other constructors with less arguments to call a constructor with more arguments e.g. this(context, null) and this(context, null, 0)
--- Removed init()
--- Moved the code around and removed some methods 

- Implemented the use of a StateListDrawable
--- Added 2 new Drawable XML files defining a StateListDrawable (one for the Bookmark- and one for the Reading List Button)
--- Added a method setDrawable(StateListDrawable drawable) to set the drawable from outside
--- Adapted the code in ShareDialog

- Added a method setLabels(String enabledLabel, String disabledLabel) to set the labels
- Removed setEnabledLabelAndIcon(), setDisabledLabelAndIcon() and updateViews()

So far it works, but I'm not finished yet.


Up to now, I have two questions left:
- The user of OverlayDialogButton can specify an OnClickListener (e.g. to call addToReadingList() or addBookmark() in ShareDialog)[1]. You want me to use only one OnClickListener for both, enabled and disabled state. Currently the disabled state fires an animation and the enabled state executes the caller defined OnClickListener. I renamed enabledOnClickListener to customOnClickListener and call it's onClick()-method from inside my new mOnClickListener. I don't think that this is the solution you thought of. Could you give me an advice on this?


- I'm not really sure how to deal with the labels. The enabled and disabled labels are retrieved from a XML file in ShareDialog.

193         final String readingListEnabledLabel = resources.getString(R.string.overlay_share_reading_list_btn_label);
194         final Drawable readingListEnabledIcon = resources.getDrawable(R.drawable.overlay_readinglist_icon);


I just forwarded them to my new setLabels() method, but I'm sure thats not what you intended.

        readingListButton.setLabel(readingListEnabledLabel, readingListDisabledLabel);


I will attach an (unfinished!) diff to this bug report, so you can see my progress.

Thank you in advance! :)


[1] https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/overlays/ui/ShareDialog.java?rev=ad46c4efdc6b#201

Comment 2

[pinjiz]

Attachment: progress.diff

Comment 3

[pinjiz]

Attachment: progress.diff

Sorry, forgot to add the newly created Drawable XML files. :)

Comment 4

[pinjiz]

Attachment: bug_1144807_rev_1.patch

Hi Michael,

this is my current progress on this bug. I would be happy if you could give me some feedback for this patch. I'm sure there are many things to improve. 

I still couldn't figure out how I can implement this:
  * The text can be updated in @Override setEnabled - one of the enabled/disabled text can be specified as a custom attribute, the other as android:text


I've seen you recently changed some lines in SendTabDeviceListArrayAdapter[1] to make use of an OverlayDialogButton.

I removed the methods setEnabledLabelAndIcon() and setDisabledLabelAndIcon() in OverlayDialogButton and added setDrawable() and setLabel() instead. The setDrawable() method takes a StateListDrawable as parameter. Now there is a conflict, because those two methods are missing.

Was my change correcht or should the setEnabledLabelAndIcon() and setDisabledLabelAndIcon() remain, and setDrawable() is only provided for additional use with a StateListDrawable?

Thanks! :)

[1] http://hg.mozilla.org/mozilla-central/diff/570d7c14c38d/mobile/android/base/overlays/ui/SendTabDeviceListArrayAdapter.java

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8583310 [details] [diff] [review]
bug_1144807_rev_1.patch

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

Looking good so far!

You can use custom XML attributes to specify the drawable/labels resources in XML and set them in the constructor. For example:
  * [1] the custom attr declaration
  * [2] it's use in xml
  * [3] its one-time access and set in code

Don't forget to add the `xmlns:gecko="http://schemas.android.com/apk/res-auto"` attribute to the root element if it doesn't already have it!

Let me know if you have questions on why this is preferred over dynamically setting the assets.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/values/attrs.xml?rev=75204e00b8cd#117
[2]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/toolbar_edit_layout.xml?rev=f85b33641ef7#27
[3]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/widget/ThemedEditText.java?rev=fb4c144263b2#54

::: mobile/android/base/overlays/ui/OverlayDialogButton.java
@@ +28,5 @@
>  public class OverlayDialogButton extends LinearLayout {
>      private static final String LOGTAG = "GeckoOverlayDialogButton";
>  
> +    // The state of the OverlayDialogButton (enabled/disabled)
> +    private boolean mEnabled;

We can get this from the super-class (View.isEnabled).

@@ +34,3 @@
>      // The views making up this button.
> +    private ImageView mButtonIcon;
> +    private TextView mButtonLabel;

nit: we use hungarian notation on files that currently used hungarian notation, but don't introduce it to new files.

Can you remove the `m` prefixes?

nit: If we're going to rename these, I think -> iconView & labelView are more intuitive. `Button` implies there is a Button inside the OverlayDialogButton to me.

@@ +41,2 @@
>  
> +    private OnClickListener mConsumerOnClickListener;

nit: I'd prefer -> enabledOnClickListener

@@ +60,5 @@
> +            public void onClick(View v) {
> +
> +                if (mEnabled) {
> +                    // Call the consumer's OnClickListener's onClick() method when
> +                    // OverlayDialogButton is enabled.

nit: I think these comments are redundant - they explain what the code is doing, but I find the code readable enough that I think it's unnecessary! :)

@@ +61,5 @@
> +
> +                if (mEnabled) {
> +                    // Call the consumer's OnClickListener's onClick() method when
> +                    // OverlayDialogButton is enabled.
> +                    mConsumerOnClickListener.onClick(v);

Don't forget to check that `mConsumerOnClickListener != null` before calling a method on it!

@@ +78,3 @@
>      }
>  
> +    public void setDrawable(StateListDrawable drawable) {

You can specify this through custom XML attrs.

@@ +90,5 @@
>       * Set the enabledness state of this view. We don't call super.setEnabled, as we want to remain
>       * clickable even in the disabled state (but with a different click listener).
>       */
>      @Override
> +    public void setEnabled(boolean isEnabled) {

This should call super.setEnabled - not sure why that wasn't called before!

::: mobile/android/base/resources/drawable/overlay_share_bookmark_button.xml
@@ +5,5 @@
> +
> +<selector xmlns:android="http://schemas.android.com/apk/res/android">
> +    <item
> +        android:drawable="@drawable/overlay_bookmark_icon"
> +        android:state_enabled="true"/>

nit: I'd prefer if the states are on top - they're the distinguishing factor between items.

You also can exclude the enabled="true" state if you put it at the bottom - it's the default state and, imo, it's preferred to have this default state.

See [1] for an example (though keep the newlines between attrs!).

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/drawable/fxaccount_linkitem_textcolor.xml

::: mobile/android/base/resources/layout/overlay_share_button.xml
@@ +11,5 @@
>          android:padding="30dp"
>          android:scaleType="center"/>
>  
>      <TextView
> +        android:id="@+id/overlay_share_button_label"

We generally try not to change lines for readability unless the readability is pretty bad or we're specifically changing those lines, or enough lines around those lines. This is to preserve commit/blame history.

Do you mind undoing these changes? Thanks for trying to improve the code though! :)

(and I actually prefer to have my ids below my styles/textAppearances; to each their own :P)

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to pinjiz from comment #4)
> Was my change correcht or should the setEnabledLabelAndIcon() and
> setDisabledLabelAndIcon() remain, and setDrawable() is only provided for
> additional use with a StateListDrawable?

You'll have to keep some form of setEnabledLabelAndIcon because it's used dynamically in SendTabDeviceListArrayAdapter. I'd actually prefer two separate methods: setDrawable() and setText().

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to pinjiz from comment #1)
> I renamed enabledOnClickListener to
> customOnClickListener and call it's onClick()-method from inside my new
> mOnClickListener. I don't think that this is the solution you thought of.

That's pretty much what I thought of - nice. :)

> I just forwarded them to my new setLabels() method, but I'm sure thats not
> what you intended.

As per my review, we can get these in XML.

Please let me know if you have any more questions - nice work so far! :D

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

By the way, don't worry about working on it now, but I also have bug 1144965 opened - perhaps you want to think about the changes that need to be made for that bug while you make the changes for this one (so some of your work doesn't end up having to be undone!).

Comment 9

[pinjiz]

Attachment: bug_1144807_rev_2.patch

Hi Michael,

thank you for your good feedback! :) 

I fixed all the suggestions you had in this patch, except one: 
> This should call super.setEnabled - not sure why that wasn't called before!

When we call super.setEnabled(false), our view is no longer clickable in disabled state. Even when we explicitly call super.setClickable(true) or setClickable(true) on all the other views involved, the button stays unclickable in disabled state.

For now, I just renamed the "isEnabled" member variable to "enabled" and used it to take care of the buttons state. It's not the nicest way, but it works.

Any idea to fix this?

> By the way, don't worry about working on it now, but I also have bug 1144965
> opened - perhaps you want to think about the changes that need to be made for
> that bug while you make the changes for this one (so some of your work
> doesn't end up having to be undone!).
Thanks for the hint, I will look into that one next.

Please let me know if you wish me to change something in this patch.

> (and I actually prefer to have my ids below my styles/textAppearances; to
> each their own :P)
I do prefer that as well, it looks a lot cleaner that way. But the Fennec coding style says different[1] :D.

[1] https://wiki.mozilla.org/Mobile/Fennec/Android#Coding_Style

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8584625 [details] [diff] [review]
bug_1144807_rev_2.patch

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

Nice! Looks good to me w/ nits.

Thanks, Jannis!

::: mobile/android/base/overlays/ui/OverlayDialogButton.java
@@ +29,5 @@
>   */
>  public class OverlayDialogButton extends LinearLayout {
>      private static final String LOGTAG = "GeckoOverlayDialogButton";
>  
> +    private boolean enabled = true;

Add a comment explaining the motivations for mirroring isEnabled.

nit: I think I prefer isEnabled - it better follows boolean naming conventions.

@@ +58,5 @@
> +
> +            @Override
> +            public void onClick(View v) {
> +
> +                if (isEnabled()) {

Use this.enabled directly here.

@@ +90,5 @@
> +        }
> +
> +        typedArray.recycle();
> +
> +        this.setEnabled(true);

nit: Don't need `this`

@@ +116,5 @@
> +        this.enabled = enabled;
> +        iconView.setEnabled(enabled);
> +        labelView.setEnabled(enabled);
> +
> +        if(enabled) {

nit: `if (`

@@ +126,4 @@
>  
> +    @Override
> +    public boolean isEnabled() {
> +        return this.enabled;

I'm afraid of the side effects this could cause down the line (e.g. maybe in the Android framework?) - I think I'd rather you use `this.enabled` directly where necessary

::: mobile/android/base/overlays/ui/ShareDialog.java
@@ +31,5 @@
>  import android.content.Intent;
>  import android.content.IntentFilter;
>  import android.content.res.Resources;
>  import android.graphics.drawable.Drawable;
> +import android.graphics.drawable.StateListDrawable;

nit: Unnecessary import.

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to pinjiz from comment #9)
> When we call super.setEnabled(false), our view is no longer clickable in
> disabled state.
> 
> ...
> 
> Any idea to fix this?

It looks like we're hosed [1] because Android returns early if disabled. I think you found a good solution! :)

[1]: http://androidxref.com/4.4.4_r1/xref/frameworks/base/core/java/android/view/View.java#8627

> I do prefer that as well, it looks a lot cleaner that way. But the Fennec
> coding style says different[1] :D.

Wow, nice find!

Unfortunately, I don't think anyone maintains the style guide so it's not always correct. If you want to put the styles above the id, feel free! Just try to follow the styles established in the file you're working in.

Comment 12

[pinjiz]

Attachment: bug_1144807_rev_3.patch

Hi Michael,

thanks again for your good feedback! :)

I renamed the "enabled" variable to "isEnabled", added a comment for it, replaced  calls to isEnabled() with this.isEnabled where it was used and removed the isEnabled() method.

> nit: `if (`
Fixed.

> nit: Unnecessary import.
Fixed.


> Just try to follow the styles established in the file you're working in.
Thanks, I will do that! :)

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8586633 [details] [diff] [review]
bug_1144807_rev_3.patch

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

Ah, the code is so clean! :D

Nice work!

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=65ba840a41db

Comment 15

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

If you'd like another bug to work on, perhaps bug 1076692? I'm currently assigned there but I can give it to you if you'd like. Note: that bug is time sensitive so if you'd rather take another bug for any reason, let me know.

Comment 16

[pinjiz]

Hi Michael,

> Ah, the code is so clean! :D
> Nice work!
thanks! :)

(In reply to Michael Comella (:mcomella) from comment #14)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=65ba840a41db

The try server build fails, because the drawables (overlay_share_bookmark_button.xml, overlay_share_reading_list_button.xml) are not included in [1].

[1] https://hg.mozilla.org/try/rev/65ba840a41db

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=363f728834cf

Comment 18

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Sorry about that, I made a mistake while rebasing - this push should do the trick.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9e59022cfd2a