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

[FR] Add X11 Clipboard support #1

Open
gezihuzi opened this issue Oct 26, 2024 · 19 comments
Open

[FR] Add X11 Clipboard support #1

gezihuzi opened this issue Oct 26, 2024 · 19 comments

Comments

@gezihuzi
Copy link
Contributor

Hello! I'm interested in using Yaxi for my project and wondering about clipboard functionality.

Does Yaxi have plans to support X11 clipboard operations (like CLIPBOARD and PRIMARY selection)? This would include basic operations like:

  • Copy text/data to clipboard
  • Paste from clipboard
  • Monitor clipboard changes
  • Handle different selection types

If there's no immediate plan for this feature, I'd be willing to contribute to implementing it. Would you be open to accepting a PR for clipboard support?

Thanks for creating this promising X11 Rust library!

@proxin187
Copy link
Owner

Thanks for the issue!

Clipboard support is definitely a feature needed, it shouldn't be a problem for me to implement it so ill try to get it done today!

Also, error reporting is a little wacky right now so ill probably make it better too.

@gezihuzi
Copy link
Contributor Author

Thanks for the quick response! That's great to hear.

I'd be happy to help with testing and provide feedback once the implementation is ready. I can try different use cases and report any issues or edge cases I find.

Please let me know when there's something ready for testing.

@proxin187
Copy link
Owner

Hello, the clipboard implementation should be working now.

Its gated behind the "clipboard" feature and there should be an example of how to use it inside the examples folder.

Currently it only supports UTF8_STRING but adding support for other types shouldn't be hard!

Please let me know if there is anything missing!

@gezihuzi
Copy link
Contributor Author

Thanks for the implementation! I've noticed some unexpected behavior while testing the clipboard feature:

Observed behavior:

  1. When I copy text from other applications -> run paste example -> successfully read the content
  2. When I run copy example (writes predefined text) -> shows success -> run paste example -> fails to read the content

I've also added three unit tests in PR #2 to verify basic clipboard operations:

  • Read text from clipboard
  • Write text to clipboard
  • Write-then-read consistency check

However, these tests are not passing as expected.

Could you help take a look at what might be causing these issues? I can provide more details or add debug logging if needed.

@proxin187
Copy link
Owner

Thanks so much for the feedback and PR.

The issue where you couldn't read the clipboard after running copy is most likely because you lose x11 selections after the selection owner exits, on my machine it works as long as the copy example is still running while the paste example is run.

Short summary of x11 selection

Example here:
example gif

When it comes to your tests you made, ill try to debug them and get them working as soon as possible, (im at school right now so it might not be done before later today)

Thanks for letting me know!

@proxin187
Copy link
Owner

Hello, Sorry for the inconveniences but the clipboard is finally in a semi-stable state, i had to refactor the concurrency model in yaxi so it took a little longer then i would have liked but it is done.

feel free to submit PR's for more clipboard formats if you need them.

@gezihuzi
Copy link
Contributor Author

gezihuzi commented Nov 4, 2024

Thank you for improving the clipboard, The implementation is working much better now.

I plan to submit new PRs to enhance the clipboard support, including:

  • Support for additional clipboard formats (imges, files, custom mime)
  • Monitor clipboard changes
  • More comprehensive test coverage
  • Potential improvements to clipboard operations
  • CLIPBOARD_MANAGER , SAVE_TARGETS and INCR

I'll create separate PRs for each enhancement to keep changes focused and manageable. Looking forward to contributing more to this project.

@gezihuzi gezihuzi changed the title [Feature Request] Add clipboard support [FR] Add clipboard support Nov 7, 2024
@gezihuzi gezihuzi changed the title [FR] Add clipboard support [FR] x11 Clipboard support Nov 7, 2024
@gezihuzi gezihuzi changed the title [FR] x11 Clipboard support [FR] Add X11 Clipboard support Nov 7, 2024
@gezihuzi
Copy link
Contributor Author

gezihuzi commented Nov 11, 2024

I've submitted a new PR #7 that primarily focuses on type extensions. While implementing the planned features, I encountered several challenges that I'd like to discuss:

  1. After writing content to the clipboard, attempting to read it immediately may result in a deadlock. I noticed the related comments in the code - are there any known solutions for this issue?
  2. Regarding ownership transfer: How should we handle data cleanup when the process exits after the clipboard ownership has been transferred to another process?
  3. Integration with CLIPBOARD_MANAGER: What's the best approach for implementing support for the clipboard manager protocol, specifically ensuring that written content is properly saved to the CLIPBOARD_MANAGER when our process terminates?
  4. The current design uses std::thread::spawn with ListenerHandle and Listener to poll and process X11 requests and events asynchronously. However, this implementation only allows one clipboard operation at a time. I believe this design could be optimized for better concurrency and performance - perhaps we could explore a more efficient event handling architecture?

These questions are particularly important for ensuring robust clipboard handling, efficient event processing, and graceful process termination. I'd appreciate any insights or guidance on addressing these concerns.

@proxin187
Copy link
Owner

Thanks for your great PR, these features where much needed.

Answers

  1. Could you please make an example that reproduces this behavior? i wasnt able to reproduce it myself, but if i get a example i will most likely be able to fix it quite fast.
  2. If you meant if we should clean/empty the ClipboardData structure once we lose the selection ownership, then yes but it isn't extremely important. On the other hand if you meant when our process exits and the clipboard structure is dropped then i think we should destroy the window and delete the atoms that are only used by us eg. (storage property).
  3. When it comes to the clipboard manager i havent done too much research yet, but when it comes to saving properly when the process terminates, you could most likely just fit some sort of a save mechanism inside the Drop implementation for Clipboard like this:
