Closed
Bug 1196387
Opened 9 years ago
Closed 9 years ago
Remove unneeded "reading time" code and layouts
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox43 fixed)
RESOLVED
FIXED
Firefox 43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: sebastian, Assigned: inouju, Mentored)
Details
(Whiteboard: [lang=java][lang=xml][good first bug])
Attachments
(1 file, 2 obsolete files)
8.13 KB,
patch
|
sebastian
:
review+
|
Details | Diff | Splinter Review |
Our reading list had a feature to calculate and display the estimated reading time. This feature had some issues (UX and localization) so we disabled it in bug 1110461. However up to now we still keep the code and layout around. Let's remove the unused code and the layout. It's in Mercurial so we can always get it back. To start, set up a build environment - you can see the instructions here: https://wiki.mozilla.org/Mobile/Fennec/Android 1. To fix this bug, you need to remove the unused parts in these files: https://dxr.mozilla.org/mozilla-central/rev/d5cf4e7900df6b2351bf3677b49fb70bedf68b99/mobile/android/base/resources/layout/reading_list_row_view.xml#32-38 https://dxr.mozilla.org/mozilla-central/rev/d5cf4e7900df6b2351bf3677b49fb70bedf68b99/mobile/android/base/home/ReadingListRow.java?offset=0#68-90 2. For bonus points, after removing the TextView, you can simplify the structure of reading_list_row_view.xml. 3. For even more bonus points, after cleaning everything up: The Adapter in ReadingListPanel [1] inflates reading_list_item_row.xml for every row . This layout file just contains a ReadingListRow (It's actually a LinearLayout) which again inflates reading_list_row_view.xml in itself. This two step approach could be simplified by moving everything into reading_list_item_row (inside of ReadingListRow) and then getting rid of reading_list_row_view.xml. The findViewById calls in the constructor of ReadlingListRow would need to be moved to onFinishInflate() because we need to wait for the child views to be created. [1]: https://dxr.mozilla.org/mozilla-central/rev/d5cf4e7900df6b2351bf3677b49fb70bedf68b99/mobile/android/base/home/ReadingListPanel.java#202 If you need any help, you can reply to this bug, or feel free to message me on IRC - my nick is "sebastian" and you can find me and a bunch of other helpful people in #mobile. If you need IRC setup instructions, see https://wiki.mozilla.org/IRC
Assignee | ||
Comment 1•9 years ago
|
||
How's this fix? I went for 3 and removed reading_list_row_view.xml. Side question: should unused imports and variables be removed? I removed them in the patch, but they were left in before so I wasn't quite sure on what to do.
Attachment #8653016 -
Flags: review?(s.kaspari)
Reporter | ||
Updated•9 years ago
|
Assignee: nobody → inouju
Reporter | ||
Comment 2•9 years ago
|
||
Comment on attachment 8653016 [details] [diff] [review] bug-1196387-fix Review of attachment 8653016 [details] [diff] [review]: ----------------------------------------------------------------- Great patch! Just remove the "resources" (see below) and this is ready to land! Thank you! :) ::: mobile/android/base/home/ReadingListRow.java @@ +32,5 @@ > > public ReadingListRow(Context context, AttributeSet attrs) { > super(context, attrs); > > + resources = context.getResources(); It seems like we assign resources but never use it. Remove it completely. :)
Attachment #8653016 -
Flags: review?(s.kaspari) → review+
Reporter | ||
Comment 3•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=013759dcf95a
Assignee | ||
Comment 4•9 years ago
|
||
I removed the "resources". Also, does that mean that my change broke the build?
Attachment #8653016 -
Attachment is obsolete: true
Flags: needinfo?(s.kaspari)
Reporter | ||
Comment 5•9 years ago
|
||
Comment on attachment 8653878 [details] [diff] [review] bug-1196387-fix Review of attachment 8653878 [details] [diff] [review]: ----------------------------------------------------------------- > I removed the "resources". Also, does that mean that my change broke the > build? The errors seems to be unrelated to your changes and to be triggered by some mercurial issue. I re-triggered the jobs on try. I ran a build with the patches locally and don't really expect errors here. :) ::: mobile/android/base/home/ReadingListRow.java @@ +18,5 @@ > import android.widget.LinearLayout; > import android.widget.TextView; > > public class ReadingListRow extends LinearLayout { > + NIT: There are some trailing whitespaces in this line.
Attachment #8653878 -
Flags: review+
Assignee | ||
Comment 6•9 years ago
|
||
Sorry! Must've overlooked that and an unused import. Should be fixed now.
Attachment #8653878 -
Attachment is obsolete: true
Flags: needinfo?(s.kaspari)
Updated•9 years ago
|
Attachment #8653952 -
Flags: review?(s.kaspari)
Reporter | ||
Comment 7•9 years ago
|
||
Comment on attachment 8653952 [details] [diff] [review] bug-1196387-fix Review of attachment 8653952 [details] [diff] [review]: ----------------------------------------------------------------- Great patch. Thank you!
Attachment #8653952 -
Flags: review?(s.kaspari) → review+
Reporter | ||
Comment 8•9 years ago
|
||
Sorry for the delay. Make sure to add the "review" flag so that the reviewer will be notified about outstanding reviews. :) I pushed the patch to try again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cabb4b1946a3 If everything is going to be green then feel free to add the "checkin-needed" keyword to get this patch landed.
Assignee | ||
Updated•9 years ago
|
Attachment #8653952 -
Flags: checkin?(inouju)
Assignee | ||
Updated•9 years ago
|
Attachment #8653952 -
Flags: checkin?(inouju) → checkin?(s.kaspari)
Reporter | ||
Updated•9 years ago
|
Attachment #8653952 -
Flags: checkin?(s.kaspari)
Reporter | ||
Comment 9•9 years ago
|
||
Sorry for the confusion I caused. What I meant is adding the "checkin-needed" keyword to the bug, not on the patch itself. :) https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_checked_into_the_tree
Keywords: checkin-needed
Comment 10•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/7be913efcdfb
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7be913efcdfb
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Updated•3 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
•