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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add disable-hclip function #1089

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d151e47
Move Hclip to yi-core
junjihashimoto Jun 12, 2018
d6528c5
Use YiString for clipboard
junjihashimoto Jun 19, 2018
dce2db8
Add comment for IOREF
junjihashimoto Jun 19, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions yi-core/src/Yi/Clip.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
{-# OPTIONS_HADDOCK show-extensions #-}

-- |
-- Module : Yi.Clip
-- License : GPL-2
-- Maintainer : [email protected]
-- Stability : experimental
-- Portability : portable
--
-- A proxy of clipboard.

module Yi.Clip ( setClipboard
, getClipboard
)
where

import qualified System.Hclip as H (getClipboard, setClipboard)
import Data.IORef
import System.IO.Unsafe

import Yi.Types (configDisableSystemClipboard, askCfg)
import Yi.Utils (io)
import Yi.Keymap (YiM)

clipboard :: IORef String
clipboard = unsafePerformIO $ newIORef ""
Copy link
Member

Choose a reason for hiding this comment

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

  1. Don't store String, use YiRope straight away.
  2. 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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. We do not need performance. Clipboard is poor one.
    When vty-mode, Hclip calls a command of xclip, it is really slow.
    　So I think we do not need YiRope.
  2. 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.

Copy link
Member

Choose a reason for hiding this comment

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

  1. 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.

  1. 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.

Copy link
Contributor Author

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?

Copy link
Member

@noughtmare noughtmare Jun 14, 2018 •
edited
Loading

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 YiStrings, to prevent conversion between YiStrings and Strings which saves memory and CPU time. The system clipboard can then has to explicitly convert the YiStrings to Strings, because the system clipboard can't store Strings. 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?

Copy link
Member

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.


getClipboard' :: IO String
getClipboard' = readIORef clipboard

setClipboard' :: String -> IO ()
setClipboard' = writeIORef clipboard

getClipboard :: YiM String
getClipboard = do
config <- askCfg
if configDisableSystemClipboard config
then io getClipboard'
else io H.getClipboard

setClipboard :: String -> YiM ()
setClipboard text = do
config <- askCfg
if configDisableSystemClipboard config
then io $ setClipboard' text
else io $ H.setClipboard text
1 change: 1 addition & 0 deletions yi-core/src/Yi/Config/Default.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ defaultConfig =
, bufferUpdateHandler = mempty
, layoutManagers = [hPairNStack 1, vPairNStack 1, tall, wide]
, configVars = mempty
, configDisableSystemClipboard = False
}

nilKeymap :: Keymap
Expand Down
3 changes: 2 additions & 1 deletion yi-core/src/Yi/Config/Simple.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,8 @@ import Yi.Config(Config, UIConfig, startFrontEndA, configUIA,
CursorStyle(..), configLeftSideScrollBarA,
configAutoHideScrollBarA, configAutoHideTabBarA,
configLineWrapA, configWindowFillA, configThemeA,
layoutManagersA, configVarsA, configLineNumbersA
layoutManagersA, configVarsA, configLineNumbersA,
configDisableSystemClipboardA
)


Expand Down
5 changes: 4 additions & 1 deletion yi-core/src/Yi/Types.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,8 +434,11 @@ data Config = Config {startFrontEnd :: UIBoot,
bufferUpdateHandler :: !(S.Seq (S.Seq Update -> BufferM ())),
layoutManagers :: ![AnyLayoutManager],
-- ^ List of layout managers for 'cycleLayoutManagersNext'
configVars :: !ConfigState.DynamicState
configVars :: !ConfigState.DynamicState,
-- ^ Custom configuration, containing the 'YiConfigVariable's. Configure with 'configVariableA'.
configDisableSystemClipboard :: !Bool
-- ^ Set to 'True' not to use system clipboard.
-- When vty-mode, system clipboard is not available in some environments.
}


