Bugzilla – Full Text Bug Listing
| Summary: | [client] line delimiter mess | ||
|---|---|---|---|
| Product: | [ECD] Orion | Reporter: | Felipe Heidrich <eclipse.felipe> |
| Component: | Editor | Assignee: | Felipe Heidrich <eclipse.felipe> |
| Status: | RESOLVED WONTFIX | QA Contact: | |
| Severity: | normal | ||
| Priority: | P3 | CC: | bokowski, mihai.sucan, Mike_Wilson, pwebster, Silenio_Quarti, simon_kaegi, susan, tomasz.zarna |
| Version: | 0.2 | ||
| Target Milestone: | --- | ||
| Hardware: | PC | ||
| OS: | Windows 7 | ||
| Whiteboard: | |||
|
Description
Felipe Heidrich
The easiest fix (for now only!) seems to be to only do LF (i.e. don't insert CRLF even on Windows) in the editor, and to convert all the files that are in a bad state now to use LF only. We would have to set core.autocrlf=false as well. Or is there another low-risk option? (In reply to comment #1) > The easiest fix (for now only!) seems to be to only do LF (i.e. don't insert > CRLF even on Windows) in the editor, and to convert all the files that are in a > bad state now to use LF only. We would have to set core.autocrlf=false as well. > Or is there another low-risk option? I agree this is the safest fix. The line delimiter is a creation parameter of the TextModel, so we would have to change the textview factories to create a empty model (new TextModel("", "\n")) and pass as an option parameter to the text view constructor. I believe there are a few places to change (minimaleditor.js, embeddededitor.js, setup.js, compare-editor.js). The last one might be good already. We (Silenio and I) talked to Simon and this is what we decided: - No mixing line delimiters in the same file - Okay to have files with different line delimiter in the same folder, the editor should be able to handle this case. - New files a created using the line delimiter of the client (browser) Basically if the user is running on Windows it should be possible to create a file with CRLF. For this to work the editor needs to detected the delimiter of a file and set it on the model. This auto detection can not be done by the textview, the only place it could be done in the view is during setText(). Because in setText() the textview does not know if a new file is being loaded or the current file is being changed. We think the auto detection code should only run when a new file is being loaded. Silenio and I put the code in InputManager#setInput (setup.js). Because we are officially saying that is okay to have delimiter different that the client delimiter we had to fix a old bug in the clipboard to convert line delimiter when interacting with the clipboard (StyledText has the same code). --- We can improve the support in the future by adding ui action to convert line delimiter. Simon please review branch bug349096 http://git.eclipse.org/c/e4/org.eclipse.orion.client.git/commit/?h=bug349096&id=b4f9908add80b93341983ecba564b6f7d80449d0 Note, before we can close this bug we need to fix all the files that already wrong (mixing CRLF/LF). +1 for RC2 - I did a light review but assuming Silenio and you are doing this together etc. We released the code in the branch and we changed all files in org.eclipse.orion.client to use LF. User using msysgit should set autocrlf to true (on windows), or input (on linux) as described in: http://help.github.com/dealing-with-lineendings/ This allows them use change files using notepad or eclipse without messing up the files again. User on orion are fine too: - jgit does not convert line delimiters - InputMa8nager#setInput has the auto detect line delimiter (so new lines are inserted using the same delimiter) - Textview clipboard is fixed to convert line delimiters between model and platform. --- Note is possible (and always will be) that files get in a bad state, for example: user using jgit on windows and changing files with notepad/eclipse; user using msysgit without autocrlf=true and changing files with notepad/eclipse; fixed (In reply to comment #6) > User using msysgit should set autocrlf to true (on windows), or input (on > linux) as described in: http://help.github.com/dealing-with-lineendings/ > > This allows them use change files using notepad or eclipse without messing up > the files again. > This means we have a really weird workflow if we want to use msysgit clones linked into Orion. In bug 339397 we are told to tuse autocrlf=false or else git status will be wrong. Here we need to use autocrlf=true. So as I read it, if you want to use an msysgit clone with Orion, you must now turn autocrlf=true, but you'll have to run git status on the command line before you try to use the git status Orion page. True? We need to document this well. (copy from the mailing list) "Hi Susan, If you don't use autocrlf true then you have to use orion editor to edit your files. This is what happens, I'll assume you are working on Windows, without autocrlf true all files in your local repo will use LF (like the files in the remote). Native editor (notepad, eclipse, etc) will enter CRLF in your file causing the file to have again a mix of CRLF and LF that we would like to avoid. Note that some editors (like Eclipse) allows you to choose what line delimiter to use, so in theory you should be able to use them as long as you set line delimiter to LF (I didn't test it). The orion editor will detect what line delimiter is already in the file and use it (avoid mixing CRLF-LF). I would also recommend to you (and everybody else working on org.eclipse.orion.client) to: 1 - pull 2 - delete all the files from your working dir 3 - checkout master This will make sure all files are changed to the correct line delimiter (I tried git reset hard but it didn't replace the files that had only the line delimiter changed). " Interesting poing Susan, the problem is that we have developer working very differently, For example: Dev1: Uses Orion to clone (JGIT) Uses Orion to edit files Uses Orion to diff files/commit Dev2: Uses command line to clone (Msysgit) Uses Orion to edit files Uses Orion to diff files/commit Dev3: Uses command line to clone (Msysgit) Uses Orion to edit files Uses external diff files (windiff/eclipse) Dev4: Uses command line to clone (Msysgit) Uses notepad/eclipse to edit files Uses external diff files (windiff/eclipse) I suspect that you are Dev2, correct ? I'm personally Dev3 trying to move to Dev1 (but I need to get some fixes in before I can do that). When we were thinking of a solution for this problem I had Dev1 and Dev3 in mind, that said. I think we can make it work for Dev2, read the last comment. I believe the following... 1) The line delimiter of the machine you are on when you develop *should* not matter -- you are likely not developing code that will run locally. 2) The line delimiter of the machine which is hosting orion *should* not matter -- the normal use (outside of our team) will be to develop code that will be pushed to another server. 3) There will be a line delimiter that is appropriate for (at least) a particular project that a developer is working on, and potentially at a finer level of detail. Given this, the fix seems like it should be: 1) allow the user to set (and hopefully remember) the delimiter at the project level 2) use that when new files are created 3) make sure our tools maintain the delimiter, which includes, for example, changing the delimiter when you save a file in the editor The only place where we get into trouble then, is environments where external tools end up producing files that have incorrect and/or inconsistent delimiters. For these cases, we should 1) do what we can to make our tools robust in the presence of this (including fixing the delimiters when possible) 2) provide an explicit operation probably at the navigator level to manually convert the delimiters when it would not be possible for us to automatically do it. Re-opening since we clearly have not yet resolved the "mess". I think we've gone as far as we will in 0.2. Re-targetting for 0.3. Simon, note that the solution outlined by McQ in comment 11 (i.e. project level property for line delimiter) lies outside of the editor. Are we, in the editor, missing support ? In Firefox developer tools we also have a bug report about issues with the line delimiter. Here is one report: https://bugzilla.mozilla.org/show_bug.cgi?id=757071 Is there ongoing work to fix this bug? It would make sense to: 1. detect the line ending type for the "new file" the user is editing - eg. when setText() is called without any range (which means a whole new document is being edited). Use the detected line ending whenever getText() is called - do not use the platform-specific line ending. 2. when a new "empty file" is being edited (eg. when setText("foo") is called - with no new lines) use the platform-specific line ending. 3. provide an API in TextView/TextModel to set the desired output line ending. Use this when getText() is called. This would allow us to pick the line ending we want based on any factors we see fit - eg. an UI option to set the desired line end. If the bug we have encountered is different than this one please let me know - I can open a different bug. Thank you! I believe the API you are asking already exists: TextModel.setLineDelimiter(delimiter). It does 1) and 2) if the parameter is "auto", otherwise it sets the delimiter to the specified parameter. I am not sure I understand 3). Are you suggesting to replace the line delimiter of the existing lines in the text when calling getText()? If so, It seems wrong. All we should be able to do is the determine the delimiter of new lines added to the text (which can be done with the API above already). (In reply to comment #16) > I believe the API you are asking already exists: > > TextModel.setLineDelimiter(delimiter). > > It does 1) and 2) if the parameter is "auto", otherwise it sets the delimiter > to the specified parameter. > > I am not sure I understand 3). Are you suggesting to replace the line delimiter > of the existing lines in the text when calling getText()? If so, It seems > wrong. All we should be able to do is the determine the delimiter of new lines > added to the text (which can be done with the API above already). Somehow I missed the setLineDelimiter() method. Thanks! For 3) yes, in some cases we might want to change the text from one delimiter to another - obviously this should be optional so we can do this on demand. (In reply to comment #17) > > For 3) yes, in some cases we might want to change the text from one delimiter > to another - obviously this should be optional so we can do this on demand. I could extend setLineDelimiter() to replace all the delimiters in the text to the specified parameter when all==true. setLineDelimiter(delimiter, all); Would that work for you? Some how changing the delimiters in getText() does not seem right to me. What would be the answer of getCharCount(): the original text length or the modified text length? (In reply to comment #18) > (In reply to comment #17) > > > > For 3) yes, in some cases we might want to change the text from one delimiter > > to another - obviously this should be optional so we can do this on demand. > > I could extend setLineDelimiter() to replace all the delimiters in the text to > the specified parameter when all==true. > > setLineDelimiter(delimiter, all); > > Would that work for you? Some how changing the delimiters in getText() does not > seem right to me. What would be the answer of getCharCount(): the original text > length or the modified text length? That would work. The result of getCharCount() would be the modified text length. I would expect that setLineDelimiter(newDelimiter, all) changes the text. This is in line with other text editors that allow you to convert the delimiter of existing lines. Please do correct me if I am wrong - I might be! However, please do note this is far from being a high-priority bug for us. Once we figure out the things we really need fixed, I will email you a list of bugs and priorities for the next Orion update in Firefox. Thank you! Closing as part of a mass clean up of inactive bugs. Please reopen if this problem still occurs or is relevant to you. For more details see: https://dev.eclipse.org/mhonarc/lists/orion-dev/msg03444.html |