reujab / wallpaper.rs

A cross-platform Rust library for getting and setting desktop wallpapers
The Unlicense
96 stars 24 forks source link

fix inconsistent function name #1

Closed NyxCode closed 6 years ago

NyxCode commented 6 years ago

For every platform, the function for setting the wallpaper from a file path is set_from_file - except for linux. This fix does break backwards compatibility.

reujab commented 6 years ago

I'm surprised I didn't notice this when I was porting from my Golang version. I actually prefer set_from_path, though and would rather have the macOS and Windows versions use set_from_path.

If you make that change, I'll merge, switch from using &str to Into<Path> or Into<PathBuf>, and publish v2.0.0.

Thanks for the pull request.

NyxCode commented 6 years ago

Sure, no problem. Thanks btw for making this awesome crate, you're really helping me out 🙂

NyxCode commented 6 years ago

I think using AsRef<Path> is a better idea - The standard library uses it everywhere too.

reujab commented 6 years ago

I was thinking about using AsRef or Into until I looked at the code again. The functions just need a string, so converting to a Path or PathBuf just to convert it to a string again would be pointless. I'm going to stick to using &str unless you can think of something better/more convenient.

NyxCode commented 6 years ago

well.. When someone needs to call the method he could then call it with &str, String PathBuf, Path, ...

You're right, the conversion would be pointless, but it would make the functions more easy to call. (Just try to convert a PathBuf into a str - its painfull) In the end, both seems fine - you decide!

reujab commented 6 years ago

I did some testing and tried to make the crate work with AsRef<OsStr> because that would take a &str, String, Path, and PathBuf. And std::process::Command takes OsStrs, so I shouldn't theoretically have to do any further conversion, but I couldn't find any easy way to concatenate OsStrs without converting them to Strings.

I could probably convert the OsStr to a Vec to concatenate it, but I'll just keep things simple by using &str.


I just published v2.0.0 with your patch.