WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#26631 closed task (blessed) (fixed)

Add a "playlist" shortcode

Reported by: wonderboymusic Owned by: wonderboymusic
Milestone: 3.9 Priority: normal
Severity: normal Version:
Component: Media Keywords: needs-testing has-patch needs-unit-tests
Focuses: Cc:

Description

Gallery = list of images, Playlist = list of audio files. There's a gallery shortcode, there should be a playlist shortcode. The existing audio shortcode is only meant to create the markup for 1 <audio>. Playlist generation shouldn't be shoe-horned in there. A playlist probably needs a lot more wiring up with JS anyways.

The UI for managing galleries is great, the same UI can used for playlists.

Attachments (21)

26631.diff (35.9 KB) - added by wonderboymusic 4 years ago.
26631.2.diff (54.1 KB) - added by wonderboymusic 4 years ago.
26631.3.diff (61.9 KB) - added by wonderboymusic 4 years ago.
26631.4.diff (58.9 KB) - added by wonderboymusic 4 years ago.
26631.5.diff (53.3 KB) - added by wonderboymusic 4 years ago.
26631.6.diff (57.3 KB) - added by wonderboymusic 3 years ago.
26631.7.diff (57.9 KB) - added by wonderboymusic 3 years ago.
26631.8.diff (62.6 KB) - added by wonderboymusic 3 years ago.
26631.9.diff (36.4 KB) - added by wonderboymusic 3 years ago.
26631.10.diff (39.1 KB) - added by wonderboymusic 3 years ago.
26631.11.diff (1.8 KB) - added by xknown 3 years ago.
Add some escaping/whitelisting to the shortcode attributes.
26631.12.diff (625 bytes) - added by kovshenin 3 years ago.
26631.13.diff (554 bytes) - added by bassgang 3 years ago.
26631.14.patch (1.1 KB) - added by gcorne 3 years ago.
26631.15.patch (13.6 KB) - added by gcorne 3 years ago.
26631.16.patch (958 bytes) - added by gcorne 3 years ago.
playlist-settings.jpg (300.6 KB) - added by melchoyce 3 years ago.
26631.14.diff (1.6 KB) - added by wonderboymusic 3 years ago.
26631.15.diff (2.0 KB) - added by DrewAPicture 3 years ago.
playlist_styles filter docs + string contexts
26631.16.diff (2.2 KB) - added by wonderboymusic 3 years ago.
26631.17.diff (3.1 KB) - added by wonderboymusic 3 years ago.

Download all attachments as: .zip

Change History (87)

@wonderboymusic
4 years ago

#1 @wonderboymusic
4 years ago

26631.diff is a work in progress, but it indeed allows you to make playlists in the same way you make galleries - one caveat: the shortcode doesn't do anything yet - only outputsTHISISMYPLAYLIST

#2 @nacin
4 years ago

This is pretty cool. One thing I'm noticing is how much gallery code it duplicates and modifies. Some of it looks to be completely new, just following the set pattern. Totally kosher. But how much of it is shared code that could benefit from solid abstraction? Not saying it needs it now in order to go in, but it is good to evaluate it from this perspective, as at some point, core or a plugin will want video playlists, document collections, etc.

#3 @wonderboymusic
4 years ago

Yeah - there needs to be a generic "collection" manager. First pass was just brute force making it exist, I actually have no clue what most of the code does.

#4 @sabreuse
4 years ago

I'd also love to see a more generic way of handling collections beyond just images -- thanks for starting this up!

#5 @ocean90
4 years ago

Master question: Why is a playlist just for audio files? What about videos?

#6 @wonderboymusic
4 years ago

We may be able to abstract it and make it work for both - the gallery sorting definitely needs to become type-agnostic. The shortcodes may be the only place where branching the implementation is necessary

#7 @wonderboymusic
4 years ago

  • Keywords needs-testing has-patch needs-ui needs-docs needs-unit-tests added
  • Milestone changed from Future Release to 3.9

