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

unix: move clipboard utility discovery out of init() #64

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

Conversation

tstromberg
Copy link

@tstromberg tstromberg commented May 18, 2022

Fixes #63

This moves the clipboard utility code from init() to a function named findClipboardUtility(). While this has a minor performance impact if the WriteAll or ReadAll command is called in a loop, this does improve binary start time.

It also looks a whole lot less scary in system call traces. This is particularly important for programs that indirectly import but never use this library.

As part of the cleanup, I removed references to an unused Primary global. Please let me know if you would like me to put them back.

@tstromberg tstromberg changed the title clipboard_unix: move utility discovery code out of init() unix: move utility discovery code out of init() May 18, 2022
@tstromberg tstromberg changed the title unix: move utility discovery code out of init() unix: move clipboard utility discovery code out of init() May 18, 2022
@tstromberg tstromberg changed the title unix: move clipboard utility discovery code out of init() unix: move clipboard utility discovery out of init() May 18, 2022
}

func readAll() (string, error) {
if Unsupported {
c := findClipboardUtility()
Copy link

@norbjd norbjd May 22, 2022

Choose a reason for hiding this comment

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

Hi 👋 just a small comment: instead of instantiating a new commandInfo every time readAll() or writeAll() is called, should it be possible to create a singleton:

 var instance commandInfo
 var once sync.Once

 func GetInstance() commandInfo {
     once.Do(func() {
         instance = findClipboardUtility()
     })
     return instance
 }

And call c := GetInstance() instead of c := findClipboardUtility()?

It will be quicker since findClipboardUtility() will be called only once (at the first call).

What do you think? BTW, thanks for this PR 🙌

Copy link
Author

Choose a reason for hiding this comment

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

I considered this option, but it didn't seem worth the additional complication for a call that is unlikely to be called very often. If making this change helps the PR get merged though, I'm all for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move scary $PATH clipboard utility scan out of init() to as-needed
2 participants