Make ListView in about:home bookmarks a class of its own

RESOLVED FIXED in Firefox 26

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: sriram, Assigned: sriram)

Tracking

unspecified
Firefox 26
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-fig)

Attachments

(4 attachments)

Currently BookmarksPage holds everything related to the ListView. However, BookmarksPage will soon have a GridView. It's better to factor out the ListView in its own class.
Assignee

Updated

6 years ago
Blocks: 862796
This moves the ListView to an XML -- so that we can add a grid view later.
Attachment #762831 - Flags: review?(margaret.leibovic)
This moves a big chunk from BookmarksPage to BookmarksListView. (nothing scary ;) ).
Attachment #762834 - Flags: review?(margaret.leibovic)

Updated

6 years ago
Attachment #762831 - Flags: review?(margaret.leibovic) → review+
Minor documentation ;)
Attachment #762840 - Flags: review?(margaret.leibovic)
Comment on attachment 762834 [details] [diff] [review]
Part 2: ListView as its own class

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

I really like where this is going, it makes BookmarksPage much more manageable. I just have some questions about some of the OnUrlOpenListener stuff.

::: mobile/android/base/home/BookmarksListView.java
@@ +28,5 @@
> +import java.util.LinkedList;
> +
> +public class BookmarksListView extends HomeListView
> +                               implements AdapterView.OnItemClickListener{
> +    

Nit: whitespace

::: mobile/android/base/home/BookmarksPage.java
@@ +46,5 @@
>      @Override
>      public void onDetach() {
>          super.onDetach();
>  
>          mUrlOpenListener = null;

Should we also be setting ListView.setOnUrlOpenListener(null) here, and then reset it onAttach?

I'm not familiar with the lifecycle relationship between onAttach/onDetatch and onViewCreated, but it would be good to make sure we don't end up with a bad reference.

::: mobile/android/base/home/HomeListView.java
@@ +31,5 @@
>      // ContextMenuInfo associated with the currently long pressed list item.
>      private HomeContextMenuInfo mContextMenuInfo;
>  
> +    // On URL open listener
> +    private OnUrlOpenListener mUrlOpenListener;

I don't really like that we're adding OnUrlOpenListener code in even more places, since I find it confusing to follow as it is. Maybe we can file a follow-up to try to clean up how we need to store all these references?
Attachment #762834 - Flags: review?(margaret.leibovic) → review-
Comment on attachment 762834 [details] [diff] [review]
Part 2: ListView as its own class

Oops, I meant to make that an r+. But I still want to make sure we look into the things I mentioned :)
Attachment #762834 - Flags: review- → review+
Comment on attachment 762840 [details] [diff] [review]
Part 3: Minor documentation

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

::: mobile/android/base/home/BookmarksListView.java
@@ +27,5 @@
>  
>  import java.util.LinkedList;
>  
> +/**
> + * A List view of bookmarks.

Nit: why is List capitalized? I would expect this to either say ListView or list view. Also, maybe we should add a bit more to this comment about what this is doing, since right now it basically just says exactly what the class name says :)
Attachment #762840 - Flags: review?(margaret.leibovic) → review+
(In reply to :Margaret Leibovic from comment #4)
> Comment on attachment 762834 [details] [diff] [review]
> Part 2: ListView as its own class
> 
> Review of attachment 762834 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I really like where this is going, it makes BookmarksPage much more
> manageable. I just have some questions about some of the OnUrlOpenListener
> stuff.
> 
> ::: mobile/android/base/home/BookmarksListView.java
> @@ +28,5 @@
> > +import java.util.LinkedList;
> > +
> > +public class BookmarksListView extends HomeListView
> > +                               implements AdapterView.OnItemClickListener{
> > +    
> 
> Nit: whitespace
> 
> ::: mobile/android/base/home/BookmarksPage.java
> @@ +46,5 @@
> >      @Override
> >      public void onDetach() {
> >          super.onDetach();
> >  
> >          mUrlOpenListener = null;
> 
> Should we also be setting ListView.setOnUrlOpenListener(null) here, and then
> reset it onAttach?
> 
> I'm not familiar with the lifecycle relationship between onAttach/onDetatch
> and onViewCreated, but it would be good to make sure we don't end up with a
> bad reference.
> 

Oh, I can reset it in HomeListView -- as it owns this variable -- in onDetachedFromWindow(). Anyways, list view is recreated everytime, and when we detach, we are sure that we are not using the ListView again.
(In reply to Sriram Ramasubramanian [:sriram] from comment #7)
> (In reply to :Margaret Leibovic from comment #4)
> > Comment on attachment 762834 [details] [diff] [review]
> > Part 2: ListView as its own class
> > 
> > Review of attachment 762834 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I really like where this is going, it makes BookmarksPage much more
> > manageable. I just have some questions about some of the OnUrlOpenListener
> > stuff.
> > 
> > ::: mobile/android/base/home/BookmarksListView.java
> > @@ +28,5 @@
> > > +import java.util.LinkedList;
> > > +
> > > +public class BookmarksListView extends HomeListView
> > > +                               implements AdapterView.OnItemClickListener{
> > > +    
> > 
> > Nit: whitespace
> > 
> > ::: mobile/android/base/home/BookmarksPage.java
> > @@ +46,5 @@
> > >      @Override
> > >      public void onDetach() {
> > >          super.onDetach();
> > >  
> > >          mUrlOpenListener = null;
> > 
> > Should we also be setting ListView.setOnUrlOpenListener(null) here, and then
> > reset it onAttach?
> > 
> > I'm not familiar with the lifecycle relationship between onAttach/onDetatch
> > and onViewCreated, but it would be good to make sure we don't end up with a
> > bad reference.
> > 
> 
> Oh, I can reset it in HomeListView -- as it owns this variable -- in
> onDetachedFromWindow(). Anyways, list view is recreated everytime, and when
> we detach, we are sure that we are not using the ListView again.

Do we still need to hold onto mUrlOpenListener at all in BookmarksPage? I only see it being used to call ListView.setOnUrlOpenListener.
I can clean that in a separate patch. :)
This removes the URL listener. BookmarksPage is so clean (and empty :D ).
Attachment #762865 - Flags: review?(margaret.leibovic)
(In reply to :Margaret Leibovic from comment #6)
> Comment on attachment 762840 [details] [diff] [review]
> Part 3: Minor documentation
> 
> Review of attachment 762840 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/home/BookmarksListView.java
> @@ +27,5 @@
> >  
> >  import java.util.LinkedList;
> >  
> > +/**
> > + * A List view of bookmarks.
> 
> Nit: why is List capitalized? I would expect this to either say ListView or
> list view. Also, maybe we should add a bit more to this comment about what
> this is doing, since right now it basically just says exactly what the class
> name says :)

Exactly :D It's just a start. When we spin documentation for our code, we'll be more inclined to come up with a good message. I somehow didn't like a class name following a big list of imports. Readability was lacking a bit. So I added 2 lines of comments to separate it :D
Comment on attachment 762865 [details] [diff] [review]
Part 4: Url listener cleanup

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

Nice. And thanks for clarifying about the lifecycle issues.
Attachment #762865 - Flags: review?(margaret.leibovic) → review+
http://hg.mozilla.org/projects/fig/rev/5b02134ccd2d
Assignee: nobody → sriram
Whiteboard: fixed-fig
You need to log in before you can comment on or make changes to this bug.