feat: add up/down key support #24
Conversation
* Add support for newlines. * Fix newline and deletion bugs in editor. Applied patch that changes Editor struct to have one cursor field instead of having x and y fields. Changed performOperation() to operate on the absolute cursor position, not just the x value. Fixed deletion by making cursor updating happen after the operation and removing some "-1"s from the cursor positions. * Start to implement document syncing. * Consolidate calls to e.Draw. Changed handleTermboxEvent to only call e.Draw once at the end of the function. Removed a call to e.Draw in performOperation. * WIP: adding flag for login process * Make login process automatic. Added a flag to make the manual login process optional for faster iteration. The default behavior is now to use automatically generated random names. Used the github library github.com/Pallinder/go-randomdata for random name generation. * Add document syncing for new session members (WIP) Got document syncing working for new session members, but current architecture is still rough. The intention is to move more document syncing logic to handlMsg. * Refactor doc sync Changed the doc sync code path to be a bit more clear. Some work could still be done, like moving the whole document syncing process into its own function. Slightly modified client-side logging to be a bit more useful. * Improve error handling Incorporated error handling changes from a patch. * Add unique site id generation Server now generates unique site ids and assigns them to clients. Also added more logging. * chore: cleanup * ci: fix linter issues Co-authored-by: Ben Muthalaly <benmuthalaly@gmail.com> Co-authored-by: burntcarrot <aadhav.n1@gmail.com>
Added unit tests for the termbox-based editor.
* add editor status bar for displaying messages when a new user joins and when the client loses the connection * refactor editor a separate package
* fix defer calls on file and WebSocket close * add server read and write timeout * use single layer for running commands in Dockerfile * cleanup control flow
Up and down arrows now have some behavior, but it is currently not as intended.
Movement is working, but behavior is still undefined at the and end of files, so there are a few bugs to be fixed.
Newline index variables n1 and n2 now initialize to -1 to avoid conflict when running into the beginning of the document.
Down arrow behavior seems to be working in standard use and some edge cases. Up arrow has not been fixed.
Got an implementation of up/down arrow behavior working. The implementation is rather convoluted and hard to reason about. There is definitely room for tightening the logic and improving readability. It's also worth considering a change to the way editor text is currently represented, so that it maps more closely to Termbox's 2D structure. A 2D array of runes might make a more natural interface between the linear CRDT document and the 2D Termbox terminal, and therefore make the code cleaner and more maintainable. Future: - tighten logic - remove logging - refactor (explore alternate ways to store editor text)
Sure, those stand for 'current line start' and 'previous line start'. There's also If my cursor was on the first character on the second line, These are used to calculate the length of the line (for instance, when moving the cursor down a line, 'next line end' - 'current line end' = length of next line) to see where the cursor should be placed. If the cursor 'offset' from the start of the line is greater than the length of the next line, then the cursor should just be placed at the end of the next line. Or, if nle-cle < offset {return nle}. I kept those variables separate instead of just storing the line length in its own variable, because the line length is only necessary for one check (to see if the cursor offset is greater than the length of the line to move to) while the separate variables are useful for checking edge cases like moving to the first or last line in the document. |
|
Thank you for the explanation! It would be great if you could:
Writing unit tests will help in finding out certain edge cases in the current implementation. |
Fixed a couple of crashes that happened due to out of bounds errors if the document was empty. MoveCursor would try to do some invalid calculations which are now avoided by checking the text length first. Also, on delete, the cursor was getting reset to 1 instead of 0, which caused a crash when characters were inserted.
Added many tests for cursor movement. Still in the process of fixing the bugs found in the tests. Also temporarily added logging to help with bug fixing.
Rewrote calcCursor up with slightly different logic. It is simpler and cleaner than the previous attempts. Further refinement might be possible.
Changed calcCursorDown to match calcCursorUp logic. Both functions now pass tests.
Fixed a couple more edge cases with the new cursor movement logic, including some cases where panics occurred. Behavior now seems to match expectations. Also updated comments for both calcCursorUp and calcCursorDown.
|
Sorry for the crazy delay! I added test cases and found bugs (of course) but got sidetracked with some other stuff before I got around to fixing it. |
|
Thanks for the update! Looks good to me; I'll try testing it locally once. |
There was a problem hiding this comment.
Tried running this locally, but I still have a character remaining in the editor, and I cannot delete it:
a
I'm not sure where this is originating from. The server logs say that the document doesn't have any characters, so it must be the UI that's displaying the character:
writing message to: a968f017-4822-4232-af35-ef5bacd446b2, msg: {Username: Text: Type:operation ID:c173cb43-1848-4a31-a377-492e1dbc2ba9 Operation:{Type:delete Position:0 Value:} Document:{Characters:[]}}
|
Right, my mistake. That was a holdover of a previous fix to handle panics that could occur with empty documents (e425b85). Should be working now. |
|
@benmuth Great work on this one, thanks! Merging! |
Up/down cursor movement is working. The solution here is to note the cursor offset from the beginning of the line, and the length of the adjacent line to be moved to. If the offset is greater than the length of the adjacent line, move to the end of the adjacent line. Otherwise move to the adjacent line at the same offset.
The code is a bit messy, mostly due to dealing with edge cases, and can probably be cleaned up.
I'd be happy to hear any suggestions for improvements or see an alternate implementation.