Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reader: Show preview images in lists for more posts #12895

Merged
merged 12 commits into from Sep 8, 2020
Merged

Conversation

@aforcier
Copy link
Contributor

@aforcier aforcier commented Sep 5, 2020

Fixes #12894, allowing the reader to find suitable preview images for posts more often.

Feed list

reader-android-feed-beforeafter

Site list

reader-android-site-beforeafter

This PR has two parts:

1. Fixing an accidental regex breakage

A huge checkstyle refactor from two years ago ended up accidentally adding spaces to a few regex patterns used by the Reader to parse images. This almost entirely broke support for preview images that weren't set as featured. I undid the breakage in 0379e37 (and added some tests).

2. Adding support for srcset and data-large-file img tag attributes

The above fix improves things a bit but still fails to recognize images in several kinds of posts. This is especially noticeable in the site view. As explained in #12894 the /read/feed/ and /read/sites/ endpoints return different content sometimes. For some reason one of the differences is that some images have no width attribute in the /read/sites/ endpoint, so what works in the feed doesn't work sometimes in the site view.

To mitigate this, I added support for a few new img tag attributes that seem pretty common in WP posts: srcset and data-large-file. These are checked for in a fallback style.

To test:

With and without this patch, follow a few sites with a variety of posts containing images and compare.

Mainly looking for posts with a story block, and posts with image blocks (but no featured image set). (I can recommend a few.)

(For Automatticians: please proxy on your device to fully test posts with the Story block, since depending on the endpoint those won't work correctly otherwise. See pbArwn-14B-p2.)

You can also check the regex fix individually by checking out 86dedbe first and running the (failing) unit tests added.

Notes:

  • Some stories (where the first slide is a video) still won't show a preview image - I'll look into that separately
  • Detail view of stories is broken right now

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.
@aforcier aforcier added the Reader label Sep 5, 2020
@aforcier aforcier added this to the 15.8 milestone Sep 5, 2020
@aforcier aforcier requested review from zwarm, ashiagr and malinajirka Sep 5, 2020
@peril-wordpress-mobile
Copy link

@peril-wordpress-mobile peril-wordpress-mobile bot commented Sep 5, 2020

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@peril-wordpress-mobile
Copy link

@peril-wordpress-mobile peril-wordpress-mobile bot commented Sep 5, 2020

You can test the changes on this Pull Request by downloading the APK here.

@malinajirka malinajirka self-assigned this Sep 7, 2020
Copy link
Contributor

@malinajirka malinajirka left a comment

Thanks @aforcier! I haven't tested the app yet. I've reviewed the regexes and they all look great.
I've a few questions regarding the code changes though - all of them are related to a single method. There is a pretty high chance I'm missing something obvious. Let me know what you think :).

import org.junit.Test
import org.wordpress.android.ui.reader.utils.ReaderHtmlUtils

class ReaderHtmlUtilsTest {

This comment has been minimized.

@malinajirka

malinajirka Sep 7, 2020
Contributor

❤️

@aforcier aforcier force-pushed the fix/reader-images branch from 135df67 to e872a64 Sep 7, 2020
@aforcier
Copy link
Contributor Author

@aforcier aforcier commented Sep 7, 2020

Thanks @aforcier! I haven't tested the app yet. I've reviewed the regexes and they all look great.
I've a few questions regarding the code changes though - all of them are related to a single method. There is a pretty high chance I'm missing something obvious. Let me know what you think :).

Thanks @malinajirka! I got a bit too in the weeds there and neglected some of the bigger picture of what that function was trying to do.

To summarize the approach I'm taking, I think the getLargestImage function should obey these rules:

  1. Choose an image whose size is known (as long as it's larger than the given minImageWidth) over one whose size isn't known.
  2. All things being equal, keep the first image found. This means:
    • If there are multiple images with the same (maximum) width, keep the first one.
    • Don't replace an existing URL with a fallback one (hasSuitableClassForFeaturedImage and getLargeFileAttr). An existing URL is either a known-width image (so the first rule applies), or we got it from an earlier fallback (in which case we should keep the first image since we don't know the real width of either).

The current state should follow those rules properly now. Ready for a second pass 🙁

Copy link
Contributor

@malinajirka malinajirka left a comment

Thanks for the changes ;)! I've tested the app and it works great. Great catch ;)

@malinajirka malinajirka merged commit c8333fc into develop Sep 8, 2020
8 checks passed
8 checks passed
Peril All green. Woo!
Details
ci/circleci: Installable Build Your tests passed on CircleCI!
Details
ci/circleci: Optional Tests/Hold Your job is on hold on CircleCI!
Details
ci/circleci: WordPressUtils Connected Tests Your tests passed on CircleCI!
Details
ci/circleci: gutenberg-bundle-build Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: strings-check Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
@malinajirka malinajirka deleted the fix/reader-images branch Sep 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.