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

feat: export and import document content #22

Merged
merged 8 commits into from
Dec 24, 2022

Conversation

benmuth
Copy link
Contributor

@benmuth benmuth commented Dec 20, 2022

Added the ability to save and load files. If the client is run with the '-file' flag, the user will be prompted to enter the name of a file to save to and load from. Pressing Ctrl-s in the editor will overwrite the contents of the file with the contents of the CRDT document. Pressing Ctrl-l will update the editor with the contents of the file, and sync every other client's editor as well.

Some possible improvements for this feature:

  • A better way to select files
  • The ability to change what file you're saving to or loading from
  • A warning, message, or prompt that a sync is about to overwrite the contents of the editor

Documents can now be saved and loaded if the program is started with a
-file flag, at which time the user is prompted to enter the name of a
file to use for saving and loading. Currently documents are only loaded
on initialization, but saving happens whenever the user presses Ctrl-s.

Future:
- Load at any time. Loading should update the document of everyone in
the session using document synchronization.
- More integrated file picking. Users should be able to change the file
they want to save to and load from.
Ctrl-l now loads the contents of the file as a CRDT document at any
time. To make the implementation cleaner, all passing of the document
between functions is removed. Every modification to the document only
happens to the global document variable, so that an update to the
document in one function easily propogates everywhere.
Right now, one client loading a document doesn't sync up with any other
client in the session, so this implementation doesn't yet work in a
multi-client setting.
Whenever a client loads a document with Ctrl-l, the document state is
now synced across all clients. I used the already existing document
syncing channel for this but changed the name to better reflect its
expanded use. Also added handling for the case where there is no file
entered by the client.
client/main.go Outdated Show resolved Hide resolved
Copy link
Owner

@burntcarrot burntcarrot left a comment

Choose a reason for hiding this comment

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

@benmuth Thank you for working on this! Looks good to me; could you please fix the linter issues? I'll be happy to do a review once the issues are fixed.

@burntcarrot
Copy link
Owner

A better way to select files

I think a dialog box would work. Not sure about how it would be implemented using termbox.

The ability to change what file you're saving to or loading from

I'd personally prefer a telescope-like navigation system, which is common in a lot of editors. We could probably use pre-built file picker "components" made for termbox. If implementing that would be too hard, then, we could move back to the charm.sh ecosystem, if possible.

A warning, message, or prompt that a sync is about to overwrite the contents of the editor

This might be tricky. If a client refuses to sync, then they'd be considered to be out of sync. I'll need to think about the suitable behavior for this situation - I'd love ideas from your side as well! The best we could do is to display a prompt/warning in the status bar that new changes are incoming; with some prettified red/orange-colored text.

@burntcarrot burntcarrot linked an issue Dec 20, 2022 that may be closed by this pull request
@benmuth
Copy link
Contributor Author

benmuth commented Dec 20, 2022

I'd personally prefer a telescope-like navigation system, which is common in a lot of editors.

I think this would be my preference as well.

The best we could do is to display a prompt/warning in the status bar that new changes are incoming; with some prettified red/orange-colored text.

The only other thing I would add to that is the possibility of 'pausing' everyone's editor for a brief period while displaying the notification so that the change isn't as jarring.

@burntcarrot
Copy link
Owner

The only other thing I would add to that is the possibility of 'pausing' everyone's editor for a brief period while displaying the notification so that the change isn't as jarring.

"Pausing" seems like a challenge here; what happens to local edits, or the editor state in this case?

server/main.go Outdated Show resolved Hide resolved
crdt/woot.go Outdated Show resolved Hide resolved
crdt/woot.go Outdated Show resolved Hide resolved
Copy link
Owner

@burntcarrot burntcarrot left a comment

Choose a reason for hiding this comment

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

@benmuth Looks good to me! Would love if you could write some unit tests for the CRDT.

@benmuth
Copy link
Contributor Author

benmuth commented Dec 21, 2022

The only other thing I would add to that is the possibility of 'pausing' everyone's editor for a brief period while displaying the notification so that the change isn't as jarring.

"Pausing" seems like a challenge here; what happens to local edits, or the editor state in this case?

I was thinking that everyone's editor would stop accepting local termbox events for a brief period while a notification is displayed, like "burntcarrot is loading a new file, please wait". Remote edits could still propagate until everyone is in sync.

I'm trying to avoid a scenario where multiple people are actively typing at the same instance when someone loads a new file, in which case there will be some unintended edits that have to be undone. A pause of a second or two would make sure everyone stops typing and help everyone adjust to the new document, ensuring they're making the edits they intend to.

@burntcarrot
Copy link
Owner

I was thinking that everyone's editor would stop accepting local termbox events for a brief period while a notification is displayed, like "burntcarrot is loading a new file, please wait". Remote edits could still propagate until everyone is in sync.

That seems like a nice way to handle local edits. How do we "stop" accepting termbox events though? There needs to exist some sort of a mechanism where events being sent to the channel could be paused for some time.

Copy link
Owner

@burntcarrot burntcarrot left a comment

Choose a reason for hiding this comment

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

Looks good to me! I'll test this out locally today. Once done, it's ready to merge! 🚀

@burntcarrot
Copy link
Owner

@benmuth Tested it locally, works great! Had to fix the file permission for writing to a local file, it kept on making the file write-protected, which made me unable to view the contents using cat.

@burntcarrot burntcarrot changed the title Persistence feat: export and import document content Dec 24, 2022
@burntcarrot burntcarrot merged commit 86779c0 into burntcarrot:main Dec 24, 2022
@benmuth benmuth deleted the persistence branch December 24, 2022 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Export and import document content
2 participants