Page MenuHomePhabricator

[BUG] Images not saved when article saved for offline reading
Open, NormalPublicBug

Description

Issue:

Media is not saved while saving articles in reading lists

Steps to reproduce:
Save article to reading list
Download it for offline reading
Go offline
Open saved article

**Expected:
Images and other media are saved and shown
From @Charlotte

Ok, so here's the way forward: we're just not going to show images in the gallery view whilst a user is offline. When users tap on an image in a saved article whilst offline, let's pop up a snackbar saying "Gallery view not available offline." The action on the snackbar can say "Dismiss"

Actual:
Images and other media are not saved

Related Objects

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 22 2019, 12:10 AM
Charlotte renamed this task from Provide an option in settings to save media while saving articles for offline reading to [BUG] Images not saved when article saved for offline reading.Jun 24 2019, 4:29 PM
Charlotte triaged this task as Normal priority.
Restricted Application changed the subtype of this task from "Task" to "Bug Report". · View Herald TranscriptJun 24 2019, 4:29 PM

@Charlotte @schoenbaechler We need a product/design clarification here:
When saving an article for offline reading, do we actually want to save all the gallery-resolution images belonging to the article, in addition to the images that are inline with the article? (This would enable the user to tap on images and view them fullscreen while offline, but it would also incur quite a bit more data usage and device storage upfront.)

Charlotte added a comment.EditedTue, Jul 23, 10:02 AM

I do not think it would be worth the usage/storage to save all gallery-resolution images on the off-chance that someone will open them in the gallery view. Users who are offline will expect at least some degradation in service. But we should make sure that inline media is saved so that it can be shown properly in the article view.

@schoenbaechler - any disagreements?

Thanks @Dbrant for pointing us to this. Mainly agree with @Charlotte’s previous assessment with one addition: all images that are visible in the article (inline) should also be available offline (in the higher resolution) in gallery view. I recommend to disable swiping left and right in gallery view when users are offline (instead of showing an error like this):

This way, users will get the feeling of having a complete article at hand when they’re not connected to the internet.

@schoenbaechler - Your one addition is the opposite of what I meant. :) Think of the people who don't have a lot of storage on their phone. Are they really going to want to download 20-50-100mb or more of images for each article, if the article has a lot of images? I don't think it's reasonable to also store the huge gallery-sized images from that perspective.

@Charlotte:

D'oh, my bad 🤦‍♀️ TMW you don’t know the app you’re working on 24/7: I oddly believed that the gallery features additional images that aren’t used in the article.

Under these circumstances, I think you’re proposal to 'show images inline but not full-res in gallery view' makes sense. Another addition though (*drumroll*): Can we display low-res images in the gallery (the inline article images)? I think it’s better to keep the gallery’s functionality rather than displaying the “Cannot connect to the internet" screen.

In T226290#5357094, @schoenbaechler wrote:
Another addition though (*drumroll*): Can we display low-res images in the gallery (the inline article images)? I think it’s better to keep the gallery’s functionality rather than displaying the “Cannot connect to the internet" screen.

That would seem like a reasonable halfway house if it's technically feasible, but I am not sure what the relative complexity would be. @Dbrant, can you comment?

The short answer is Maybe, but it would need to be a separate task, prioritized on its own.

cooltey added a comment.EditedTue, Jul 23, 4:14 PM

The patch that I worked on does exactly downloads low-res images for the gallery, and there one concern that should we also download *video* files for the gallery?

Good to know @cooltey! How do you currently handle video files in the article? Ideally, videos would also be downloaded in a low resolution. If not feasible, I think it’s fine to output the placeholder image of the video without play button in the gallery for now.

The patch that I worked on does exactly downloads low-res images for the gallery

Well, yes and no: Your patch downloads additional thumbnail-resolution versions of the images, in addition to the inline images in the article. This is better than downloading high-resolution images, but it still means that we're making additional requests and downloading more stuff. Ideally we should just take the exact image URL of the inline image in the article, and reuse it in the gallery, without having to download anything else.

Good to know @cooltey! How do you currently handle video files in the article? Ideally, videos would also be downloaded in a low resolution. If not feasible, I think it’s fine to output the placeholder image of the video without play button in the gallery for now.

Currently, it downloads the "original" quality of the video; I think it would be better if *not* to download video files and show thumbnails.

Well, yes and no: Your patch downloads additional thumbnail-resolution versions of the images, in addition to the inline images in the article. This is better than downloading high-resolution images, but it still means that we're making additional requests and downloading more stuff. Ideally we should just take the exact image URL of the inline image in the article, and reuse it in the gallery, without having to download anything else.

Make sense, will do that!

Good to know @cooltey! How do you currently handle video files in the article? Ideally, videos would also be downloaded in a low resolution. If not feasible, I think it’s fine to output the placeholder image of the video without play button in the gallery for now.

Currently, it downloads the "original" quality of the video; I think it would be better if *not* to download video files and show thumbnails.

Let's not download videos - take pity on the people with small capacity phones!

+1 to Charlotte’s comment and the solution you suggested sounds good @cooltey:

I think it would be better if *not* to download video files and show thumbnails.

Hi @Charlotte and @schoenbaechler

Since we start using the media-list endpoint (T229353), we'll need another mediaInfo call for fetching the actual image URL.

Can we just open the Gallery as a general image viewer and load the cached image from the article and hide the description area?

(tap the image -> open the image in Gallery -> not able to swipe to the next image)

Hey @cooltey – do I understand correctly that all images in an article would still be tappable (to get to the gallery view) and it would just not be possible to go left and right from any particular article image? From my perspective, that behavior is fine.

Not sure if it’s ok from a legal perspective to hide the information below the image. @Charlotte might know more about it?

Eee. I'm not sure that we're ok to hide the attribution - it may contravene the CC-BY license, but I'm obviously not a lawyer so can't say for sure. @Slaporte?

Thanks, @schoenbaechler and yes, you're correct.

Hi, @Slaporte, just wants to clarify, the behavior we've discussed will happen when the device is offline.

Thanks, @Charlotte. I've replied via email

Ok, so here's the way forward: we're just not going to show images in the gallery view whilst a user is offline. When users tap on an image in a saved article whilst offline, let's pop up a snackbar saying "Gallery view not available offline." The action on the snackbar can say "Dismiss"

Will do it. Thanks @Charlotte

cooltey updated the task description. (Show Details)Thu, Aug 8, 5:11 PM
cooltey updated the task description. (Show Details)

Thanks @cooltey - I know it's annoying to rip out work you've just done, but given the constraints of mediaInfo I think this is as good as it's going to get for now.

I think we found a good solution for this! Thanks @cooltey for leading the efforts.

ABorbaWMF added a subscriber: ABorbaWMF.

This looks about 90% good on 2.7.50287-alpha-2019-08-13

Note - I have seen some articles where the lead image does not appear to load, but I believe maybe those articles were downloading when I went offline. If I then restore connection and view the article the lead image appears thereafter when viewing offline.