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

LibGUI: Read and write clipboard metadata to json #25026

Merged

Conversation

mkanilsson
Copy link
Contributor

This PR adds support for reading and writing metadata in Clipboard::DataAndType:::from_json() and Clipboard::DataAndType::to_json(), respectively.

The metadata field will only be set if there is metadata to store.

@github-actions github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label Sep 16, 2024
@nico
Copy link
Contributor

nico commented Sep 24, 2024

I'm not super familiar with this area – what's the effect of this PR?

@LucasChollet
Copy link
Member

From what I remember (I'm the author of that FIXME, but it feels that I did that forever ago), metadata fields are what we use to save image/files in the clipboard. Or at least stuff big enough for me to consider it silly to save them in a JSON. Especially as this JSON file is read and written quite often.

@mkanilsson
Copy link
Contributor Author

mkanilsson commented Sep 25, 2024

Yes, like Lucas said, the metadata field is used for things like images, their size, format and so on. They are also used for glyphs in the font editor. More information can be found here: https://github.com/SerenityOS/serenity/blob/master/Base/usr/share/man/man5/clipboard.md

Currently there isn't anything that uses metadata AND saves it to disk as images and glyphs are only stored in memory.

I mostly did this PR to remove those FIXME's but it might be useful in the future if someone find a use-case for it.

@LucasChollet
Copy link
Member

It's already a very nice improvement as it allows us to:

  • Copy an image from Browser or the host (through the spice client) - or anything copying image/x-serenityos
  • Reboot serenity
  • Paste in PixelPaint - or anything supporting to paste image/x-serenityos

The issue that I see, and the reason I left it as a FIXME is that the file we save the clipboard history in is read and written quite often. Embedding images (so huge amount of bytes) written as ASCII is far from optimal and might cause lags. One suggestion would be to save the metadata as binary in separate files and only have keys linking to these files in the JSON. It would also come with its share of issues like the deletion of the files etc...

@nico nico merged commit 796cd35 into SerenityOS:master Sep 25, 2024
12 checks passed
@nico
Copy link
Contributor

nico commented Sep 25, 2024

Thanks for the details!

@mkanilsson
Copy link
Contributor Author

mkanilsson commented Sep 25, 2024

I'm unsure that Lucas comment applies as the image data is store in the data field.

https://github.com/SerenityOS/serenity/blob/master/Userland/Libraries/LibGUI/Clipboard.cpp#L192

{
    "data": "image data here",
    "mime_type": "image/x-serenityos",
    "metadata": {
        "not": "here"
    }
}

Metadata for images are: width, height, scale, pitch and format.

From my testing, mine types images/x-serenityos and glyph/x-fonteditor aren't saved to disk and won't show up in the clipboard history applet, however, I can't find any code to verify this claim.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👀 pr-needs-review PR needs review from a maintainer or community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants