-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Hide ^M in editor. #1652
Comments
|
Or even better:
suggest or "guess" line endings upon saving a file (WIN/LIN/MACOS). |
Replying to birdie:
|
ups...
You mean #1571? |
If you implement an option, \r SHOULD BE SHOWN by default.
Mcedit is used as part of Cygwin by poor people who are forced to use Windows in the office (like myself). On Windows not all editors are preserving \n linefeeds like Far-manager, other editors may insert \r. The best way to check it is mcedit. |
|
|
Currently this is one of the biggest issues with mc. The inability to work with different line ending give problems in lots of cases. It is something really common for see CR/LF even on linux from files coming from windows share, ftp, source control.
Like other editor mc should be able to detect the line ending and keep it. It could should the line ending in the status bar. |
Oh really? How did you conclude that mc is *unable* to work with different line endings?! It currently keeps the endings of edited files, however, \r endings are not shown by default, whereas ^M are. So it's perfectly safe to edit any kinds of files. Moreover, you can save files using any scheme that you want.
All it takes is to add the ability to optionally display alien endings and add options to select default endings scheme / and heuristics on keeping auto-detected endings or auto-converting them to the default scheme. |
|
|
This is truly needed feature, and I lost my patience and went to implement it myself. At first I thought that just making mcedit to not display ^M if followed by \n would be enough, but turned out that I'd need to teach cursor to skip them too in various places, and that would be lot of effort to learn mcedit's internals and do it right in all places.
So I indeed went for the following solution:
That's basicly all - alien text files are converted to unix on load, then converted vice-versa on save. Only extra thing needed on this stage was to make Save As to treat "don't change" as indeed don't change (to not reset auto-detected line ending type).
Patch is attached (note - debug output). I'd appreciate review for the approach. Then, further work might be:
|
Replying to pfalcon:
|
pfalcon: |
branch: 1652_autodetect_lb |
|
Replying to angel_il:
Well, so far I do minimal hacking, so git diff/format-patch is less effort. If I'll do more, I'll push to github (btw, is there some repo to fork there? I cloned from midnight-commander.org)
Well, that would mean loss of operation precision. What edit_insert_file() does is inserts file filename with encoding lb into editor object edit with encoding edit->lb. In general case, lb != edit->lb. Yes, for what I implemented so far it doesn't matter, but "Insert file..." menu can be made to take advantage of lb detection too. And if "Insert file..." instead presume that inserted file's lb's are the same as currently being edited, that sooner or later will lead to unexpected behavior for some users, which then will complain that mc is buggy. So, worth doing it right from the first time ;-). |
Updated patch to show current (non-LB_ASIS) encoding in status line. Here's sample, still fits in 80 chars:
|
|
Oops, I actually didn't check into 1652_autodetect_lb, where status line feature was already added. Nevermind. I still think it's better to pass LineBreaks to edit_insert_file explicitly.
And with changes to edit_get_save_file_as() in [3de1e2f26299017afa887f849199995c893de3e1], the whole option N_("&Do not change"), should be removed, as user's choosing it will lead to file being converted to LF, which is rather unexpected. But you cannot just remove that choice from dialog, as it is tied to LB_ASIS being 0. So, either LB_ASIS should be removed at all, or handled properly - not changing edit->lb, like my original patch did. |
Also, [8d28ef0b4756b3af0177a59433bd1be347d3c274] shows LB only for simple_statusbar case, probably worth adding for both types. |
mceditor should be stay as binary-safe editor, therefore need to implement configuration option for switching 'crlf autodetection' behaviour. |
|
|
please review |
|
please review |
If autodect is off, the editor has new behaviour that looks like a bug.
How to reproduce:
2a. Open a new text file pressing shift-f4.
then save file with DOS line breaks (shift-f2 -> Windows/DOS format (CR LF)) and close the editor. |
|
Replying to andrew_b:
Simplest testcase:
2a. Open a test file with DOS line breaks. You can see ^M at line ends. |
Replying to andrew_b:
fix: [8c84722307e873a37aa88c1a6f5caaeae97421a9] |
Have some new tests and have changed code in branch. Also, need to describe new feature and editor option in man pages and WIKI. |
Is this ticket stalled? I'm missing this feature. Can this be integrated to main trunk? |
Well, I'm, as a contributor of the initial patch, found it perplexing that my patch is rewritten for no apparent reason, and while that being done, new bugs are added to it - exactly those which I could predict to happen and deliberately avoided in my original patch. So, it's a strange situation - I cannot continue with updated patch because I know it's buggy, and my original patch was essentially thrown away. And nobody else seem to continue with this either.
I guess, I should just swallow that and try again, because I like mc and use it daily, and each time I see ^M there (and that happens often!) I think to myself: "damn, I did patch to fix that, how on earth it happens I still see that issue?" |
Thanks for notice.
Branch rebased to current master.
Changesets:
Review, please.
pfalcon, as your original patch was changed, review and vote too, please. |
|
My patch includes both patches
with minor modifications for updated codebase.
I hope it will be included in recent release. |
Fixed bug with edit->stat1 not updated properly if linebreaks are not ASIS on save (F2). |
Oh my god, isn't this fixed already. I nowadays run distro package again and suffering thru this issue almost everyday, like, on checking out each new github repo I'm shooting out bugreports "you have CRLF in repo, get git linebreak settings right", though of course that's issue on mc side. So I was hoping to switching to run from git master to get this fix and it's still not there ;-((.
huksley, thanks for bringing this up. Will test your patch. |
This patch doesn't hide ^M. This patch modifies file. With this patch, when you load file and then immediately save it, you get modified file. This is wrong way. If we really want hide ^M, we should do that without file modification. |
Patch against 4.8.1 for this feature |
Upon investigation, I found out this patch modifies file only in case if there is different types of line breaks throughout the file. I.e. if it contains \r\n and later on contains just \n then it will write all linebreaks as \r\n.
I`ve introduced modification to behaviour so it will not touch linebreaks (forces LB_ASIS) if option [ ] Autodetect linebreaks is not checked. |
I would have thought that's expected behavior right from the start - in my original patch there was provision for that (but no UI to force skipping any linebreaks munging). I refrained from answering to andrew_b's comment before I tested the latest patch version (still in my backlog). I'm glad that bug with not following that behavior was found and fixed.
So, idea how it should work is:
As for making editor not showing ^M, while having all of them in the buffer, I commented 2 years (comment 12) that it was my original idea, but turned out that mc doesn't have enough abstraction to deal with buffer handling, and checks for ^M would need to be sprinkled in a lot of places (doubled by checks for not skipping them if not enabled), or such level of abstraction would need to be introduced. In either case, that would be substantial change to editor, which would take a lot of time to implement, and even more to actually make bug-free. So, that's risky and low-return way to do it, with no really good benefits achieved (and only missing nice features like linebreaks conversion). The fact that over 4 years nobody went for it is a good indication of that. |
Just a thought. Couldn't we use file utility (as in e.g. file --brief --mime-type <filename> or similar) to detect if the file we're about to edit it a text one? At least if it's installed.
As for files with varying linebreaks, as well as binary files, doing nothing (disabling any modification operations implied by autodetection) seems like a good choice to me. |
What external utility does what mc code couldn't do without relying on external utility availability and incurring overhead to call to it? I'm using mc on ~100MHz embedded routers, and would hope for mc to become more efficient in the future, not less. And of course, file utility can be gullible too - it's all heuristics after all. |
huksley: you patch doesn't apply to the current HEAD at all. git currently is at 4.8.9-pre, so patch against 4.8.1 is out of date from the start. Can you please fork https://github.com/MidnightCommander/mc on github and forward-port the patch to it? Or let me know if you'd rather have me go for it. |
I took the latest authoritative source for this feature, git branch "1652_autodetect_lb" and rebased it to latest master (rebase changes were considerable). It is available in "1652_autodetect_lb-revive" branch of my fork on github: https://github.com/pfalcon/mc/tree/1652_autodetect_lb-revive . Pull request submitted: #49 . Maintainers notified: https://mail.gnome.org/archives/mc-devel/2014-December/msg00009.html |
For reference, contents of the mail which describe changes in "1652_autodetect_lb-revive" branch in detail:
|
huksley: Unfortunately, I wasn't able to decipher what changes you did on top of changes done to 3 other contributors in your cumulative patch. The patchset as presented above works well for me (and backed by unit tests). I would suggest that you suggest your changes as a separate commit on top of the branch above. Thanks. |
What we get as ticket topic? Hide ^M.
We already have the way to remove ^M. This way is "Save is...". |
Thanks for the reply. As often happens, after consideration of an issue as reported, it turned out that scalable solution doesn't exactly match (usually highly informal) issue description. In particular here, the problem turns out not in "^M", but in the fact that mcedit doesn't handle files with alternative line-endings. And that's exactly the issue being fixed, and #49, the actual patch, has the correct title. This issue is being updated to keep in loop all people who were interested in it over the years. |
I use mcedit to handle a lot of text files every day and the single most important feature to me is that mc not silently change the files. Some files have unix style line-endings, some have windows style, but many have been sent back and forth between windows users and unix web servers and have a mix of line-endings. I don't mind if mcedit hides the ^M visually, but it's very important to me that mc isn't altering files behind the scenes unless it alerts me that it's going to and offers me a choice. If I load a file and then save it without making an edit, I expect the file to be unchanged. |
|
|
Important
This issue was migrated from Trac:
slavazanko
(@slavaz)zaytsev
(@zyv),gotar@….pl
,pmiscml@….com
,cravchik@….ru
(@kravich)Need to hide ^M symbols at end of lines in text files with '\r\n' line ends.
Better way: create keybinding for toggle ^M symbols.
Note
Original attachments:
pfalcon
(pmiscml@….com) onJan 15, 2011 at 23:38 UTC
angel_il
(@ilia-maslakov) onMar 29, 2011 at 14:01 UTC
angel_il
(@ilia-maslakov) onMar 29, 2011 at 14:01 UTC
huksley
(huksley@….ru) onMay 31, 2013 at 7:54 UTC
The text was updated successfully, but these errors were encountered: