WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#17578 closed task (blessed) (fixed)

Merge media buttons into one

Reported by: mitchoyoshitaka Owned by: nacin
Milestone: 3.3 Priority: normal
Severity: trivial Version:
Component: Media Keywords: has-patch dev-feedback
Focuses: Cc:

Description

Here's a patch with the filter name 'media_buttons_media_types'.

Attachments (7)

17578.diff (1.3 KB) - added by mitchoyoshitaka 3 years ago.
17578.2.diff (18.5 KB) - added by nacin 3 years ago.
17578.3.diff (22.4 KB) - added by nacin 3 years ago.
auto-detect-img.patch (2.4 KB) - added by azaozz 3 years ago.
17578.break-line.patch (1.2 KB) - added by ocean90 3 years ago.
17578.break-line.2.patch (1.2 KB) - added by SergeyBiryukov 3 years ago.
17578.break-line.3.patch (1.2 KB) - added by SergeyBiryukov 3 years ago.

Download all attachments as: .zip

Change History (47)

mitchoyoshitaka3 years ago

comment:1 scribu3 years ago

Pretty sure there's already a filter for this somewhere in there.

comment:2 nacin3 years ago

Nope. The whole thing is attached to a hook, but that's about it.

You could wedge something into a filter off the mu_media_buttons site option (if multisite) or media_buttons_context (a filter for a string), but that's quite lame.

There should be a filter on the array.

comment:3 mitchoyoshitaka3 years ago

Indeed, I was just about to filter on the HTML string via 'media_buttons_context' but decided to write a bug and complain about it first. :)

comment:4 azaozz3 years ago

I'm unsure why there are four buttons there that do the same thing. Perhaps they were needed when this was introduced but have lost their meaning since. It seems confusing that you can upload anything by using any of these buttons. Perhaps when we change the uploader in 3.3 we will switch to using only one "Insert/Upload" button.

Looking at the patch: we already have most of the logic there, don't see a need to repeat it (also really dislike using extract() especially without EXTR_IF_EXISTS / EXTR_SKIP). So this should probably be:

$media_buttons = array('image' => true, 'audio' => true, 'video' => true);
if ( is_multisite() )
  $media_buttons = get_site_option( 'mu_media_buttons' );

$media_buttons = apply_filters( 'media_buttons_media_types', $media_buttons );

