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(other): paste text from buffer to input with ctrl + v #1233

Closed
wants to merge 1 commit into from
Closed

feat(other): paste text from buffer to input with ctrl + v #1233

wants to merge 1 commit into from

Conversation

fanantoxa
Copy link
Contributor

@glennsl It works but I'm not sure that this is ideal implementation.
What to start with something. This kinda missing feature when you for example copied path of failed test from console (CI).

@fanantoxa fanantoxa requested a review from glennsl January 20, 2020 21:16
Copy link
Member

@glennsl glennsl left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this. Unfortunately it isn't quite so straight-forward. There's a few issues with this:

  1. We want the keyboard shortcut to be configurable
  2. We want side-effects to be encapsulated and restricted to a very small part of the code-base, partly because it makes the code easier to reason about, and partly because it makes it testable.
  3. We don't want this tightly coupled to SDL2

We already have a paste keybinding for the editor here:
https://github.com/onivim/oni2/blob/master/src/Store/KeyBindingsStoreConnector.re#L73-L82

We tend to use vscode's keybinding commands for compatibility when possible, but I'm not entirely sure which applies here as there are several, pase and default:paste, and they're unbound in my installation. paste seems like reasonable one to go with though.

Then the command needs to be handled. The editor paste command is handled in the vim storw:
https://github.com/onivim/oni2/blob/master/src/Store/VimStoreConnector.re#L756-L759

SDL2 is decoupled there by passing getClipboardText in as a plain function:
https://github.com/onivim/oni2/blob/master/src/Store/VimStoreConnector.re#L24

Because of the common side effect and decoupling I think it would be better to handle all paste commands in the same place, like the command store, then dispatch separate messages (ie. Actions.t variants) with the clipboard text after retrieving it. The new editor=spcific action can be handled in the vim store, where it is now, and the others basically the same way keyboard input is handled with routing based on focus, but using a new InputModel.paste function instead of InputModel.handleInput.

And that's it! It'll now be configurable, the side-effect encapsulate and SDL2 decoupled.

I hope this isn't too overwhelming, and should be a nice exercise for getting to know the architecture at least (which is a bit messy and confusing right now, and also in the middle of a major refactoring)! I think this is a great early task for getting a feel for things though. Let me know if you have any questions, or just feel a bit overwhelmed.

@fanantoxa
Copy link
Contributor Author

fanantoxa commented Jan 21, 2020

@glennsl Sounds REasonable! :)

In general I've got the point how to do that and will try to implement.

The most problems that I actually experience with implementing new stuff is typing system.
It sounds good but if you already know notes.
I think code might be much easier to work with if we'd add some type annotations.
It takes a lot of time to figure out what coming from where. (even with many years experience in other languages incl. functional)

Is that any easy way to get a hint on variable type for sublimetext or print it to console or smth? )

@glennsl
Copy link
Member

glennsl commented Jan 21, 2020

I don't know much about sublime text, but it seems to use the same language server that vscode does, which supports type hints. In vscode it's triggered by hovering over the definition, or typing gh when the cursor is on it. I (almost?) never need to use that myself though, even when first getting into a codebase.

Any module that exports definitions should have an interface file with type definitions for all the exports, and we're slowly moving towards that goal but a lot are unfortunately still missing. Otherwise there are usually plenty of hints other than type annotations, but it can take some getting used to seeing those. If there aren't any hints, that pretty much means it actually can be anything. There are still plenty of places where the code is structured badly so that the hints are too far away or there's just too much going on, and while local type annotations can certainly help there, it's usually better to refactor the code so they're not needed, which is happening, but slowly.

I was brand new to the codebase just three months ago myself, so I definitely see where you're coming from. But sounds like you're new to the language too, which of course doesn't help. What I struggled with the most was finding where the modules came from, because of inconsistent conventions, some being from Revery, and some being submodules that aren't searchable by filename. And the lack of imports in Reason certainly doesn't help. This has also gotten better though. There are fewer submodules and the structure is steadily improving, but still far to go. We could also be more explicit about external modules, like explicitly referencing or aliasing Revery.Color instead of opening Revery and using Color directly.

But let me know if you come across code that is really hard to follow. Then maybe I can explain it to you in a way that teaches you some new patterns and idioms that'll make it easier to understand similar code. Or I might just agree that it's really bad and refactor it right away :)

@fanantoxa
Copy link
Contributor Author

@glennsl Could you please add label WIP for it?
I think It'd be nice to implement ctrl+c, ctrl+A, ctrl+X along with that.
But here another bug issue that prevent implementation for any of those

@glennsl glennsl added the WIP label Jan 27, 2020
@glennsl glennsl changed the title Updated allow paste text from buffer to input with ctrl + v feat(other): paste text from buffer to input with ctrl + v May 5, 2020
@fanantoxa
Copy link
Contributor Author

This is already implemented.

@fanantoxa fanantoxa closed this Jan 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants