[CLOSED] undo history not working, when line-endings change after save

[core-ai-bot]

Issue by zaggino
Tuesday Nov 01, 2016 at 06:59 GMT
Originally opened as adobe/brackets#12865

---

For a bug identified in adobe/brackets#11826 (comment)

credits to@vahid-sanati for identifying this, put up a PR so we can have it merged asap

cc@petetnt@ficristo

---

zaggino included the following code: https://github.com/adobe/brackets/pull/12865/commits

[core-ai-bot]

Comment by ficristo
Tuesday Nov 01, 2016 at 07:45 GMT

---

The test added in adobe/brackets#12681 probably should be updated or we should add a new one for this case.
It exists a Document.normalizeText which normalize line endings: maybe we should update to check also for \r and use it, but it doesn't check for null text, so maybe not.

Looking at the commits for 1.5 adobe/brackets@release-1.4...release-1.5
there are a couple of upgrade of CodeMirror. In that version of CodeMirror it was changed a bit the way new line endings where used and maybe that broke some assumptions of Brackets.

[core-ai-bot]

Comment by petetnt
Tuesday Nov 01, 2016 at 08:13 GMT

---

Would it be worth it to extract and extend Document.normalizeText to FileUtils.js with other line ending related methods? Then we could just use FileUtils.normalizeLineEndings(text) which would return normalized text or null.

The solution for the problem LGTM, just 🚲 🏠

[core-ai-bot]

Comment by zaggino
Tuesday Nov 01, 2016 at 08:47 GMT

---

I wanted to use FileUtils.normalizeLineEndings but when I included it in the file it lead to circular dependencies and whole Brackets broke 😒 Really hate requirejs

[core-ai-bot]

Comment by zaggino
Tuesday Nov 01, 2016 at 08:49 GMT

---

Feel free to push/modify this branch if you want, I have very little time to give to this urgently.

[core-ai-bot]

Comment by petetnt
Tuesday Nov 01, 2016 at 08:50 GMT

---

Really hate requirejs

Ditto. I say lets merge this now and bikeshed the normalization later.

[core-ai-bot]

Comment by ficristo
Tuesday Nov 01, 2016 at 08:59 GMT

---

If you want to merge go ahead, but a test is really needed here. At least file a followup for it.

[core-ai-bot]

Comment by zaggino
Tuesday Nov 01, 2016 at 10:08 GMT

---

put up a test;

• i believe that CM is returning always \n line-endings after the change@ficristo mentioned but i don't have time to investigate properly

[core-ai-bot]

Comment by ficristo
Tuesday Nov 01, 2016 at 10:14 GMT

---

AFAIK if the lineSeparator option is not set CodeMirror return always \n line endings.

[core-ai-bot]

Comment by petetnt
Tuesday Nov 01, 2016 at 10:16 GMT

---

Kinda worried about possible performance issues with larger files (possibly leading to External file changes-dialogs?) but otherwise LGTM.

[core-ai-bot]

Comment by swmitra
Tuesday Nov 01, 2016 at 11:08 GMT

---

Merging.

[core-ai-bot]

Comment by swmitra
Tuesday Nov 01, 2016 at 11:11 GMT

---

Great work@vahid-sanati@zaggino@ficristo@petetnt