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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

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

fangel’s picture

fangel’s picture

Status: Needs work » Needs review
ParisLiakos’s picture

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?

fangel’s picture

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..

fangel’s picture

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

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

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

xcafebabe’s picture

Patch #6 works as expected! Kudos to @fangel

ParisLiakos’s picture

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

fangel’s picture

Status: Needs work » Needs review
FileSize
3.62 KB

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

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

ParisLiakos’s picture

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

ParisLiakos’s picture

Status: Needs review » Fixed
fangel’s picture

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.