golang-design / clipboard

📋 cross-platform clipboard package that supports accessing text and image in Go (macOS/Linux/Windows/Android/iOS)
https://golang.design/x/clipboard
MIT License
579 stars 64 forks source link

suggestion: making `Write` blocking instead of returning a channel #52

Closed hajimehoshi closed 1 year ago

hajimehoshi commented 1 year ago

Thank you for such a great library!

The current Write returns a channel

func Write(t Format, buf []byte) <-chan struct{}

but an example in the comments doesn't use this channel. Is this intended? So I was a little confused whether the channel should be used or not.

In order to avoid confusion, wouldn't it better to change Write blocking and let users decide whether they wait for returning or not? They could use their own channels if they wanted to use Write as non-blocking. This suggestion would simplify the API and consistent with Read, which is already blocking.

What do you think? Thanks,

changkun commented 1 year ago

I remember why I decided to do the api in this way. The main reason is for Linux users, they always have to use the blocking version. https://github.com/golang-design/clipboard/blob/b50badc062a526673961e1465a673e3f3dfc1464/clipboard_linux.go#L107-L126 because only in this way the content will be served in a goroutine and wait until the reader reads it. Otherwise, If the linux users don't block on the channel, they will not be able to read what was written before.

Also, if a user ignores the channel, it won't leak anything and as long as the channel is GCed the internal goroutines will be closed.

This is also sometimes confusing from the user perspective see also https://github.com/golang-design/clipboard/issues/15, https://github.com/golang-design/clipboard/issues/35

Maybe we could better revise the document of Write?

hajimehoshi commented 1 year ago

I'm afraid I don't understand.

The main reason is for Linux users, they always have to use the blocking version.

So, always blocking at Write is better...?

changkun commented 1 year ago

Hm. Not really.

For linux x11 users, they have to block on this channel to make sure other readers can read from this write:

ch := clipboard.Write(clipboard.FmtText, []byte("x"))
go func() { <-ch }
clipboard.Read(clipboard.FmtText) // x

Otherwise

clipboard.Write(clipboard.FmtText, []byte("x"))
clipboard.Read(clipboard.FmtText) // "" empty

On darwin:

ch := clipboard.Write(clipboard.FmtText, []byte("x"))
go func() { <-ch }
clipboard.Read(clipboard.FmtText) // x

do not differ from

clipboard.Write(clipboard.FmtText, []byte("x"))
clipboard.Read(clipboard.FmtText) // x
hajimehoshi commented 1 year ago
ch := clipboard.Write(clipboard.FmtText, []byte("x"))
go func() { <-ch }
clipboard.Read(clipboard.FmtText) // x

Hmm? This sounds indeterministic and Read can be called before <-ch, right?

My suggestion is to change the function like this:

func Write(t Format, buf []byte) {
    lock.Lock()
    defer lock.Unlock()

    changed, err := write(t, buf)
    if err != nil {
        if debug {
            fmt.Fprintf(os.Stderr, "write to clipboard err: %v\n", err)
        }
        return
    }
        <-changed
}

so that

  1. Users don't have to care about the finishing state
  2. Users can use their own channels if they want (but the clipboard state is not guaranteed so they have to wait for the result in their own responsibility)
  3. The API would be consistent with Read
changkun commented 1 year ago

How will this suggestion change the behavior of darwin/windows? On darwin/windows, a write can be observed as long as the Write returns, and users might not be interested in blocking until the content is changed.

Or maybe I still do not fully grasp the whole idea, would you mind sending a PR?

Either way this change will be breaking and thankfully we haven't in 1.x release yet :)

hajimehoshi commented 1 year ago

How will this suggestion change the behavior of darwin/windows? On darwin/windows, a write can be observed as long as the Write returns, and users might not be interested in blocking until the content is changed.

This sounds purely a performance issue. So, we can make closing the channel changed earilier in clipboard_windows.go and clipboard_darwin.go (but I am not sure an approprite timing). I am not sure the time difference, but I assume this is microseconds order.

Or maybe I still do not fully grasp the whole idea, would you mind sending a PR?

My current whole idea is the above new Write implementation. I'm happy to send a PR if we agree.

changkun commented 1 year ago

This sounds purely a performance issue.

I think there is still a misunderstanding about the usage here.

ch := clipboard.Write(clipboard.FmtText, []byte("x"))
<-ch // block forever if there is no other write changes the "x" to other content.

So basically the suggested change will be:

clipboard.Write(clipboard.FmtText, []byte("x")) // block forever if there is no other write changes the "x" to other content.

