Hi,

I've created the "Media Multiselect" module (http://drupal.org/sandbox/fangel/1652676), and got a report that the multiselection-feature broke on page 2 of the browser (#1815812: multiselect doesn't work after using pagination).

So I went to investigate, and it wasn't just multiselection that broke on page 2 - all Media Browser parameters (multiselect, types, etc) gets reset on pagination. This is because the Media Browser parameters is only ever detected in the menu-callback media_browser(). On AJAX-requests, this menu-callback is never called, so the requests are never parsed and stored, and hence it will send along an empty list of parameters.

So I've created a patch, that moves the parsing of parameters out of the menu-callback, and into media_set_browser_params.

I've also done a slight alteration to the browser-js so it deselects any files selected on previous pages.

So please review the patch, so we can get the AJAX-pagination working properly with non-standard browser parameters.

Kind regards
Morten.

Files: 
CommentFileSizeAuthor
#10 1848506-media_browser_resets_parameters_after_ajax-10.patch3.62 KBfangel
PASSED: [[SimpleTest]]: [MySQL] 5 pass(es).
[ View ]
#6 1848506-media_browser_resets_parameters_after_ajax-6.patch3.67 KBfangel
PASSED: [[SimpleTest]]: [MySQL] 5 pass(es).
[ View ]
#2 1848506-media_browser_resets_parameters_after_ajax-2.patch3.42 KBfangel
PASSED: [[SimpleTest]]: [MySQL] 5 pass(es).
[ View ]
media-multiselect-pagination.patch2.99 KBfangel
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch media-multiselect-pagination.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Status:Needs review» Needs work

The last submitted patch, media-multiselect-pagination.patch, failed testing.

StatusFileSize
new3.42 KB
PASSED: [[SimpleTest]]: [MySQL] 5 pass(es).
[ View ]

Rerolled patch.

Status:Needs work» Needs review

Everything looks good from code perspective..just this really small one.

Drupal.media.browser.views.setup = function(view) {
+  // Ensure we only setup each view once..
+  if ($(view).hasClass('media-browser-views-processed')) return;
+  $(view).addClass('media-browser-views-processed');
+

is there a reason we add the processed class there and not in the end of the function, when it is actually processed?

No, that might make sense - I sometimes like to group the two lines together for ease of reading, but I guess it should belong at the bottom. I'll revise the patch later today, or tomorrow..

StatusFileSize
new3.67 KB
PASSED: [[SimpleTest]]: [MySQL] 5 pass(es).
[ View ]

Okay, patch updated - the addClass call is moved to the bottom now.

Status:Needs review» Reviewed & tested by the community

Thanks..I will check its functionality once more and then commit it

Patch #6 works as expected! Kudos to @fangel

Status:Reviewed & tested by the community» Needs work

just a question..why we need this?
+     $params = media_set_browser_params($params);
inside the very same function?
I guess it is a copy-paste failure since $params parameter is removed now.

Also i am trying this with unstable7 and your latest code from your sandbox and it is not working..debugging further

Status:Needs work» Needs review
StatusFileSize
new3.62 KB
PASSED: [[SimpleTest]]: [MySQL] 5 pass(es).
[ View ]

Ahh, yes - correct. It is unneeded. I removed it.

I'm running git versions of Media, File Entity and my sandbox, fwiw.

ah ok..i was actually expecting it to remember the selections i made in page 1 when i was in page 2..
So if i select 2 items from page 1 and then 2 items from page 2 all 4 items are being added when i hit submit button.
But thats not the case and i guess a different issue..
thanks

Status:Needs review» Fixed

rootatwc: Yes, because there is currently no GUI to display items selected on the previous page, I made the patch so that it clears any selection when you go to a new page.
This makes it consistent with switching library tabs (if you select something in library and in my files, both selections are not used)

Thanks for committing..

Status:Fixed» Closed (fixed)

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