Make WordPress Core

Opened 11 years ago

Closed 11 years ago

#24753 closed defect (bug) (fixed)

Create Gallery Page Shows Wrong Selection

Reported by: miqrogroove's profile miqrogroove Owned by: markjaquith's profile markjaquith
Milestone: 3.6 Priority: normal
Severity: normal Version: 3.6
Component: Media Keywords: has-patch needs-testing
Focuses: Cc:


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.

Attachments (3)

24753.diff (2.6 KB) - added by garyc40 11 years ago.
24753.2.diff (2.8 KB) - added by garyc40 11 years ago.
properly update model if there's any change
24753.3.diff (1.2 KB) - added by garyc40 11 years ago.
This is to be patched against latest trunk

Download all attachments as: .zip

Change History (11)

#1 @garyc40
11 years ago

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:

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

Last edited 11 years ago by garyc40 (previous) (diff)

11 years ago

#2 @garyc40
11 years ago

  • Keywords has-patch needs-testing added

#3 @nacin
11 years ago

  • Milestone changed from Awaiting Review to 3.6

#4 @nacin
11 years ago

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

11 years ago

properly update model if there's any change

#5 @markjaquith
11 years ago

  • Owner set to markjaquith
  • Resolution set to fixed
  • Status changed from new to closed

In 24771:

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

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

#6 @koopersmith
11 years ago

Looks sane, good job.

#7 @garyc40
11 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

11 years ago

This is to be patched against latest trunk

#8 @markjaquith
11 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 24800:

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

Props garyc40. Fixes #24753.

Note: See TracTickets for help on using tickets.