1067925 - Audit layout files to use TextView compound drawables whenever possible

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect)

Description

[Lucas Rocha (:lucasr)]

From mfinkle:

> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/home_empty_panel.xml#19
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/home_empty_reading_panel.xml#17
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/home_empty_reading_panel.xml#33
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/fxaccount_account_verified.xml#19
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/fxaccount_confirm_account.xml#21
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/doorhanger.xml#13
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/site_identity.xml#12
> (could all these TextViews be compounded?)
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/overlay_share_dialog.xml#41
> (could these TextViews be compounded?)
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/notification_icon_text.xml#16
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/notification_progress.xml#17
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/notification_progress_text.xml#16
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/validation_message.xml#24
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/sync_list_item.xml#14
> (in a listview? could be a big deal)
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/preference_search_tip.xml#25
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/suggestion_item.xml#15
> (in a listview? could be a big deal)
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/icon_grid_item.xml#39
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/fxaccount_get_started.xml#23
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/fxaccount_update_credentials.xml#51
> (there were a few of these but I skipped them until I realized the the
> spacer might be handled as padding)
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/panel_article_item.xml#8
> (I wonder if this could be compounded somehow)
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/fxaccount_create_account_not_allowed.xml#37
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/panel_image_item.xml#8
> (maybe not since it's a custom image, but maybe the two TextViews?)
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/home_banner_content.xml#9
> (custom TextView, but maybe?)
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/home_remote_tabs_group.xml#16
> (custom TextView, but maybe?)
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/panel_auth_layout.xml#12
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/panel_back_item.xml#8
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/onboard_start_pane.xml#31
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/overlay_share_button.xml#7

Comment 1

[Gordon Loery]

Hello, would it be ok if I worked on this? 

I did some research on making compound drawables and just wanted to ask some questions to make sure I understand things correctly. So it sounds like in order to create a compund drawable, I can remove the body of <ImageView> and <LinearLayout>, but keep the body of <TextView>, and add a line in <TextView>, something like "android:drawableTop= [android:src line in <ImageView>]" Is that about right? Do I always want to use drawableTop, or are there factors that determine where I should put the image relative to the text?

Also, I didn't really get a sense from what I read of when to use a compound drawable vs. when not to use one. In what kinds of cases should I just leave the layout as is and not try to turn it into a compound drawable?

Comment 2

[Lucas Rocha (:lucasr)]

(In reply to Gordon Loery from comment #1)
> Hello, would it be ok if I worked on this? 
> 
> I did some research on making compound drawables and just wanted to ask some
> questions to make sure I understand things correctly. So it sounds like in
> order to create a compund drawable, I can remove the body of <ImageView> and
> <LinearLayout>, but keep the body of <TextView>, and add a line in
> <TextView>, something like "android:drawableTop= [android:src line in
> <ImageView>]" Is that about right? Do I always want to use drawableTop, or
> are there factors that determine where I should put the image relative to
> the text?

Yeah, replacing the ImageView with drawableTop/Left/Right/Bottom is what we need in the simple cases. For images that change dynamically this might involve some changes to the code dealing with these images though. You'll have to decide on a case-by-case basis. I suggest having a look at the list posted in comment #0 and find the simplest cases.

> Also, I didn't really get a sense from what I read of when to use a compound
> drawable vs. when not to use one. In what kinds of cases should I just leave
> the layout as is and not try to turn it into a compound drawable?

This is about reducing the number of views in the UI. Compound drawables are a simple way to achieve this whenever you need a image+text combo in the UI. This is not applicable in some more complex cases though e.g. the alignment between the image and text is non-trivial.

Comment 3

[Gordon Loery]

Attachment: trying-a-compound-drawable.patch

Ok, I tried a small example of using a compound drawable. Even with that though, I ran into some things I wasn't sure about. I guessed that the image was to the left of the text in this case, since they both had a gravity of top|center. Is that right/is there an easy way to tell the relative positions of the text and image?

I kept the Linear Layout in that file because I wasn't sure what to do with the View(I suppose the examples I'd encountered before were all one imageview and one textview, so I didn't really know what to do with all the extra stuff). 

I also think that the second ImageView/TextView pair in that file looks like one of those more complicated examples you mentioned, since the ImageView has some padding options that I wouldn't be able to modify with a compound drawable, so I left that in. If I left that in, would I need to leave the LinearLayout in the same spot as well?

For finding code that affects images and adding compound drawables dynamically, I'm using mozilla cross-refrence to search for code in /mobile/android/base that calls findViewById([id of the textView associated with the image]), since I figured those were the locations I'd want to insert the code. Is that the right way to go to find relevant code? Sorry for the barrage of questions (I'm still fairly new to this), and thanks for your help so far!

Comment 4

[Lucas Rocha (:lucasr)]

Comment on attachment 8514796 [details] [diff] [review]
trying-a-compound-drawable.patch

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

Thanks for the patch Gordon! Unless I'm missing something, the patch doesn't change anything?

Comment 5

[Gordon Loery]

(In reply to Lucas Rocha (:lucasr) from comment #4)
> Comment on attachment 8514796 [details] [diff] [review]
> trying-a-compound-drawable.patch
> 
> Review of attachment 8514796 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the patch Gordon! Unless I'm missing something, the patch doesn't
> change anything?

You're right, sorry about that! I think I might have undone some of the changes by accident before putting the patch together.

Unfortunately, I don't think I'm going to have much free time to work on this in the near future, so it might be better to throw it back out for someone else to work on. I'm really sorry for the inconvenience!

Comment 6

[Prateek Arora]

Quick question, when going case by case, should the changes be done for large configurations well ?

Comment 7

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