920930 - "Switch to tab" loads the page in the current tab from Reading List

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Adrian Tamas (:AdrianT)]

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

Comment 1

[Aaron Train [:aaronmt]]

Adrian, can you confirm if this is a regression?

Comment 2

[Adrian Tamas (:AdrianT)]

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

Comment 3

[Mark Capella [:capella]]

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?

Comment 4

[Mark Capella [:capella]]

s/expected/actual/

Comment 5

[Sunny [:darkowlzz]]

Attachment: Made |getTabIdForUrl| better to handle AboutReader url

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

Comment 6

[Sunny [:darkowlzz]]

Attachment: Made |getTabIdForUrl| better to handle AboutReader url

Correct patch

Comment 7

[Lucas Rocha (:lucasr)]

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?

Comment 8

[Sunny [:darkowlzz]]

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

Comment 9

[Eduard Neculaesi (:eduardn)]

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!

Comment 10

[Lucas Rocha (:lucasr)]

(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 11

[Eduard Neculaesi (:eduardn)]

Attachment: bug-920930-fix.patch

Comment 12

[Lucas Rocha (:lucasr)]

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.

Comment 13

[Lucas Rocha (:lucasr)]

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

Comment 14

[Sola Ogunsakin [:sola]]

(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 :)

Comment 15

[Sola Ogunsakin [:sola]]

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 :)

Comment 16

[Eduard Neculaesi (:eduardn)]

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?

Comment 17

[Lucas Rocha (:lucasr)]

(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 :-)

Comment 18

[Eduard Neculaesi (:eduardn)]

Attachment: Bug 920930 Fix

Updated patch

Comment 19

[Lucas Rocha (:lucasr)]

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.

Comment 20

[Eduard Neculaesi (:eduardn)]

Attachment: bug_920930.patch

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

Comment 21

[Lucas Rocha (:lucasr)]

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.

Comment 22

[Eduard Neculaesi (:eduardn)]

Attachment: bug_920930.patch

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

Comment 23

[Lucas Rocha (:lucasr)]

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?

Comment 24

[Lucas Rocha (:lucasr)]

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

Comment 25

[Lucas Rocha (:lucasr)]

Comment on attachment 8401742 [details] [diff] [review]
bug_920930.patch

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

Nice, thanks.

Comment 26

[Sola Ogunsakin [:sola]]

(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.

Comment 27

[Lucas Rocha (:lucasr)]

(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!

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/08559330ee5b

Comment 29

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/08559330ee5b

Comment 30

[u421692]

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?

Comment 31

[Aaron Train [:aaronmt]]

New bug please.