Closed Bug 1126978 Opened 7 years ago Closed 7 years ago

Remove duplicated styles on TabsGridLayout

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(firefox39 fixed)

RESOLVED FIXED
Firefox 39
Tracking Status
firefox39 --- fixed

People

(Reporter: mcomella, Assigned: pbover, Mentored)

References

Details

(Whiteboard: [lang=java][good first bug])

Attachments

(1 file, 3 obsolete files)

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
Flags: needinfo?(mhaigh)
Flags: needinfo?(mhaigh)
Whiteboard: [lang=java][good first bug]
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.
Mentor: michael.l.comella, mhaigh
I'd like to be assigned to this bug. Could someone assignme please? This is my first bug. Thanks
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! ^_^
Assignee: nobody → pobover
Attachment #8568099 - Flags: review?(mhaigh) → review?(michael.l.comella)
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.
Attachment #8568099 - Flags: review?(michael.l.comella) → feedback+
Attachment #8570446 - Flags: review?(michael.l.comella)
Attachment #8570446 - Attachment description: rev2 - Remove duplicated styles on TabsGridLayout. → 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…
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.
Attachment #8570446 - Attachment description: 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 → rev2 - Remove duplicated styles on TabsGridLayout.
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?
Attachment #8570446 - Flags: review?(michael.l.comella) → feedback+
Attachment #8568099 - Attachment is obsolete: true
Attachment #8570446 - Attachment is obsolete: true
Attachment #8570936 - Flags: review?(michael.l.comella)
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
(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 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
Attachment #8570936 - Flags: review?(michael.l.comella) → review+
Removed unused dimension from dimens.xml: tabs_grid_view_column_width
Attachment #8571630 - Flags: review?(michael.l.comella)
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.
Attachment #8571630 - Flags: review?(michael.l.comella) → review+
Attachment #8570936 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Keywords: checkin-needed
Thanks, Ponç!

May I recommend working on bug 1133157 or bug 1127578 next?
https://hg.mozilla.org/integration/fx-team/rev/2a836c4403c9
Keywords: checkin-needed
Whiteboard: [lang=java][good first bug] → [lang=java][good first bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/2a836c4403c9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [lang=java][good first bug][fixed-in-fx-team] → [lang=java][good first bug]
Target Milestone: --- → Firefox 39
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.