Copy to and paste from system clipboard

[karlhorky]

Okay, my first try to implement this feature of copying to and pasting from the system clipboard!

Not sure if this is what you meant @aioutecism. Happy to take feedback and improve this still!

I tried changing from Thenable to Promise to allow for async methods on putAfter and putBefore so that there's not this .then() callback hell, but I guess async methods are not supported? Anyway, would love to do this a better way if possible.

This will not overwrite the stash register in case:

1. there is nothing returned from the system clipboard promise OR...
2. what is in the system clipboard is already in the existing stash

Closes #17
Closes #166

[alisonatwork]

Hey @karlhorky - you might want to experiment with running this on top of the changes from #259 which upgrade the dependencies. I found that upgrading fixed some of the async weirdness i was experiencing with my work on the gd/gD feature, especially during tests.

[karlhorky]

Ping @aioutecism @jackfranklin, could this PR be reviewed?

[karlhorky]

@alisonatwork Thanks for the heads up.

Depending on the feedback to both of the PRs, maybe it will make sense to rebase on your changes.

[karlhorky]

Hey there @aioutecism @jackfranklin, would you have a chance to take a look at this soon?

[alisonatwork]

Hi @karlhorky I just had a first look at the code in this PR, and it seems okay to me on the face of it. To make it a bit easier to manage, could you rebase and force push the branch?

Second question, did you attempt to write a test for this yet? I'm not sure if it's easy to do with the existing test setup, but it would be cool if you could preload the clipboard and config and run something like the p test, or do a post-assert in a modified yy test.

Regarding your promise problem, I see what you mean by the awkward looking callback. It also makes the diff a bit ugly, having to indent everything. I tried to play around with the order of the code and came up with something like this:

// snip
        if (Configuration.useSystemClipboard === true) {
            return env.clipboard
                .readText()
                .then((result) => {
                    // ...
                    return Promise.resolve(ActionRegister.stash);
                })
                .then((stash) => this._putAfter(args, activeTextEditor, stash));
        }

        return this._putAfter(args, activeTextEditor, ActionRegister.stash);
    }

    private static _putAfter(args, activeTextEditor, stash): Thenable<boolean> {
        if (!stash) {
            return Promise.resolve(false);
        }
// snip

This structure would retain most of the indentation from before because it just refactors most of the method into another private method, and it avoids the case where you need to check Configuration.useSystemClipboard twice. I'm not sure if it's better or not (and _putAfter is probably not a good function name), but it might be something you could play around with and see how it looks in the diff.

[alisonatwork]

Looking back at that code snippet i just posted, perhaps it could be simplified to just return true in the first then(), since we are in native ES6 Promise which should anyway wrap the result into another Promise... And if ActionRegister is global perhaps it doesn't need to be passed into the private function.

[karlhorky]

Rebased + force pushed. Now looking into the other feedback...

[karlhorky]

Second question, did you attempt to write a test for this yet?

Just looked into it now. It seems like it needs some more plumbing to do this... right now none of the configuration options have test coverage.

We do however know that it is not causing a regression with the normal clipboard mode (not using the system clipboard), due to the non-option tests.

[karlhorky]

So in 4dd608a I changed the type of actions to also include Promises (instead of Thenables, which don't allow for async methods), and the code is very clean now.

I would suggest iteratively rewriting methods to async/await when they are being worked on, there's a lot of .then chaining that can be avoided.

I found this reference back in 2015 to async/await in VS Code extensions, so I guess compatibility is fine as it is?

I also tested it out in my VS Code by hot-patching my live extensions folder, and it works :)

[alisonatwork]

Thanks for updating the PR @karlhorky! Using async/await makes the diff much cleaner. I have given it some manual testing on my machine trying a bunch of different ways to break it and so far so good. It's unfortunate we can't easily write an automated test for this, but I'm happy to merge this now and iterate as needed.

[karlhorky]

Great, sounds good. Automated testing for options would be a cool thing, maybe I should make an issue for it?

[alisonatwork]

Yep, if this is something you'd like to take a look at, that would be great!

[karlhorky]

Nice, opened #271 for the test.

And... oops, forgot to add documentation for this option. Opened #272 for this.

Thanks @alisonatwork !