-
-
Notifications
You must be signed in to change notification settings - Fork 15
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
Feature/clipboard sync #39
Conversation
gtluszcz
commented
Oct 3, 2018
- Replacing text is now made with synchronous clipboard replacements so that we can be sure that they are executed in order
- Any error with clipboard will be indicated with notification (if possible)
src/main/modules/SnippetsManager.js
Outdated
@@ -2,18 +2,18 @@ const chars = require('./chars') | |||
const NativeKeymap = require('native-keymap') | |||
const _ = require('lodash') | |||
const Notification = require('./Notification') | |||
const clipboardy = require('clipboardy') |
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 dependency should be injected via the constructor to preserve testability.
src/main/modules/SnippetsManager.js
Outdated
@@ -188,12 +188,31 @@ class SnippetsManager { | |||
} | |||
|
|||
replace(value) { | |||
const clipboardContent = this.clipboard.readText() | |||
try { | |||
// 1. store old clipboard data |
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 don't think comments are necessary nor are adding any value. Let's write code that doesn't require 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.
Sure, as you wish
src/main/modules/SnippetsManager.js
Outdated
this.keyboardSimulator.keyTap('v', 'command') | ||
// 4. place stored data back in clipboard | ||
setTimeout(() => clipboardy.writeSync(clipboardContent), 50) | ||
return true |
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.
Returning a boolean may be a nice thing to do, but we are not using this returned information nowhere, so I don't see any value.