Tell the frontend when a file has been changed since opening it

[Cogitri]

This would be a continuation of #538, I suppose. Right now xi only automatically reloads the file if no changes have been done by xi yet, but it'd be nice if xi-editor told the frontend that the file has changed and as such the user should reconsider saving.

[mikaelmello]

I'm interested in tackling this issue and thus asking some questions:

After reading the modifications made in #538, I guess that we would like to send a rpc notification here:

xi-editor/rust/core-lib/src/tabs.rs

Lines 783 to 785 in 4afbfd8

	} else {
	ed._set_file_has_changed(true);
	}

, asking if the user would like to save the current changes or reload the file.

If the user chooses to reload the file, my guess is that the our approach would be to basically call ed.reload(&contents).

But, if the user decides to save its current file, the do_save function would be called and we would get to these lines:

xi-editor/rust/core-lib/src/tabs.rs

Lines 583 to 589 in 4afbfd8

	if self.buffers.lock().editor_for_view(view_id)
	.unwrap().get_file_has_changed() {
	let err_msg = format!("File {:?} has changed on disk. \
	Please save as something else. \
	I'm sorry if this is annoying.", file_path);
	peer.send_rpc_notification("alert", &json!({"msg": err_msg}));
	return

My question is: since file_has_changed is set to true, would this mean that this error message would be sent? Or is that sent only on a specific race condition that I'm failing to see? The comments say about being already open, but I don't see any open call on this function. Should we then set file_has_changed to false if the user decides to save and overwrite changes from outside?

Edit: By the way, what happened to inline code previews on comments?

[Cogitri]

asking if the user would like to save the current changes or reload the file.

I don't think the user ever wants to reload the file (which discards his edits). Instead we should ask it the user wants to either:

• Save the file anyway
• Save to another file
• Cancel (and do something on his own)

It would be really fancy if xi even send a diff to the frontend, but I guess that's out or scope or this (and seems clunky)

Edit: By the way, what happened to inline code previews on comments?

I don't think that went anywhere

[mikaelmello]

As an avid user of text editors (aren't we all?), I can remember multiple times when I wanted to reload the file and VS asked me if I wanted that. Mainly when updating the files from git commands, probably a hard reset, a checkout, etc, you get the idea. There's also the situation when I wanted to reload all files, probably nuking my latest failed change. I don't think that saving to another path would be appropriate? It's something I've never seen and never thought about it until now. From my point of view, copying the behavior from VS or VSCode, an approach that I like is to offer a reload or a cancel. If the user wants to reload, great. Otherwise we simply maintain the editor in its current state and then the user can decide whether to save (overwriting), save to another file, discarding, etc

[mikaelmello]

My general point of view is to follow the current industry standards and improve from there, since I'm new to this community I don't know if that is also your approach or if you'd prefer to innovate from the very beginning Cheers!

[Cogitri]

Yeah, on 2nd thought maybe it should be "Save to another file and reload" instead (or some other measure to undo that action ane have the previous state again).

My workflow when reloading files in CLion usually is:

• Cancel action
• Save to another file
• Then reload

To make sure I don't abandon valuable changes. CLion also offers the ability to undo the reloading via Ctrl+Z though

[mikaelmello]

When I get home later today I'll start to work on it. In the meantime it would be nice if more people from the community could chime in

[mikaelmello]

CLion also offers the ability to undo the reloading via Ctrl+Z though

I agree that this is important.

To replicate this behavior I usually reload, Ctrl z, copy everything and Ctrl Y to do redo the reload, it rarely happens but I see that it's a bit too much.

Perhaps we should have three options:

• Save temp and reload (we should work on a smaller sentence to fit in a button)
• reload (discarding changes)
• cancel (keep state, letting the user decide what to do)

[Cogitri]

I miss a 'continue and overwrite' option

[mikaelmello]

That would be doable by Cancel -> Ctrl+S

It is an important flow but which buttons are the most important ones?

[Cogitri]

Huh? Won't the user be offered the three options you've mentioned previously be asked again and as such be stuck in an endless loop?

[mikaelmello]

The dialog would only appear when an external change was detected, isn't that so?

Then the user would possibly want some things:

1. Keep the current content
   1.1. Save the current content overwriting the external changes
   1.2. Do not save yet, possibly wanting to do more changes, or check the external changes somewhere else
2. Save the current content somewhere and load the external changes
3. Discard the current content and load the file

In order to achieve each one of them, the user can:

1.1. Have a button that does exactly that or a "Cancel" button that does nothing and then press Ctrl+S
1.2. Hit the "Cancel" button
2. That is the most tricky one, we can have a direct button or let the user click "Cancel", manually save the file somewhere else and manually reloading the file
3. A "Reload" button

Now it's just a matter of deciding which buttons appear in the dialog

[Cogitri]

1.1. Have a button that does exactly that or a "Cancel" button that does nothing and then press Ctrl+S

Yes, but wouldn't xi complain that the file has been changed since opening again during save, opening the same dialog at that point?

[mikaelmello]

Yes, but wouldn't xi complain that the file has been changed since opening again during save, opening the same dialog at that point?

Hmm. I think you have a point.

In my mind, once we show this dialong upon detecting external changes, if we decide to not act on them, the file still has them, the editor has the your current edit and upon receiving subsequent actions, we act like if there wasn't any changes. Unless new changes happened, prompting a new dialog

[Cogitri]

I thought about that too, but that seems to be a little too magic to me, what if a user hits cancel, but then doesn't make changes, remembers to save his changes at some later point and then accidentally overwrites his stuff? (Maybe that's too artificial tho)

[mikaelmello]

what if a user hits cancel, but then doesn't make changes, remembers to save his changes at some later point and then accidentally overwrites his stuff? (Maybe that's too artificial tho)

I see that this could be a problem for some users. But, at least for me, seems something that isn`t that common, and maybe if he hit cancel and forgot about it, it's on him?

Here is the behavior of VS after detecting changes on an edited open file:

On VSCode, things are a little different, no pop-up is displayed and when we try to save a file that is newer on disk, we get this:

Getting then a diff view (exactly as you suggested! I didn't know about this feature up until now):

I guess that sums up the two paths we can take:

1. Act on detection of changes
2. Act on saving upon a newer file on disk

Also, as a core and not a full editor, couldn't we provide both options, leaving the front-end to decide?

[cmyr]

It's worth noting that we currently will not save a file if it has changed on disk; if they try, we will send an alert, and the user has to save elsewhere.

This is definitely 'placeholder' behaviour; for the time being it's most important that we don't silently overwrite anything.

Being able to open a diff would be nice, but we don't have built-in diffing yet.

Until that is possible, I'm not sure what the best behaviour is; everything feels equally dissatisfying. 😒

[mikaelmello]

I'm more a fan of the VS approach, not sure which one you guys prefer

[cmyr]

yes, I think that is the most reasonable approach for now.

I would implement this as a new core -> client notification, first; the client can handle the logic of presenting the dialog.

Currently core has a flag that gets set when a file has changes; if we're going to clear that flag, we would need a new client -> core notification that handles that, and then we'll be able to save as normal; otherwise we want to just force load the changes on disk.

This sounds like a pretty well-scoped project, and I'd encourage you to dig into it @mikaelmello!

[cmyr]

okay this looks good but it needs an implementation in xi-mac to merge.

I'll open an issue, and if nobody gets around to this I can do it early next week.

[mikaelmello]

Great! I don't own a mac so my options are limited