[wp-trac] [WordPress Trac] #24753: Create Gallery Page Shows Wrong Selection

[WordPress Trac]

#24753: Create Gallery Page Shows Wrong Selection
--------------------------+------------------------------
 Reporter:  miqrogroove   |       Owner:
     Type:  defect (bug)  |      Status:  new
 Priority:  normal        |   Milestone:  Awaiting Review
Component:  Media         |     Version:  trunk
 Severity:  normal        |  Resolution:
 Keywords:                |
--------------------------+------------------------------

Comment (by 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 new models will cause Backbone.Collection to run those models
 through `Collection._prepareModel()`, which will:

 - if the model 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 already fetching a complete model out
 of the `Attachments.all` cache. 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! Removing that creates this
 regression where each view has their own attachments with the same
 attachment IDs but different `cid`, which causes 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).

 It 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 Model objects as JSON 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.

--
Ticket URL: <http://core.trac.wordpress.org/ticket/24753#comment:1>
WordPress Trac <http://core.trac.wordpress.org/>
WordPress blogging software