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) Open a new tab go to the Reading list page and tap on the item in the reading list - observe that the item is labeled as "Switch to tab" instead of displaying the url Expected results: The tab is switched to the original article and the page is loaded in Reader mode Actual results: The article is opened in Reader Mode in the current tab and the original article remains loaded in the first tab

Adrian, can you confirm if this is a regression?

Not a regression. The issue is reproducible on both Firefox Mobile 24 and Firefox Mobile 25 beta 2

There's a little more going on here ... when you open the areticle in step 1), you have the url in a tab "not" in reader mode ... then in step 2) you long-click to add the url to the reading list ... so good so far when in step 3) you open a (new / 2nd / current) tab and navigate to the readinglist, the item does have a "switch-to-tab" button ... *I think this is the real problem* You can have a url open in one tab "not" in readingmode, and the same article open in another tab "in" reading mode ... Observe that the expected results in the original comment is the correct behaviour given your STR, tapping an item in reading list at this point will |The article is opened in Reader Mode in the current tab and the original article remains loaded in the first tab| Yes?

s/expected/actual/

Taking this bug and submitting a patch. This patch gets rid of the problem of new tab every time a ReadingList item is opened (even if they are already open in some other tab). Now I need to remove 'Switch-to-tab' from ReadingList items when the page is opened in Normal mode (not reading mode) and make it show 'Switch-to-tab' when a page is opened in a separate tab in Reading Mode. I am stuck with it! I think figuring out whether a url is in reading list or not, here [1], and then passing AboutReader url to [2] could be one way of doing this. It would be great if someone could help me out with this :) Thanks! [1]: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/TwoLinePageRow.java?#177 [2]: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/TwoLinePageRow.java#154

Correct patch

As per discussion on IRC, what we actually want here is to avoid triggering 'switch to tab' behaviour on reading list items when a tab has the original article. 'switch to tab' should only show up in reading list items if a tab has the article in reader mode. This means you'll have to somehow change TwoLinePageRows in ReadingListPage to only consider reader mode pages in getTabIdForUrl(). Maybe create a subclass of TwoLinePageRows just for ReadingListRow that always use about:reader as URL?

Making the bug free for anyone to take and work. Will try to submit another patch.

I would like to work on this bug. Lucas, is your last comment the preferred solution? If not, could you give me some direction? Thanks!

(In reply to Eduard Neculaesi (:eduardn) from comment #9) > I would like to work on this bug. Lucas, is your last comment the preferred > solution? If not, could you give me some direction? Thanks! Hi Eduard! Yep, comment #7 is my preferred solution. Have you got your development environment up and running yet? I see that you've already fixed some bugs but here are the build instructions, just in case: https://wiki.mozilla.org/Mobile/Fennec/Android I suggest you to join us on IRC (irc.mozilla.org, #mobile channel) to get more immediate help :-)

Comment on attachment 8365652 [details] [diff] [review] bug-920930-fix.patch Review of attachment 8365652 [details] [diff] [review]: ----------------------------------------------------------------- First of all, apologies for the delay on reviewing your patch. This patch is likely to overlap with sola's work on reading list (bug 889351). It would be nice if you could coordinate your work here.

Sola, could you have a look at Eduard's patch and see how you could work together on it?

(In reply to Lucas Rocha (:lucasr) from comment #13) > Sola, could you have a look at Eduard's patch and see how you could work > together on it? sure :)

hey Eduard, looking through your patch there isn't too much of a clash. I do create a ReadingListRow in bug 889351. I guess the main difference is that version extends LinearLayout instead of TwoLinePageRow for UI purposes. I think you should go ahead with this version for now since it fixes this issue, I'll merge your changes in when this lands :)

Hi Sola, thanks for taking a look at the patch! :) Lucas, since Sola approved can we merge the last patch or should I make a new one since it's a bit old?

(In reply to Eduard Neculaesi (:eduardn) from comment #16) > Hi Sola, thanks for taking a look at the patch! :) > > Lucas, since Sola approved can we merge the last patch or should I make a > new one since it's a bit old? A new patch would be nice :-)

Updated patch

Comment on attachment 8384227 [details] [diff] [review] Bug 920930 Fix Review of attachment 8384227 [details] [diff] [review]: ----------------------------------------------------------------- Nice and clean, thanks! The only change I'd like to make is to avoid using mPageUrl directly in the subclass to a more 'blackbox-style' inheritance. So, here's what I suggest: - Keep mPageUrl as private and add a getUrl() protected method that returns it. - Use getUrl() instead of mPageUrl in ReadingListRow.updateDisplayedUrl(). ::: mobile/android/base/home/HomeListView.java @@ +6,5 @@ > package org.mozilla.gecko.home; > > import org.mozilla.gecko.R; > import org.mozilla.gecko.home.HomePager.OnUrlOpenListener; > +import org.mozilla.gecko.home.ReadingListRow; Unused, remove.

Thanks for the review! Here's the new patch with the included changes.

Comment on attachment 8386312 [details] [diff] [review] bug_920930.patch Review of attachment 8386312 [details] [diff] [review]: ----------------------------------------------------------------- Awesome, just needs this final tweak. ::: mobile/android/base/home/ReadingListRow.java @@ +25,5 @@ > + } > + > + @Override > + protected void updateDisplayedUrl() { > + String mPageUrl = getUrl(); This should be: final String pageUrl = getUrl(); nit: add empty line after it.

Hi, Sorry for the delay! I attached the updated patch.

Sola, I'm afraid Eduard's patch here conflicts with your changes in bug 889351. Could you please coordinate? Maybe you could rebase your patch on top of his?

In fact, does this bug still make sense once bug 889351 lands?

Comment on attachment 8401742 [details] [diff] [review] bug_920930.patch Review of attachment 8401742 [details] [diff] [review]: ----------------------------------------------------------------- Nice, thanks.

(In reply to Lucas Rocha (:lucasr) from comment #24) > In fact, does this bug still make sense once bug 889351 lands? There is no "switch-to-tab" icon in the new design. But I think we should land this for now.

(In reply to Sola Ogunsakin [:sola] from comment #26) > (In reply to Lucas Rocha (:lucasr) from comment #24) > > In fact, does this bug still make sense once bug 889351 lands? > > There is no "switch-to-tab" icon in the new design. But I think we should > land this for now. Ok. Eduard, I'm adding the 'checkin-needed' keyword but you can do it yourself next time, thanks!

Tested with the latest Nightly 31.0a1(2014-04-15). I can still reproduce this issue: Open an article in reader mode. Add article to reading list. From about:home, go to Reading List tab. Tap on the article to "Switch to tab". Result: Article opened in the same tab and not switch to tab where allready opened. Should I log another bug for this, or this bug should be reopened?

New bug please.