Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

adding lineEndings UI in statusbar for documents #13152

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

zaggino
Copy link
Contributor

@zaggino zaggino commented Mar 6, 2017

continue from #10346

fixes #10346
fixes #10106
fixes #10594

Copy link
Collaborator

@petetnt petetnt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, you managed to fix some of my review comments before I had the time to submit them ^^

@@ -216,7 +217,7 @@ define(function (require, exports, module) {
if (!doNotAnimate) {
AnimationUtils.animateUsingClass($statusOverwrite[0], "flash", 1500);
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: trailing whitespace

*/
function _updateLineEndings(event) {
var editor = EditorManager.getActiveEditor(),
document = editor.document,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assignment to a window global, change to doc or similar

currentLineEndings;

// update label
if($statusLineEndings.text() === Strings.STATUSBAR_LINE_ENDINGS_CRLF) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: space between if and (

*/
function _toggleLineEndings(event) {
var editor = EditorManager.getActiveEditor(),
document = editor.document,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assignment to window global document, change to doc or similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, we should really make ESLint catch this.

@zaggino
Copy link
Contributor Author

zaggino commented Mar 7, 2017

@petetnt :-)

@zaggino
Copy link
Contributor Author

zaggino commented Mar 7, 2017

I was looking at this old PR, tried to polish it up a bit to see what we can do. It works fine I think but undo-redo doesn't work and won't work until we can add custom history events to CodeMirror. I'd have to check CodeMirror's code to see if that would be possible.

@zaggino
Copy link
Contributor Author

zaggino commented Mar 7, 2017

Also, partial solution to the problem is better than nothing at all. With this we can at least change line endings in the UI. In my opinion it's quite questionable how many users would report that this change is not part of the history, and if, we could be solving this as a separate problem (agile way). Let me know what are your opinions on merging something like this without it being part of the undo-redo functionality for now. @ficristo @petetnt @ingorichter @redmunds @swmitra

@redmunds
Copy link
Contributor

redmunds commented Mar 7, 2017

@zaggino Nice to have this in the UI! Would be nice to have undo, but I suppose it's not the bad.

My main question: is it common to want to change the newline type for a single file? After changing newline for a file and then I create a new file, the newline type is initially set to type of file currently being edited, then it changes to hard-coded OS default as soon as I make an edit.

It seems that people usually want all files on system or project to use a certain newline. Should a newline preference be set that overrides getPlatformLineEndings()?

@zaggino
Copy link
Contributor Author

zaggino commented Mar 7, 2017

@redmunds Of course it should, thanks for the idea. I'll have a look at it. Also do some tests to this.

@petetnt
Copy link
Collaborator

petetnt commented Mar 13, 2017

Re: history. Are the changes visible in the doc even if they don't affect the history? In my persistent history extension I work around similar issue by something like this:

https://github.com/petetnt/brackets-persistent-history/blob/master/main.js#L37

        // Set editor value to cachedDoc before setting history and then set the value back to the modified file
        // so that the history applies to the modified document too.
        if (wasModified) { 
          var currentDoc = codeMirrorDoc.getValue(); // current document
          codeMirrorDoc.setValue(cachedDoc); // the cached document
          codeMirrorDoc.setHistory(history); // history from a JSON file
          codeMirrorDoc.setValue(currentDoc); // apply the current state
        } else {
          codeMirrorDoc.setHistory(history);          
        }

In my case the I work around changes done outside of Brackets while keeping the old history, so the history becomes last state known to Brackets -> outside changes -> current doc. Would this work for old line endings -> new line endings scenario?

@zaggino
Copy link
Contributor Author

zaggino commented Mar 14, 2017

@petetnt The issue with history is that we'd need somehow to push a custom entry there, which CodeMirror would be able to process. The API for this could be quite simple I believe. Something like:

codeMirrorDoc.pushCustomHistoryEvent({
  undo: function() { /* this would revert line ending changes when invoked */ },
  redo: function() { /* this would re-apply line ending changes when invoked */ }
});

but I'm not sure how to achieve this without doing any modifications to CodeMirror itself.
Do you have any ideas how to achieve this? cc @marijnh

@petetnt
Copy link
Collaborator

petetnt commented Mar 14, 2017

Oh yeah, now I get what you mean. Due to how it's hard to create the undo/redo objects is exactly why I asked/suggested in #13152 (comment) if it was just possible to setValue so that the whole doc essential changes itself, while retaining the old history if just changing the line-endings doesn't call setValue (and thus creating the undo/redo states). After that one could just add a listener to the CodeMirror#changes event (or similar) to see if the line-endings had changed and adjust the UI etc. accordingly.

pushCustomHistoryEvent API sounds much more saner though.

(Sorry if I am repeating myself here, haven't had the time to test this PR out properly yet myself)

@saurabh95
Copy link
Contributor

I just did some testing around this PR. It is working fine and I agree with @redmunds, would have been better if we had undo/redo as well, but this is also fine. We can work on the history part later. Code also seems ok to me.
It would be really great if we could merge @ficristo's PR #11614 for 1.10, for which this PR is pre-requisite, because users have been demanding for that feature for quite a long time now :)

if (this._lineEndings === lineEndings) {
return;
}
this._lineEndings = lineEndings;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is toggled twice, the document is effectively in unedited state and hence should not throw a save/don't save prompt while closing the file.

Other thing is should the change of line ending be part of undo stack?

Copy link
Collaborator

@ficristo ficristo Jun 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tryed Notepad++ with a txt file. When you change the line ending, the first time goes in edited state, if you change back to the previous line ending it remains in an edited state.
So, to me, the prompt is expected.

As stated above, the line ending changes should be part of the history but requires to add custom entry in CodeMirror. If someone at Adobe could do the courtesy it would be great.

@ErikThiart
Copy link

Any feedback on this guys, I am currently forced to use notepad++ because brackets default to CR LF instead I want the default to be UNIX's line endings LF since the majority of PHP projects built with brackets on a Windows client, will run on a UNIX system.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Saving sh files always uses Windows line breaks How do you to force \n new lines instead of \r\n on Windows?
8 participants