Wouldn't that be a leak of goroutine for the user? With the suggested change we will have to do it in this way to do a write call:

go func() {
    clipboard.Write(clipboard.FmtText, []byte("x")) // block forever if there is no other write changes the "x" to other content.
}()
hajimehoshi commented 1 year ago

Hmm this is confusing. So the meaning of changed depends on platforms, right?

So what about:

func Write(t Format, buf []byte) {
    lock.Lock()
    defer lock.Unlock()

    changed, err := write(t, buf)
    if err != nil {
        if debug {
            fmt.Fprintf(os.Stderr, "write to clipboard err: %v\n", err)
        }
        return
    }
        // This is quite dirty :P
        if runtime.GOOS != "windows" && runtime.GOOS != "darwin" {
                <-changed
        }
}

or

func Write(t Format, buf []byte) <-chan struct{} {
    lock.Lock()
    defer lock.Unlock()

    ready, changed, err := write(t, buf) // ready is a new channel to be closed when users can read the result
    if err != nil {
        if debug {
            fmt.Fprintf(os.Stderr, "write to clipboard err: %v\n", err)
        }
        return nil
    }
        <-ready
        return changed
}

?

changkun commented 1 year ago

I am not sure I can understand the impact now.. I think the best approach is to sketch the PR and see what will happen.

Before doing that, as I mentioned before, the reason was due to the X11's clipboard system, and commented before here https://github.com/golang-design/clipboard/issues/15#issuecomment-959519399.

Also a read to this might be helpful https://www.uninformativ.de/blog/postings/2017-04-02/0/POSTING-en.html "Program 3: Owning a selection"

hajimehoshi commented 1 year ago

I don't understand either..

ch := clipboard.Write(clipboard.FmtText, []byte("x"))
go func() { <-ch }
clipboard.Read(clipboard.FmtText) // x

I think this is indeterministic but do you agree?

<-ch // block forever if there is no other write changes the "x" to other content.

Is this the same even for Linux?

EDIT: From https://github.com/golang-design/clipboard/issues/15 , it looks like so...

changkun commented 1 year ago

I think this is indeterministic but do you agree?

No. It is deterministic, we can always expect this code the read to return "x"

But this we can't:

clipboard.Write(clipboard.FmtText, []byte("x"))
clipboard.Read(clipboard.FmtText)
hajimehoshi commented 1 year ago

From the specification, there is no guarantee when reading the channel starts, so I believe this code is indeterministic. Also, I don't understand why reading the channel in another goroutine affects the result.

hajimehoshi commented 1 year ago

On Linux, I could get an expected result without reading a channel..

parallels@parallels-Parallels-Virtual-Platform /media/psf/Home/test/clipboard 
$ go version
go version go1.21.0 linux/amd64
parallels@parallels-Parallels-Virtual-Platform /media/psf/Home/test/clipboard 
$ cat main.go
package main

import (
        "golang.design/x/clipboard"
)

func main() {
        clipboard.Write(clipboard.FmtText, []byte("x"))
        println(string(clipboard.Read(clipboard.FmtText)))
}
parallels@parallels-Parallels-Virtual-Platform /media/psf/Home/test/clipboard 
$ go run main.go
x
changkun commented 1 year ago

I could get an expected result without reading a channel..

This is probably by luck. Also, you can try to read again after the program is existed. The "x" will disappear

hajimehoshi commented 1 year ago

OK I'll read the current implementation carefully later. I still think there is room to improve the current API.

Thank you for elaborating!

changkun commented 1 year ago

PR always welcome.

hajimehoshi commented 1 year ago
ch := clipboard.Write(clipboard.FmtText, []byte("x")) // (1)
go func() { <-ch }() // (2)
clipboard.Read(clipboard.FmtText) // (3)

This program can run in the order 1 -> 3 -> 2, and in this case, Read can return an empty string on Linux. Is my understanding correct?

changkun commented 1 year ago

Is my understanding correct?

No. (2) ensures that the internal goroutine in the Write's implementation holds the ownership of the written content, and the returning ch won't be garbage collected and then lose ownership of "x".

This is not the most important case, we are likely to want to write "x" to the clipboard, and read and paste it out from another application, e.g. paste it to the URL address bar of a browser. If the application that writes the "x" exited, and lost ownership of the content, on Linux, the "x" will not be pastable in the browser anymore.

hajimehoshi commented 1 year ago

No. (2) ensures that the internal goroutine in the Write's implementation holds the ownership of the written content, and the returning ch won't be garbage collected and then lose ownership of "x".

OK so the lifetime of ch matters rather than this is closed or not, right? Then, would this work?

ch := clipboard.Write(clipboard.FmtText, []byte("x"))
clipboard.Read(clipboard.FmtText)
runtime.KeepAlive(ch)
changkun commented 1 year ago

It can work, but only if the application is still alive.

This is not the most critical case, we are likely to want application A to write "x" to the clipboard, and read and paste it out from another application B, e.g. paste it to the URL address bar of a browser. If the application A does a write "x", but A exits immediately after that write, on Linux, the "x" will disappear and cannot be observed from B anymore.

hajimehoshi commented 1 year ago

It can work, but only if the application is still alive.

ch := clipboard.Write(clipboard.FmtText, []byte("x")) // (1)
go func() { <-ch }() // (2)
clipboard.Read(clipboard.FmtText) // (3)

Is this in the same situation? i.e. (2) might not be called before the application exits.

hajimehoshi commented 1 year ago

This is not the most important case, we are likely to want to write "x" to the clipboard, and read and paste it out from another application, e.g. paste it to the URL address bar of a browser. If the application that writes the "x" exited, and lost ownership of the content, on Linux, the "x" will not be pastable in the browser anymore.

I don't understand what the ownership means here. Write creates an internal goroutine, which refers the done channel. This should not be GCed as long as the goroutine lives even if the returned done channel is not referred by the caller. So, I still don't understand the difference between

clipboard.Write(clipboard.FmtText, []byte("x"))
clipboard.Read(clipboard.FmtText)

and

ch := clipboard.Write(clipboard.FmtText, []byte("x"))
clipboard.Read(clipboard.FmtText)
runtime.KeepAlive(ch)

. If both always succeed, or both are flaky at the same time, that would make sense. However, you said the former is flaky and the latter always succeeds. That doesn't make sense to me. (Actually I have not seen the former case failed yet, so I am not sure which is correct. My guess is both are flaky...)

For other applications, the channel might have to be closed in order to sync the state, but the lifetime of it should not matter. If it matters, this is beyond my understanding of Go... I'm sorry if my questions bother you.

changkun commented 1 year ago

Let's take one step back: What is the goal of the discussion right now? Understanding the platform specific behavior or shaping a new design of the API?

For the first one I suggest to read a link that I sent in previous comments which explains what does ownership mean in X11's design; For the second I would suggest to sketch a PR, adapting the existing examples, a few possible users of the package, and understand the overall impact.

This thread grows significantly more than expected and I am pretty much like to address them via a video call and chat about it rather than typing a lot.. Will you available for a Google meet call?

hajimehoshi commented 1 year ago

Let's take one step back: What is the goal of the discussion right now? Understanding the platform specific behavior or shaping a new design of the API?

Understanding the platform specific behavior for now.

For the first one I suggest to read a link that I sent in previous comments which explains what does ownership mean in X11's design; For the second I would suggest to sketch a PR, adapting the existing examples, a few possible users of the package, and understand the overall impact.

My question is whether the caller's reference to a channel affects the behavior (and you said yes, but I don't think so). This doesn't related to X11's design. This is purely a Go question.

This thread grows significantly more than expected and I am pretty much like to address them via a video call and chat about it rather than typing a lot.. Will you available for a Google meet call?

I would like to focus on my current Go question for now. I don't think we need a video call to resolve this question... After that, we might need a video call to design an API, but I cannot go futher until my question is resolved.

changkun commented 1 year ago

OK, you are correct, then?

hajimehoshi commented 1 year ago

So wasn't https://github.com/golang-design/clipboard/issues/52#issuecomment-1681742055 correct? (This is just confirming)

changkun commented 1 year ago

Confirmed. Then?

hajimehoshi commented 1 year ago

So, my understanding is

clipboard.Write(clipboard.FmtText, []byte("x"))
println(string(clipboard.Read(clipboard.FmtText))) // x

always works on Linux, without holding the channel.

Then?

Let me take tiime to polish my idea. I might send a PR later.

I just wanted to confirm what you meant by the ownership (and apprently there is no ownership notion there). I'm sorry if I bothered you, but I think understanding this point is critical.

changkun commented 1 year ago

Thanks. Looking forward.

hajimehoshi commented 1 year ago

As the clipboard state is reliable, at least for the current process, after Write returns, I don't come up with an idea to improve the API. Let me close this issue. I might file a new issue later when I come up with another idea.

Thank you for answering my many questions, @changkun!