Page MenuHomePhabricator

Link preview does not search for articles in cache
Closed, ResolvedPublic

Description

When the device is offline and a link link is clicked in an article the app doesn't seem to be looking in the cache for getting the content of link preview.

Steps to reproduce

  1. Go to the Generic programming article.
  2. Open the Computer Programming article by clicking the link in the main section
  3. Turn on airplane mode
  4. Go back to the Generic programming article.
  5. Click the link to Computer programming article.

Expected behaviour
Link preview shows description (the page was just loaded so it would most probably be in the cache)

Actual behaviour
Link preview shows loading circle

I took screen shots with another link and attach it here.
Screen shot of app showing Template metaprogramming article when device is offline

Screen shot showing loading circle for link preview


I was able to see the Template metaprogramming article when I clicked the Read article in link preview (so the article was in the cache)

Event Timeline

Kaartic created this task.Nov 4 2016, 10:35 AM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Kaartic updated the task description. (Show Details)Nov 4 2016, 10:50 AM

Specifically, link previews request the page/summary endpoint and articles use page/mobile-sections-lead. However, the following scenario works:

  1. Go to the Barack Obama article
  2. Tap the U.S. Senate article
  3. Dismiss the link preview
  4. Enable airplane mode
  5. Tap the U.S. Senate article

If page/summary and page/mobile-sections-lead are expected to share data this is a feature request

Kaartic added a comment.EditedJan 31 2017, 12:45 PM

However, the following scenario works:

The scenario described isn't working (at least for me) as stated. I didn't get the preview after i enabled airplane mode. Moreover, i don't see how this differs from the one stated in the task description.

@Kaartic, sorry but did you try on this patch? These changes haven't been merged yet

@Kaartic, sorry but did you try on this patch? These changes haven't been merged yet

Sorry, I missed that. I have tested as per the steps in your comment and it worked. Though, incidentally I found that the above scenario doesn't work if a link is a redirection link. For example, the following scenario doesn't work,

  1. Go to the Barack Obama article
  2. Tap the Illinois state senate link
  3. Dismiss the link preview
  4. Enable airplane mode
  5. Tap the Illinois state senate link

The link preview isn't displayed because it is a redirection link to the Illinois senate article.

Thanks for the follow-up, @Kaartic! That makes sense! If you wouldn't mind, I'd like to address redirects in a separate ticket: T156994. Thanks!

Kaartic added a comment.EditedFeb 2 2017, 2:32 PM

If you wouldn't mind, I'd like to address redirects in a separate ticket: T156994. Thanks!

I don't. Moreover, why would I in the first place. Please don't mention thanks, i just consider it as a little help that i could do :)

@Niedzielski I would like to add that the scenario you described doesn't work if the preview has images. For example the following scenario doesn't work,

  1. Go to the Barack Obama article
  2. Tap the African American link
  3. Dismiss the link preview
  4. Enable airplane mode
  5. Tap the African American link

Also it seems that the cache could store only one link preview at any point of time.

@Kaartic, geez this seems to be broken already. I can repro and I'll look into it.

@Kaartic, oops! The African American article is actually a redirect to the plural, African Americans.

@Kaartic, oops! The African American article is actually a redirect to the plural, African Americans.

@Niedzielski I'm sorry, I screwed things up. Should have noticed it correctly. But I hope that the last line of the comment is still correct,

Also it seems that the cache could store only one link preview at any point of time.

Also it seems that the cache could store only one link preview at any point of time.

I was having all kinds of issues yesterday but I just retried this on master and can't repro:

  1. Go to the banana article
  2. Tap and dismiss the first 8 link previews
  3. Enable airplane mode
  4. Repeat step 2

Would you mind giving it a shot?

Would you mind giving it a shot?

You're right. I seemed to have missed the redirect links, sorry about that.

@Kaartic, got me too! Thanks!

@Niedzielski I wanted to know something, just out of curiosity. Is the change specified in one of your previous comments the only one that's going to fix the problem, specified in the task description, for now ?

o/ @Kaartic, there were some bug fixes for caching released with today's beta but yes. Caching should be working except for redirects and link previews originating from a MediaWiki (rather than the Content Service). Please report any issues you find :]

o/ @Kaartic, there were some bug fixes for caching released with today's beta but yes. Caching should be working except for redirects and link previews originating from a MediaWiki (rather than the Content Service). Please report any issues you find :]

Sure! I actually wanted to clear a little doubt I have about one of your previous comments,