Updated the patch with latest progress:

  • Abstracts wp.media.gallery code in wp.media.collection which wp.media.gallery, wp.media.playlist, and wp.media['video-playlist'] use.
  • Abstracts wp.media.controller.GalleryAdd and wp.media.controller.GalleryEdit into wp.media.controller.CollectionAdd and wp.media.controller.CollectionEdit which all 3 use with minimal registration code
  • Allows any new types/shortcodes to be simply added by convention with a few lines of code, the only excessive code is registering new Toolbars and Menus when you add a new type, there really isn't any way around it
  • Adds TinyMCE view switch for [playlist] and [video-playlist] using the wpplaylist plugin, which I added. Theoretically, all media shortcodes could use the same TinyMCE plugin - all it does it turn a shortcode into HTML and the reverse. Adding another plugin each time seems excessive.
  • Abstracts the shortcode handlers for [playlist] and [video-playlist] into one helper function:
    function wp_playlist_shortcode( $attr ) {
    	return wp_get_playlist( $attr, 'audio' );
    }
    add_shortcode( 'playlist', 'wp_playlist_shortcode' );
    
    function wp_video_playlist_shortcode( $attr ) {
    	return wp_get_playlist( $attr, 'video' );
    }
    add_shortcode( 'video-playlist', 'wp_video_playlist_shortcode' );
    

The player on the front end will use a MediaElementPlayer object and some custom Backbone Views. The ME class is necessary to control the player from code and sequence tracks / support next-prev, etc. Next steps will be skinning the audio player to show metadata and images.

This code actually works now - audio and video, all clips will play one after the other.

Gonna shoot for this or something like it in 3.9.

#8 @wonderboymusic
4 years ago

26631.3.diff adds more settings related to theme/skinning the playlists

#9 @wonderboymusic
4 years ago

I accidentally added a 3rd T in a function when I clicked save and made the patch, refreshed

#10 @wonderboymusic
4 years ago

26631.4.diff​ adds a UI for the audio playlist shortcode, theoretically works for video as well

This ticket was mentioned in IRC in #wordpress-ui by bassgang. View the logs.


4 years ago

This ticket was mentioned in IRC in #wordpress-dev by bassgang. View the logs.


4 years ago

#13 @bassgang
4 years ago

patch -p0 < 26631.5.diff

gives me

patching file src/wp-content/themes/twentyfourteen/functions.php
patching file src/wp-includes/class-wp-editor.php
Hunk #1 succeeded at 236 (offset 4 lines).
patching file src/wp-includes/css/editor.css
Hunk #1 succeeded at 481 (offset 9 lines).
Hunk #2 succeeded at 492 (offset 9 lines).
Hunk #3 succeeded at 508 (offset 9 lines).
patching file src/wp-includes/css/media-views.css
Hunk #1 succeeded at 595 (offset 6 lines).
Hunk #2 succeeded at 1416 (offset 6 lines).
Hunk #3 succeeded at 1692 (offset 6 lines).
Hunk #4 succeeded at 1740 (offset 6 lines).
Hunk #5 succeeded at 1765 (offset 6 lines).
Hunk #6 succeeded at 1791 (offset 6 lines).
Hunk #7 succeeded at 1800 (offset 6 lines).
Hunk #8 succeeded at 1873 (offset 6 lines).
Hunk #9 succeeded at 1910 (offset 6 lines).
patching file src/wp-includes/js/media-editor.js
patching file src/wp-includes/js/media-views.js
Hunk #1 succeeded at 461 (offset 1 line).
Hunk #2 FAILED at 755.
Hunk #3 succeeded at 842 (offset 4 lines).
Hunk #4 succeeded at 1793 (offset 5 lines).
Hunk #5 succeeded at 1854 (offset 5 lines).
Hunk #6 succeeded at 1866 (offset 5 lines).
Hunk #7 succeeded at 1880 (offset 5 lines).
Hunk #8 succeeded at 1936 (offset 6 lines).
Hunk #9 succeeded at 2100 (offset 6 lines).
Hunk #10 succeeded at 2220 (offset 6 lines).
Hunk #11 succeeded at 5154 (offset 12 lines).
1 out of 11 hunks FAILED -- saving rejects to file src/wp-includes/js/media-views.js.rej
patching file src/wp-includes/js/mediaelement/wp-mediaelement.css
patching file src/wp-includes/js/plupload/handlers.js
patching file src/wp-includes/js/swfupload/handlers.js
patching file src/wp-includes/js/tinymce/langs/wp-langs-en.js
patching file src/wp-includes/js/tinymce/skins/wordpress/wp-content.css
patching file src/wp-includes/media-template.php
patching file src/wp-includes/media.php
Hunk #2 succeeded at 2186 (offset 18 lines).
Hunk #3 succeeded at 2215 (offset 18 lines).
patching file src/wp-includes/script-loader.php

