1126978 - Remove duplicated styles on TabsGridLayout

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [needinfo or I won't see it]]

Semes we duplicate from code [1] and style sheets [2].

NI Martyn: mentorable? I'm not sure if perhaps there's an issue with the style sheets working.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tabs/TabsGridLayout.java?rev=0c7496cca523#83
[2]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/values/styles.xml?rev=b1424a861cca#201

Comment 1

[Martyn Haigh (:mhaigh)]

This should be a simple case of removing the Java styles and making sure that the GridView looks the same afterwards.  

The order of precedence in styling is described here: http://growingupwithandroid.blogspot.co.il/2010/03/order-precedence-in-styling.html  so if there are visual differences, you may need to adjust the styles.xml file to match the styles removed from the Java code.

Comment 2

[Ponç Bover [:pbover]]

I'd like to be assigned to this bug. Could someone assignme please? This is my first bug. Thanks

Comment 3

[Michael Comella (:mcomella) [needinfo or I won't see it]]

Hey, Ponç!

Welcome to Bugzilla! We'll assign you to the bug once you've got a patch uploaded!

To start, set up a build environment - you can see the instructions here: https://wiki.mozilla.org/Mobile/Fennec/Android

If you need any help, you can reply to this bug, or feel free to message me on IRC - my nick is "mcomella" and you can find me in #mobile. If you need IRC setup instructions, see https://wiki.mozilla.org/IRC

Thanks and happy coding! ^_^

Comment 4

[Ponç Bover [:pbover]]

Attachment: rev1 - Avoid duplicated styles on TabsGridLayout, it uses styles.xml

Comment 5

[Michael Comella (:mcomella) [needinfo or I won't see it]]

Comment on attachment 8568099 [details] [diff] [review]
rev1 - Avoid duplicated styles on TabsGridLayout, it uses styles.xml

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

A good start.

::: mobile/android/base/tabs/TabsGridLayout.java
@@ +83,2 @@
>          setGravity(Gravity.CENTER);
>          setNumColumns(GridView.AUTO_FIT);

Can you move these to the styles.xml too?

Also, maybe setColumnWidth in styles.xml.

Comment 6

[Ponç Bover [:pbover]]

Attachment: rev2 - Remove duplicated styles on TabsGridLayout.

Comment 7

[Ponç Bover [:pbover]]

Comment on attachment 8570446 [details] [diff] [review]
rev2 - Remove duplicated styles on TabsGridLayout.


- setGravity(Gravity.CENTER);
- setNumColumns(GridView.AUTO_FIT);

- setPadding(padding, paddingTop, padding, paddingBottom);

I have tried to move all styles found in TabsGridLayout to styles.xml

Ponç Bover.

Comment 8

[Michael Comella (:mcomella) [needinfo or I won't see it]]

Comment on attachment 8570446 [details] [diff] [review]
rev2 - Remove duplicated styles on TabsGridLayout.

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

Looking pretty good - almost there!

::: mobile/android/base/resources/values/styles.xml
@@ +202,3 @@
>          <item name="android:numColumns">auto_fit</item>
> +        <item name="android:columnWidth">@dimen/new_tablet_tab_panel_column_width</item>
> +        <item name="android:horizontalSpacing">@dimen/new_tablet_tab_panel_horizontal_spacing</item>

No reason to add this to dimens if it's not used in multiple places - otherwise, it just causes more clutter in our already cluttered dimens.

Thanks for trying to clean up though! :)

@@ +205,4 @@
>          <item name="android:verticalSpacing">@dimen/new_tablet_tab_panel_grid_vspacing</item>
>          <item name="android:drawSelectorOnTop">true</item>
>          <item name="android:clipToPadding">false</item>
> +        <item name="android:padding">@dimen/new_tablet_tab_panel_grid_padding</item>

This is redundant since you're specifying multiple other dimensions.

@@ +206,5 @@
>          <item name="android:drawSelectorOnTop">true</item>
>          <item name="android:clipToPadding">false</item>
> +        <item name="android:padding">@dimen/new_tablet_tab_panel_grid_padding</item>
> +        <item name="android:paddingTop">@dimen/new_tablet_tab_panel_grid_padding_top</item>
> +        <item name="android:paddingBottom">@dimen/new_tablet_tab_panel_grid_padding_bottom</item>

Same - no need to make this a dimen.

Also, dd a comment saying paddingBottom is intentionally 2x paddingTop.

::: mobile/android/base/tabs/TabsGridLayout.java
@@ -83,5 @@
> -        setStretchMode(GridView.STRETCH_SPACING);
> -        setGravity(Gravity.CENTER);
> -        setNumColumns(GridView.AUTO_FIT);
> -
> -        // The clipToPadding setting in the styles.xml doesn't seem to be working (bug 1101784)

Were you able to figure out why this wasn't working?

Comment 9

[Ponç Bover [:pbover]]

Attachment: rev3 - Remove duplicated styles on TabsGridLayout

Comment 10

[Ponç Bover [:pbover]]

ok, based on the points you've told me I have made changes.

I want to consult you and comment you about a point:

In styles.xml file to setColumnsWidth appears:
<item name="android:columnWidth">@dimen/tabs_grid_view_column_width</item>

The value for tabs_grid_view_column_width is 200dp
<dimen name="tabs_grid_view_column_width">200dp</dimen>

In the java source code in TabsGridLayout the width is equals to dimension:
mColumnWidth = resources.getDimensionPixelSize(R.dimen.new_tablet_tab_panel_column_width);

and if we see the value of new_tablet_tab_panel_column_width, it’s equals to: 178dp, in <dimen name="new_tablet_tab_panel_column_width">178dp</dimen> 

In the patch I’ve modified the line
<item name="android:columnWidth">@dimen/tabs_grid_view_column_width</item>
TO
 <item name="android:columnWidth">@dimen/new_tablet_tab_panel_column_width</item>
to maintain coherence with the source code of TabsGridLayout.
If the change is ok, the dimension <dimen name="tabs_grid_view_column_width">200dp</dimen> that appears to not be used in other place and it's not necessary. 

Another approach is change de value of the dimension
<dimen name="tabs_grid_view_column_width">200dp</dimen>
to 
<dimen name="tabs_grid_view_column_width">178dp</dimen>
and then in file styles.xml for setColumnsWidth appears:
<item name="android:columnWidth">@dimen/tabs_grid_view_column_width</item>

I think to maintain coherence with source code one solution is to apply the first approach and change
<item name="android:columnWidth">@dimen/tabs_grid_view_column_width</item>
TO
 <item name="android:columnWidth">@dimen/new_tablet_tab_panel_column_width</item>
to maintain coherence with the source code of TabsGridLayout.
In this case, the  <dimen name="tabs_grid_view_column_width">200dp</dimen> appears that it isn’t necessary.

In the patch I haven't removed the dimension tabs_grid_view_column_width. Let me know if desirable that it will be deleted from dimens.xml

Comment 11

[Michael Comella (:mcomella) [needinfo or I won't see it]]

(In reply to Ponç Bover [:pbover] from comment #10)
> I think to maintain coherence with source code one solution is to apply the
> first approach and change
> <item name="android:columnWidth">@dimen/tabs_grid_view_column_width</item>
> TO
>  <item
> name="android:columnWidth">@dimen/new_tablet_tab_panel_column_width</item>
> to maintain coherence with the source code of TabsGridLayout.
> In this case, the  <dimen name="tabs_grid_view_column_width">200dp</dimen>
> appears that it isn’t necessary.
> 
> In the patch I haven't removed the dimension tabs_grid_view_column_width.
> Let me know if desirable that it will be deleted from dimens.xml

Works for me! You should remove the unused dimension from dimens.xml.

Thanks for the thorough explanation! :)

Comment 12

[Michael Comella (:mcomella) [needinfo or I won't see it]]

Comment on attachment 8570936 [details] [diff] [review]
rev3 - Remove duplicated styles on TabsGridLayout

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

lgtm w/ green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=292562ce7597

Comment 13

[Ponç Bover [:pbover]]

Attachment: rev4 - Remove duplicated styles on TabsGridLayout

Removed unused dimension from dimens.xml: tabs_grid_view_column_width

Comment 14

[Michael Comella (:mcomella) [needinfo or I won't see it]]

Comment on attachment 8571630 [details] [diff] [review]
rev4 - Remove duplicated styles on TabsGridLayout

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

try in comment 12 should suffice.

Comment 15

[Michael Comella (:mcomella) [needinfo or I won't see it]]

Thanks, Ponç!

May I recommend working on bug 1133157 or bug 1127578 next?

Comment 16

[Carsten Book [:Tomcat]]

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

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/2a836c4403c9