$out = '';
if ( ! empty($media_buttons['image']) ) 
  $out .= _media_button(__('Add an Image'), ....
Last edited 3 years ago by azaozz (previous) (diff)

comment:5 scribu3 years ago

+1 on having a single upload button.

Next to it, we could have the "Insert link" button, as discussed in another ticket.

comment:6 patrickgarman3 years ago

Another +1 for a single media upload button. Small issue, but it could cause minor confusing even though it will not hinder anyone's ability to upload content.

While we're at it... in multisite installs network admins have the option of what buttons to show. http://screencast.com/t/vlv9CyTANB ... might avoid another bug report by taking out those options once the different buttons are gone for good?

comment:7 ocean903 years ago

  • Milestone changed from Awaiting Review to 3.3

Ticket about the new file uploader #18206

comment:8 nacin3 years ago

  • Keywords 3.4-early added
  • Milestone changed from 3.3 to Future Release
  • Summary changed from Add filter to control which media buttons get shown to Merge media buttons into one

Sadly, we've yet to merge these buttons. Let's do that in 3.4.

comment:9 nacin3 years ago

  • Milestone changed from Future Release to 3.3
  • Type changed from feature request to defect (bug)

Resurrecting this after an IRC discussion, re-classifying it as a UI bug, as it confuses people left and right and comes up in testing all of the time.

I have a patch on the way.

nacin3 years ago

comment:10 nacin3 years ago

  • Keywords dev-feedback added; 3.4-early removed

Attached patch:

  • Stubs media_buttons(). We are now using the generic 'media' button (which is type = 'file'). I switched to the video image for now as it looks better. Per IRC, we should steal the image from the Media Library screen/menu icon.
  • Removes media_upload_audio(), media_upload_video(), and media_upload_file(). These can be stubbed to wrap media_upload_image().
  • Removes type_url_form_video(), type_url_form_audio(), and type_url_form_file(). These can be stubbed to wrap type_url_form_image().
  • media_upload_type_url_form(), which is the "Insert from URL" form, is the only thing that actually needed any changes. Here, we now have radio buttons that default to "Image" insertion, and allow for "Audio, Video, or Files." Dead-simple jQuery toggles which fields to show. This dates back to crazyhorse, according to jane.
  • Replaces the callbacks for the media_upload_audio, media_upload_video, and media_upload_file hooks to be media_upload_image(), to ensure some compatibility when type=audio, type=video, or type=file is requested. (It will take basically no work to also ensure that such links will result in the image fields being hidden by default.)
  • Removes the old add_action( 'async_upload_*' ) calls, which are from pre-plupload.
  • Removes mu_media_buttons from network settings and code.

Probably needs some tweaking based on other things we find, but it's pretty solid as-is, and handles the merger quite elegantly indeed. I will say though, my original diff will never see the light of day. It was much easier the second time around.

comment:11 nacin3 years ago

  • Component changed from Administration to Media

comment:12 nacin3 years ago

We'll need to do a quick headcount of filters to make sure too much isn't trapped in the rubble.

comment:13 SergeyBiryukov3 years ago

The patch removes audio_send_to_editor_url, video_send_to_editor_url, file_send_to_editor_url filters.

comment:14 nacin3 years ago

17578.3.diff is the result of a closer inspection of the guts of what we're working with here.

  • Merges in code from media_upload_file() (and audio, and video -- these are all the same) to properly put together a simple hyperlink to then send to the editor.
  • Restores the three filters that I previously needed to remove. It triggers *_send_to_editor_url with audio, video, or file by checking the file extension (file being the fallback).
  • Enables old links to tab=type_url&tab=video (or audio, or file) to still work, by making the audio/video/file form visible and the image form hidden.
  • Only do a check on whether the image URL is valid for images. (This is where the green checkmark comes from. We don't do this for audio/video/file.)
  • Channeled my inner azaozz and changed how toggle is implemented, moving it to a single class on a single parent element, with CSS to then handle on/off.
  • Killed a filter, 'type_url_form_$type', which filtered the entirety of the URL form previously. This is silly. While we can now predict what the type will be (it'll always be image, unless someone does a custom URL, as stated above), the form structure has changed as elements have been added. In return, I've introduced type_url_form_media, a generic filter people can now use.
  • Restores the async_upload_* hooks as they're still needed for async-upload.php (though I've put in a Q to azaozz as to whether we need to keep that file around).

At this point, it's been pretty well hammered by both myself and ryan.

nacin3 years ago

comment:15 nacin3 years ago

Only things left really are to study the restructuring of functions.

  • A number of functions have been removed. These can become deprecated wrappers.
  • The remaining functions, like type_url_form_image() and media_upload_image(), should probably be renamed, and turned into deprecated wrappers.
  • We should grep through the plugins directory and see how plugins are using the various functions and filters at play here.

Also, Ryan suggested we refactor mu_media_buttons into a single checkbox for enabling/disabling uploads, as that's what people use it for. (Even thought it doesn't do that currently, and never blocked anything.) Seems fine to me, though we'll need to probably hook in a bit deeper in spots (like actually restricting things from being uploaded).

comment:16 nacin3 years ago

The more I look, the more completely dead code I find in wp-admin/includes/media.php. Some of it rendered useless by Plupload, others over the years.

media_upload_image(), in the patch, currently handles all "Insert into Post" clicks for video/audio/files and images. Only, the image handling code is dead. It's unreachable. The form will never submit data back for images. It does it via JS. The image_send_to_editor_url hook is thus completely dead.

I found this while doing a full audit of all hooks, using the 'all' hook. And I did find we're in good shape here.

Doing a commit on this, minus the changes for mu_media_buttons (other than that they won't hide buttons). Not dealing with function renaming or deprecation yet. Will follow up with any breakage in the morning. Have yet to grep the plugins repo -- a new slurp is currently in progress. Doing the commit mainly so Jane can have it during testing.

comment:17 nacin3 years ago

In [18831]:

Merge the four media buttons into one.

  • Lots of red.
  • Removes the type_url_form_\$type filter, where \$type is audio, video, file, or image.
  • Replacement filter is a generic type_url_form_media filter for the new unified form.
  • Some functions have gone missing; they'll be restored and deprecated in a later pass.

see #17578.

comment:18 nacin3 years ago

In [18832]:

Compress and bump media.css. see #17578.

azaozz3 years ago

comment:19 azaozz3 years ago

auto-detect-img.patch changes the JS so it shows the image options when the user inserts an URL pointing to image. It removes the radios for selecting type.

comment:20 follow-up: jane3 years ago

I thought we were going to use the Media icon from the left nav menu: camera + musical note. Using the video icon makes it more confusing. Can that be replaced, please?

comment:21 in reply to: ↑ 20 nacin3 years ago

Replying to jane:

I thought we were going to use the Media icon from the left nav menu: camera + musical note. Using the video icon makes it more confusing. Can that be replaced, please?

Yep, will handle it on the next pass.

comment:22 ryan3 years ago

  • Type changed from defect (bug) to task (blessed)

comment:23 jane3 years ago

Let's get this icon swapped out so we can close this ticket before calling beta 1.

comment:24 nacin3 years ago

In [18890]:

UI touches for the single media button. see #17578.

comment:25 nacin3 years ago

In [18892]:

Kill the multisite mu_media_buttons option. see #17578.

comment:26 nacin3 years ago

In [18893]:

Oops. see #17578.

comment:27 nacin3 years ago

In [18894]:

type_url_form_image() becomes wp_media_insert_url_form(), media_upload_image() becomes wp_media_upload_handler(). type_url_form_* and media_upload_* are restored as wrappers. Don't deprecate yet. see #17578.

comment:28 jane3 years ago

Looking good. Can we drop the "or" to be small and on line with Select Files button, and put cancel a p space below the select?

ocean903 years ago

comment:29 follow-up: ocean903 years ago

Can we drop the "or" to be small and on line with Select Files button, and put cancel a p space below the select?

Done in 17578.break-line.patch. See http://cl.ly/Ajg7. But I think this could be difficult to translate for some languages.

comment:30 in reply to: ↑ 29 SergeyBiryukov3 years ago

Replying to ocean90:

But I think this could be difficult to translate for some languages.

With a placeholder in the string (17578.break-line.2.patch), should be fine, I guess.

comment:31 follow-up: ocean903 years ago

should be fine, I guess.

Should be, but breaks if there is a language which needs or on the first place: {or} {option1}, {option2}.
Is there a language like this? I don't know. We can add a second placeholder, but then translators can/will change the order of the items. Is that okay?

comment:32 SergeyBiryukov3 years ago

Ah, I see your point now. They'd probably have to work around this in some way.

"Add New" isn't very flexible in that sense too: comment:ticket:16406:20.

comment:33 azaozz3 years ago

In [18918]:

Fix prepending $editor_id to the Add Media link, see #17578

comment:34 azaozz3 years ago

We should deprecate _media_button($title, $icon, $type, $id) as it's not used any more. Or should we just delete it as a punishment to plugins that use "private" functions? (joking)

comment:35 nacin3 years ago

I hadn't yet deprecated anything since I was waiting to un-deprecate the stuff in #18785 and didn't want a straight merge -c to fail. I'll sweep through, there are a number of remaining tweaks I'd like to make.

Thanks for fixing the $editor_id. Thought I committed that.

comment:36 in reply to: ↑ 31 nacin3 years ago

Replying to ocean90:

Is there a language like this? I don't know. We can add a second placeholder, but then translators can/will change the order of the items. Is that okay?

We can just do %s or %s. That forces the upload to always come second and I think it always works, even RTL.

comment:37 follow-up: nacin2 years ago

Can someone please spin off the patches for the upload text into another ticket?

comment:38 nacin2 years ago

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

In [18992]:

Fix media/audio/file URL insertion. Strip slashes off title text. Leave red X to images only. fixes #14996. fixes #17578.

comment:39 in reply to: ↑ 37 SergeyBiryukov2 years ago

Replying to nacin:

Can someone please spin off the patches for the upload text into another ticket?

#18994

comment:40 nacin2 years ago

In [19046]:

Deprecate media_upload_(image|audio|video|file)(), type_url_form_(image|audio|video|file)(). These now wrap wp_media_upload_handler() and wp_media_insert_url_form(). see #17578.

Note: See TracTickets for help on using tickets.