brackets-userland / brackets-git

brackets-git — git extension for adobe/brackets
Other
656 stars 193 forks source link

Clicking commit nukes the edit history #1274

Open petetnt opened 8 years ago

petetnt commented 8 years ago

I was hoping that fixing #1272 would have caused this too but I guess not.

There's a long running issue in Brackets where the users have gotten their edit history cleared after saving: this usually happens at random given time and does not currently have any reproducible steps. However, I managed to track the issue down to https://github.com/adobe/brackets/pull/12175 which hopefully acts as an hotfix for the issue.

There were some folks on the issue topic saying that the issue could have been caused by Brackets-Git, which I think that is completely separate issue?

See this comment: https://github.com/adobe/brackets/issues/11826#issuecomment-177193603

Same happens to me: I make some changes, I save the file, I stage the file. Everything works as expected. And the undo-history is still intact.

But when I click the commit button, the undo history is gone instantly (regardless of if I actually commit or not).

I tried the following:

but nothing changed the behavior for me. Only thing I did not change, was the version of Brackets which was always the latest master, which might mean that something has changed in Brackets :ghost:

zaggino commented 8 years ago

This is probably caused by clear whitespace before commit feature. Brackets should not kill history when an external change to the file is detected but count it as an edit in the file history.

petetnt commented 8 years ago

@zaggino, yup, that's it!

What's weird is that I only had Add endline to the end of the file option on (by default?) and it still occurred: when I clicked the Trim trailing whitespace option on, it toggled endline, remove BOM and normalize line ending options on too, and same the thing happened. But when I removed the whitespace tick and the others were toggled off too, the issue disappeared (and now I can enable any of the other 3 options separately too without breaking the history).

zaggino commented 8 years ago

As far as I know, any of the file modifications done externally (or by Brackets Git) break file history ... this has been like this as far as I remember ... I've complained about it a few times in a very distant past but it doesn't seemed to be an issue for anybody.

petetnt commented 8 years ago

@zaggino Yup, I have tried to keep the discussion separate of the bug where history stops working when saving, not due external changes. Are the whitespace trims etc. done in editor or outside of Brackets?

zaggino commented 8 years ago

@petetnt https://github.com/zaggino/brackets-git/blob/71027c3fd374d8c02cfc2806e7ed7127069461b6/src/Utils.js#L491-L528

zaggino commented 8 years ago

this is the write line: https://github.com/zaggino/brackets-git/blob/71027c3fd374d8c02cfc2806e7ed7127069461b6/src/Utils.js#L441

basically it's done using the File System Utilities provided by Brackets

petetnt commented 8 years ago

This call will eventually lead to Editor._resetText which will reset the history:

https://github.com/zaggino/brackets-git/blob/71027c3fd374d8c02cfc2806e7ed7127069461b6/src/Utils.js#L368

It should be enough to trigger an change-event with [{text: text.split(/\r?\n/)}] as the changeList parameter (https://github.com/adobe/brackets/blob/master/src/editor/Editor.js#L908). This should create a new CodeMirror history event so you can CTRL+Z to the previous state (Not sure if this should be done to the active document only: will the other docs update when they are opened?). See https://github.com/adobe/brackets/blob/master/src/document/Document.js#L358 why the splitting to new lines is needed.

edit: On second thought that might not be enough as the masterEditor is the same for the current document too. https://github.com/adobe/brackets/blob/master/src/editor/Editor.js#L914

petetnt commented 8 years ago

Tried some of the stuff above (and others like using doc.setText and then marking it clean but those actions resulted in the same end result.

One way to get past this would be trimming the whitespace on save, not during commiting, but that's a bit different thing conceptually (or is it? Files you commit are usually modified by yourself).

zaggino commented 8 years ago

No, definitely don't want to clean whitespace on save, it makes cursor jump all over the place. Doing this before commit is the only proper time to me.

There are multiple whitespace cleanup extensions, but the ability to do this before commit is the main feature here.

petetnt commented 8 years ago

@zaggino fair enough :+1: ! I agree that before the commit is the main feature in question too.

However, when using Miguel Castillos ws-sanitizer https://github.com/MiguelCastillo/Brackets-wsSanitizer cursor doesn't jump around at all (and it adds the whitespace removal as a single history step) but not exactly sure how it would be best applied to the precommit situation: one option would be cleaning the files that are not open in the editor as it is done now, and do the whitespace removal differently (?) to the files that are open in the workspace so the history would be kept intact.

I'll look into the options that are present.

zaggino commented 8 years ago

I believe you're trying to fix the wrong place ... why not rather fix Brackets in a way that when external change is detected, one step in history is made instead of whole history being dropped? That would solve more issues.

petetnt commented 8 years ago

@zaggino Yeah you are correct about that too: I have mentioned it (recently) in https://github.com/adobe/brackets/issues/12181 too but I am not quite sure about the implementation yet.