[CLOSED] [cmv4] 'origin' argument starting with "*" no longer works

[core-ai-bot]

Issue by peterflynn
Thursday Mar 13, 2014 at 00:30 GMT
Originally opened as adobe/brackets#7182

---

I've been testing with everyscrub (patched to use Alt+drag instead of Ctrl) and none of its replaceRange() calls are batched. ReplaceRange with a "*" prefix is supposed to batch all contiguous undos regardless of timestamp.

Note that the inline color picker still gets undo-batched properly using a "+" prefix.

I debugged into addChangeToHistory() a little bit, and it looks like all the checks pass in the first if statement except for the last one: lastChangeEvent() is returning undefined.

[core-ai-bot]

Comment by redmunds
Thursday Mar 13, 2014 at 01:41 GMT

---

Take a look at this commit for InlineColorEditor -- I think it fixes the same problem.

[core-ai-bot]

Comment by njx
Thursday Mar 13, 2014 at 05:27 GMT

---

Yes - you now have to make sure that if you do a replaceRange followed by a selection change, and you want everything to be merged, you have to wrap them in an operation.

[core-ai-bot]

Comment by peterflynn
Thursday Mar 13, 2014 at 20:19 GMT

---

@njx Something to add to the API change guide I guess?

It's too bad there's no way to just pass an "origin" for selection changes too -- that seems like a less disruptive fix for existing code...

[core-ai-bot]

Comment by njx
Thursday Mar 13, 2014 at 20:33 GMT

---

Ah, yes, CM actually takes the origin parameter in its selection APIs now. I could add that parameter to setSelection()/setSelections() on Editor. I'm not entirely sure that obviates the need to wrap them in an operation, but it might.

[core-ai-bot]

Comment by njx
Thursday Mar 13, 2014 at 20:37 GMT

---

@peterflynn - if you have time, would you mind locally trying to add the origin argument to setSelection()/setSelections() and passing it through to the CM selection API (see http://codemirror.net/4/doc/manual.html#api_selection), and seeing if that fixes your problem without having to wrap in an operation?

[core-ai-bot]

Comment by njx
Thursday Mar 13, 2014 at 20:41 GMT

---

(or I could put up a PR to try if you want)

[core-ai-bot]

Comment by njx
Thursday Mar 13, 2014 at 21:36 GMT

---

@peterflynn - see #7196

[core-ai-bot]

Comment by njx
Thursday Mar 13, 2014 at 23:21 GMT

---

OK, I just took a little time to test this out myself (using the InlineColorEditor), and it looks like selection origins only allow merging with other selections, not edits. You still need to wrap the associated edit and selection in an operation in order to merge an edit and a selection together. I can ask Marijn if he could merge selections and edits with the same origin. In principle, though, it makes sense to me that (now that selections are a separate part of undo history) if you want an edit and a selection to be considered part of the same history event, you should group them in a single operation. It is a behavior change though.

[core-ai-bot]

Comment by njx
Thursday Mar 13, 2014 at 23:26 GMT

---

(See continued discussion in codemirror/codemirror5#2305)

[core-ai-bot]

Comment by njx
Monday Mar 24, 2014 at 17:19 GMT

---

FBNC@peterflynn now that we can specify origins in the selection API and they should merge properly.

[core-ai-bot]

Comment by njx
Friday Mar 28, 2014 at 19:30 GMT

---

Oops, forgot to reassign to@peterflynn

[core-ai-bot]

Comment by peterflynn
Friday Apr 11, 2014 at 18:22 GMT

---

Confirmed this is fixed for my use case, and we've added it to the migration docs as well