This repository has been archived by the owner on Jul 9, 2020. It is now read-only.
Implement up/down scrolling (CTRL-E, CTRL-Y, CTRL-U, CTRL-D) #58
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
+ Subdivide the monolith EditorTypes.fs + Remove CoreEvent.BufferAdded + Add BufferEventMessage and BufferEvent message types + Added FileBufferProxy so that the FileBuffer, which is likely going to be a complex object and quite subject to change and probably subdivision as we go and shouldn't be depended on by other services, especially not in other assembly, could be made fully private. Even though I had made its constructor private, I still did not want the view model calling out to even Buffer.readlines.
I have realized that NormalMode is a much bigger thing and that this module/service's responsibility is really only the bindings portion of it.
Began to realize my request/response stuff was badly mangled...
Still need to update the unit tests
I'm glad to see that we have tests that catch this kind of error
which the window buffer map service then maps onto a request/response for the appropriate buffer's contents
I went in to make my changes for scrolling, and once again the huge mass that is the view model hindered me. I was somewhat at a loss for what to do, so I thought I would start by finishing my decoupling of the command bar, which I have already decoupled to a large degree, thinking that might be instructive on how to proceed with scrolling in windows. In Go terms, tesuji. I've played a move elsewhere.
I have to break this thing down small enough that it is possible to work with it
Window is still not redrawing properly, however
Though without bounds checking, CTRL-E and CTRL-Y are somewhat working! Just in time to start work for the morning.
Not sure why I wasn't getting that good and appropriate error locally.
Also sketched out some ideas for other tests. I had observed this bug when testing manually but hadn't really understood it. The unit test showed me what it was right away.
They were low-hanging fruit now we have CTRL-E and CTRL-Y, are more useful anyway, and are good for testing.
Before submitting the PR, still need to:
1. Write tests for scrolling half screens
2. Refine boundary conditions for when scrolling multiple lines
3. Clean up duplication in window scrolling unit tests
4. Add any integration tests or "get buffer contents" request tests as
appropriate
Making the former "bus implementation" just a facade for the messaging system which implements the bus interface.
and move the messaging code to Void.Core so anything can use it (while at the same time providing loose coupling for the potential of future extraction of Void.Messaging).
…specs passing by fixing the window scrolling code
|
Looking now! |
|
Thanks! Good luck though, it's huge... On the other hand, a lot of it is pretty clean, fresh stuff. |
| type NoMessage = | ||
| | NoMessage | ||
| interface Message | ||
| type NoMessage = NoMessage interface Message |
There was a problem hiding this comment.
Just curious, was this picked up by a warning?
There was a problem hiding this comment.
No... not sure why I did that actually. Stylistic thrashing I guess. :)
The UI is in C#, so it wasn't building that
| @@ -40,7 +40,7 @@ public Message HandleViewModelEvent(VMEvent eventMsg) | |||
| { | |||
| if (eventMsg.IsViewPortionRendered) | |||
| { | |||
| _drawings = ((VMEvent.ViewPortionRendered)eventMsg).Item2; | |||
| _drawings.AddRange(((VMEvent.ViewPortionRendered)eventMsg).Item2); | |||
There was a problem hiding this comment.
Exactly. Because I didn't name the elements, effectively it's a tuple, and that's how you access the second element from C#. Not exactly the smoothest interop, but oh well.
|
Well, I looked through everything! I definitely don't understand everything going on here, but what I'm getting looks good. Is this good to merge? |
|
Thanks for looking at it! I'm good with it when you are. Have you pulled it down and tried it out? |
|
Not yet! Will do, though. |
Kazark
added a commit
that referenced
this pull request
Oct 24, 2015
Implement up/down scrolling (CTRL-E, CTRL-Y, CTRL-U, CTRL-D)
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This implements the ability to scroll up and down in a window, with the proper bounds checking. Specify, four normal mode commands have been implemented:
Due to F#'s composable (algebraic) data types and units of measure, this is thoroughly configurable just by bindings, without even any configuration values, i.e. we should not implement the
&scrolloption. You can just rebind one of these keys to a different command with a different number of lines or half screens, etc. Very very cool. Programmable rather than configurable.@mjgpy3 Sorry for the humongous pull request! Do you want to field this one?
I had to do some technical changes to get this to work, specifically in the messaging system.
This is a solid basis for getting
h,j,kandlworking, which I will move to release three, and consider this the last thing for this release since it has dragged on a bit.