adding feedback to update event of collection #3711

linus-amg · 2015-07-13T15:08:14Z

collection's update event receives 2 additional attributes, in case of removal of existing models, previousModels is an array containing the removed models which where referenced to the collection before .set was called. The second attribute, addedModels contains an array of models which were added to the collection calling the .set method.

Fixes #3700

collection's update event receives 2 additional attributes, in case of removal of existing models, previosModels is an array containing the removed models which where references to the collection before .set was called. The second attribute, addedModels contains an array of models which were added to the collection calling the .set method. Signed-off-by: elgubenis <elgubenis@gmail.com>

# By Linus-A. M. Gubenis # Via Linus-A. M. Gubenis * 'master' of https://github.com/elgubenis/backbone: Update collection.js

megawac · 2015-07-13T15:12:45Z

backbone.js

      if (!options.silent) {
        var addOpts = at != null ? _.clone(options) : options;
        for (var i = 0; i < toAdd.length; i++) {
          if (at != null) addOpts.index = at + i;
          (model = toAdd[i]).trigger('add', model, this, addOpts);
        }
+        if (toAdd.length || previousModels.length) {
+          if (toAdd.length) options.addedModels = toAdd;
+          if (previousModels.length) options.previousModels = previousModels;


is previousModels the right term or should it be removed models?

i thought it should be removedModels, but i saw that .reset returns previousModels aswell, so i think keeping both the same would not be wrong.

Mind calling these options.add and options.remove? It mirrors the add and remove event names.

I think past tense works better here, added & removed?

I'm pro past tense, but nothing else in Backbone's public API uses it. @jashkenas, @braddunbar, or @akre54 want to weigh in?

options.add and options.remove are already set, because they are the options passed to the .set (like options.merge)

megawac · 2015-07-13T15:13:30Z

👍

linus-amg · 2015-07-13T15:13:38Z

i would also like to include mergedModels but i think that wont be possible (see #3708)

jridgewell · 2015-07-13T15:16:09Z

Mind adding the removed models to the update event in #remove?

linus-amg · 2015-07-13T15:16:33Z

sure

returning removedModels, previousModels and addedModels Signed-off-by: elgubenis <elgubenis@gmail.com>

* develop: update events of remove/set

Signed-off-by: elgubenis <elgubenis@gmail.com>

jridgewell · 2015-07-13T16:08:43Z

Looks good to me, the only thing I want feedback on is the addedModels/removedModels naming.

linus-amg · 2015-07-13T16:12:37Z

yeah, i didnt like it either, but since .reset returns previousModels, i thought they should be named alike, well lets wait for the feedback.

linus-amg · 2015-07-13T16:49:09Z

should i write some tests to see that the events in .remove return the removed models which it should return? as well as the removed/previous and added ones for .set? or is that not necessary

jridgewell · 2015-07-13T16:51:03Z

should i write some tests to see that the events in .remove return the removed models which it should return? as well as the removed/previous and added ones for .set?

👍

these 6 tests check if the update event for the remove and set methods return added and removed models. - remove 1 model returns the 1 model removed - remove multiple models, returns the multiple models removed - setting 1 model, returns the 1 model added - setting multiple models, return the multiple models added - setting 2 models when there was 1 model at first, returns the 1 model removed - setting 1 model when there were 2 models at first, returns the 2 models removed. Signed-off-by: elgubenis <elgubenis@gmail.com>

akre54 · 2015-07-23T04:38:05Z

test/collection.js

+    var collection = new Backbone.Collection([model]);
+    collection.on('update', function(context, options) {
+      if (options.removedModels[0] !== model) {
+        ok(false);


Try just ok(options.removedModels[0] === model) or even better, strictEqual(options.removedModels[0], model) instead of this if block.

like that? !strictEqual(options.removedModels[0], model) i guess?

notStrictEqual()

Signed-off-by: elgubenis <elgubenis@gmail.com>

* origin/develop: close feedback

Signed-off-by: elgubenis <elgubenis@gmail.com>

linus-amg · 2015-08-17T17:53:25Z

sorry, fixed the test a little too late

jridgewell · 2015-08-17T18:09:31Z

LGTM.

Signed-off-by: elgubenis <elgubenis@gmail.com>

jashkenas · 2015-08-18T21:31:14Z

Naming isn't quite there yet — but more importantly without some sort of mergedModels, this change feels incomplete. Having the full bookkeeping of changes to models isn't possible?

jridgewell · 2015-08-18T21:48:27Z

We can add a toMerge here, and push to it here.

jashkenas · 2015-08-18T21:54:57Z

Alright. And there isn't a big perf cost from the extra bookkeeping?

Naming-wise added, removed, merged sounds good. Perhaps even under a sub-object if necessary.

jridgewell · 2015-08-18T22:03:44Z

Alright. And there isn't a big perf cost from the extra bookkeeping?

With everything else this method is doing, I don't image one.

linus-amg · 2015-08-19T13:36:16Z

hi, im working on the PR, how could the sub-object be named? changes, status, updates, move(s)/ments, details, models?

linus-amg · 2015-08-19T13:39:06Z

i will dub it 'changes' for now if thats ok

Signed-off-by: elgubenis <elgubenis@gmail.com>

jridgewell · 2015-08-19T14:27:29Z

@elgubenis: You'll have to rebase off of the current master for this to merge cleanly.

# By Justin Ridgewell (18) and others # Via Graeme Yeates (6) and others * 'master' of https://github.com/jashkenas/backbone: (35 commits) kill old email fix localStorage link include -> contains doc fixes and tightening Add Modernizr style check for 'onhashchange' to avoid issues in IE compat modes. [closes #3730] Draft change log for Backbone 1.3 Test null set doesn't clear model. Use some, includes, map in tests instead of any, include, pluck. Setup DOM for every view test Use `_.bind` instead of `context` param of `_.sortBy`. Add Collection#includes for underscore v1.8 Update index.html: adjust toc_section font fix grammario update docs according to issue #3694 Fix #3724; add qunit dependency Refactor example code in View#events docs style cleanup Add docs for View#events typos Update collection.js ... Signed-off-by: elgubenis <elgubenis@gmail.com> # Conflicts: # backbone.js

added, removed and merged models Signed-off-by: elgubenis <elgubenis@gmail.com>

Signed-off-by: elgubenis <elgubenis@gmail.com>

linus-amg · 2015-08-19T15:20:53Z

will write some tests for the merging

Signed-off-by: elgubenis <elgubenis@gmail.com>

update gets not fired anymore because models are not added anymore, and it does not get merged either. Signed-off-by: elgubenis <elgubenis@gmail.com>

jridgewell · 2015-09-18T16:37:36Z

LGTM.

jridgewell · 2015-10-28T19:14:32Z

@elgubenis: Mind rebasing? I'll merge this afterwards.

mordentware · 2015-11-09T00:17:42Z

Just wanted to add that I was after this exact functionality, cheers guys for implementing. Any chance of getting the conflicts sorted out?

linus-amg · 2015-11-10T16:21:49Z

hi, will do.

akre54 · 2015-12-15T16:49:17Z

backbone.js

@@ -912,7 +917,13 @@
          model.trigger('add', model, this, options);
        }
        if (sort || orderChanged) this.trigger('sort', this, options);
-        if (toAdd.length || toRemove.length) this.trigger('update', this, options);
+        if (toAdd.length || toRemove.length || toMerge.length) {
+          options.changes = {};


minor style nit:

options.changes = { added: toAdd, removed: toRemove, merged: toMerge }

megawac · 2016-01-27T19:35:18Z

@elgubenis mind addressing @akre54 feedback and we'll merge? Thanks

Linus-A. M. Gubenis and others added 3 commits July 13, 2015 09:48

Update collection.js

f55d3b5

Merge branch 'master' of https://github.com/elgubenis/backbone

57db882

# By Linus-A. M. Gubenis # Via Linus-A. M. Gubenis * 'master' of https://github.com/elgubenis/backbone: Update collection.js

megawac reviewed Jul 13, 2015
View reviewed changes

elgubenis added 3 commits July 13, 2015 10:33

update events of remove/set

1dacae8

returning removedModels, previousModels and addedModels Signed-off-by: elgubenis <elgubenis@gmail.com>

Merge branch 'develop'

434fcbb

* develop: update events of remove/set

update typos in collection tests

cea9ed1

Signed-off-by: elgubenis <elgubenis@gmail.com>

jridgewell mentioned this pull request Jul 20, 2015

Bump #3720

Closed

akre54 reviewed Jul 23, 2015
View reviewed changes

elgubenis added 3 commits August 17, 2015 12:46

close feedback

6298647

Signed-off-by: elgubenis <elgubenis@gmail.com>

Merge remote-tracking branch 'origin/develop'

c5db447

* origin/develop: close feedback

#3711 - fix tests

2c3a1e8

Signed-off-by: elgubenis <elgubenis@gmail.com>

elgubenis added 2 commits August 17, 2015 13:17

use strictEqual

41c04c5

Signed-off-by: elgubenis <elgubenis@gmail.com>

use more strictEqual

077d19e

Signed-off-by: elgubenis <elgubenis@gmail.com>

elgubenis added 2 commits August 19, 2015 08:40

collection set, changes: removed, added, merged

108ab3b

Signed-off-by: elgubenis <elgubenis@gmail.com>

collection set, trigger merge

79dae25

Signed-off-by: elgubenis <elgubenis@gmail.com>

elgubenis added 3 commits August 19, 2015 10:04

collection set, update returns feedback

b13581d

added, removed and merged models Signed-off-by: elgubenis <elgubenis@gmail.com>

removedModels attr when removing model from set

6911ecc

Signed-off-by: elgubenis <elgubenis@gmail.com>

elgubenis added 2 commits August 19, 2015 10:43

adds 2 tests for checking merged models on set

7e862eb

Signed-off-by: elgubenis <elgubenis@gmail.com>

sets update event should not merge models existing

c9c1ed6

update gets not fired anymore because models are not added anymore, and it does not get merged either. Signed-off-by: elgubenis <elgubenis@gmail.com>

jridgewell added the enhancement label Oct 27, 2015

jridgewell mentioned this pull request Oct 27, 2015

v1.2.4 #3839

Closed

10 tasks

akre54 reviewed Dec 15, 2015
View reviewed changes

megawac mentioned this pull request Feb 1, 2016

Pass updated models in the Collection "update" event #3948

Merged

akre54 closed this in #3948 Feb 1, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding feedback to update event of collection #3711

adding feedback to update event of collection #3711

linus-amg commented Jul 13, 2015

megawac Jul 13, 2015

linus-amg Jul 13, 2015

jridgewell Jul 13, 2015

megawac Jul 13, 2015

jridgewell Jul 13, 2015

linus-amg Jul 13, 2015

megawac commented Jul 13, 2015

linus-amg commented Jul 13, 2015

jridgewell commented Jul 13, 2015

linus-amg commented Jul 13, 2015

jridgewell commented Jul 13, 2015

linus-amg commented Jul 13, 2015

linus-amg commented Jul 13, 2015

jridgewell commented Jul 13, 2015

akre54 Jul 23, 2015

linus-amg Jul 23, 2015

megawac Jul 23, 2015

linus-amg commented Aug 17, 2015

jridgewell commented Aug 17, 2015

jashkenas commented Aug 18, 2015

jridgewell commented Aug 18, 2015

jashkenas commented Aug 18, 2015

jridgewell commented Aug 18, 2015

linus-amg commented Aug 19, 2015

linus-amg commented Aug 19, 2015

jridgewell commented Aug 19, 2015

linus-amg commented Aug 19, 2015

jridgewell commented Sep 18, 2015

jridgewell commented Oct 28, 2015

mordentware commented Nov 9, 2015

linus-amg commented Nov 10, 2015

akre54 Dec 15, 2015

megawac commented Jan 27, 2016

adding feedback to update event of collection #3711

adding feedback to update event of collection #3711

Conversation

linus-amg commented Jul 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

megawac commented Jul 13, 2015

linus-amg commented Jul 13, 2015

jridgewell commented Jul 13, 2015

linus-amg commented Jul 13, 2015

jridgewell commented Jul 13, 2015

linus-amg commented Jul 13, 2015

linus-amg commented Jul 13, 2015

jridgewell commented Jul 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linus-amg commented Aug 17, 2015

jridgewell commented Aug 17, 2015

jashkenas commented Aug 18, 2015

jridgewell commented Aug 18, 2015

jashkenas commented Aug 18, 2015

jridgewell commented Aug 18, 2015

linus-amg commented Aug 19, 2015

linus-amg commented Aug 19, 2015

jridgewell commented Aug 19, 2015

linus-amg commented Aug 19, 2015

jridgewell commented Sep 18, 2015

jridgewell commented Oct 28, 2015

mordentware commented Nov 9, 2015

linus-amg commented Nov 10, 2015

Choose a reason for hiding this comment

megawac commented Jan 27, 2016