Skip to content
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

Use dialog for external changes #1309

Merged
merged 46 commits into from
Jul 9, 2023
Merged

Conversation

jeremypw
Copy link
Collaborator

@jeremypw jeremypw commented Apr 16, 2023

Fixes #965
Fixes #1327

Following #1308, use dialog instead of infobar when there are external changes

  • Add external change dialog inline
  • Connect focus in signal at same time as focus out signal
  • Check file status before autosaving
  • Set buffer modified state to false after save and after loading external changes
  • Remove dependence on autosave setting
  • Dialog text reflects whether there are unsaved internal changes
  • Fix issue with new documents by opening them the same way as existing documents
  • Do not show dialog when no unsaved changes e.g. when git branch changed
  • Loading external changes is undoable
  • Lose now unused infobar

The next Code release is waiting for this to be merged.

Base automatically changed from ask-savelocation-dialog to master April 26, 2023 21:28
@jeremypw jeremypw mentioned this pull request May 25, 2023
@jeremypw jeremypw mentioned this pull request Jun 4, 2023
10 tasks
@jeremypw jeremypw requested a review from a team June 17, 2023 08:50
@jeremypw jeremypw added the Priority: High To be addressed after any critical issues label Jun 17, 2023
Copy link
Contributor

@zeebok zeebok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good. Gonna give a test over the next day or two for you.

src/Services/Document.vala Outdated Show resolved Hide resolved
@zeebok
Copy link
Contributor

zeebok commented Jul 6, 2023

Trying to test by editing the same file in nano is causing two things:

  1. I don't see the dialog appear, it simply refreshes the doc with the new version. If autosave is off it still refreshes but the document tab has a * indicating that it isn't saved.
  2. If I get to the scenario of the doc refreshing from an external change and perform an undo, the whole document clears. If I undo again the previous version comes back as would be expected.

@jeremypw
Copy link
Collaborator Author

jeremypw commented Jul 6, 2023

@zeebok Thanks very much for testing this out. Autoloading of external changes is mention in the comments above and is motivated by the need to change git branches without triggering the dialog unnecessarily. The changes should only autoload if there are no unsaved changes to the document.

There is a small improvement wrt master in that the behaviour is the same regardless of the autosave setting - even if autosave is "off" external changes are loaded immediately if there are no unsaved internal edit and in this case the document is not marked as "unsaved".

There is an open question as to whether there needs to be a toast or something indicating the changes have been loaded. Since the changes were presumably made by the same user not sure this is needed.

Undoing shouldn't clear the document though - I'll look into that.

You should get the dialog if there are conflicting changes made internally and externally which in effect means "autosave" is off.

I used gedit when testing as that has its own conflict warning system.

@jeremypw
Copy link
Collaborator Author

jeremypw commented Jul 6, 2023

Regarding the undo issue: It seems that setting the buffer text programmatically records two actions in the buffer undo manager - "delete old text" and "insert new text", hence it takes two <Ctrl>Z to get back to the old text. Not sure if there is a way round this.

@jeremypw jeremypw marked this pull request as draft July 6, 2023 10:09
@jeremypw
Copy link
Collaborator Author

jeremypw commented Jul 6, 2023

I have achieved single-click undo of autoload by using copy/paste rather than just setting the buffer text. This is at the cost of more code and dealing with the asynchronous nature of pasting. Its not quite perfect because after undo the old text remains selected but selection is not recorded by the undo manager so cannot be undone. Questionable whether this is worth the extra complexity as this would be likely to be used very rarely in practice.

@jeremypw jeremypw marked this pull request as ready for review July 6, 2023 11:28
@zeebok
Copy link
Contributor

zeebok commented Jul 7, 2023

Would a better path be to clear the undo history so trying to undo doesn't do anything?

@zeebok
Copy link
Contributor

zeebok commented Jul 7, 2023

Okay now that I know that I can confirm the detection and dialog work as describe! I would say once we settle on what to do about undo, this is good to merge.

@jeremypw
Copy link
Collaborator Author

jeremypw commented Jul 7, 2023

It would be simpler to disable undoing changes made by autoloading external changes, if that is acceptable.

@zeebok
Copy link
Contributor

zeebok commented Jul 8, 2023

I think it is but I am just a single user

@jeremypw
Copy link
Collaborator Author

jeremypw commented Jul 9, 2023

@zeebok I agree with you. Most users would probably rarely, if ever, trigger this code anyway.

@jeremypw jeremypw merged commit 0c49cb7 into master Jul 9, 2023
@jeremypw jeremypw deleted the asksave-externalchange-dialog branch July 9, 2023 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: High To be addressed after any critical issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Git branch change can result in unwanted warnings Undo-ing sometimes deletes the entire contents of a file
2 participants