964100 - Reading list is sorted unexpectedly

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Reader View, defect)

Description

[wsh]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20131215 Firefox/24.0 Iceweasel/24.2.0 (Nightly/Aurora)
Build ID: 20131215070311

Steps to reproduce:

Go to http://lwn.net/Articles/580542/ and add it to reading list (1).
Go to http://lwn.net/Articles/580599/ and add it to reading list (2).
Go to http://lwn.net/Articles/580451/ and add it to reading list (3).
View reading list.


Actual results:

Reading list shows:
(2)
(1)
(3)


Expected results:

Reading list shows:
(1)
(2)
(3)

Comment 1

[Teodora Vermesan (:TeoVermesan)]

I am not able to reproduce on LG Nexus 4 (Android 4.4.2) on Firefox for Android 29.0a1 (2013-01-27) and Firefox for Android 27 Beta 9.
What device and Firefox version are you using? Thanks!

Comment 2

[wsh]

I have Google's Nexus 7 and Firefox for Android 27.0.

I've tried it again with an empty profile and it is not reproducible as
described in my bug report.

The problem seems to be that the order is remembered even if the item is
removed from the list. So the steps to reproduce the problem are:

Add (1) to reading list
Add (2) to reading list
Add (3) to reading list
Remove (1), (2), (3) from reading list
Add (3) to reading list
Add (2) to reading list
Add (1) to reading list

Reading list is now sorted
(1)
(2)
(3)

Expected result:

Reading list is  sorted
(3)
(2)
(1)

Best regards,
-Michal

Comment 3

[Aaron Train [:aaronmt]]

Teodora, are you able to reproduce this with the revised steps?

Comment 4

[Teodora Vermesan (:TeoVermesan)]

Build: Firefox for Android 29.0a1 (2014-02-02)
1)Device: LG Nexus 4 
Steps: adding the articles to the reading list in the order (3). (2). (1). will appear as (1). (2) and (3)
2)Device: Samsung Galaxy Nexus 
Steps: adding the articles in the order (3). (2). (1) will appear as (3). (2). (1)

Comment 5

[:Margaret Leibovic]

This sound similar to bug 750219. We don't actually do anything to sort lists on about:home right now :/

Comment 6

[wsh]

(In reply to :Margaret Leibovic from comment #5)
> This sound similar to bug 750219. We don't actually do anything to sort
> lists on about:home right now :/

But it seems that you somehow remember the order of deleted items and use that order when the item is added again.

Comment 7

[Sola Ogunsakin [:sola]]

i believe this occurs when articles are re-added to the reading list. currently when a reading list item is removed, it is marked as "removed". If the same item is re-added, the "removed" flag on the existing entry is cleared. To get the "insertion" sort order we could sort by DATE_MODIFIED since this field is updated whenever an article is added or re-added/updated.

margaret, should i upload a patch for this?

Comment 8

[:Margaret Leibovic]

(In reply to Sola Ogunsakin [:sola] from comment #7)
> i believe this occurs when articles are re-added to the reading list.
> currently when a reading list item is removed, it is marked as "removed". If
> the same item is re-added, the "removed" flag on the existing entry is
> cleared. To get the "insertion" sort order we could sort by DATE_MODIFIED
> since this field is updated whenever an article is added or re-added/updated.
> 
> margaret, should i upload a patch for this?

Let's double-check with ibarlow to verify that this is the behavior we want, but DATE_MODIFIED sounds like a reasonable order to sort the reading list by.

Similarly to bug 750219, we just never added any sorting logic to any of our about:home lists, so we should see if there was some spec that we just failed to implement originally.

Comment 9

[Ian Barlow (:ibarlow)]

(In reply to :Margaret Leibovic from comment #8)

> Let's double-check with ibarlow to verify that this is the behavior we want,
> but DATE_MODIFIED sounds like a reasonable order to sort the reading list by.
 
yes, that sounds great.

Comment 10

[Alexandru Chiriac]

Hello, I would like to work on this bug, if it's ok with you. I've tried to reproduce the issue with Fennec build 31.0a1 (2014-04-07) on LG Nexus 5 and noticed that the URLs are not even added to the reading list (tried to add the http://lwn.net/Articles/580542/ for example). Is this a known issue ?

Thanks, 
Alex

Comment 11

[Lucas Rocha (:lucasr)]

(In reply to Alexandru Chiriac from comment #10)
> Hello, I would like to work on this bug, if it's ok with you. I've tried to
> reproduce the issue with Fennec build 31.0a1 (2014-04-07) on LG Nexus 5 and
> noticed that the URLs are not even added to the reading list (tried to add
> the http://lwn.net/Articles/580542/ for example). Is this a known issue ?

Do you mean the little 'book' icon doesn't show up? If that's the case, this is a separate issue.

First thing you have to do is setting up your development environment. See instructions here:

https://wiki.mozilla.org/Mobile/Fennec/Android

Comment 12

[Alexandru Chiriac]

(In reply to Lucas Rocha (:lucasr) from comment #11)
> (In reply to Alexandru Chiriac from comment #10)
> > Hello, I would like to work on this bug, if it's ok with you. I've tried to
> > reproduce the issue with Fennec build 31.0a1 (2014-04-07) on LG Nexus 5 and
> > noticed that the URLs are not even added to the reading list (tried to add
> > the http://lwn.net/Articles/580542/ for example). Is this a known issue ?
> 
> Do you mean the little 'book' icon doesn't show up? If that's the case, this
> is a separate issue.

The 'book' icon is there. The issue is that after long-pressing the 'book' icon the URL item is not added in the Reading List. It may be related with https://bugzilla.mozilla.org/show_bug.cgi?id=888854 

> 
> First thing you have to do is setting up your development environment. See
> instructions here:

The development environment is set up, I've managed to succesfully build and run Fennec. 

> 
> https://wiki.mozilla.org/Mobile/Fennec/Android

Comment 13

[Lucas Rocha (:lucasr)]

(In reply to Alexandru Chiriac from comment #12)
> The development environment is set up, I've managed to succesfully build and
> run Fennec. 

Awesome. So, maybe the first thing you could try is modifying the default sort order of reading list items to use 'DATE_MODIFIED DESC' instead of _ID. The specific line is this one:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/BrowserContract.java#405

Then check if this change fixes the issue following the steps to reproduce described in previous comments in this bug.

Comment 14

[Alexandru Chiriac]

Attachment: The reading list items are sorted descending by the 'DATE_MODIFIED' field.

Comment 15

[Alexandru Chiriac]

Following the steps to reproduce the issue described in previous comments, the problem seems to be fixed. The behaviour after the fix is:

I.
Add (1) to reading list
Add (2) to reading list
Add (3) to reading list

II.
Reading list is now sorted (3), (2), (1)

III.
Remove (1), (2), (3) from reading list

IV.
Add (2) to reading list
Add (3) to reading list
Add (1) to reading list

V.
Reading list is now sorted (1), (3), (2)

Comment 16

[Lucas Rocha (:lucasr)]

Comment on attachment 8406139 [details] [diff] [review]
The reading list items are sorted descending by the 'DATE_MODIFIED' field.

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

Awesome, please add the checkin-needed keyword so that someone pushes your patch.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/3335f21a3784

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3335f21a3784

Comment 19

[Teodora Vermesan (:TeoVermesan)]

The current behavior is the one presented in comment#15, so marking this as VERIFIED FIXED.