Garbled contents when reloading file with DOS line endings

[scurest]

Garbled contents when reloading file with DOS line endings

[Gavin-Holt]

Hi

I noted this problem a while back (#2756), but could not make it reproducible.

At that time I assumed it was due to my inferior OS!

We should be careful not to confuse reopen and reload these are different commands:

• reopen : undocumented command to reopen current buffer using bp.Buf:ReOpen() function
• reload : this command reloads all runtime files (bindings.json, settings.json, init.lua, ?plugins?)

Confusingly, settings.json includes an option named reload:

• "controls the reload behavior of the current buffer in case the file has changed. The available options are prompt, auto & disabled" which calls bp.Buf:ReOpen() !

I would be interested in a fix, so my spellchecking function (#2744) correctly reloads altered files.

Kind Regards Gavin Holt

[dmaluka]

FWIW I can easily reproduce it, with the newest micro from master.

1. open a file with DOS line endings (\r\n)
2. open the same file in another editor (or even in another instance of micro), add any text somewhere in the middle of the file, and save it
3. reload the file (i.e. answer "yes" to the reload prompt) in the original instance of micro => result: the new text is inserted in the wrong place.

It's clearly a bug, and hopefully easy to fix (just need to find free time to do that).

We should be careful not to confuse reopen and reload these are different commands:

Yeah, they are doing completely different things.

• reopen : undocumented command to reopen current buffer using bp.Buf:ReOpen() function

Indeed, and it might be a good moment to document it.

• reload : this command reloads all runtime files (bindings.json, settings.json, init.lua, ?plugins?)

Yep, it reloads plugins too.
We are considering improving its documentation: #3240 (review)

Confusingly, settings.json includes an option named reload:

• "controls the reload behavior of the current buffer in case the file has changed. The available options are prompt, auto & disabled" which calls bp.Buf:ReOpen() !

The reopen command does the same thing that is done automatically if the reload option is set to prompt or auto, i.e. reloads the file. So it's not surprising that ReOpen() is called in both cases.

Yeah, it might be confusing that the reload command (not option) does a completely different thing.

[scurest]

Thanks for the pointer to reopen. So it looks like the error occurs in EventHandler.ApplyDiff. Here's a fragment of the diff operations performed

State of LineArray
  "569"
  "489"
  "45"

Diff to Process
  {"Equal" "9\r\n"} @ {X:2 Y:0}

State of LineArray
  "569"
  "489"
  "45"

Diff to Process
  {"Delete" "48"} @ {X:1 Y:1}      <- NOTE: should be {X:0 Y:1}

State of LineArray
  "569"
  "4"           <- NOTE: 89 deleted instead of 48
  "45"

So the error seems to be that when \r\n is encountered, the location advances two positions to the right, instead of only one. This misaligns the locations for all subsequent diff operations, which produces the garbled text.

[JoeKar]

So the error seems to be that when \r\n is encountered, the location advances two positions to the right, instead of only one.

Thank you for the great hint!

micro/internal/util/unicode.go

Line 90 in 35630aa

if !isMark(r) {

->

if r != '\r' && !isMark(r) {

Most probably there is a more elegant and especially generic way, since this was just shot out of my hip. 😉

@scurest:
Does the proposal work for you too?

[scurest]

ApplyDiff is also wrong in the presence of marks.

echo -e 'Te\u0302st' > test.txt  # COMBINING CIRCUMFLEX ACCENT
# Open in micro, shows 'Têst'
echo -e 'Test' > test.txt
# Reload in micro, still shows 'Têst'

[Gavin-Holt]

Hi,

ApplyDiff sounds complicated, and not strictly necessary for reopen.

Could reopen just read the whole disc copy and reposition the cursor to the last known editing location?

Kind Regards Gavin Holt

PS. I would also expect reopen to clear the undo stack.

[scurest]

ApplyDiff maintains the "logical" cursor position.

echo 'The bottle says DRINK ME' > test.txt
# Open in micro, 'The bottle says DRINK |ME' (| denotes cursor location)
echo 'The cake says EAT ME' > test.txt
# Reload in micro, 'The cake says EAT |ME' (cursor maintained at ME)

Is this the only reason it's used? I would also be fine giving this up for correct reopen.

Also the line-ending mode is not re-detected when reopening.

[JoeKar]

Is this the only reason it's used?

No. The main purpose is to handle every insertion and/or deletion independent to track it inside the undo buffer. Furthermore the modifications can be visualized with the diffgutter.
The cursor position is handled here:

micro/internal/buffer/buffer.go

Line 560 in e9bd1b3

b.RelocateCursors()

So bypassing or removing ApplyDiff is no option so far. It just need to work correctly in these scenarios.

Currently you broke it down to some minimal reproduction cases, which is good so far.
Just image the use case where you've a much larger file which is edited outside of you current buffer...maybe you would like to see the diff and maybe you'd like to undo something.