@wonderboymusic can you please update the diff? Thanks.

#14 @wonderboymusic
3 years ago

26631.6.diff refreshes the patch against latest trunk.

Gonna do another round of UI. wp.media.collection.instance() and the like could use a review by a @gcorne. It should be obvious what I am trying to abstract, there are probably other ways to do it. Could use some eyes.

#15 @wonderboymusic
3 years ago

26631.8.diff is a good alpha.

  • Adds playlist and video-playlist to the wpgallery TinyMCE plugin (where audio and video currently reside)
  • Makes Attachment Settings checkboxes work for playlist and video-playlist, this logic is weird in general for checkboxes and data-setting
  • Supports the Featured Image in the media model on Edit Media page for audio and video when supported
  • Supports poster images for videos in video-playlist
  • Makes audio responsive to size of $content_width
  • Loads videos at proper aspect ratio in relation to $content_width
  • Supports light and dark styles
  • Works out of the box in 2010, 2011, 2012, 2013, and 2014 themes - tested in all 5
  • Adds theme_supports_thumbnails( $post ) and post_supports_thumbnails( $post ) to check more than just $post->post_type in scenarios that need to check post_type_supports( 'attachment:video', 'thumbnail' ) and current_theme_supports( 'post-thumbnails', 'attachment:video' )
  • contains all of the code to abstract Gallery JS code into "Collection code

#16 @wonderboymusic
3 years ago

In 27209:

Allow pseudo post types attachment:audio and attachment:video to get the media modal on Edit Media when they support featured images.

Introduces post_supports_thumbnails( $post ) and theme_supports_thumbnails( $post ) to cut down on duplicated code everytime this needs to be checked. There will be more cases forthcoming.

See #26631.

#17 @wonderboymusic
3 years ago

In 27212:

Add an abstraction of the gallery code in media-editor.js called wp.media.collection. This will be the basis for parsing [gallery]-like shortcodes in the media editor (thing playlists, collections of PDFs, etc).

There are currently no instances of this. Those will be forthcoming.

See #26631.

#18 @wonderboymusic
3 years ago

In 27213:

Replace the current wp.media.gallery instance in media-editor.js with one that leverages wp.media.collection.

See #26631.

#19 @wonderboymusic
3 years ago

In 27214:

In media-views.js, add wp.media.controller.CollectionAdd and wp.media.controller.CollectionEdit to provide an abstraction for Add and Edit screens in the media modal for collection-type things.

There are currently no instances of this. Those will be forthcoming.

See #26631.

#20 @wonderboymusic
3 years ago

In 27215:

Register wp.media.controller.GalleryEdit and wp.media.controller.GalleryAdd using the new wp.media.controller.Collection* abstraction code.

See #26631.

#21 @wonderboymusic
3 years ago

26631.9.diff is 1/2 of the size of .8.diff and way more sane. It is also strictly playlist changes.

#22 @wonderboymusic
3 years ago

In 27216:

Correct a missing conditional in edit-form-advanced.php.

See #26631, [27209].
Props nacin for noticing.

#23 @juliobox
3 years ago

