1144965 - Collapse OverlayDialogButton layout into a single TextView

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect)

Description

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

We currently have an ImageView and a TextView - we could probably use setCompoundDrawables:

https://developer.android.com/reference/android/widget/TextView.html#setCompoundDrawablesWithIntrinsicBounds%28int,%20int,%20int,%20int%29

Not sure how much control we have over padding without inset drawables (and thus resource spam).

NI self for mentorability.

Comment 1

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

(In reply to Michael Comella (:mcomella) from comment #0)
> Not sure how much control we have over padding without inset drawables (and
> thus resource spam).

It might be preferable to not do this if we have to spam the resource folder with inset drawables.

Comment 2

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

We can do this without inset drawables, see android:drawablePadding [1].

[1]: http://developer.android.com/reference/android/widget/TextView.html#attr_android:drawablePadding

Comment 3

[Tushar Saini (:shatur)]

Hi Michael
  I am not getting, what we want to do here. A little bit of insight will be helpful.
  
Thanks.

Comment 4

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

Hey Shatur.

We currently have an OverlayDialogButton class [1] that has a TextView side-by-side with an ImageView [2]. This is inefficient because Android provides methods in TextView that allow us to put an image into the TextView – see any methods using CompoundDrawable (e.g. [3]).

Your mission, should you choose to accept it, is to remove the ImageView and put the image directly in the TextView. There should be no visible change between the current implementation and the new implementation.

Let me know if you have any more questions.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/overlays/ui/OverlayDialogButton.java
[2]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/overlay_share_button.xml
[3]: https://developer.android.com/reference/android/widget/TextView.html#setCompoundDrawables%28android.graphics.drawable.Drawable,%20android.graphics.drawable.Drawable,%20android.graphics.drawable.Drawable,%20android.graphics.drawable.Drawable%29

Comment 5

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

To ensure shatur saw my irc messages:

11:30 <mcomella> You can access the share overlay by opening another app that can share a web page (e.g. chrome)
11:30 <mcomella> Sharing to firefox
11:30 <mcomella> The list item is called something like "Add to Fennec"
11:30 <mcomella> And the share overlay, with your buttons, should appear
---

By the way, if you ask questions in #mobile, rather than in private messages, if I'm not around another developer might see and be able to answer your questions so we don't miss each other as we did today.

Let me (or us :D) know if you have any more questions!

Comment 6

[Tushar Saini (:shatur)]

Attachment: Overlay.patch

Hi Michael,
     I am uploading my patch for review.
Thanks.

Comment 7

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

Sorry for the delay, Tushar – it was busy last week! I'll look at this now.

Comment 8

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

Comment on attachment 8722612 [details] [diff] [review]
Overlay.patch

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

Looking pretty good – just fix up these style issues and we'll be good to land!

::: mobile/android/base/java/org/mozilla/gecko/overlays/ui/OverlayDialogButton.java
@@ +32,5 @@
>  
>      // We can't use super.isEnabled(), since we want to stay clickable in disabled state.
>      private boolean isEnabled = true;
>  
> +    //private final ImageView iconView;

You can remove these lines rather than commenting them out – if we need them again, we can get them from version control.

@@ +94,5 @@
>          setEnabled(true);
>      }
>  
>      public void setDrawable(Drawable drawable) {
> +        labelView.setCompoundDrawablesWithIntrinsicBounds(drawable,null,null,null);

nit: space between arguments: `...(drawable, null, null, null);`

Comment 9

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

Attachment: screenshot post patch: unaligned icons

Upon looking closer, it looks like our icons got squished! Off the top of my head, we may need to add an inset drawable to correct the size, look into providing bounds for the compound drawable, or adding a separate icon for this menu.

Anthony, what do you think about the change in icon size? I assume we should fix this up?

Comment 10

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

Comment on attachment 8722612 [details] [diff] [review]
Overlay.patch

Specifically the reading list icon got squished.

Comment 11

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

Sorry for not getting back to you on irc – I had to step out.

The method you suggested [1] does appear to the method you want to call (according to the TextView.setCompoundDrawables javadoc). Unfortunately, I'm not sure how it works, but you can try to cause a visible difference by passing in extreme parameters (e.g. setBounds for one million or one, so if they're pixel values, it'll completely warp the return values).

Some alternative solutions are mentioned in comment 9.

[1]: https://developer.android.com/reference/android/graphics/drawable/Drawable.html#setBounds%28android.graphics.Rect%29

Comment 12

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

Now that I think about it, we're getting rid of reading list anyway and the other items don't look (too) warped – let's wait for antlam feedback before we decide if this is an issue or not.

That being said, it'd be good to know how to make this work in the event that we ever add a new icon. If I had to guess, I'd say the icon just has to be a square – maybe using an inset drawable will help.

Comment 13

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Michael Comella (:mcomella) from comment #9)
> Created attachment 8725050 [details]
> screenshot post patch
> 
> Upon looking closer, it looks like our icons got squished! Off the top of my
> head, we may need to add an inset drawable to correct the size, look into
> providing bounds for the compound drawable, or adding a separate icon for
> this menu.
> 
> Anthony, what do you think about the change in icon size? I assume we should
> fix this up?

Yes, we should fix up incorrectly sized / proportioned icons.

Comment 14

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

Anthony, we're removing the reading list icon – are the other icons skewed at all? Is this something to worry about if we remove the reading list icon?

Comment 15

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

The other icons _look_ OK, I don't think they're skewed but I can't say for sure.

Yes, we are removing that Reading List icon but I want to make sure none of the others are skewed.

Comment 16

[Tushar Saini (:shatur)]

Attachment: Overlay.patch

Hi Michael
  Really sorry for late respond. This is just a crude patch.

Comment 17

[Tushar Saini (:shatur)]

In above patch I have some questions : 

   In this patch, I have drawn inset over drawable(device_desktop and overlay_readinglist_icon for now) making it 45x45 in size.
   > drawable.setBounds(0, 0, x_param, y_param);
   > labelView.setCompoundDrawables(drawable, null, null, null);
  
 So, when I used getIntrinsicHeight & width in place of x_param & y_param, I am getting a slight(really small) difference in readinglist_icon, but when I give actual parameter like (45, 45), I am getting proper size icon but don't know if it will work for all devices? 


And I want to ask on which icons I have to draw insets. According to me we are using following icons in overlay :
 1. device_desktop
 2. device_mobile
 3. shareplane
 4. overlay_bookmark
 5. overlay_reading_list_icon
 6. overlay_readinglist_already_icon
  
 Except these, are there are any other icons??

Thanks.

Comment 18

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

Hi Tushar.

When you tried to programmatically set the bounds using intrinsicHeight/Width, how are you doing it? I think the thing that would make the biggest difference is that we're making the bounds explicitly a square (as opposed to the ibafe shape) so I'd expect:

final int maxBound = Math.max(drawable.getIntrinsicHeight(), drawable.getIntrinsicWidth());
drawable.setBounds(0, 0, maxBound, maxBound);

Or similar. Is that what you tried?

That being said, I'd expect the following icons:

 1. device_desktop
 2. device_mobile
 3. shareplane
 4. overlay_bookmark

And maybe a already bookmarked icon – but that might just be a tint on bookmark.

We recently removed the reading list icons – make sure you pull down the latest changes to remove those!

Comment 19

[Tushar Saini (:shatur)]

Attachment: Screenshot_2016-04-08-19-07-20.png

Hi Michael,
   I have found a new way to disable squishiness of icons. The icons were squished due to StateListDrawable. So, by adding the following line in xml, the squishiness of icons can be avoided 

 <selector xmlns:android="http://schemas.android.com/apk/res/android"
>		  android:constantSize="true">
    <item
        android:state_enabled="false"
        android:drawable="@drawable/overlay_readinglist_already_inset"/>
    <item
        android:drawable="@drawable/overlay_readinglist_inset"/>
 </selector>
 
   But the padding between text and icon isn't looking good (can be seen in attachment) and can't be controlled by simple tags in xml. I think inset drawable can solve this problem, but will this work for all devices (meaning devices having different dpi)???

(In reply to Michael Comella (:mcomella) from comment #1)
> (In reply to Michael Comella (:mcomella) from comment #0)
> > Not sure how much control we have over padding without inset drawables (and
> > thus resource spam).
> 
> It might be preferable to not do this if we have to spam the resource folder
> with inset drawables.
 
  I am asking in context to above comment.

Regards.

Comment 20

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

Hey Tushar.

Nice find with constantSize – I had no idea!

Did you try setting intrinsic bounds with constantSize? I think that might just work.

If not, then I'm okay with the inset drawable solution.

Comment 21

[Tushar Saini (:shatur)]

Attachment: Bug-1144965.patch

Hi Michael,
     Sorry for this long delay. I was busy with my exams, training, and all.

    I have done all the required changes, and I think icons are looking good now. Please review and provide any feedback.

Regards.

Comment 22

[Tushar Saini (:shatur)]

Attachment: Firefox.png

This is how it looks now.

Comment 23

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

Comment on attachment 8781675 [details] [diff] [review]
Bug-1144965.patch

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

Tushar, don't worry about the delay. This looks good! The one issue I see is that the padding in each menu item has changed from the previous version to the current version. Could you try to make it look like the original?

fwiw, in the previous version, the size of the image asset was 28dp wide [1], the total view was 60dp wide, and there was no scaling (see overlay_share_button.xml). I'm unclear how the additional 30dp padding would affect the image.

[1]: device_desktop.png in xhdpi (2.0x multiplier) is 56x36 pixels, which is 28x18 dp.

::: mobile/android/base/java/org/mozilla/gecko/overlays/ui/OverlayDialogButton.java
@@ +19,4 @@
>  import android.widget.ImageView;
>  import android.widget.LinearLayout;
>  import android.widget.TextView;
> +import java.lang.Math;

nit: I don't think you need to explicitly import Math.

@@ +94,4 @@
>      }
>  
>      public void setDrawable(Drawable drawable) {
> +        final int maxBound = Math.max(drawable.getIntrinsicHeight(), drawable.getIntrinsicWidth());

nit: I think we should add a note here to say that all drawables we set here have to have the same maximum dimension, or else the alignment will be thrown off.

::: mobile/android/base/resources/drawable/overlay_share_bookmark_button.xml
@@ +3,5 @@
>     - License, v. 2.0. If a copy of the MPL was not distributed with this
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
> +<selector xmlns:android="http://schemas.android.com/apk/res/android"
> +	android:constantSize="true">

nit: align this with `xmlns`

::: mobile/android/base/resources/layout/overlay_share_button.xml
@@ +4,4 @@
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
>  <merge xmlns:android="http://schemas.android.com/apk/res/android">
> +     <TextView

nit: add a note to see the note I suggest to add in OverlayDialogButton.setDrawable.

Comment 24

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

If need be, you can also ask :antlam for exact measurements.

Also:
 * double-check that a mobile device and tablet have the correct positioning
 * If you need to know how to create a test sync account in order to see all of the devices, let me know and I can help out.

Comment 25

[Tushar Saini (:shatur)]

Attachment: Bug1144965.patch

Hi Michael

  In previous patch, the alignment of device_mobile was little thrown off. In this patch, I have drawn insets over drawable, making it all square. The dimensions is now matching to overlay_bookmark. 

  Also, I tried to match the padding of menu item to previous version, and I think I have achieved it.

  I have tested this patch on my phone and tablet(having xhdpi display)(emulator).

  Please review.

Regards

Comment 26

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

Hey Tushar, I'll get to this tomorrow.

Comment 27

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

Comment on attachment 8792554 [details] [diff] [review]
Bug1144965.patch

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

The code looks good but unfortunately, the phone icon for sharing appears to be missing on my phone – Tushar, do you know what's going on?

::: mobile/android/base/java/org/mozilla/gecko/overlays/ui/OverlayDialogButton.java
@@ +94,4 @@
>      }
>  
>      public void setDrawable(Drawable drawable) {
> +        // Draw inset on drawables, to make the drawable square, so to avoid alignment issues.

nit: I think we should put the sentiment of this comment into the XML (I commented on it), rather than into the code.

::: mobile/android/base/resources/layout/overlay_share_button.xml
@@ +15,5 @@
>          android:textSize="14sp"
> +        android:textColor="@color/primary_text_selector"
> +        android:gravity="center_vertical"
> +        android:paddingLeft="15dp"
> +        android:drawablePadding="15dp"/>

nit: add a comment here to explain how the drawables are used. Also explain what the insets are accomplishing.

Comment 28

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

Attachment: Screenshot post patch: missing phone icon

Comment 29

[Tushar Saini (:shatur)]

Attachment: NewBug1144965.patch

Hi. 
  This is the latest patch.Let me know if this works.

Thanks!

Comment 30

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

Hey Tushar. I have the same issues with the latest patch. I also tried rebasing my changes on the latest mozilla-central and I have the same issue.

Have you tried pulling the latest revision of the code? I wonder if something has changed in the code since you wrote the patch that could be causing these issues.

Comment 31

[Tushar Saini (:shatur)]

Hey Michael.
   
   The problem is causing on devices having api =<19 rather than due to differences in dpi. Inset drawable is behaving differently on api 19 and on other above apis. The images are getting squished(on devices having api=19) for the same code. When drawing inset over images, the dimension of the image (height & width) doesn't change on devices with api=19 but changes on devices having api>19.

Regards.

Comment 32

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

Hi Tushar. The APIs' behavior must have changed for API 19+. I wouldn't want to introduce code that branches between Android versions because it greatly increases complexity in coding & testing.

That being said, if you can find out what specifically changed between versions (maybe find the changelog for version 19), perhaps you can find a single solution that works across all API levels.

I'm concerned that the complexity/time-investment is starting to outweigh the trade-offs but, as before, you want to keep going, you're welcome to! :)

Comment 33

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

Hi Tushar. It looks like someone has posted a patch for bug 1155860, the bug I suggested you might work on after this one. Please let :grisha know if you'd like him to find you another bug to work on.

Comment 34

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