Make WordPress Core

Opened 5 years ago

Closed 2 years ago

Last modified 21 months ago

#49852 closed enhancement (fixed)

Use feature image of video attachment as preview instead of icon

Reported by: spacedmonkey's profile spacedmonkey Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.2 Priority: normal
Severity: normal Version:
Component: Media Keywords: good-first-bug has-patch has-screenshots commit early
Focuses: Cc:

Description

In the media library list view, it currently shows a video icon. A user is able to selected featured image for video attachments ( used for poster image in video tag ). If a feature image is set for a video attachment, show this instead of the icon.

Attachments (6)

49852.patch (400 bytes) - added by samful 5 years ago.
samful's patch for video thumbnails not appearing on list view
49852.2.patch (854 bytes) - added by samful 5 years ago.
samful's patch for video and audio thumbnails not appearing on list view
49852.3.patch (852 bytes) - added by samful 5 years ago.
fixed a silly mistake where i left the array in quotes…
49852.4.patch (962 bytes) - added by samful 4 years ago.
refreshed with proper /src path and line numbers
49852.5.diff (907 bytes) - added by seanchayes 4 years ago.
Updated spacing matching coding standards - no functional changes
49852.6.patch (964 bytes) - added by cadic 2 years ago.
Code styles: Yoda conditions, strict comparison, spacing

Download all attachments as: .zip

Change History (31)

@samful
5 years ago

samful's patch for video thumbnails not appearing on list view

#1 @samful
5 years ago

I managed to replicate this and fix it in the "wp-admin/includes/class-wp-media-list-table.php" file and submitted a patch.

Using my patch seems to use the featured image of the video (if set) instead of the icon in list view and solve the issue.

This is my first bug fix, I hope it helps :)

Last edited 5 years ago by samful (previous) (diff)

#2 @johnbillion
5 years ago

  • Keywords needs-refresh has-patch added; needs-patch removed

Thanks for the patch @samful and welcome to WordPress Trac!

Some feedback on your patch, as it's your first one:

  • It looks like the formatting of your patch file isn't right, here's a guide for creating a patch file correctly: https://make.wordpress.org/core/handbook/tutorials/trac/submitting-a-patch/
  • Not all videos will have a featured image, so we'll need some logic which only uses the featured image ID if one exists, and still uses the ID of the video attachment if not so the correct icon is displayed.
  • Audio files can actually have a featured image too, although this is not a commonly used feature. They're used if you create an audio playlist in the classic editor. We might as well add support for featured images for audio too.

Feel free to resubmit your patch using the same file name, and just ask here if you have any questions.

@samful
5 years ago

samful's patch for video and audio thumbnails not appearing on list view

#3 @samful
5 years ago

Thank you for the feedback @johnbillion, very much appreciated.

  • Formatting should be correct now.
  • Logic has been added to keep using the video/audio ID if a featured image doesn't exist.
  • Audio featured images have now been fixed in the same style as the video ones.
Last edited 5 years ago by samful (previous) (diff)

@samful
5 years ago

fixed a silly mistake where i left the array in quotes...

#4 @JavierCasares
5 years ago

  • Keywords has-screenshots added

https://i.ibb.co/xz58YqG/Screenshot-240.png

vs

https://i.ibb.co/sgC9BpR/Screenshot-239.png

#5 @JavierCasares
5 years ago

Also, works great :)

@samful
4 years ago

refreshed with proper /src path and line numbers

#6 @seanchayes
4 years ago

I downloaded 49852.4.patch and applied it to my local trunk installation.

To test it out I did the following:

  • I added a new video file to the media library.
  • In the media library list view the video I uploaded showed a default thumbnail.
  • I edited the media item and chose a featured image and clicked update.
  • I returned to the media library list view and saw that now my featured image selection was being used as the thumbnail for my video file.
  • I successfully repeated the same test with an audio file.

To that end 49852.4.patch tested out successfully.

I iterated on 49852.4.patch, made some coding standards changes (no functional changes) and have submitted 49852.5.diff for consideration.

@seanchayes
4 years ago

Updated spacing matching coding standards - no functional changes

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


2 years ago

#8 @antpb
2 years ago

#55164 was marked as a duplicate.

@cadic
2 years ago

Code styles: Yoda conditions, strict comparison, spacing

This ticket was mentioned in PR #3264 on WordPress/wordpress-develop by cadic.


2 years ago
#9

  • Keywords needs-refresh removed

#10 @mukesh27
2 years ago

  • Keywords changes-requested added

@cadic Left one miner change request in PR.

cadic commented on PR #3264:


2 years ago
#11

@cadic Looks good to me. Left one miner change.

Thanks @mukeshpanchal27, what was the request?

mukeshpanchal27 commented on PR #3264:


2 years ago
#12

Check now

#13 @spacedmonkey
2 years ago

  • Milestone changed from Awaiting Review to Future Release

Given my feedback. Good work. @cadic

#14 @spacedmonkey
2 years ago

  • Owner set to spacedmonkey
  • Status changed from new to assigned

#15 @cadic
2 years ago

@spacedmonkey thank you for the suggestion. I just curious, isn't it a potential performance issue? Using wp_attachment_is() twice inside a loop on the $post object which was already processed during upload and have assigned post_mime_type attribute?

#16 @cadic
2 years ago

@spacedmonkey oh, nevermind, I see it will use post_mime_type early if it exists!

#17 @spacedmonkey
2 years ago

@cadic Didn't say it was a performance issue.

This check mirrors this. A developer could filter wp_get_audio_extensions or wp_get_video_extensions. So we have to use wp_attachment_is.

#18 @JeffPaul
2 years ago

@spacedmonkey probably too late for 6.1, but if that patch looks good then perhaps we milestone for 6.2 and mark for commit?

#19 @spacedmonkey
2 years ago

  • Keywords commit added; changes-requested removed
  • Milestone changed from Future Release to 6.2

@JeffPaul I would be happy to commit as it is a small change, but I will defer to you on this one.

#20 @spacedmonkey
2 years ago

  • Keywords early added

#22 @spacedmonkey
2 years ago

@cadic I have updated the patch to ensure that caches are primed and made the logic a little simpler. If you are happy with the changes, I will commit.

#23 @spacedmonkey
2 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 54941:

Media: Use featured image if available for attachment as preview instead of icon.

When rendering a list of attachments in WP_Media_List_Table class, none image attachments, show a generic icon. However, attachment types like audio and video support adding a featured image. For attachments that have featured images, us this image instead of the icon. This featured image is a better preview than a generic icon.

Props spacedmonkey, samful, johnbillion, JavierCasares, seanchayes, antpb, cadic, JeffPaul.
Fixes #49852.

#24 @milana_cap
22 months ago

  • Keywords add-to-field-guide added

#25 @milana_cap
21 months ago

  • Keywords add-to-field-guide removed
Note: See TracTickets for help on using tickets.