If page/summary and page/mobile-sections-lead are expected to share data this is a feature request

Your comment seems to imply that the app doesn't actually have a way of loading link previews from cache. I was actually taking a glimpse of the code that handles the Link previews in the app some time ago.

I noticed the code that I have put in P4921 in the LinkPreviewDialog class. The code has a call to a method named loadContentsFromCache, which I guess is supposed to load the preview from contents in the cache. So why isn't that code doing what it's supposed do?

Note : I request you in advance to forgive my ignorance, as I'm not well acquainted with the code base.

loadContentFromCache() has since been removed along with the cache this method referred to. The reason is that we already have a cache that all other network requests pass through and treating pages differently was difficult to maintain. Link previews should load correctly from the new cache implementation as long as they aren't redirects and they come from the Content Service (instead of MediaWiki). We're having a little trouble supporting all requests correctly, such as link previews from MediaWiki, as many of the Content Service and MediaWiki endpoints require clients to contact the server before using a cached copy which is impossible when offline.

Thanks for the reply. The local version of the project files weren't in sync and I didn't realise that it was removed. Please excuse my ignorance.

@Niedzielski I currently see that there is a loadContentFromSavedPage method in the LinkPreviewDialog class. I guess it's supposed to load the content from a saved page (if it's present) when the content cannot be loaded through a network. As far as I could try, I don't think it's working as I mentioned in the previous statement.

Steps to reproduce

  1. Open the Git article
  2. Add it to a reading list (if it's not in one)
  3. Open the GitHub article
  4. Enable Airplane mode
  5. Tap the link to Git article in the GitHub article

I couldn't see any link preview for the Git article. Is this an issue or am I getting it wrong by any chance?

Hello,

I gave this a quick test this morning and found it working well. I tried Stephen's generic programming and bananas examples. I was able to view the cached previews in airplane mode.

Nexus 4
Android 5.1
Wikipedia Alpha 2.5.188-alpha-2017-02-14

Saved pages are persisted by a background service that doesn't run immediately. Maybe that's the issue @Kaartic saw?

Here is what I found.

  1. Go to Ayrton Senna article and save it
  2. Open previews for all the links in the 1st paragraph
  3. Close the article
  4. Turn on airplane mode
  5. Open article from saved

Now I can open previews for all the links in the 1st paragraph. If I open a preview on a link beyond that I get the endless spinner. Seems normal.

Testing on Samsung-SM-JI20A Galaxy Express 3 (Android 6.0.1) and Wikipedia app 2.5.188-alpha-2017-02-14. I recreated the @ABorbaWMF steps and got the same results, if I opened previews online they would be saved in the cache during Airplane Mode, if I did not open them the spinner would still show. This is consistent with Airplane Mode testing I did for the Android app regressions so this is fixed.

Kaartic added a comment.EditedFeb 15 2017, 1:30 AM

So, am I missing something or is this an issue?

Kaartic added a comment.EditedFeb 15 2017, 1:13 PM

Just wanted to add a little note. I guess one case in which the current implementation fails is the following,

  1. Go to the Generic programming article.
  2. Open the Computer Programming article by using the "Open" option that could be seen by long pressing the link.
  3. Turn on airplane mode
  4. Go back to the Generic programming article.
  5. Click the link to Computer programming article.

Edit : The link preview would not be displayed though the article itself would be in cache (except in some low memory devices).

@Kaartic, this is correct behavior. We do not issue a link preview data request from the Content Service backend unless it will be shown to the user

Kaartic added a comment.EditedFeb 16 2017, 12:11 PM

@Kaartic, this is correct behavior. We do not issue a link preview data request from the Content Service backend unless it will be shown to the user

👍

@Niedzielski: Sorry for asking this again but I'm still confused with the loadContentFromSavedPage method in the LinkPreviewDialog class. What's it supposed to do?

@Kaartic, loadContentFromSavedPage() retrieves pages saved to disk for offline viewing. However, the underlying implementation is being replaced as part of T156917 and this method is expected to disappear

Kaartic added a comment.EditedFeb 17 2017, 1:56 AM

@Kaartic, loadContentFromSavedPage() retrieves pages saved to disk for offline viewing. However, the underlying implementation is being replaced as part of T156917 and this method is expected to disappear

@Niedzielski 👍

Edit: So, it is supposed to load link previews from saved page but isn't for the reason you specified in your previous comment.

Dbrant closed this task as Resolved.Feb 23 2017, 7:50 PM
Dbrant claimed this task.