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

System clipboard copy/paste #395

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

Conversation

madd-games
Copy link
Contributor

This PR removes the internal clipboard, and instead encoding the selection for copying into a string which it then places into the system clipboard, and then pastes from that.

Thanks to this, it is now possible to copy voxels from one instance of Goxel into another.

Let me know if you have any comments, I'll be happy to improve it and change it around. Let me know if any features are missing.

@guillaumechereau
Copy link
Owner

I don't really like this, at least as it is: it fills up the clipboard with random text. The other problem is that serializing and de-serializing like that makes the copy/past much slower for large models. There must be at least some way to do it using binary blob no?

@madd-games
Copy link
Contributor Author

I was indeed thinking about the size of these strings, but I realised that you can copy/paste massive amounts of text anyway, and it does not cause issues, so I'd imagine this wouldn't be a problem either.

This feature is very useful and you can do similar in 2D software like GIMP, and I added it specifically because I couldn't find any other solution to me having to copy voxels from one project to another, and it woudl be a shame if we couldn't have this feature.

I will look into whether there are better ways to do this and I'll get back to you. I do wonder how GIMP does it.

@guillaumechereau
Copy link
Owner

Yeah I am not sure. Ideally the conversion should be done when we past and not when we copy. I think this is how some software do it, but I don't know about the OS support. I think at the very least it should be a binary blob. I don't think gimp serializes the images into text each time we do a copy.

@guillaumechereau
Copy link
Owner

One ugly workaround could be to add a "copy to text" command maybe?

@madd-games
Copy link
Contributor Author

I have reviewed GIMP code which handles copy/pasting pixels. In the file gimpclipboard.c, there is a function named gimp_clipboard_set_buffer which actually performs this operation. It is slightly different to how we do it here: it uses GTK's "virtual clipboard", by calling gtk_clipboard_set_with_owner and providing a GtkClipboardGetFunc callback, named gimp_clipboard_send_buffer. Thus, it initially doesn't store anything in the clipboard; the callback will be invoked when another program tries to paste. It sets the pixels using gtk_selection_data_set_pixbuf; thus, instead of copying text it just copies pixel data using the appropriate system API.

Are we allowed to use GTK functions inside Goxel? At first glance, it seems like the answer is no; but let me know if that's incorrect.

It looks like we are stuck using GLFW, which only allows copying text, so we have to do this serialization. That being said, in GIMP, when a whole image gets copied, it does serialize it into XCF and PNG when pasting, so I don't think serializing would be a major issue; especially in my case, where I'm just converting to/from hex.

The "copy to text" command would be an ugly workaround like you said; and would need a different key combo than the intuitive Ctrl+C/Ctrl+V. (Note that there would need to be a separate "paste from text" command for this too, making it even worse).

Let me know your thoughts on the above, I'll be happy to do more programming.

@guillaumechereau
Copy link
Owner

Goxel can use GTK if it is specified during the build time. For the AppImage it doesn't link with gtk and instead uses dbus. From what I can find, dbus doesn't support clipboard operations, but I might be wrong.

I like the idea of doing the serialization only when we past (like gimp is doing) because then in the case when we stay in the same instance we don't need to serialize at all.

I can see two solutions here that I would find acceptable:

  1. Implement the copy / past mechanism more or less like gimp (only serialize in case of past into a different instance of goxel). This would only work for goxel compiled with gtk (so not the AppImage, if that is what you are using).

  2. Add a new 'copy to text' command, so that we have the option to serialize or not (the past can be smart and supports both format). This is not as good, but maybe easier if you don't use the gtk version.

What do you think?

@madd-games
Copy link
Contributor Author

If we use the GTK approach, would it work on Windows as well?

One potential gotcha right now is that although the window itself does not have to be created with GTK, we do have to call gtk_init() before we can use it; but I guess that's already happening since you're using GTK for the file dialog?

@guillaumechereau
Copy link
Owner

No, currently on windows we don't link with gtk at all. There must be some similar API for clipboards on Windows I assume, but I haven't checked into it.

@madd-games
Copy link
Contributor Author

Windows does indeed have a similar API, but you have to handle messages (in the event main loop) to respond when another program wants to paste. It is therefore integrated with the window manager. The GTK clipboard does work on Windows, so there is in fact a cross-platform way to do it.

The truly good solution, I think, would be to add support for this into GLFW; it already supports text, and already handles the main loop, so that would be the cleanest place to put this functionality (the API on the GLFW side could be similar to GTK). I can look into the feasibility of doing this, that might take a bit longer as I presume they have quite a rigorous process for reviewing changes.

If we do a command similar to "copy to text", it would work as maybe a temporary solution, but is not ideal as it wouldn't be unified like users expect.

Let me look into this further, we don't need to rush this PR anyway.

@madd-games
Copy link
Contributor Author

Another possible temporary solution would be to automatically copy to text if the selection is small, but for a large selection, show a modal dialog asking the user which they want to do.

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.

2 participants