Closed Bug 920935 Opened 6 years ago Closed 6 years ago

Reading list items opened using keywords are not opened in Reading List

Categories

(Firefox for Android :: Awesomescreen, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED WONTFIX
Firefox 28
Tracking Status
fennec + ---

People

(Reporter: AdrianT, Assigned: darkowlzz)

References

Details

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

Attachments

(1 file, 4 obsolete files)

Nightly 27.0a1 2013-09-25
Samsung Galaxy Tab 2 (Android 4.1)

Steps to reproduce:
1) Load news.google.com and load any article that supports reader mode
2) Add the article to reading list by long tapping on the reader mode button in the title bar
3) Go to the Reading List page and add a keyword to the item
4) Close all tabs and open the reader item by typing the keyowrd in the url bar and pressing enter

Expected results:
The item is opened in reading list

Actual results:
The item is opened as a normal bookmark not in Reader Mode
Whiteboard: [mentor=lucasr][lang=java]
tracking-fennec: --- → ?
Keywords: regression
Adrian, can you confirm if this is a regression?
Flags: needinfo?(adrian.tamas)
Not a regression. Reproducible on Firefox Mobile 24 and 25 beta 2
Flags: needinfo?(adrian.tamas)
tracking-fennec: ? → +
Attached patch Patch1 (obsolete) — Splinter Review
Assignee: nobody → indiasuny000
Status: NEW → ASSIGNED
Taking this bug and submitting a patch, which seems to work fine :)
Attachment #819442 - Attachment is obsolete: true
Attachment #819454 - Flags: feedback?(markcapella)
Attachment #819454 - Flags: feedback?(lucasr.at.mozilla)
Comment on attachment 819454 [details] [diff] [review]
Tested and modified suggestion list items to open in ReaderMode

This reflects what we discussed on irc :)
Attachment #819454 - Flags: feedback?(markcapella) → feedback+
Comment on attachment 819454 [details] [diff] [review]
Tested and modified suggestion list items to open in ReaderMode

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

Thanks for the patch Sunny! Unfortunately, this will cause browser search to open any URLs in reading list directly in reader mode which is not the intended design. For instance, this is why we added the 'Open in Reader Mode' item in the context menu for the reading list items in the search results.

I've never looked into how we handle bookmark keywords, but the right thing to do here is probably to check if the bookmark with keyword is in the reading list before opening it.
Attachment #819454 - Flags: feedback?(lucasr.at.mozilla) → feedback-
The previous patch was the result of misunderstanding the term `keyword` :)

This patch seems to do what is expected.
Please test it and suggest any improvement in code format, if required.

Thanks!
Attachment #819454 - Attachment is obsolete: true
Attachment #821691 - Flags: feedback?(lucasr.at.mozilla)
Comment on attachment 821691 [details] [diff] [review]
Added a condition to check for pages with keyword to be present in ReadingList

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

Looks good to me :-)

::: mobile/android/base/BrowserApp.java
@@ +1472,5 @@
>                      Tabs.getInstance().loadUrl(url, Tabs.LOADURL_USER_ENTERED);
>                      return;
>                  }
>  
> +                // If the keywordUrl is in ReadingList, convert the url to AboutReader url and load

nit: "convert the url to an about:reader url and load it"

@@ +1473,5 @@
>                      return;
>                  }
>  
> +                // If the keywordUrl is in ReadingList, convert the url to AboutReader url and load
> +                final boolean inReadingList = BrowserDB.isReadingListItem(getContentResolver(), keywordUrl);

Maybe this looks a bit cleaner?
final ContentResolver cr = getContentResolver();
final boolean inReadingList = BrowserDB.isReadingListItem(cr, keywordUrl);

@@ +1476,5 @@
> +                // If the keywordUrl is in ReadingList, convert the url to AboutReader url and load
> +                final boolean inReadingList = BrowserDB.isReadingListItem(getContentResolver(), keywordUrl);
> +                if (inReadingList) {
> +                    final String readerUrl;
> +                    readerUrl = ReaderModeUtils.getAboutReaderForUrl(keywordUrl);

nit: do the assingment in the same line than the var declaration:
final String readerUrl = ReaderModeUtils.getAboutReaderForUrl(keywordUrl);

@@ +1477,5 @@
> +                final boolean inReadingList = BrowserDB.isReadingListItem(getContentResolver(), keywordUrl);
> +                if (inReadingList) {
> +                    final String readerUrl;
> +                    readerUrl = ReaderModeUtils.getAboutReaderForUrl(keywordUrl);
> +                    Tabs.getInstance().loadUrl(readerUrl);

You should still keep the Tabs.LOADURL_USER_ENTERED flag in the loadUrl() call.
Attachment #821691 - Flags: feedback?(lucasr.at.mozilla) → feedback+
Corrected the nits!
Attachment #821691 - Attachment is obsolete: true
Attachment #823928 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 823928 [details] [diff] [review]
Added a condition to check for pages with keyword to be present in ReadingList

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

Nice, thanks!

::: mobile/android/base/BrowserApp.java
@@ +1473,5 @@
>                      Tabs.getInstance().loadUrl(url, Tabs.LOADURL_USER_ENTERED);
>                      return;
>                  }
>  
> +                // If the keywordUrl is in ReadingList, convert the url to an about:reader url and load it

nit: add a period at the end of the comment.
Attachment #823928 - Flags: review?(lucasr.at.mozilla) → review+
Period and r=lucasr added! :D
Attachment #823928 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/90d6f7ce0fc2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
This doesn't work at all for me. I add a keyword to a reading list item, query the item with the string used in the keyword field for the reading list item and my query yields 0 results.

Also, this patch has no review + on it?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I have tested it and yes, it isn't working.
Logs shows that the conversion of url to about:reader works fine, but when the url is passed to loadUrl, it doesn't load.

lucasr any idea why doesn't it loading?
(In reply to Aaron Train [:aaronmt] from comment #15)
> This doesn't work at all for me. I add a keyword to a reading list item,
> query the item with the string used in the keyword field for the reading
> list item and my query yields 0 results.
> 
> Also, this patch has no review + on it?

I gave r+ (see comment #11)
Hi Lucas,

This patch seems to work now, but Editing option is going away from Reading List as per [1].
So this bug becomes a WONTFIX.
But the patch has already been checked-in. How do we back it out?

[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=930589#c5
(In reply to Sunny [:darkowlzz] from comment #18)
> Hi Lucas,
> 
> This patch seems to work now, but Editing option is going away from Reading
> List as per [1].
> So this bug becomes a WONTFIX.
> But the patch has already been checked-in. How do we back it out?
> 
> [1]: https://bugzilla.mozilla.org/show_bug.cgi?id=930589#c5

Done: https://hg.mozilla.org/integration/fx-team/rev/ec8042ab4075

Thanks for the patch anyway ;-)
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.