Closed Bug 964100 Opened 11 years ago Closed 10 years ago

Reading list is sorted unexpectedly

Categories

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

27 Branch
All
Android
defect
Not set
normal

Tracking

(fennec+)

VERIFIED FIXED
Firefox 31
Tracking Status
fennec + ---

People

(Reporter: wsh, Assigned: alexandru.chiriac)

Details

(Whiteboard: [mentor=lucasr][lang=java])

Attachments

(1 file)

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)
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!
OS: Linux → Android
Hardware: x86_64 → All
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
Teodora, are you able to reproduce this with the revised steps?
Flags: needinfo?(teodora.vermesan)
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)
Flags: needinfo?(teodora.vermesan)
Status: UNCONFIRMED → NEW
tracking-fennec: --- → ?
Ever confirmed: true
This sound similar to bug 750219. We don't actually do anything to sort lists on about:home right now :/
(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.
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?
Flags: needinfo?(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.
Flags: needinfo?(margaret.leibovic) → needinfo?(ibarlow)
tracking-fennec: ? → +
Whiteboard: [mentor=lucasr][lang=java]
(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.
Flags: needinfo?(ibarlow)
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
(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
(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
(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.
Assignee: nobody → alexandru.chiriac
Attachment #8406139 - Flags: review?(lucasr.at.mozilla)
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 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.
Attachment #8406139 - Flags: review?(lucasr.at.mozilla) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/3335f21a3784
Keywords: checkin-needed
Whiteboard: [mentor=lucasr][lang=java] → [mentor=lucasr][lang=java][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/3335f21a3784
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=lucasr][lang=java][fixed-in-fx-team] → [mentor=lucasr][lang=java]
Target Milestone: --- → Firefox 31
The current behavior is the one presented in comment#15, so marking this as VERIFIED FIXED.
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: