Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants