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

Add Init() #18

Merged
merged 6 commits into from
Jan 22, 2022
Merged

Add Init() #18

merged 6 commits into from
Jan 22, 2022

Conversation

hrntknr
Copy link
Contributor

@hrntknr hrntknr commented Jan 22, 2022

If you are using Linux without X11 enabled, an error will occur at the time of import.
Add Test() methods to check if the clipboard is accessible for conditional branching.

Fixes #19

@hrntknr
Copy link
Contributor Author

hrntknr commented Jan 22, 2022

please check @changkun

Copy link
Member

@changkun changkun left a comment

Choose a reason for hiding this comment

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

I am not sure how useful this is. Is this PR relevant to #19 ?

@hrntknr
Copy link
Contributor Author

hrntknr commented Jan 22, 2022

I've just seen #13 for the first time, but the problem we want to solve is the same: changing the interface will cause problems for existing applications, so the implementation is backwards compatible.

I like the idea of Init but not entirely sure if this is how we should proceed.

Checking the implementation in init makes sense and is useful for some users. However, it also narrows down the use cases, and in my opinion, it would be great if the architecture was error-free in import.

@@ -77,6 +81,10 @@ func read(t Format) (buf []byte, err error) {
}

func readc(t string) ([]byte, error) {
if !canAccessClipbord && !test() {
Copy link
Member

Choose a reason for hiding this comment

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

Could you elaborate more on why do we need this check at every call?

@hrntknr
Copy link
Contributor Author

hrntknr commented Jan 22, 2022

We believe that if you can access clipbord once, you can always access it a second time or later. Therefore, to eliminate the overhead, we use canAccessClipbord to manage it.
Also, to maintain backward compatibility as much as possible, we check each time and display an appropriate error message (errmsg). Without this check, the test results will look like the following

write failed with status: -1
write failed with status: -1
--- FAIL: TestClipboard (0.00s)
    --- FAIL: TestClipboard/image (0.00s)
        clipboard_test.go:60: read clipboard that stores image data as image should success, but got: nil
    --- FAIL: TestClipboard/text (0.00s)
        clipboard_test.go:105: read clipboard taht stores text data as text should success, but got: nil
write failed with status: -1
write failed with status: -1
--- FAIL: TestClipboardMultipleWrites (0.00s)
    clipboard_test.go:153: read clipboard that should store text data as text should success, got: nil
write failed with status: -1
write failed with status: -1
write failed with status: -1
write failed with status: -1
write failed with status: -1
write failed with status: -1
write failed with status: -1
--- FAIL: TestClipboardWatch (2.00s)
    clipboard_test.go:238: clipboard watch never receives a notification
write failed with status: -1
--- FAIL: ExampleRead (0.00s)
got:

want:
Hello, 世界
write failed with status: -1

@changkun changkun changed the title implementation Test() methods Add Init() Jan 22, 2022
@changkun
Copy link
Member

Without this check, the test results will look like the following

I think if one can pass Init(), then the system environment is sufficient to access clipboard information. However, indeed it may fail at some point for some reason. But as the current API is designed to behave: if there is an error when accessing the clipboard, it returns nothing and one may retry calling it afterward. Panic after the success of Init is an undesired behavior as discussed in #19.

@changkun changkun merged commit 076d92f into golang-design:main Jan 22, 2022
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.

Using clipboard trough ssh
2 participants