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 support for XG_MIME_PNG and NSPasteboardTypePNG in both copy&paste directions. #47

Closed
wants to merge 1 commit into from

Conversation

rmottola
Copy link
Member

@rmottola rmottola commented Mar 5, 2024

No description provided.

@rmottola
Copy link
Member Author

rmottola commented Mar 5, 2024

PNG support needs to be conditional. How can it done be best? I think a run-time feature check from base (NSImage? NSImageRep?) is best.

@rmottola rmottola self-assigned this Mar 5, 2024
@rmottola rmottola requested a review from rfm March 5, 2024 10:14
Copy link
Contributor

@rfm rfm left a comment

Choose a reason for hiding this comment

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

I'm afraid I already made conflicting changes while adding support for text/plain;charset=utf8 (which seems to be a commonly used type) and clearer debug output.

The only actual bug I can see is that the line:
xTypes[numTypes++] = XG_MIME_PNG;
should be paired with an increase in the size of the xTypes buffer, to avoid any possibility of a buffer overrun.

@rfm
Copy link
Contributor

rfm commented Mar 5, 2024

PNG support needs to be conditional.

In case you missed by response to this idea elsewhere; in gpbs support for every type should be unconditional (I want to add all the pasteboard types publicly declared in the GUI library). That's because gpbs only deals with declaring types and moving the data around, so unless it's going to do format conversion itself (which it mostly shouldn't, though admittedly it does for strings) the data is opaque and the code doesn't need to be concerned about whether other parts of the system support it or not: if other part of the system don't support a type, they just won't copy items of that type to the pasteboard and they won't try to paste in items of that type form the pasteboard.

This probably means that the issue is a lot simpler than you have been thinking, and you are worrying unnecessarily.

@rfm
Copy link
Contributor

rfm commented Mar 5, 2024

Here is a full list of the pasteboard types I found in the GUI headers. Some of them are duplicates (because Apple changed the type names, eg NSTIFFPboardType is the same as NSPasteboardTypeTIFF), and some of them are unlikely to have an equivalent in X, but we should be able to support things like PDF, URL, HTML, VCard, perhaps Color, Font Ruler

GSMovableToolbarItemPboardType
NSColorPboardType
NSDataLinkPboardType
NSFileContentsPboardType
NSFilenamesPboardType
NSFilesPromisePboardType
NSFontPboardType
NSGeneralPboardType
NSHTMLPboardType
NSPasteboardTypeColor
NSPasteboardTypeFont
NSPasteboardTypeHTML
NSPasteboardTypeMultipleTextSelection
NSPasteboardTypePDF
NSPasteboardTypePNG
NSPasteboardTypeRTF
NSPasteboardTypeRTFD
NSPasteboardTypeRuler
NSPasteboardTypeSound
NSPasteboardTypeString
NSPasteboardTypeTabularText
NSPasteboardTypeTextFinderOptions
NSPasteboardTypeTIFF
NSPDFPboardType
NSPICTPboardType
NSPostScriptPboardType
NSRTFDPboardType
NSRTFPboardType
NSRulerPboardType
NSStringPboardType
NSTabularTextPboardType
NSTIFFPboardType
NSURLPboardType
NSVCardPboardType

@rmottola
Copy link
Member Author

rmottola commented Mar 5, 2024

I'm afraid I already made conflicting changes while adding support for text/plain;charset=utf8 (which seems to be a commonly used type) and clearer debug output.

The only actual bug I can see is that the line: xTypes[numTypes++] = XG_MIME_PNG; should be paired with an increase in the size of the xTypes buffer, to avoid any possibility of a buffer overrun.

Well; I found it cleaner to work on a branch, after we hacked deep into the night and I continued... I did some test this morning. Told you I would push a branch.

My changes are now stale, I think this branch is now useless. I think you missed a bit, but I will look twice and just push to master in case.

@rmottola
Copy link
Member Author

rmottola commented Mar 5, 2024

Here is a full list of the pasteboard types I found in the GUI headers. Some of them are duplicates (because Apple changed the type names, eg NSTIFFPboardType is the same as NSPasteboardTypeTIFF), and some of them are unlikely to have an equivalent in X, but we should be able to support things like PDF, URL, HTML, VCard, perhaps Color, Font Ruler

Yes, you are probably correct. the pastboard server is just a "messenger" there. As we have seen in Price, both available types in copy and in paste are decided by the application and it can be made dynamic from available types. It is true for Image Reps, for other types I don't know, but there even more it may be app-specific? (e.g. RTF, HTML... different text formats, who could transform UTF8 to e.g. latin1 in the copy?)

@@ -1376,6 +1390,11 @@ - (BOOL) xProvideSelection: (XSelectionRequestEvent*)xEvent
xTypes[numTypes++] = XG_MIME_TIFF;
}

if ([types containsObject: NSPasteboardTypePNG])
{
xTypes[numTypes++] = XG_MIME_PNG;
Copy link
Member

Choose a reason for hiding this comment

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

You also need to increase the size of xTypes in line 1353.

Copy link
Member

@fredkiefer fredkiefer left a comment

Choose a reason for hiding this comment

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

This change won't work when guy was build without PNG support. I am not sure what will happen then.

@fredkiefer
Copy link
Member

I did read the comments from Richard only after I commented myself. I agree with his considerations on PNG and other types. In the end it is the application that decides what it offers or what it wants to consume. The pasteboard server should not interfere with that.

@rmottola rmottola closed this Mar 6, 2024
@rmottola rmottola deleted the PNG_clipboard_support branch March 6, 2024 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants