The new file edit modal is great!

Would it be possible to have an option that would make it open automatically after you have uploaded a file.

So in cases where you always have additional fields on the file entity that need filling in, the user doesn't have to click the edit button after uploading.

This would be great for the user experience.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

becw’s picture

Assigned: Unassigned » becw
Status: Active » Needs review
FileSize
3.42 KB

Here's a patch. Dave, can we review this in the sprint tomorrow?

becw’s picture

gmclelland’s picture

@becw - your patch isn't applying with the latest media 2.x-dev release and drush make. Can you reroll the patch?

becw’s picture

@gmclelland hmm, it applies cleanly with git apply 1426730-1-media-edit_after_upload_in_browser.patch and with patch -p1 < 1426730-1-media-edit_after_upload_in_browser.patch. Is your makefile block like one of these?

; Plain download
projects[media][version] = "2.x-dev"
projects[media][patch][] = "http://drupal.org/files/1426730-1-media-edit_after_upload_in_browser.patch
; Git repository
projects[media][type] = "module"
projects[media][download][type] = "git"
projects[media][download][branch] = "7.x-2.x"
projects[media][download][url] = "http://git.drupal.org/project/media.git"
projects[media][patch][] = "http://drupal.org/files/1426730-1-media-edit_after_upload_in_browser.patch"
rooby’s picture

when you run drush make you could also use -vd options and hopefuly it will tell you why it is failing.

gmclelland’s picture

Sorry @becw - your patch does apply. It was conflicting with another patch.

gmclelland’s picture

After a quick test, I get the following warning after uploading a file.

Warning: filesize(): stat failed for temporary://dsc_0048.jpg in file_entity_image_dimensions() (line 160 of /home/quickstart/websites/updater.dev/profiles/cmf/modules/contrib/file_entity/file_entity.file.inc).

The file dsc_0048.jpg was uploaded

Everything still seems to work fine. After the file is uploaded, I can immediately edit the name and an added caption text field.

I'm running with the latest dev version of File Entity and Media 2.x.

gmclelland’s picture

@becw

That error message is related to this commit of the file entity module http://drupalcode.org/project/file_entity.git/commit/d8975331aa752a229e4... from #1447790: image_load makes admin/content/media unusably slow.

If I back up commit earlier, I no longer see the warning and everything works as intended with the patch.

So, I'm not sure if this patch needs to be reworked to accommodate the changes in the latest commit of the file entity module or if we need to open a new issue with the file entity module?

Hope that helps,
-Glenn

swentel’s picture

FileSize
3.58 KB

I was getting also troubles when trying to apply the patch (but maybe because of some other commits that occured since march 22, haven't checked).

Did a reroll. I think the edit entity form could use some padding still.

A related question maybe, but somehow the view library denies clicking on a link - I was just trying to add an edit link underneath a file. With this patch, people would be allowed to browse the library and edit from there as well if they wanted.

gmclelland’s picture

Thanks @swentel - I tested the file uploading and immediate editing portion of your patch and everything seems to work as you said. I didn't test the second part of the patch for the edit links.

The errors I was getting in #7 were just fixed in the file_entity module with this patch http://drupal.org/node/1533576#comment-5882834.

Hope that helps

gooddesignusa’s picture

Thanks @swentel this patch works for me. A couple thing I noticed
* the modal that renders automatically after you upload a file is different than when you click on the edit media button. Not a big deal. Seems like when you click edit it uses ctools.
* the modal that renders after uploading a file is kinda small and causes a scrollbar. Its using the same size as the upload modal was. I looked at the code and it looks like it resizes the iframe. I'm not sure why it isn't working - if i add #media-browser-page-wrapper div.messages.status{display:none;} to my styles the scroll bar goes away.
* Once the modal switches to the edit screen console throws a js error but still works. Uncaught TypeError: Cannot read property 'browser' of undefined in media.popups.js:103

gmclelland’s picture

related discussions #1553114: Adding files should be a multi-step process over in the file entity issue queue.

Dave Reid’s picture

Doesn't looks like this currently checks if the user has permission to edit files? In which we shouldn't open the file edit modal?

steinmb’s picture

gmclelland’s picture

#9: 1426730-9.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1426730-9.patch, failed testing.

gmclelland’s picture

Status: Needs work » Needs review

The patch in #9 no longer applies against the latest dev versions of media and file_entity modules.

redndahead’s picture

FileSize
3.45 KB

Let's try this one.

fangel’s picture

In the media_form_alter, the if-statement that control whether or not set the new submit-handler on the file_entity_edit-form looks like this:

  elseif ($form_id == 'file_entity_edit' && !empty($params['render']) && $params['render'] == 'media-popup') {

However, $params isn't defined anywhere in the functions scope, so I don't see how this could ever be triggered. I changed all $params into $_GET and it worked as expected.

Otherwise, very nice patch.

redndahead’s picture

FileSize
3.72 KB

This one fixes the issue found in #19 and provides user access support as requested in #13.

redndahead’s picture

FileSize
3.68 KB

Another one using media_access() instead of user_access() per davereid

gmclelland’s picture

The patch in #18 didn't work, but the patch in #21 DID work. I didn't test the access calls. I just tested that it would automatically open the file edit modal when you upload a file.

Thank you @redndahead

redndahead’s picture

Status: Needs review » Reviewed & tested by the community

So after reviewing the access control really doesn't make sense quite yet. I think we can leave it as is for now, but once the access api goes in we may have to revisit it. Since @gmclelland tested the patch and it works and access isn't an issue I'm marking this as rtbc

Dave Reid’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/media.moduleundefined
@@ -562,19 +562,48 @@ function media_form_alter(&$form, &$form_state, $form_id) {
+  // Expose file edit form after selecting files from the media browser.
+  // @see media_browser_form_submit(), media_browser_edit_form_submit()
+  $params = drupal_get_query_parameters();
   if (!empty($form_state['#media_browser'])) {
     $form['#submit'][] = 'media_browser_form_submit';
   }
+  elseif ($form_id == 'file_entity_edit' && !empty($params['render']) && $params['render'] == 'media-popup') {
+    $form['#attached']['js'][] = drupal_get_path('module', 'media') . '/js/media.browser.edit.js';
+    $form['actions']['cancel']['#access'] = FALSE;
+    $form['actions']['delete']['#access'] = FALSE;
+    $form['actions']['submit']['#submit'][] = 'media_browser_edit_form_submit';
+    $form['#submit'][] = 'media_browser_edit_form_submit';

I think we should move this logic to media_form_file_entity_edit_alter() instead so we're not calling drupal_get_query_parameters() on every single form ever?

+++ b/media.moduleundefined
@@ -562,19 +562,48 @@ function media_form_alter(&$form, &$form_state, $form_id) {
+    if (media_access('edit')) {
+      $form_state['redirect'] = array("file/{$file->fid}/edit", array('query' => array('render' => 'media-popup')));
+    }
+    else {

What happens if we add 'destination' => 'media/browser?render=media-popup&fid=' . $file->fid to 'query' in this redirect? Do we even need the second redirect form logic then?

+++ b/media.moduleundefined
@@ -562,19 +562,48 @@ function media_form_alter(&$form, &$form_state, $form_id) {
-  if (!empty($form_state['files'])) {
-    $files = $form_state['files'];
-    $form_state['redirect'] = array('media/browser', array('query' => array('render' => 'media-popup', 'fid' => array_keys($files))));

Why is this being removed? Can we leave it in for now or does it cause problems?

Dave Reid’s picture

When I have the patch applied, after I submit the upload form, I see the modal window changes pages, but then it shrinks down to a 20px high dialog and I cannot see or do anything with the modal.

Dave Reid’s picture

This seems to work just fine when used with the WYSIWYG button, but not when I am selecting a media file for a normal file/image field. The JS errors I get are:

Uncaught Error: can't load XRegExp twice in the same frame
Uncaught TypeError: Cannot read property 'browser' of undefined in media.popups.js:103

Dave Reid’s picture

Revised patch with the changes, but still encountering the JS error.

fangel’s picture

I'll take a stab at trying to get the above mentioned error fixed, and review the patch today.

If it doesn't already do so, I'll try to get multi-select/upload working too, as I need that for an upcoming project... But that might belong to a separate issue :)

fangel’s picture

This error Uncaught TypeError: Cannot read property 'browser' of undefined in media.popups.js:103 is easily solvable.

It's because Drupal.media.popups.mediaBrowser.mediaBrowserOnLoad checks to see if this.contentWindow.Drupal.media.browser.selectedMedia contains anything. If it does, it should auto-"submit" the dialog, by "clicking" the OK, button.
However, on the edit-page, Drupal.media isn't set, because it's only set on 'media/browser' by the MediaBrowser plugin.

So the onload-function just needs to check if Drupal.media exists before trying to read properties from it. The patch is trivial, so I'll continue with a few other clean-ups/todos before updating this issue with a new patch.

Haven't been able to recreate the Uncaught Error: can't load XRegExp twice in the same frame error in Safari - which browser is that in?

fangel’s picture

Okay, here's the patch that solves the above mentioned JS bug.

I've also added support for multiselect upload-edits (that is, uses plupload to upload multiple files, then ask you to edit all of then, then returns). Note that this requires plupload and multiform.
I've tested this in the browser-testbed, and appears to work fairly solidly.

mrfelton’s picture

Status: Needs review » Needs work

Tried with patch in #30, and I'm still experiencing the problem with the window shrinking down to 20px or so in height after seleting and confirming the file. This makes it impossible to finalise the added media. Looks like the some fields show for a split second before the modal being reduced in height and breaking.

I'm trying to attach a pdf file to a file field with the media file selector widget.

I have a js error in the console:

Uncaught TypeError: Cannot read property 'browser' of undefined media.popups.js:103

fangel’s picture

mrfelton: I'll try to take a look at it tomorrow. What browser are you using when you experience this?

mrfelton’s picture

@fangel Chrome, OSX. Although I tried with Firefox too and had the same problem.

fangel’s picture

mrfelton: Could you verify that you have the latest edition of the patch, and that you have refreshed the JS in your browsers cache? I have my line 103 as

if (this.contentWindow.Drupal.media == undefined) return;

which is specifically there to prevent the error you're mentioning above. (Note, I had a lot of trouble getting my browser to properly refresh the JS files, it kept reading the old from it's cache, even after reloading. I had to manually visit the JS file directly in the browser and hit refresh there)

--

I believe the problem with not resizing to the correct size, is caused by the JS error, halting JS execution, so you never get to the JS that resizes the popup.

mrfelton’s picture

Status: Needs work » Needs review

May have been a caching issues after all. Patch in #30 is now eorking. Nice one fangel!

redndahead’s picture

Status: Needs review » Reviewed & tested by the community

I think this is ready to go. @davereid is your issue fixed with this?

vood002’s picture

Tested and working great for me, thank you!

fangel’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
7.3 KB

Okay, I was wrong about my assumption about what could force the dialog to resize to a incorrect size.

It was a incorrect way of figuring out how much "air" was above the dialog, that didn't take page-scroll into account, so if you were scrolled down on the page when you launched the Media browser, it would resize incorrectly if it didn't fit in the size dictated by Media.

Here's an updated patch. Only changes made is the re-ordering of the setting of padding, and the resize routine (to help get the size closer to the real size) and then the crucial addition of $(top).scrollTop()

Marking as 'needs review' again, so please test out the new changes and mark it as RTBC if it works for you.

Dave Reid’s picture

Issue tags: +sprint
lsolesen’s picture

Status: Needs review » Needs work
error: patch failed: js/media.popups.js:100
error: js/media.popups.js: patch does not apply
redndahead’s picture

Status: Needs work » Needs review
FileSize
6.13 KB

Rerolled

lsolesen’s picture

FileSize
148.71 KB

I think the behavior is a bit strange after applying the patch (see attached screenshot). Why does that page need to show (and it is strange that I already get a chance to replace the file just uploaded).

fangel’s picture

Hi Lars: The page need to change into the edit-page - it just shows you file/:fid/edit. And the standard behaviour isn't to have a replace file-field, that must be another contrib module you have installed that enables that functionality.

I've attached a screenshot of a vanilla Drupal 7.14 with latest git checkout of Media 7.x-2.x and the patch applied (I've also rerolled my patch against latest git, just in case)

gmclelland’s picture

@lsolesen Haven't tested yet, but the recent commit on the File Entity module #1123570: Replace file while retaining field metadata might be showing you the file replace stuff.

steinmb’s picture

FileSize
34.78 KB

Env

PHP 5.3.6-13ubuntu3.8 with Suhosin-Patch
Drupal 7.14

Applying against latest 2.x-dev:

Checking patch js/media.browser.edit.js...
Checking patch js/media.format_form.js...
Checking patch js/media.popups.js...
Checking patch js/plugins/media.views.js...
Checking patch media.module...
Hunk #1 succeeded at 440 (offset 9 lines).
Hunk #2 succeeded at 472 (offset 9 lines).
Hunk #3 succeeded at 537 (offset 9 lines).
Hunk #4 succeeded at 548 (offset 9 lines).
Hunk #5 succeeded at 577 (offset 9 lines).
Applied patch js/media.browser.edit.js cleanly.
Applied patch js/media.format_form.js cleanly.
Applied patch js/media.popups.js cleanly.
Applied patch js/plugins/media.views.js cleanly.
Applied patch media.module cleanly.

Attached screen show of modal after I submit upload image and it this work's OK. What not is working just right is if you now click 'Replace file' and upload another file. This will close the modal and you are not able to change the media title without reopening the media file.

fangel’s picture

steinmb: So what you're saying is that you want the 'edit'-dialog to keep popping up, if you keep replacing the file? Like Upload -> Edit -> Replace -> Edit -> Replace -> Edit -> [repeat] -> OK?

Why would you want to replace a file you just uploaded? Wouldn't it make more sense to not have the option to replace files that was just created?

steinmb’s picture

FileSize
38.71 KB

I'm just pointing out inconsistency in the user experience. If we want to keep the replace function, I do think we have to loop back and repeat, though a much better idea, as you pointed out, is simply to remove the replace option as it does not make any sense to me either.
media file replace

fangel’s picture

I agree, but that is an issue to bring up with the File Entity module, as it's them that introduced it (in #1123570: Replace file while retaining field metadata).

Dave Reid’s picture

I think it would be ok to hide the replace file field in this context. It doesn't make sense to show it in this context and it's causing problems (although it does if you're using the 'Edit media' button).

redndahead’s picture

FileSize
7.39 KB

This one removes the replace field in the modal.

steinmb’s picture

The patch remove the field though I have not reviewed the code but to me is it RTBC.
modal replace removed

I also played with 'Edit media' button and found another problem. It might be unrelated to this issue but I should still mention it:

If you click 'Edit Media' and choose to replace media file or/and media name, press save. The modal is then closed but the form is not updated to show the new image/name.
modal problem

If I preview the node or save it I then get the drupal_set_message() 'Image my_image_name has been updated.' and the thumb and name is updated.

fangel’s picture

Yes, the edit-button is unrelated to this issue, as it only deals with being able to edit just uploaded files – but you have a very valid point. It is a weird behaviour.

redndahead’s picture

Status: Needs review » Reviewed & tested by the community

Pushing this to RTBC then

Jackinloadup’s picture

Lets open another issue for the problem stated in #54. It would be nice to have that solved eventually but probably shouldn't hold back this from reaching out end users.

Pomliane’s picture

#50 works as expected. Nice enhancement!

becw’s picture

Status: Reviewed & tested by the community » Needs work

@davereid re: #49, I don't think it makes sense to remove the "replace file" field as a part of this patch.

becw’s picture

Assigned: becw » Unassigned
Status: Needs work » Reviewed & tested by the community
FileSize
6.12 KB

If we want to hide the "replace file" field, we should discuss it in a separate issue.

The patch from #43 is RTBC; here's a re-rolled version so it applies without fuzz.

Dave Reid’s picture

Status: Reviewed & tested by the community » Fixed

Committed #60 to Git (with the missing js file from the patch as well): http://drupalcode.org/project/media.git/commit/1a0304d

Let's file followup issues as new issues.

klonos’s picture

Have different issues been filed for removing the "replace file" and the issue mentioned in #54? Are we following up on these?

bbdata’s picture

Out of curiosity, if I prefer not having the modal popup after an image is uploaded. What is the best way to disable it?

RobW’s picture

Good point. I'd like to see the modal on by default, with some smart logic for popping up (e.g., don't pop up if there are no fields), and a configuration checkbox per file field to enable/disable.

Jackinloadup’s picture

@@ -100,6 +100,8 @@ Drupal.media.popups.mediaBrowser = function (onSelect, globalOptions, pluginOpti
diff --git a/js/plugins/media.views.js b/js/plugins/media.views.js
index aec2323..95b673b 100644
--- a/js/plugins/media.views.js
+++ b/js/plugins/media.views.jsundefined
@@ -32,6 +32,12 @@ Drupal.behaviors.mediaViews = {
       for (index in Drupal.settings.media.files) {
         if (Drupal.settings.media.files[index].fid == fid) {
           selectedFiles.push(Drupal.settings.media.files[index]);
+
+          // If multiple tabs contains the same file, it will be present in the
+          // files-array multiple times, so we break out early so we don't have
+          // it in the selectedFiles array multiple times.
+          // This would interfer with multi-selection, so...
+          break;
         }
       }

Can someone tell me how this works? I don't understand how this break is accomplishing the task the comment states.

I ran into this while trying to re-roll #1658452: Regression: Views Library doesn't support mutliselect, 1.x library did

fangel’s picture

Jackinloadup: The reason it's there is because Drupal.settings.media.files sometimes contain the file-entity multiple times (if the file is present in more than one tab), so the array can look like this

[
  {fid: 1, filename: ....},
  {fid: 2, filename: ....},
  {fid: 1, filename: ....}
]

So therefore, we break out of the loop going through the array as soon as we've found the first instance of the file. Otherwise we would add all instances of the file in the Drupal.settings.media.files to selectedFiles, which - if you have multi-selection enabled - ends up selecting the same file multiple times.

Was that a good enough explanation?

Jackinloadup’s picture

Yeah, it took a new day and that great explanation to realize where my thought process was wrong.

Goodbye and thanks for all the fish ^_^

fish = hard work

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

David_Rothstein’s picture

Out of curiosity, if I prefer not having the modal popup after an image is uploaded. What is the best way to disable it?

Not clear there's a way yet, but looks like an issue for that exists at #1823622: Provide a way to suppress the edit screen after file upload.

Elijah Lynn’s picture

Status: Closed (fixed) » Active

Just upgraded from 7.x-2.x-unstable7+6 to unstable7+38 (latest dev) and it appears this has been reverted/removed since then. When I upload a file it does not present a modal to enter additional information. It does work when I click the 'edit media' button though.

ericduran’s picture

Status: Active » Closed (fixed)

Issue is closed. No re-opening :-p

Olivier Beerens’s picture

Status: Closed (fixed) » Active

I have the same has Elijah Lynn, i can't make it working, either on unstable 7 or last dev...

ericduran’s picture

Status: Active » Closed (fixed)

I tested this on the latest dev version of media and file_entity and it seems to be working.