Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
Reader: Show preview images in lists for more posts #12895
Conversation
The regex was broken 2 years ago by a whitespace refactor in ed574ea.
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
You can test the changes on this Pull Request by downloading the APK here. |
Thanks @aforcier! I haven't tested the app yet. I've reviewed the regexes and they all look great. |
.../main/java/org/wordpress/android/ui/reader/utils/ReaderImageScanner.java
Outdated
Show resolved
Hide resolved
.../main/java/org/wordpress/android/ui/reader/utils/ReaderImageScanner.java
Outdated
Show resolved
Hide resolved
.../main/java/org/wordpress/android/ui/reader/utils/ReaderImageScanner.java
Outdated
Show resolved
Hide resolved
.../main/java/org/wordpress/android/ui/reader/utils/ReaderImageScanner.java
Outdated
Show resolved
Hide resolved
import org.junit.Test | ||
import org.wordpress.android.ui.reader.utils.ReaderHtmlUtils | ||
|
||
class ReaderHtmlUtilsTest { |
malinajirka
Sep 7, 2020
Contributor
❤️
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
The current state should follow those rules properly now. Ready for a second pass |
Thanks for the changes ;)! I've tested the app and it works great. Great catch ;) |
aforcier commentedSep 5, 2020
•
edited
Fixes #12894, allowing the reader to find suitable preview images for posts more often.
Feed list
Site list
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
anddata-large-file
img
tag attributesThe 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:
PR submission checklist:
RELEASE-NOTES.txt
if necessary.