WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#22743 closed defect (bug) (fixed)

Add a link to the image editor in the media modal

Reported by: nacin Owned by: markjaquith
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.5
Component: Media Keywords:
Focuses: Cc:

Description

I'm a bit concerned about the potential backlash we might get for removing "Edit Image" from the media dialog from 3.4 to 3.5. The WordPress.com forums are providing brutal feedback for it. We don't really know how many installs have GD on them to the point where they would miss it — could be 10%, 50%, or 90% for all I know. None of those numbers are good, though.

I think we should probably set an early 3.6 goal (after a few weeks of rest) of bringing image editing to the modal. Obviously, that got punted from 3.5 scope early on (if it was ever truly a part of scope beyond a "nice if we get to it").

In the meantime though, we should implement a stopgap. Here's a plan of action:

  • Add an "Edit" link next to "Delete Permanently", with target=_blank. If they can edit the attachment, of course.
  • Add a query running on a timeout to see if any attachments have been modified since X.
  • Re-load those attachments, which should refresh any changes.

It's not perfect. It's subject to the general "race" condition of doing a last-modified on a timeout. (We could speed up that timeout after an "Edit" click, but still.) It is also subject to a deeper race condition where we do this:

$id = wp_insert_attachment($attachment, $file, $post_id);
if ( !is_wp_error($id) ) {
	wp_update_attachment_metadata( $id, wp_generate_attachment_metadata( $id, $file ) );
}

Aside from the fact that IIRC, wp_insert_attachment() can't actually return WP_Error, there is a pretty bad race condition wherein we update the attachment itself (and thus post_modified), and then generate and update the attachment metadata, which includes an image editing multi_resize() call, so it is fairly slow. And, since wp_update_attachment_metadata() only touches metadata, post_modified is not updated a second time after. (We could hack in a second update simply to bump the modified date, though.)

That said, it's fairly easy to do. I'm attaching an admin-ajax endpoint, along with some sample code from Koop that will automatically refresh the attachments (that's the .more() call):

modified = wp.query( args_that_set_modified );
setInterval( function() {
  modified.more();
}, 30000 );

Attachments (8)

22743.diff (1.6 KB) - added by nacin 6 years ago.
22743.2.diff (2.8 KB) - added by nacin 6 years ago.
Strings up an edit link, which needs styling.
22743.3.diff (7.3 KB) - added by nacin 6 years ago.
22743.4.diff (7.4 KB) - added by lessbloat 6 years ago.
22743.4b.diff (7.4 KB) - added by lessbloat 6 years ago.
22743.5.diff (7.0 KB) - added by koopersmith 6 years ago.
22743.6.diff (397 bytes) - added by nacin 6 years ago.
22743.7.diff (1.2 KB) - added by helenyhou 6 years ago.

Download all attachments as: .zip

Change History (15)

@nacin
6 years ago

@nacin
6 years ago

Strings up an edit link, which needs styling.

#1 @Viper007Bond
6 years ago

  • Cc viper007bond added

@nacin
6 years ago

#2 @nacin
6 years ago

22743.3.diff:

  • Adds an Edit Image link next to Delete Permanently, styles both (including RTL), and makes the Delete Permanently link a darker red except on hover (the style used on the media list table and elsewhere for action links, though not "Move to Trash" on post.php).
  • The Edit Image link opens up post.php with the image editor open.
  • Patch includes an admin-ajax handler to handle the last-modified query. This still needs to be strung up with the example code above.

@lessbloat
6 years ago

#3 @lessbloat
6 years ago

Changed a tiny bit of CSS in 22743.4.diff to keep the details block from always being pushed under the preview image. Now it looks like:

http://f.cl.ly/items/1y222p2420241D213g46/image-details-on-right.jpg

@lessbloat
6 years ago

#4 @lessbloat
6 years ago

Adjusted line-height in 22743.4b.diff:

http://f.cl.ly/items/390O09240e1z162E3T1W/details-right-line-height.jpg

#5 @ethitter
6 years ago

  • Cc erick@… added

@koopersmith
6 years ago

@nacin
6 years ago

@helenyhou
6 years ago

#6 @koopersmith
6 years ago

Looks good.

#7 @markjaquith
6 years ago

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

In 23095:

Add the ability to click "Edit" and kick out to the advanced image editor from within the Media modal. New window, with "Refresh" offered on your return. fixes #22743. props koopersmith, nacin, helenyhou.

Note: See TracTickets for help on using tickets.