Edit: damn i saw this an hour ago, i thought i clicked on "submit" but no, so i did, then i saw you already fixed it, delete this -_- thanks

Hello, tell me if i'm wrong but i just read the code and i saw this:

if ( 'attachment' && $post_ID ) {

i think it's

if ( 'attachment' === $post_type && $post_ID ) {

See you

Last edited 3 years ago by juliobox (previous) (diff)

#24 @ocean90
3 years ago

[27213] breaks removing of default shortcode attributes, see #27183.

#25 @wonderboymusic
3 years ago

In 27239:

Add core support for Playlists and Video Playlists.

  • Playlists operate like galleries in the admin.
  • Provide default UI and JS support in themes using MediaElement and Backbone.
  • The shortcodes are clickable, editable, and configurable using the media modal.
  • Playlists support images for each item, whether or not the current theme supports images for attachment:audio and attachment:video
  • Playlists respond to $content_width and resize videos accordingly.
  • All playlist data is included inline, using a script tag with type="application/json", allowing anyone to unenqueue the WP playlist JS and roll their own.
  • Playlist styles are minimal and work out of the box in the last 5 default themes. They inherit and adapt to the current theme's font styles, and their rules are easily overrideable.

See #26631.

#26 @DrewAPicture
3 years ago

  • Keywords needs-docs removed

In 27240:

(The changeset message doesn't reference this ticket)

(Forgot to mention the ticket number in the original commit)

#27 @DrewAPicture
3 years ago

In 27241:

Spacing fixes for code introduced in [27239].

See #26631

#28 @wonderboymusic
3 years ago

In 27242:

Load default settings state when creating a new playlist in the media modal. Add a few missing inline @this annotations in media-editor.js.

See #26631.

#29 @wonderboymusic
3 years ago

In 27243:

Remove a few unnecessary jQuery selectors added in [27239]. Playlists don't exist in some older contexts that galleries do.

See #26631.

#30 @wonderboymusic
3 years ago

In 27244:

Supply mime-type instead of extension for an item's type property in playlist JSON.

See #26631.

#31 @wonderboymusic
3 years ago

In 27245:

Get the proper image size for an item's thumb in playlist JSON.

See #26631.

#32 @nacin
3 years ago

In 27266:

Fixed mixed spaces and tabs. Also remove trailing comma. see #26631.

This ticket was mentioned in IRC in #wordpress-dev by DH-Shredder. View the logs.


3 years ago

@xknown
3 years ago

Add some escaping/whitelisting to the shortcode attributes.

@kovshenin
3 years ago

#34 follow-up: @kovshenin
3 years ago

Defaults don't seem to work for me in Chrome on OS X. If you create a new playlist and don't touch any of it's settings (which are checked by default), the inserted shortcode's attributes are all false because they're undefined in media.collection.shortcodeAttrs. We can handle it the same way as we do in media.collection.attachments, see 26631.12.diff.

#35 in reply to: ↑ 34 @helen
3 years ago

Replying to kovshenin:

Defaults don't seem to work for me in Chrome on OS X. If you create a new playlist and don't touch any of it's settings (which are checked by default), the inserted shortcode's attributes are all false because they're undefined in media.collection.shortcodeAttrs. We can handle it the same way as we do in media.collection.attachments, see 26631.12.diff.

They *aren't* set if you check them, which is driving me crazy. It's backward. Just hadn't gotten around to leaving a comment on that yet :)

#36 @wonderboymusic
3 years ago

In 27308:

Populate playlist setting values with their default value if they don't exist while parsing shortcode attributes.

Props kovshenin.
See #26631.

#37 follow-up: @wonderboymusic
3 years ago

  • Keywords needs-ui removed

@xknown: I would imagine styles will be filterable soon, so let's leave that alone for now. The idea is that your theme or plugin could eventually add new theming styles in the settings dropdown.

#38 in reply to: ↑ 37 @xknown
3 years ago

Replying to wonderboymusic:

@xknown: I would imagine styles will be filterable soon, so let's leave that alone for now. The idea is that your theme or plugin could eventually add new theming styles in the settings dropdown.

Ok. However, it still needs to be escaped/sanitized. The current implementation is vulnerable to XSS.

@bassgang
3 years ago

#39 @bassgang
3 years ago

If an attachment neither has a meta title nor a caption the content of ".wp-playlist-current-item" is empty in an audio playlist in the current implementation, which looks not so good. attachment:26631.13.diff fixes this due to falling back to "data.title" if "data.meta.title" and "data.caption" are both not available.

Last edited 3 years ago by bassgang (previous) (diff)

#40 @wonderboymusic
3 years ago

In 27311:

Add some security hardening to passed playlist attributes.

Props xknown.
See #26631.

#41 @wonderboymusic
3 years ago

In 27312:

In the tracklist for playlist, fall back to title if there is no caption.

Props bassgang.
See #26631.

#42 follow-up: @wonderboymusic
3 years ago

Next step is to make the value for "style" filterable in Playlist Settings and in the shortcode attribute whitelist. Ideally, a plugin or theme could install additional styles.

I don't think we want to bundle more in core, although I do like the idea of a playlist that has a "cover image" - I have some SoundCloud embeds on the homepage of this site: http://highforthis.com. The image could change with each track.

Since assets (JS/CSS) are only loaded on the first instance of a playlist shortcode, we can probably fire an action (something like wp_playlist_{$style}) where one could hook. We should make our enqueue'ing using the action so it can be easily unhooked.

#43 @wonderboymusic
3 years ago

In 27313:

wp.media.collection should be its own civilized instantiable class, not a wrapper/factory. The class shall contain no reference to specific instances, and shall not try to grab static properties of itself. self, meet this.

See #26631.

#44 @wonderboymusic
3 years ago

In 27315:

This comment was pasted and does not apply to specified code block.

See #26631.

#45 @wonderboymusic
3 years ago

In 27317:

In media-editor.js, use _.isUndefined() when available.

See #26631.

#46 in reply to: ↑ 42 @celloexpressions
3 years ago

Replying to wonderboymusic:

Next step is to make the value for "style" filterable in Playlist Settings and in the shortcode attribute whitelist. Ideally, a plugin or theme could install additional styles.

I feel like we shouldn't have the style option, at least not here. In most cases, the selected style should probably be the same for all of your playlists until/unless you change your theme. I'd rather see this as a sitewide option if we feel like we need to have it (at least for light/dark), or if possible try to find a way to remove the option altogether (haven't thought of anything yet). Themes and plugins can, of course, still add their own implementations, and even add this sort of an option, but I don't think it's necessary out-of-the-box so that we can keep it as simple as possible.

Tested in IE11 and it works great, by the way.

#47 @wonderboymusic
3 years ago

In 27320:

Some playlist cleanup:

  • Check properties against the window object when using _.isUndefined() on globals
  • Fix a typo for $safe_type introduced in [27311]

See #26631.

#48 @wonderboymusic
3 years ago

In 27321:

Restore / re-tools some docs after [27313].

See #26631.

#49 @wonderboymusic
3 years ago

In 27322:

Rather than extending media.controller.CollectionAdd 3 times, make it a constructor that dynamically extends media.controller.Library and use instances of it instead.

See #26631.

#50 @wonderboymusic
3 years ago

I am going to do something similar for CollectionEdit.

My goal here:

  • Reusable classes that can just be instantiated with attributes, and don't require extending every time
  • When new features are added, it would good if more code was shared, versus duplicating 200 lines and only changing one or two properties
  • Use actual objects instead of hacking together factories

@gcorne
3 years ago

@gcorne
3 years ago

@gcorne
3 years ago

#51 @gcorne
3 years ago

I worked up a few patches that adjust the code a bit and make one small visual change.

26631.14.patch adds some braces to a couple if/else statements to make jshint happy.

26631.15.patch refactors CollectionEdit and CollectionAdd.

  • Instead of generating new constructors use the initialize method to set attributes based on the collectionType
  • Use the reference to the SettingsView instead of grabbing it from the namespace. This better matches with existing AttachmentDisplay attribute.
  • Switch from using tag to collectionType to better communicate the purpose of the attribute.
  • Make the design of the CollectionEdit and CollectionAdd consistent with one another

The refactoring cuts down the lines of code by about 20 lines and also in my opinion makes the code a little easier to follow. I am curious to know whether or not others agree.

One small concern I have about this change is that it removes the GalleryEdit and GalleryAdd constructors, which a plugin may be manipulating. A search of the .org plugin repository didn't return any results so removing them is probably not a big deal.

26631.16.patch adjusts the priority of the separator in the Edit Playlist and Edit Video Playlist menus to match the structure of the Edit Gallery menu.

#52 @wonderboymusic
3 years ago

In 27361:

Add some braces for jshint in media-views.js.

See #26631.
Props gcorne.

#53 @wonderboymusic
3 years ago

In 27362:

Make CollectionEdit and CollectionAdd less dynamically quirky. Rename some instance properties for disambiguation. Pass some properties from options when creating instances in wp.media.view.MediaFrame.Post.

See #26631.
Props gcorne.

#54 @wonderboymusic
3 years ago

In 27363:

Adjust the priority of the separator in the Edit Playlist and Edit Video Playlist menus to match the structure of the Edit Gallery menu.

Props gcorne.
See #26631.

#55 @nacin
3 years ago

  • Type changed from feature request to task (blessed)

#56 follow-up: @johnbillion
3 years ago

I think the "Create Playlist" tab in the media manager should be renamed "Create Audio Playlist" so it doesn't look as odd with the "Create Video Playlist" below it.

The"Create Gallery" tab should then probably be renamed "Create Image Gallery" to fit better with the two playlist tabs below it.

This ticket was mentioned in IRC in #wordpress-dev by wonderboymusic. View the logs.


3 years ago

#58 @melchoyce
3 years ago

Took a stab at cleaning up the playlist settings a bit in playlist-settings.jpg.

#59 @wonderboymusic
3 years ago

In 27486:

Playlists:

  • Add an action, wp_playlist_scripts, where a user can turn off all default script and style loading for playlists and roll their own.
  • Move the script and style loading for playlists to a function, wp_playlist_scripts( $type ), that hooks into wp_playlist_scripts.
  • Make the <noscript> playlist output an <ol>, instead of a list of links with no surrounding markup.

See #26631.

#60 @wonderboymusic
3 years ago

If anybody wants some easy props, 26631.14.diff needs some filter docs

#61 @wonderboymusic
3 years ago

In 27487:

Update the UI styles for Playlist Settings.

Props melchoyce for the design.
See #26631.

This ticket was mentioned in IRC in #wordpress-dev by DrewAPicture. View the logs.


3 years ago

@DrewAPicture
3 years ago

playlist_styles filter docs + string contexts

#63 @wonderboymusic
3 years ago

In 27488:

Add a filter and document it: playlist_styles. Allows users to roll their own playlist themes.

Props DrewAPicture for the docs.
See #26631.

#64 in reply to: ↑ 56 @bassgang
3 years ago

Replying to johnbillion:

I think the "Create Playlist" tab in the media manager should be renamed "Create Audio Playlist" so it doesn't look as odd with the "Create Video Playlist" below it.

I support this suggestion. In my opinion a playlist does not necessarily defaults to audio so that renaming "Create Playlist" to "Create Audio Playlist" is clearer.

#65 @wonderboymusic
3 years ago

  • Owner set to wonderboymusic
  • Resolution set to fixed
  • Status changed from new to closed

In 27512:

Change string "Create Playlist" to "Create Audio Playlist." Open new tickets for individual issues.

Fixes #26631.

This ticket was mentioned in IRC in #wordpress-dev by wonderboymusic. View the logs.


3 years ago

Note: See TracTickets for help on using tickets.