Closed Bug 1057637 Opened 5 years ago Closed 5 years ago

Extract a re-usable ExpandableListAdapter out of RemoteTabsList

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 34

People

(Reporter: nalexander, Assigned: nalexander)

References

Details

Attachments

(1 file, 2 obsolete files)

At the moment, RemoteTabsList uses a SimpleExpandableListAdapter.  It's not particularly re-usable.  This adapter makes it difficult to implement Bug 1025128, which proposes to radically change the tabs/clients DB schemas.  As an alternative to Bug 1025128, I propose to extract a re-usable adapter out of RemoteTabsList and use it both in the RemoteTabsList and eventually in the new Remote Tabs home panel.
This patch does a few other small things as well:

* It exposes the client device type.  This will be used later, as part of
  the visual refresh.
* Aligns the field names in remote_tabs_child with the names used by
  TwoLinePageRow.  This will be used later, when we finally use said
  standard view class.
Attachment #8477759 - Flags: review?(michael.l.comella)
This patch does a few other small things as well:

* It exposes the client device type.  This will be used later, as part of
  the visual refresh.
* Aligns the field names in remote_tabs_child with the names used by
  TwoLinePageRow.  This will be used later, when we finally use said
  standard view class.

This patch is rebased on top of ckitching's recent UIAsyncTask
refactoring; and it clarifies what getClientsFromCursor does to the
input cursor's position, as well as restoring the position after
enumeration.
Attachment #8477759 - Attachment is obsolete: true
Attachment #8477759 - Flags: review?(michael.l.comella)
Attachment #8478378 - Flags: review?(michael.l.comella)
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Comment on attachment 8478378 [details] [diff] [review]
Extract a re-usable ExpandableListAdapter out of RemoteTabsList. r=mcomella

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

WIP review: reviewed everything but TabsAccessor so far.

::: mobile/android/base/RemoteTabsExpandableListAdapter.java
@@ +42,5 @@
> +     * @param childLayout
> +     * @param clients
> +     *            initial list of clients; can be null.
> +     */
> +    public RemoteTabsExpandableListAdapter(int groupLayout, int childLayout, List<RemoteClient> clients) {

nit: groupLayoutID - here and for the variable name. Same for childLayout

@@ +139,5 @@
> +        final RemoteClient client = clients.get(groupPosition);
> +        final RemoteTab tab = client.tabs.get(childPosition);
> +
> +        final TextView titleView = (TextView) view.findViewById(R.id.title);
> +        titleView.setText(TextUtils.isEmpty(tab.title) ? tab.url : tab.title);

We'll show the url twice if tab.title is empty - is this desirable? Should we change the visibility of titleView instead?

::: mobile/android/base/TabsAccessor.java
@@ +49,4 @@
>      private static final String LOCAL_TABS_SELECTION = BrowserContract.Tabs.CLIENT_GUID + " IS NULL";
> +    private static final String REMOTE_TABS_SELECTION = BrowserContract.Tabs.CLIENT_GUID + " IS NOT NULL";
> +
> +    private static final String LOCAL_CLIENT_SELECTION = BrowserContract.Clients.GUID + " IS NULL";

nit: Any reason for the churn on this line?

@@ +81,5 @@
> +        public int hashCode() {
> +            final int prime = 31;
> +            int result = 1;
> +            result = prime * result + ((title == null) ? 0 : title.hashCode());
> +            result = prime * result + ((url == null) ? 0 : url.hashCode());

Where does this algorithm come from? I don't know enough about mathematics to ensure this is correct.

@@ +86,5 @@
> +            return result;
> +        }
> +
> +        @Override
> +        public boolean equals(Object obj) {

While I suppose it is good to implement these methods, I don't see this used anywhere. Is this for future use?

@@ +97,5 @@
> +            RemoteTab other = (RemoteTab) obj;
> +            if (title == null) {
> +                if (other.title != null)
> +                    return false;
> +            } else if (!title.equals(other.title))

nit: While I think not bracing the other ifs is excusable, I'd brace this `else if` to keep consistency with the associated `if`.

@@ +188,1 @@
>                  

nit: Since you were taking care of other excess whitespace, you missed a spot. :P

::: mobile/android/base/tabs/RemoteTabsList.java
@@ +29,1 @@
>                       implements ExpandableListView.OnGroupClickListener,

nit: indentation.
Comment on attachment 8478378 [details] [diff] [review]
Extract a re-usable ExpandableListAdapter out of RemoteTabsList. r=mcomella

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

Please reflag me after answering the questions.

::: mobile/android/base/RemoteTabsExpandableListAdapter.java
@@ +51,5 @@
> +            this.clients.addAll(clients);
> +        }
> +    }
> +
> +    public void replaceClients(Collection<RemoteClient> clients) {

This collection may have duplicate RemoteClients - should we be using a data structure to ensure uniqueness?

::: mobile/android/base/TabsAccessor.java
@@ +120,5 @@
> +     *            to extract records from.
> +     * @return list of clients, each containing list of tabs.
> +     */
> +    public static List<RemoteClient> getClientsFromCursor(final Cursor cursor) {
> +        final ArrayList<RemoteClient> clients = new ArrayList<TabsAccessor.RemoteClient>();

nit: TabsAccessor shouldn't be necessary here.

@@ +126,5 @@
> +        final int originalPosition = cursor.getPosition();
> +        try {
> +            // A walking partition, chunking by client GUID.
> +            RemoteClient lastClient = null;
> +            while (cursor.moveToNext()) {

This will start reading at `originalPosition + 1` - is this intended? It contradicts, "The position of the cursor is not moved before reading."

@@ +128,5 @@
> +            // A walking partition, chunking by client GUID.
> +            RemoteClient lastClient = null;
> +            while (cursor.moveToNext()) {
> +                final String clientGuid = cursor.getString(TABS_COLUMN.GUID.ordinal());
> +                if (lastClient == null || !TextUtils.equals(lastClient.guid, clientGuid)) {

Is the data always going to be sorted by `lastClient`? Should we be using a map to construct the List instead?

@@ +129,5 @@
> +            RemoteClient lastClient = null;
> +            while (cursor.moveToNext()) {
> +                final String clientGuid = cursor.getString(TABS_COLUMN.GUID.ordinal());
> +                if (lastClient == null || !TextUtils.equals(lastClient.guid, clientGuid)) {
> +                    final String clientName = cursor.getString(TABS_COLUMN.NAME.ordinal());

Are the column numbers guaranteed to remain the same through invocations? I think it's also more robust to query for the column index [1].

[1]: https://developer.android.com/reference/android/database/Cursor.html#getColumnIndex%28java.lang.String%29
Attachment #8478378 - Flags: review?(michael.l.comella) → feedback+
(In reply to Michael Comella (:mcomella) from comment #4)
> ::: mobile/android/base/RemoteTabsExpandableListAdapter.java
> @@ +51,5 @@
> > +            this.clients.addAll(clients);
> > +        }
> > +    }
> > +
> > +    public void replaceClients(Collection<RemoteClient> clients) {
> 
> This collection may have duplicate RemoteClients - should we be using a data
> structure to ensure uniqueness?

You're right that this could be the case, but I don't think we really
want the data structure to ensure uniqueness.  The uniqueness should be
per-GUID, not per RemoteClient structure.  I think commenting on this is
best.

In practice, this doesn't happen due to badly surfaced assumptions and
some luck in the code.  The cursor sort order is defined at [1], and it
has the effect of grouping by client GUID.  I'll make this explicit in a
fresh version of the patch.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/TabsProvider.java#40

> @@ +126,5 @@
> > +        final int originalPosition = cursor.getPosition();
> > +        try {
> > +            // A walking partition, chunking by client GUID.
> > +            RemoteClient lastClient = null;
> > +            while (cursor.moveToNext()) {
> 
> This will start reading at `originalPosition + 1` - is this intended? It
> contradicts, "The position of the cursor is not moved before reading."

You're absolutely right.  I think I didn't see any problems because I
always use a fresh cursor, which is *before* the first element, so the
moveToNext actually gets us to the first element.  I think I'll just
make this go to the beginning of the cursor, always, and say as much in
the comment.

> @@ +128,5 @@
> > +            // A walking partition, chunking by client GUID.
> > +            RemoteClient lastClient = null;
> > +            while (cursor.moveToNext()) {
> > +                final String clientGuid = cursor.getString(TABS_COLUMN.GUID.ordinal());
> > +                if (lastClient == null || !TextUtils.equals(lastClient.guid, clientGuid)) {
> 
> Is the data always going to be sorted by `lastClient`? Should we be using a
> map to construct the List instead?

In practice it is, but I'll ensure this by sorting the data and making
it explicit.

> @@ +129,5 @@
> > +            RemoteClient lastClient = null;
> > +            while (cursor.moveToNext()) {
> > +                final String clientGuid = cursor.getString(TABS_COLUMN.GUID.ordinal());
> > +                if (lastClient == null || !TextUtils.equals(lastClient.guid, clientGuid)) {
> > +                    final String clientName = cursor.getString(TABS_COLUMN.NAME.ordinal());
> 
> Are the column numbers guaranteed to remain the same through invocations? I
> think it's also more robust to query for the column index [1].
> 
> [1]:
> https://developer.android.com/reference/android/database/Cursor.
> html#getColumnIndex%28java.lang.String%29

I actually think they are, but I agree that this is ... non-standard.
Weird looking.  I didn't change it to reduce churn, but I'll do so now.

Thanks for an insightful review!  Fresh patch coming shortly.
> Comment on attachment 8478378 [details] [diff] [review]
> Extract a re-usable ExpandableListAdapter out of RemoteTabsList. r=mcomella
> 
> Review of attachment 8478378 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> WIP review: reviewed everything but TabsAccessor so far.
> 
> ::: mobile/android/base/RemoteTabsExpandableListAdapter.java
> @@ +42,5 @@
> > +     * @param childLayout
> > +     * @param clients
> > +     *            initial list of clients; can be null.
> > +     */
> > +    public RemoteTabsExpandableListAdapter(int groupLayout, int childLayout, List<RemoteClient> clients) {
> 
> nit: groupLayoutID - here and for the variable name. Same for childLayout

Done.

> @@ +139,5 @@
> > +        final RemoteClient client = clients.get(groupPosition);
> > +        final RemoteTab tab = client.tabs.get(childPosition);
> > +
> > +        final TextView titleView = (TextView) view.findViewById(R.id.title);
> > +        titleView.setText(TextUtils.isEmpty(tab.title) ? tab.url : tab.title);
> 
> We'll show the url twice if tab.title is empty - is this desirable? Should
> we change the visibility of titleView instead?

That's true.  This is what we did in the old code; it's merely window
dressing around a malformed tab, which should never exist.  I'm going to
keep this as is.

> ::: mobile/android/base/TabsAccessor.java
> @@ +49,4 @@
> >      private static final String LOCAL_TABS_SELECTION = BrowserContract.Tabs.CLIENT_GUID + " IS NULL";
> > +    private static final String REMOTE_TABS_SELECTION = BrowserContract.Tabs.CLIENT_GUID + " IS NOT NULL";
> > +
> > +    private static final String LOCAL_CLIENT_SELECTION = BrowserContract.Clients.GUID + " IS NULL";
> 
> nit: Any reason for the churn on this line?

Nah, was just trying to keep the tabs and clients stuff together.

> @@ +81,5 @@
> > +        public int hashCode() {
> > +            final int prime = 31;
> > +            int result = 1;
> > +            result = prime * result + ((title == null) ? 0 : title.hashCode());
> > +            result = prime * result + ((url == null) ? 0 : url.hashCode());
> 
> Where does this algorithm come from? I don't know enough about mathematics
> to ensure this is correct.

This is actually generated code (from Eclipse); it's just doing:

p^2 * hash(field-2) + p * hash(field-1) + hash(field-0).

I am using the hash for the client IDs.  I've added a comment.

> @@ +86,5 @@
> > +            return result;
> > +        }
> > +
> > +        @Override
> > +        public boolean equals(Object obj) {
> 
> While I suppose it is good to implement these methods, I don't see this used
> anywhere. Is this for future use?

Since I implement hash, I need to implement equality.  Again, this is
generated code.

> @@ +97,5 @@
> > +            RemoteTab other = (RemoteTab) obj;
> > +            if (title == null) {
> > +                if (other.title != null)
> > +                    return false;
> > +            } else if (!title.equals(other.title))
> 
> nit: While I think not bracing the other ifs is excusable, I'd brace this
> `else if` to keep consistency with the associated `if`.

I've bracketed everything.

> @@ +188,1 @@
> >                  
> 
> nit: Since you were taking care of other excess whitespace, you missed a
> spot. :P
> 
> ::: mobile/android/base/tabs/RemoteTabsList.java
> @@ +29,1 @@
> >                       implements ExpandableListView.OnGroupClickListener,
> 
> nit: indentation.

Fixed whitespace and this indentation.

(In reply to Nick Alexander :nalexander from comment #5)
> (In reply to Michael Comella (:mcomella) from comment #4)
> > ::: mobile/android/base/RemoteTabsExpandableListAdapter.java
> > @@ +51,5 @@
> > > +            this.clients.addAll(clients);
> > > +        }
> > > +    }
> > > +
> > > +    public void replaceClients(Collection<RemoteClient> clients) {
> > 
> > This collection may have duplicate RemoteClients - should we be using a data
> > structure to ensure uniqueness?
> 
> You're right that this could be the case, but I don't think we really
> want the data structure to ensure uniqueness.  The uniqueness should be
> per-GUID, not per RemoteClient structure.  I think commenting on this is
> best.
> 
> In practice, this doesn't happen due to badly surfaced assumptions and
> some luck in the code.  The cursor sort order is defined at [1], and it
> has the effect of grouping by client GUID.  I'll make this explicit in a
> fresh version of the patch.

I've made this a List<>; and I've made the sorting (by client GUID)
explicit.

> [1]
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/
> TabsProvider.java#40
> 
> > @@ +126,5 @@
> > > +        final int originalPosition = cursor.getPosition();
> > > +        try {
> > > +            // A walking partition, chunking by client GUID.
> > > +            RemoteClient lastClient = null;
> > > +            while (cursor.moveToNext()) {
> > 
> > This will start reading at `originalPosition + 1` - is this intended? It
> > contradicts, "The position of the cursor is not moved before reading."
> 
> You're absolutely right.  I think I didn't see any problems because I
> always use a fresh cursor, which is *before* the first element, so the
> moveToNext actually gets us to the first element.  I think I'll just
> make this go to the beginning of the cursor, always, and say as much in
> the comment.

Done.

> > @@ +128,5 @@
> > > +            // A walking partition, chunking by client GUID.
> > > +            RemoteClient lastClient = null;
> > > +            while (cursor.moveToNext()) {
> > > +                final String clientGuid = cursor.getString(TABS_COLUMN.GUID.ordinal());
> > > +                if (lastClient == null || !TextUtils.equals(lastClient.guid, clientGuid)) {
> > 
> > Is the data always going to be sorted by `lastClient`? Should we be using a
> > map to construct the List instead?
> 
> In practice it is, but I'll ensure this by sorting the data and making
> it explicit.

And I added a comment.

> > @@ +129,5 @@
> > > +            RemoteClient lastClient = null;
> > > +            while (cursor.moveToNext()) {
> > > +                final String clientGuid = cursor.getString(TABS_COLUMN.GUID.ordinal());
> > > +                if (lastClient == null || !TextUtils.equals(lastClient.guid, clientGuid)) {
> > > +                    final String clientName = cursor.getString(TABS_COLUMN.NAME.ordinal());
> > 
> > Are the column numbers guaranteed to remain the same through invocations? I
> > think it's also more robust to query for the column index [1].
> > 
> > [1]:
> > https://developer.android.com/reference/android/database/Cursor.
> > html#getColumnIndex%28java.lang.String%29
> 
> I actually think they are, but I agree that this is ... non-standard.
> Weird looking.  I didn't change it to reduce churn, but I'll do so now.

I removed this legacy structure: the column indices are looked up before
the table walk.

> Thanks for an insightful review!  Fresh patch coming shortly.

I think this is ready for your attention.
Attachment #8478378 - Attachment is obsolete: true
Attachment #8479478 - Flags: review?(michael.l.comella)
mcomella: I expect splinter's interdiff will work, since there was no rebasing between the cycle; but I have the incremental commit in my reflog if you want it.
Comment on attachment 8479478 [details] [diff] [review]
(In reply to Michael Comella (:mcomella) from comment #3)

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

lgtm.

::: mobile/android/base/TabsAccessor.java
@@ +160,5 @@
> +            // A walking partition, chunking by client GUID. We assume the
> +            // cursor records are already grouped by client GUID; see the query
> +            // sort order.
> +            RemoteClient lastClient = null;
> +            while (!cursor.isAfterLast()) {

nit: Why not just use `cursor.moveToNext()` here and save the moveToNext op below?
Attachment #8479478 - Flags: review?(michael.l.comella) → review+
(In reply to Michael Comella (:mcomella) from comment #8)
> Comment on attachment 8479478 [details] [diff] [review]
> (In reply to Michael Comella (:mcomella) from comment #3)
> 
> Review of attachment 8479478 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> lgtm.
> 
> ::: mobile/android/base/TabsAccessor.java
> @@ +160,5 @@
> > +            // A walking partition, chunking by client GUID. We assume the
> > +            // cursor records are already grouped by client GUID; see the query
> > +            // sort order.
> > +            RemoteClient lastClient = null;
> > +            while (!cursor.isAfterLast()) {
> 
> nit: Why not just use `cursor.moveToNext()` here and save the moveToNext op
> below?

As written, with |moveToFirst|, this will skip the first record, right?  I looked for |moveToBeforeFirst|, but it's really |setPosition(-1)|, which seemed unidiomatic.  We have this pattern more than others in the code base, as far as I can see.
https://hg.mozilla.org/mozilla-central/rev/95b0cf5c7460
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Depends on: 1062638
You need to log in before you can comment on or make changes to this bug.