Create Gallery Page Shows Wrong Selection

[miqrogroove]

In 3.6 RC1:

1. Click the Add Media button on the dashboard.
2. Click one of the images on the Insert Media modal page.
3. Click "Create Gallery".
4. Click another image.

Expected behavior:

The two images in the selection should appear to be selected

Actual behavior:

There are two images in the selection at the bottom of the window, but only one image appears to be selected.

When clicking the other image, nothing happens. Only the images other than the one originally selected can be added to or removed from the gallery.

If you click the "Create a new gallery" button and then click "Add to Gallery", that first-selected image appears as an available image, as though it hasn't been added to the gallery already.

[garyc40]

I can confirm that this is a regression, introduced by [24110].

[24110] was not an appropriate fix for #24094.

The rationale behind removing parse() for Attachments collection was incorrect (I'm quoting):

[In Backbone 1.0.0] After fetching a model or a collection, all defined parse functions will now be run. So fetching a collection and getting back new models could cause both the collection to parse the list, and then each model to be parsed in turn, if you have both functions defined.

Getting fetch responses will cause Backbone.Collection to run the responses through Collection._prepareModel(), which will:

• if the response is already a model, leave it be
• if it is not, create a new model out of it, which in turn, will call Model.parse().

So in Attachments.parse(), we're getting a Model object out of the response by fetching a complete model out of the Attachments.all cache (by calling Attachment.get()). So repeated calls to Attachments.parse() was not caused here.

The reason why Attachments.parse() was there was to prevent new Model objects from being created unnecessarily! Removing that creates this regression where each view has their own attachment models pointing to the same attachment in the database (same post_IDs but different cid), which causes this unexpected UI behavior (because throughout media-views.js, we're comparing models between Views and Selection using ===, which implies the cid must match as well).

The original issue with blank item in #24094 was caused by these changes in Backbone JS 1.0.0:

• bindSyncEvents() are no longer necessary because from 1.0.0, sync will be triggered automatically after fetch success, and error will be triggered automatically after fetch error. See this diff:

​https://github.com/jashkenas/backbone/compare/0.9.10...1.0.0#L6R436

• Collection.add() is now a wrapper for Collection.set(), which basically replaces Collection.update() in 0.9.10 as well. The difference is, Attachments.parse() should expect to be passed in these kinds of value, besides an array of non-Model JSON objects:
  1. An array containing a mix of Models as well as plain attribute objects.
  2. A single Model (not an array).

The original bug in #24094 was caused because Attachments.parse() did not take into account the fact that it can be passed a single Model (not an array), so the _.map() call inside it will pick apart that single Model's attributes and cause an undefined variable to be added to the collection (which explains for the blank second item in the Media grid).

Removing Attachments.parse() in [24110], in turn, caused this regression, where Attachments.all is now basically useless. Each view will now has duplicates of the same Model, but with different cid. As a result, you can't deselect / select an item that has already been selected in a different view.

The solution for this would be (at least to preserve the original behavior before [24110]:

• Remove bindSyncEvents() as this is not necessary any more.
• Restore the Attachments.parse() function, but this time makes sure it accounts for the single Model argument.

I'm sorry if this comment sounds cryptic, but it is indeed a very complicated issue. I'm sure Koopersmith will understand what I mean though, and will be able to provide a much easier to understand explanation :)

Working on a patch for this.

[nacin]

Not cryptic at all — your explanation and patch makes sense to me.

[garyc40]

properly update model if there's any change

[markjaquith]

In 24771:

Fix a media regression in [24110] that could cause duplicate models.

Props garyc40. Fixes #24753. See #24094, #23830.

[koopersmith]

Looks sane, good job.

[garyc40]

Just found out that the patch I submitted introduced a few sub-optimal operations. This is just a really minor issue.

Basically attachment.get() is called all the time (because attrs needs to be parsed before comparison with attachment.attributes). Apologies!

Attached is a fix for this. Hope it's not too late.

[garyc40]

This is to be patched against latest trunk

[markjaquith]

In 24800:

Parse attrs before comparing to attachment.attributes. Small efficiency gain.

Props garyc40. Fixes #24753.