Expand Down
2 changes: 2 additions & 0 deletions yi-core/yi-core.cabal
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ library
, yi-language >= 0.17
, yi-rope >= 0.10
, exceptions
, Hclip
if flag(hint)
cpp-options: -DHINT
build-depends:
Expand All @@ -74,6 +75,7 @@ library
Yi.Buffer.Region
Yi.Buffer.TextUnit
Yi.Buffer.Undo
Yi.Clip
Yi.Command
Yi.Command.Help
Yi.Completion
Expand Down
6 changes: 3 additions & 3 deletions yi-keymap-emacs/src/Yi/Keymap/Emacs/KillRing.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@ import Yi.Buffer
import Yi.Editor (EditorM, killringA, withCurrentBuffer)
import Yi.Keymap (YiM)
import Yi.KillRing (Killring (_krContents), krKilled, krPut)
import Yi.Clip (getClipboard, setClipboard)
import qualified Yi.Rope as R (YiString, fromString, toString)
import Yi.Types (withEditor)
import Yi.Utils (io)
import System.Hclip (getClipboard, setClipboard)

uses :: forall a b f s. MonadState s f => Getting a s a -> (a -> b) -> f b
uses l f = f <$> use l
Expand All @@ -32,7 +32,7 @@ uses l f = f <$> use l
-- | Adds system clipboard's contents on top of the killring if not already there
clipboardToKillring :: YiM ()
clipboardToKillring = do
text <- fmap R.fromString $ io getClipboard
text <- fmap R.fromString $ getClipboard
withEditor $ do
text' <- killringGet
when (text' /= text) $ killringPut Forward text
Expand All @@ -41,7 +41,7 @@ clipboardToKillring = do
killringToClipboard :: YiM ()
killringToClipboard = do
text <- withEditor killringGet
io . setClipboard $ R.toString text
setClipboard $ R.toString text

-- This is like @kill-region-or-backward-word@.
killRegionB :: BufferM ()
Expand Down
1 change: 0 additions & 1 deletion yi-keymap-emacs/yi-keymap-emacs.cabal
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ library
base >= 4.8 && < 5
, containers
, filepath
, Hclip
, microlens-platform
, mtl
, oo-prototypes
Expand Down
4 changes: 2 additions & 2 deletions yi-keymap-vim/src/Yi/Keymap/Vim/Ex/Commands/Copy.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import Yi.Types (YiM, BufferM)
import Yi.Rope (toString)
import Yi.Buffer.Region (readRegionB, Region)
import Control.Monad.Base (liftBase)
import System.Hclip (setClipboard)
import Yi.Clip (setClipboard)
import Yi.Core (errorEditor)

parse :: EventString -> Maybe ExCommand
Expand All @@ -38,5 +38,5 @@ parse = Common.parse $ do
copy :: Maybe (BufferM Region) -> YiM ()
copy maybeGetRegion = case maybeGetRegion of
Nothing -> errorEditor "Cannot copy: No region"
Just getRegion -> liftBase . setClipboard . toString
Just getRegion -> setClipboard . toString
=<< withCurrentBuffer (readRegionB =<< getRegion)
7 changes: 3 additions & 4 deletions yi-keymap-vim/src/Yi/Keymap/Vim/Utils.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ import Yi.Monad (whenM)
import Yi.Rope (YiString, countNewLines, last)
import qualified Yi.Rope as R (replicateChar, snoc, toString, fromString)
import Yi.Utils (io)
import System.Hclip (getClipboard, setClipboard)
import Yi.Clip (getClipboard, setClipboard)

-- 'mkBindingE' and 'mkBindingY' are helper functions for bindings
-- where VimState mutation is not dependent on action performed
Expand Down Expand Up @@ -208,11 +208,10 @@ addNewLineIfNecessary rope =

pasteFromClipboard :: YiM ()
pasteFromClipboard = do
text <- fmap R.fromString $ io getClipboard
text <- fmap R.fromString $ getClipboard
withCurrentBuffer $ insertRopeWithStyleB text Inclusive

exportRegisterToClipboard :: RegisterName -> YiM ()
exportRegisterToClipboard name = do
mbr <- withEditor $ getRegisterE name
io . setClipboard $ maybe "" (R.toString . regContent) mbr

setClipboard $ maybe "" (R.toString . regContent) mbr
2 changes: 0 additions & 2 deletions yi-keymap-vim/yi-keymap-vim.cabal
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@ library
, data-default
, directory
, filepath
, Hclip
, microlens-platform
, mtl
, oo-prototypes
Expand Down Expand Up @@ -141,7 +140,6 @@ test-suite spec
, data-default
, directory
, filepath
, Hclip
, microlens-platform
, mtl
, oo-prototypes
Expand Down