impl Drop for Clipboard {
    fn drop(&mut self) {
        // SOME SORT OF A SAVE MECHANISM HERE

        self.listener.kill();
    }
}

Ill most likely have to look a little more into the clipboard manager though!

  1. There is definitely big room for improvement when it comes to the current concurrency model of our clipboard implementation, and im a 100% in favor of exploring different architectures and trying to find the best one.

Unrelated Sidenote

Also just as a kind of unrelated side note, the queue implementation has gotten a massive performance boost as i switched it to using Condvar when waiting for events instead of the old method which would simply busy loop until the queue wasnt empty.

@gezihuzi
Copy link
Contributor Author

  1. Debugging at a specific line will result in an error.
    image
    2 & 3. I think we can store the content in CLIPBOARD_MANAGER in the Drop implementation and then clean up the data.

@proxin187
Copy link
Owner

I've implemented saving to the clipboard manager once the clipboard structure is dropped and also cleaning up the selection structure when we lose the selection, there are a few notable things with my implementation:

  1. Once the Clipboard structure is dropped it will ask the clipboard manager to save content using the SAVE_TARGETS atom then it will wait for a request from the clipboard manager with a timeout of 100ms, the potential problem with this is that this will result in a 100ms wait on all systems that dont have a clipboard manager running, im not sure if this is a problem that can be solved as when i looked into other clipboard implementations they seem to do the same thing notably arboard's implementation.

i would like to hear your opinion on this and if you have any proposals for a better way to do this!

@gezihuzi
Copy link
Contributor Author

@proxin187

Thank you for completing the CLIPBOARD_MANAGER implementation. I think waiting for 100ms at this stage is a feasible solution. In the future, maybe we can check if the current CLIPBOARD_MANAGER exists in advance so that we can skip it directly, which may be more efficient?

Share with you some of my recent progress. I am currently working on refactoring the implementation of the clipboard module in my temporary work branch. But it's not finished yet.

The purpose of refactoring is not only to enhance support for different selection (PRIMARY, SECONDARY, CLIPBOARD), but also to support the reading and writing of different types of clipboard data; advanced features (CLIPBOARD_MANAGER and INCR etc.); optimize event polling and shared data reading and writing processing.

Please feel free to share your thoughts and suggestions on the part of my work that I am currently working on.

@proxin187
Copy link
Owner

Hi

Thanks, After skimming through your code i found that i really like your refactored version of the clipboard implementation, i think its a great step forward to have more support for different clipboard data types.

Since you are working on a refactored version ill keep away from updating the clipboard implementation until the refactor is done.

@gezihuzi
Copy link
Contributor Author

gezihuzi commented Nov 29, 2024

@proxin187

Hi, I've now completed the clipboard refactoring we discussed earlier. This PR #8 implements the enhanced clipboard functionality with support for different X11 selections and data formats. Would love to get your thoughts on this implementation. Thank you!

@proxin187
Copy link
Owner

@gezihuzi

Hi, thank you so much for the PR, i had to fix some issues i had with the tests but got it to work after a while, but i do have some problems to discuss, notably the tests seem to use a unreasonable long time to complete, im not sure why this is but it would be great if you could figure it out, other than that i find your clipboard implementation to be very good and i am looking forward to expanding it!

@gezihuzi
Copy link
Contributor Author

gezihuzi commented Dec 1, 2024

@gezihuzi

Hi, thank you so much for the PR, i had to fix some issues i had with the tests but got it to work after a while, but i do have some problems to discuss, notably the tests seem to use a unreasonable long time to complete, im not sure why this is but it would be great if you could figure it out, other than that i find your clipboard implementation to be very good and i am looking forward to expanding it!

Thank you for the feedback! I'm glad you find the clipboard implementation useful.

Regarding the long test execution time - I'll definitely look into this. Could you share:

  1. Which specific tests are taking longer than expected?
  2. What kind of execution time are you seeing?

This will help me investigate and optimize the test performance. I'm also happy to collaborate on expanding the clipboard functionality further!

@proxin187
Copy link
Owner

@gezihuzi

thanks for the quick response, after further investigation i found that its the test_clipboard_clear test that fails, the issue seems to be that the first clipboard.get_targets call results in a timeout.

@gezihuzi
Copy link
Contributor Author

gezihuzi commented Dec 1, 2024

@proxin187

The current implementation of clear has known defects and may not achieve the expected results after execution. I think replacing the internal implementation with 'setting the content to an empty UTF-8 string' might achieve a similar effect.

I'll investigate further about the get_targets issue.

Also, I've left some comments about test cases in the PR discussion thread - would appreciate your thoughts on those when you have a chance.

e603bd5#commitcomment-149780469

@gezihuzi
Copy link
Contributor Author

gezihuzi commented Dec 3, 2024

@proxin187

Hi, I've submitted a new PR #10 to fix the clear operation failing to empty the clipboard. I've also added support for reading and writing uri_list and image clipboard types, and improved read performance by using targets for preliminary checks.

Additionally, I've improved the test cases based on our previous discussion at e603bd5#commitcomment-149818973

One issue I'm currently investigating is that data written to the clipboard is being lost for all types except text. I'll need some more time to look into this.

Could you please review the changes and let me know if you have any suggestions for improvement?

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

No branches or pull requests

2 participants