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 xdg_mime feature for using xdg_mime for mime type autodetection #13

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mcoffin
Copy link

@mcoffin mcoffin commented Aug 28, 2020

This adds a new feature, xdg_mime, that will force the use of xdg-mime for querying autodetected mime types instead of tree_magic.

Justification

If you use wl-copy to copy a shell script, tree_magic reports its mime type as application/x-shellscript, which is not a text mime type, and therefore you cannot paste that script in to text boxes in say, Firefox, for example.

For users that want the same functionality as the upstream wl-clipboard utilities, which uses xdg-mime to autodetect mime types, they may compile with --features xdg_mime to force the use of xdg-mime instead of tree_magic.

While sticking with the pure-Rust implementation would be preferable, providing this as optional functionality can make these binaries usable for those who need that functionality to work as it would with the other tools.

@mcoffin
Copy link
Author

mcoffin commented Aug 28, 2020

The second push is just a removal of a debug statement that I had missed when I submitted :)

@YaLTeR
Copy link
Owner

YaLTeR commented Aug 28, 2020

Hmm, if xdg-mime gives better results compared to tree_magic then perhaps it's a better idea to just use xdg-mime and drop tree_magic? Are there any major downsides?

@bugaevc
Copy link
Collaborator

bugaevc commented Aug 28, 2020

<...> mime type as application/x-shellscript, which is not a text mime type, and therefore you cannot paste that script in to text boxes in say, Firefox, for example. For users that want the same functionality as the upstream wl-clipboard utilities <...>

FYI, upstream wl-clipboard also recognizes MIME types that end with script to be text types and offers them as plain text as well as the original type.

Perhaps wl-clipboard-rs could learn a few similar tricks 😄

@YaLTeR
Copy link
Owner

YaLTeR commented Aug 28, 2020

Also for me xdg-mime also reports application/x-shellscript:

$ xdg-mime query filetype ~/source/sh/avs.sh 
application/x-shellscript

@mcoffin
Copy link
Author

mcoffin commented Aug 28, 2020

Also for me xdg-mime also reports application/x-shellscript:

$ xdg-mime query filetype ~/source/sh/avs.sh 
application/x-shellscript

@YaLTeR if you run it on the tmp file created by wl-copy, though, it will report text/x-shellscript (due to is missing the .[zcf]?sh extension, so it interprets the shebang line at the top of the file), and correctly assign the text mime types. As for getting better results than tree_magic, I would also say that making this feature a "default" feature would be a good idea, but I didn't do it by default just to be minimally intrusive with the MR. Just let me know if that's desired and I'll add it as a default, or invert the logic to have the feature be use_tree_magic.

@mcoffin
Copy link
Author

mcoffin commented Aug 28, 2020

At the end of the day, I wouldn't say that tree_magic's results are globally worse, but they do cause some issues with pasting if you pipe in a shell script.

An alternative solution would be to just detect when tree_magic reports application/x-shellscript and convert it to text/x-shellscript + application/x-shellscript.

@YaLTeR
Copy link
Owner

YaLTeR commented Aug 28, 2020

Are there any other issues with tree_magic that you've noticed? Maybe instead of using xdg-mime it's sufficient to just add additional text MIME type detection from wl-clipboard?

@mcoffin
Copy link
Author

mcoffin commented Aug 28, 2020

Are there any other issues with tree_magic that you've noticed? Maybe instead of using xdg-mime it's sufficient to just add additional text MIME type detection from wl-clipboard?

That sounds like a good solution, keep the default as tree_magic, keep a feature to force xdg-mime, but add the same detection logic that they have in wl-clipboard upstream for both cases.

If you're good with that I'll update this PR maybe... tonight when I have some time.

@YaLTeR
Copy link
Owner

YaLTeR commented Aug 28, 2020

@YaLTeR if you run it on the tmp file created by wl-copy, though, it will report text/x-shellscript (due to is missing the .[zcf]?sh extension, so it interprets the shebang line at the top of the file)

I still can't get anything different from application/x-shellscript.

┌ ~
└─ wl-copy < source/sh/avs.sh 
┌ ~
└─ xdg-mime query filetype /tmp/wl-copy-buffer-p7ELVZ/avs.sh 
application/x-shellscript
┌ ~
└─ cp source/sh/avs.sh tmp
┌ ~
└─ xdg-mime query filetype tmp
application/x-shellscript

Are you sure xdg-mime is actually any different from tree_magic and it's not just those detection logic from wl-clipboard?

@mcoffin
Copy link
Author

mcoffin commented Aug 29, 2020

Are you sure xdg-mime is actually any different from tree_magic and it's not just those detection logic from wl-clipboard?

@YaLTeR I'm positive. If you run wl-copy < some-script.sh and the script starts with #!/bin/bash or similar, then run xdg-mime /tmp/path/to/TEMPFILE_GENERATED_BY_WL_COPY, then it should give text/x-shellscript.

As a second check, my version of xdg-utils (which contains xdg-mime on arch), is 1.1.3+19+g9816ebb-1, which looks like it might be a git offset from a stable version, which is kinda odd considering I'm on the stable upstream branch of that package.

@AnyTimeTraveler
Copy link

I don't like that it calls out to a binary that may or may not be installed on the user's system, especially since there is no fallback to tree_magic upon an error, or any kind of error handling apart from panicking.

Especially, since there is a rust library implementation of xdg-mime:
https://github.com/ebassi/xdg-mime-rs

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.

4 participants