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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Sriram Ramasubramanian [:sriram]]

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.

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 1: Move the ListView to an XML

This moves the ListView to an XML -- so that we can add a grid view later.

Comment 2

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 2: ListView as its own class

This moves a big chunk from BookmarksPage to BookmarksListView. (nothing scary ;) ).

Comment 3

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 3: Minor documentation

Minor documentation ;)

Comment 4

[: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?

Comment 5

[:Margaret Leibovic]

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 :)

Comment 6

[:Margaret Leibovic]

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 :)

Comment 7

[Sriram Ramasubramanian [:sriram]]

(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.

Comment 8

[:Margaret Leibovic]

(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.

Comment 9

[Sriram Ramasubramanian [:sriram]]

I can clean that in a separate patch. :)

Comment 10

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 4: Url listener cleanup

This removes the URL listener. BookmarksPage is so clean (and empty :D ).

Comment 11

[Sriram Ramasubramanian [:sriram]]

(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 12

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/projects/fig/rev/21426026164f
http://hg.mozilla.org/projects/fig/rev/6cd11ae108b5
http://hg.mozilla.org/projects/fig/rev/202981fc6c93

Comment 13

[:Margaret Leibovic]

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.

Comment 14

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/projects/fig/rev/5b02134ccd2d

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/21426026164f
https://hg.mozilla.org/mozilla-central/rev/6cd11ae108b5
https://hg.mozilla.org/mozilla-central/rev/202981fc6c93
https://hg.mozilla.org/mozilla-central/rev/5b02134ccd2d