Repository navigation
Full support edit CRLF(win) files in editor - #5151
KuzinAndrey wants to merge 7 commits into
Conversation
github-actions
Bot
added
needs triage
ace0ef3 to
65b4540
Compare
|
Fix some editor behaviour in mixed EOL-type files and code clang-format CI tests |
|
I'd like to make such large commits a bit smaller:
|
zyv
added
area: mcedit
|
I apologize, all the code was generated by the AI agent, and I reviewed it as best I could. I even just tested the expected behavior in the editor and made corrective prompts (though some navigation issues are still possible, and I'll be using the modified editor for work now; I might find more bugs). I couldn't use the editor at all on WIN files until I decided to do something about it. |
65b4540 to
55dce2c
Compare
|
i'm certain that we already had a contribution attempting that a while ago. i can't find it, which is unfortunate, because i didn't like it, and i'd like to see it contrasted with what we have here. the way you factored out the first commit without usage and tests isn't very elegant. a personal note on AI: i don't care if you use it, but if you use it as an excuse for anything, you are so out. |
|
I don't want to waste a lot of time shuffling lines between commits, covering them with tests, and other nonsense just to get the PR accepted. Either the community is happy with the commits and they accept them (they can change them to suit their own vision), or I just use them in my private branch - they solved a big problem for me very well. I don't want to be responsible for AI slop, so I'm warning you about it right away; I've verified as best I can that it at least works. I apologize for using AI, because people accepting PR will spend their personal time analyzing what the machine generated, and even finding errors and flaws in it. Many open-source projects have switched to ignoring AI slop and are unwilling to accept it. |
I linked it at the top of the ticket. |
Just a friendly tip: consider that this is a public forum. If you are an engineer, it might be unwise to declare proper engineering practices "nonsense". Someone considering employing you in the future might read it. Anyhow, that's up to you, but I'm afraid it'll then stay in your private branch. Also, there is no "development community" of mc. I wonder where you guys get all these strange milk river and unicorn ideas from? You must be too long out of school to believe in Santa Claus. I'm seriously curious. Who do you think develops mc and why they do it, and how do they make a living? In reality, currently there are 1.5 overworked maintainers looking at the project in their non-existent spare time, trying to keep it alive - answering issues, commenting on PRs, doing some development work, that's it. So either contributors put the time to make the contribution quality match the minimum acceptable safe floor for us to merge (and the standards are very low), or the patches will just hang around until someone will rework them - which might happen later or never.
I personally have no problems with AI-assisted contributions whatsoever, as long as the contributors are the ones who put their time supervising their agents and fixing the code, instead of offloading it to the maintainers along the lines of "look what I have generated, I didn't check it and I don't understand it, and I won't change it according to your requests, but it works for me and I'll keep using it, so take it or leave it". |
|
I apologize if this seemed disrespectful to your work, but it's not. I understand perfectly well how various corporations make billions off the unpaid work of two open source project enthusiasts, so I don't want to waste your time. But my experience also involves spending weeks of work making some contribution to the project, polishing every commit to a high standard, only to have it sit abandoned for years. For example, libevent/libevent#1753 (but that's the problem with project abandonment). Even at work, I often struggle with MR due to QA tests, indentation and syntax corrections, and various bureaucratic hurdles, and it's often demotivating. I want to bring proposed changes to the highest possible quality, but moving lines between commits and writing tests for things that can be done without them is enough for my personal use. My work involves developing a Linux distribution, and I can say with certainty that 95% of projects don't have any unit tests because open source enthusiasts simply don't have the time. |
i don't see it. you linked the issue. the PR (presumably still on trac) was about a decade younger. iirc, it was just converting at read and write. |
The patches from 2011 and 2013 are attached at the top of the linked issue. The PR from 2014 is also linked there: MidnightCommander/mc-old#49 . |
|
I used a slightly different approach and implemented configurable rendering of control characters in mcedit. In the normal mode, they are displayed using caret notation, such as ^M, ^L, and ^? etc... When the option is disabled, each control character is rendered as a single blank cell, while the file contents and the bytes stored in the editor buffer remain unchanged. Screen-column calculations follow the selected mode: a visible control character occupies two columns, while a hidden one occupies one. When the mode is toggled, the layout cache is invalidated and the cursor position is recalculated. As a result, the cursor remains attached to the same byte, and all open editor windows are updated immediately. This solution required relatively few changes because it affects only the presentation of the data, not the editor’s internal data model. Loading, saving, searching, insertion, deletion, and undo continue to operate on the original bytes without special handling. The option uses the existing settings, menu, and keymap infrastructure, while a small shared width function keeps column calculations consistent across the relevant code paths. https://github.com/blue-panels/mc6/pull/223/changes If there is interest in this particular approach, it should be relatively easy to adapt it for upstream. |
|
Overall, the CRLF-handling approach used in the patch author’s branch is workable. I tried a similar approach some time ago, but eventually abandoned it, and the corresponding patches never made it upstream. |
|
Oh, now that I see that: I wish you could adopt a different name for your fork other than "mc6" before this spreads. It produces an impression of being a new incompatible version of one and the same project (and the default implication is that it is maintained by the same team), and "Midnight Commander with Plugins" is also a very weak differentiator for the users. We are already regularly getting reports from the Windows fork, and here the total confusion is simply guaranteed. I don't want to have to interact with the users of your project, and probably you don't want to get reports for our issues either. Besides, there is an even worse binary naming conflict as with the Windows fork. At least we don't support Windows, and we are not going to. But in the Unix world, there are already two It can be really anything you want, but please not yet another Midnight Commander. If you want to be able to take the name of |
Regarding the binary name conflict, I have already opened an issue in our tracker to rename the main binary to mc6 blue-panels/mcommander#219 . This should avoid a conflict with mc, but it is not a trivial change: the current binary name is referenced in many places, and all those cross-references need to be identified and updated consistently. |
|
i think you should look up what the phrase "hostile fork" actually means and match that against your actions. my impression is that it's 50%-ish hostile.
i've seen no evidence to substantiate such a blanket claim. what i did see evidence for is that it's really hard to make you understand arguments why you should choose a different direction in particular cases.
that's just not going to happen, for resource reasons. the contributor needs to be pro-active and is responsible for keeping the ball rolling. in fact, the mc project has rejected several awesome contribution (most prominently mc^2) because they came without a commitment to maintenance, which was felt impossible to provide internally given available resources. this would radically change if someone both enthusiastic AND trustworthy showed up to become another maintainer.
well, you remember wrong. |
But further down, you yourself explained why these features will not make it upstream:
|
I wasn’t writing about our interactions, but about my interactions with Yuri. As for our interactions, I can say that when I read your answer, “because I’m evil,” to my question “why” - I can’t vouch for the exact wording - I thought: “Oh, this guy has a good sense of humor.” |
Oswald, reread your own comments in #1801, for example: “are you a bit dense?” You won't find anything comparable on my side. I wrote this in response to what you said about the patterns in my replies. |
|
|
hi ilya,
most of them actually aren't controversial per se. you are just making them controversial by insisting on doing things your (demonstrably bad) way.
the reluctance to break things in a strict sense would be reduced by drastically increasing the test coverage of the code. i've seen that you actually started work on that. there is also a looser sense of "breaking", namely architectural and feature decisions that impair overall usability, stability, maintainability, extensibility, etc. This is an area where your approach is synergistic with the personell shortage of the project to achieve the worst possible outcome. under these conditions you are quite right about the outcome, but that's a self-imposed limitation.
well, you should have made that clearer. you were addressing me personally, and the paragraph can be reasonably read as a response to me.
no. you are just calmly driving me insane. and presumably yuri as well, though he is trying to be pro-social, unlike me. |
|
Hi Oswald, By the way, I spent two weekends going through all the places where the project name appeared, renamed the repository, and changed quite a few other things as well. So at this point, I think mc is safe from any naming-related threat. |
5890aef to
8abea4f
Compare
|
Please move the following text
to the first commit of the branch. I will approve this PR. |
…s files and detect line break type
editor(editbuffer): Add line break helpers to the text buffer
Add three text buffer helpers that will be used to treat a Windows
("\r\n") line break as a single unit:
- edit_buffer_is_crlf() checks whether the buffer contains a "\r\n"
line break starting at the given position.
- edit_buffer_detect_line_breaks() detects the line break type of the
buffer content: LB_WIN if all line breaks are "\r\n", LB_UNIX if all
are "\n", LB_MAC if all are "\r" and LB_ASIS for a mixture.
- edit_buffer_trailing_ws_start() returns the start offset of the
trailing whitespace (spaces and tabs) run of a line; for a CRLF line
the line content ends at the "\r" of its line break, not at the "\n".
Signed-off-by: Kuzin Andrey <kuzinandrey@yandex.ru>
…s files and detect line break type editor(edit): Extract edit_insert_line_break() helper Collect all line break insertion into a single edit_insert_line_break() helper and use it instead of direct edit_insert (edit, '\n') calls in edit_double_newline(), check_and_wrap_line() and the CK_Enter/CK_Return key handling. This is a pure refactoring without any behavior change; the helper will be extended with the line break type inheritance in a follow-up commit. Signed-off-by: Kuzin Andrey <kuzinandrey@yandex.ru>
…s files and detect line break type
Show the line break type of the file being edited as a word at end
of the status line: <LF> - all "\n", <CRLF> - all "\r\n", <CR> - all "\r"
and <?> - a mixture of line breaks or no line breaks.
The type is detected by edit_buffer_detect_line_breaks() and cached
in the buffer (edit_buffer_refresh_line_breaks()/edit_buffer_get_line_breaks()).
Hide the "\r" of a "\r\n" line break in files with pure Windows line
breaks. In any other file (a mixture of line breaks, Unix or Mac) the
"\r" is shown as "^M", so CRLF lines stay visible, e.g. in a patch
containing diffs of files saved with both "\n" and "\r\n". A
standalone "\r" (Mac) is rendered as "^M" instead of as a tab.
A "\r\n" line break is a single unit in every file: the End key and
Delete/Backspace stop at / remove the whole pair, line counting and
cursor columns ignore the "\r" part. A line break inserted with Enter
inherits the type of the current line ("\r\n" or "\n"); for the last
line of a file the previous line's type is used. The "\r" of the pair
is not an editable character: pressing Enter at the end of a CRLF line
does not duplicate the "\r" and the cursor stays before it.
File content is kept raw in the buffer: mixed line breaks are
preserved as-is on save (the default LB_ASIS mode), the Save As dialog
still offers conversion to a single line break type. Paragraph
formatting skips CRLF lines as reformatting would mangle them, and the
search end-of-line symbol is always "\n".
Fix edit_write_stream() appending an extra "\n" to files that already
end with a line break in conversion modes.
Add unit tests for line break detection, conversion, inheritance and
atomic deletion of CRLF lines in files with mixed line breaks
(tests/src/editor/edit_line_breaks.c).
Signed-off-by: Kuzin Andrey <kuzinandrey@yandex.ru>
8abea4f to
0200ba9
Compare
|
Approval from me. @zyv what do you think? There is no need to put the "Ticket #XXXX: some_brief_description" in the each commit of branch, but in the first commit only. |
|
it makes no sense at all to put such a commit message into preparatory commits; it's totally confusing and thus counter-productive. imo, only the "main" commit in the middle should mention the ticket. |
In as far as the intent and interface are concerned, I think this is a very welcome improvement that addresses a long-standing issue, so it should be merged, if done properly. Regarding the implementation - I didn't have a look until now, and I can ask to run a scan, but I won't be able to have a look until I'm back on Thursday. I think it would be worth doing, because my previous attempts have turned up serious data corruption issues so far. |
Block delete, block move, undo of Enter and Delete, delete word left, overwrite mode and regex replace of "\r" must remove exactly the bytes they are meant to remove in a text with "\r\n" line breaks. Assisted-By: Claude Opus 5.5 Signed-off-by: Yury V. Zaytsev <yury@shurup.com>
A fixed /tmp path collides when the test suite runs concurrently, e.g. from two build directories, and ignores TMPDIR. Assisted-By: Claude Opus 5.5 Signed-off-by: Yury V. Zaytsev <yury@shurup.com>
edit_delete() and edit_backspace() are byte primitives: block delete, block move, search and replace, undo and redo call them once per byte and expect exactly one byte to be removed. Making them remove both bytes of a "\r\n" pair deleted unselected text after a block, lost the "\r" in block move, joined lines in "replace \r with nothing", and made undo of Enter delete the character before the line break. Keep the primitives byte-exact and handle the "\r\n" unit in the user-facing commands instead: Delete, Backspace, delete word left and right, and delete line. In overwrite mode do not overwrite the "\r" of a "\r\n" line break, which joined the line with the next one. Assisted-By: Claude Opus 5.5 Signed-off-by: Yury V. Zaytsev <yury@shurup.com>
Left and Right moved the cursor by one byte, so the cursor could stop between the "\r" and the "\n" of a "\r\n" line break, at the same screen position as before the hidden "\r". Enter pressed there added a bare "\n", and a typed character split the line break. Both turned a pure CRLF file into a file with mixed line breaks, where every "\r" is shown as "^M" again. Move over a "\r\n" line break as a unit. If the cursor still ends up inside the pair (e.g. after a search), insert a new line break before the "\r". Assisted-By: Claude Opus 5.5 Signed-off-by: Yury V. Zaytsev <yury@shurup.com>
|
@mc-worker I've just got the review results back from the model, and I think that if you agree with the patches that I have just committed, this PR can be merged, or you have to tell me where the model got it wrong :) Unfortunately, I'm about to leave and cannot look into it in detail, but I have committed the tests patches first, which prove that the issues do exist and they fail in CI, as well as two fixes, which make the CI green again, and the explanation seems sound to me as well. I have added the abridged summary without the details below and can share the full Markdown file with you if you want, but I don't have the capacity to work it through. Let me know if this helps.
F8 (Medium, design decision): CRLF is treated as a unit even where
|
|
Весь PR написан относительно тупой локальной нейронкой qwen3.8-27b и Клод всегда в её решениях находит кучу проблем (неоднократно проверено на рабочих задачах коллегами с корпоративной подпиской), но работать с Клодом из России проблематично (по понятным причинам). Жалко просто так жечь токены на ревью, поэтому можно либо доработать то что есть Клодом (возможно это пара дорабатывающих коммитов), либо переделать с нуля. Я конечно могу все выявленные проблемы закинуть в новую сессию агенту, но на выходе можно получить что-то опять непотребное, требующее доработки, потому что он начинает неконтролируемо использовать в самых мельчайших тонкостях не самые лучшие паттерны и решения (даже многие типы передаваемых аргументов в функции со стороны @mc-worker было предложено переработать, что я сделал вручную). |
У меня вопрос для понимания: то, что я написал на английском, оно вообще не осознаваемое? Спрашиваю потому, что мне неясно, как связан этот текст с моим постом. К нему у меня только один комментарий: видимо мы уже достигли уровня "развития", когда вариант "подумать самому" не рассматривается в принципе. Все найденные критические проблемы связаны с перемещением в режиме CRLF и, по сути, имеют одну природу - обработка "\r\n" как единицы делается на уровне, где функции оперируют байтами. Поэтому, либо надо переделывать всю внутреннюю логику, либо вынести это на уровень выше, что, на мой взгляд, более правильно. Патчи, которые я запушил добавляют тесты, а потом исправляют эти проблемы. Проблема с TMPDIR в тестах тоже исправлена. Открыт только вопрос, как правильно работать со смешанными файлами; я вижу это так как написано, но интересно мнение Андрея. Либо он может сделать патч, либо я могу сгенерировать, если он захочет посмотреть. Из остальных тем все можно оставить "на потом" или вообще проигнорировать (F6, F9, F10, F11) - это просто для информации. |
|
Наверно я просто неправильно сделал допущение, что "\r\n" можно за один "символ переноса" принять поэтому разработка пошла не по тому пути. |
|
guys, please don't randomly switch to russian. of course you can use it for composing the text, but put the result through deepl or an llm (and maybe do some manual touch-ups) before posting. thanks. i think the AI's recommendation for F8 is correct. i'd squash the fixes into their "parent" commits when they are deemed correct (taking care to keep relevant parts of the commit message, in particular trailers). |
5 participants
Resolves:
Proposed changes
This is opencode AI-driven improvement in editor for support edition of Win (CRLF) files (and with mixed line ends).
Current version of editor didn't support CRLF (win) files (show only ^M at line ends), adding any new line in such file was difficult, because it add all lines in LF (unix) mode. Now it automatically detect current line-end-style in file and can add new lines with right EOL (U - \n, W - \r\n, M - \r).
Before:
After:
Checklist
git commit --amend -smake indent && make check)