Closed
Bug 1126978
Opened 10 years ago
Closed 10 years ago
Remove duplicated styles on TabsGridLayout
Categories
(Firefox for Android Graveyard :: General, defect)
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)
3.99 KB,
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
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)
Updated•10 years ago
|
Flags: needinfo?(mhaigh)
Whiteboard: [lang=java][good first bug]
Comment 1•10 years ago
|
||
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.
Reporter | ||
Updated•10 years ago
|
Mentor: michael.l.comella, mhaigh
Assignee | ||
Comment 2•10 years ago
|
||
I'd like to be assigned to this bug. Could someone assignme please? This is my first bug. Thanks
Reporter | ||
Comment 3•10 years ago
|
||
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 | ||
Comment 4•10 years ago
|
||
Attachment #8568099 -
Flags: review?(mhaigh)
Reporter | ||
Updated•10 years ago
|
Assignee: nobody → pobover
Reporter | ||
Updated•10 years ago
|
Attachment #8568099 -
Flags: review?(mhaigh) → review?(michael.l.comella)
Reporter | ||
Comment 5•10 years ago
|
||
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+
Assignee | ||
Comment 6•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8570446 -
Flags: review?(michael.l.comella)
Assignee | ||
Updated•10 years ago
|
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…
Assignee | ||
Comment 7•10 years ago
|
||
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.
Reporter | ||
Comment 8•10 years ago
|
||
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+
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8568099 -
Attachment is obsolete: true
Attachment #8570446 -
Attachment is obsolete: true
Attachment #8570936 -
Flags: review?(michael.l.comella)
Assignee | ||
Comment 10•10 years ago
|
||
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
Reporter | ||
Comment 11•10 years ago
|
||
(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! :)
Reporter | ||
Comment 12•10 years ago
|
||
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+
Assignee | ||
Comment 13•10 years ago
|
||
Removed unused dimension from dimens.xml: tabs_grid_view_column_width
Attachment #8571630 -
Flags: review?(michael.l.comella)
Reporter | ||
Comment 14•10 years ago
|
||
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+
Reporter | ||
Updated•10 years ago
|
Attachment #8570936 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Keywords: checkin-needed
Reporter | ||
Comment 15•10 years ago
|
||
Thanks, Ponç!
May I recommend working on bug 1133157 or bug 1127578 next?
Comment 16•10 years ago
|
||
Keywords: checkin-needed
Whiteboard: [lang=java][good first bug] → [lang=java][good first bug][fixed-in-fx-team]
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Whiteboard: [lang=java][good first bug][fixed-in-fx-team] → [lang=java][good first bug]
Target Milestone: --- → Firefox 39
Updated•5 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•