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)

All
Android
defect
Not set
normal

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)

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
Attached patch bug-1196387-fix (obsolete) — Splinter Review
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)
Assignee: nobody → inouju
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+
Attached patch bug-1196387-fix (obsolete) — Splinter Review
I removed the "resources". Also, does that mean that my change broke the build?
Attachment #8653016 - Attachment is obsolete: true
Flags: needinfo?(s.kaspari)
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+
Attached patch bug-1196387-fixSplinter Review
Sorry! Must've overlooked that and an unused import. Should be fixed now.
Attachment #8653878 - Attachment is obsolete: true
Flags: needinfo?(s.kaspari)
Attachment #8653952 - Flags: review?(s.kaspari)
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+
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.
Attachment #8653952 - Flags: checkin?(inouju)
Attachment #8653952 - Flags: checkin?(inouju) → checkin?(s.kaspari)
Attachment #8653952 - Flags: checkin?(s.kaspari)
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
https://hg.mozilla.org/mozilla-central/rev/7be913efcdfb
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.