-
Notifications
You must be signed in to change notification settings - Fork 203
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
Add disable-hclip function #1089
base: master
Are you sure you want to change the base?
Add disable-hclip function #1089
Conversation
9acc55f
to
f06d982
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two main complaints, rest of the code changes will follow by themselves once addressed.
yi-core/src/Yi/Clip.hs
Outdated
import Yi.Keymap (YiM) | ||
|
||
clipboard :: IORef String | ||
clipboard = unsafePerformIO $ newIORef "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Don't store String, use YiRope straight away.
- Get rid of this global IORef completely, what's the point of it? YiM can carry the state you need. If you really want an IORef, put the IORef in YiM and update it that way.
getClipboard
can then determine where to read clipboard state from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a emulation of Hclip's function, so it uses String and a global variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- We do not need performance. Clipboard is poor one.
When vty-mode, Hclip calls a command ofxclip
, it is really slow.
So I think we do not need YiRope. - I know YiM can carry the clipboard data.
I just do not want to increase the mutable state of YiM.
If you do not want to use IORef by all means, I'll use the state of YiM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- We do not need performance. Clipboard is poor one.
There's zero reason to throw away performance. In this case it's more about convenience of any users: why force them to convert from string all the time? Further, String sucks for space, there's no reason to hold onto the clipboard contents as String and put completely unnecessary pressure on GHC's copying GC.
- I just do not want to increase the mutable state of YiM.
Just add it to YiM state, it's a lot easier to reason about than some random floating globals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @noughtmare ,
Do you have any comments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that quality is very important, especially when an objectively better solution is known. I understand that it is not always possible, a balance needs to be struck between effort and quality.
I will explain my interpretation of @Fuuzetsu's proposal in more detail. The proposal consists of two parts. Firstly, to have the virtual clipboard provide and store YiString
s, to prevent conversion between YiString
s and String
s which saves memory and CPU time. The system clipboard can then has to explicitly convert the YiString
s to String
s, because the system clipboard can't store String
s. The second part is to use the dynamic state inside the YiM
monad to store the virtual clipboard contents, instead of a loose reference.
I think the first part of the proposal is objectively an improvement. We don't expect performance from the clipboard, but we do expect it from the editor as a whole and many small performance deficiencies can add up.
The second part, however, is not so clear-cut. The loose reference is not exposed by the Yi.Clip
module, so it will not be accessible to other parts of the editor. Storing the clipboard contents in the YiM
monad will make it accessible to the whole editor, to me that sounds harder to reason about.
@Fuuzetsu and @junjihashimoto have I convinced you or do you still have objections?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
YiString
Agree, use it.
Storing the clipboard contents in the YiM monad will make it accessible to the whole editor, to me that sounds harder to reason about.
This is not a Bad Thing™. If anything, this allows you to make getClipboard
and setClipboard
run in YiM
instead of IO
: this means you can actually stop someone using these somewhere arbitrary and things changing uncontrollably.
It's a bit like using global IORef instead of ReaderT with IORef in the env… I don't like it.
Having said that, this is about emulating system clipboard which is arguably this global system thing. So I don't care too strongly about it. If you want it in IORefs, hide them in the module and I'll stomach it.
yi-core/src/Yi/Types.hs
Outdated
-- ^ Custom configuration, containing the 'YiConfigVariable's. Configure with 'configVariableA'. | ||
configDisableSystemClipboard :: Bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make strict.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use strict. Thank you for your reviewing.
Add disable-hclip function
f06d982
to
d151e47
Compare
@noughtmare and @Fuuzetsu, Thank you for comments and reviewing. |
When environment is linux's terminal without xwindow,
hclip does not work, and killring does not work too.
Because
DISPLAY
variable is unset.This PR adds disable-hclip-function and uses local-variable instead of hclip.