Cratify

[dns2utf8]

Hi

I am using your code as a library for my color_blinder project.
For that I had to split it into two parts:

• src/lib.rs the public interface usable by others
• src/bin/shotgun.rs the remains of the former src/main.rs using the src/lib.rs for the core functionality.

As of now, I have not been testing your application too much, but it did still make a screenshot with cargo run.

If you are happy with the changes a new release would allow me to use a fixed version. To be clear, I had this done today in one go and it might be a little rough still.
If you wish more changes I would be happy to workout that too.

Cheers,
Stefan

[pickfire]

Why not use src/main.rs instead of src/bin/shotgun.rs?

[pickfire]

Also, I think we may need to seperate the features when being used as a library, I think getopts would not be needed.

[dns2utf8]

I use src/bin/shotgun.rs because it allows the crate to be used as a library at the same time without making the imports of the modules really hard.
The problem is that main.rs can not use lib.rs directly because it is on the same level.

I agree that getopts should not be included in the library built. Currently, the only option without splitting up the crate into two is having a feature flag for the CLI build.
So as of today, we have two options:

• Split up the crate into the lib and the bin part
• Keep them together and choose between:
  1. Have the getopts feature in by default, so linking it as a dependency the user should use default-features = false in all their Cargo.tomls
  2. Have the getopts feature optional, so building the CLI requires passing --features getopts to cargo

Do you have a favourite?
Splitting up the crate could also mean choosing a new name or adding a suffix? For example shotgun-sys

Cheers,
Stefan

[pickfire]

The problem is that main.rs can not use lib.rs directly because it is on the same level.

It should be possible if you don't use use crate::xxx but from what I see it looks possible.

[dns2utf8]

Hi @pickfire

It looks like the resolution process has been fixed with the latest version of rust (1.48.0 as of writing).
I moved the code for the binary to src/main.rs as you requested.

Cheers,
Stefan

[pickfire]

Nice, I didn't know about it. But I have left some comments last time that wasn't resolved. Mainly related to library docs, ideally we would have description and examples for the public functions.

[dns2utf8]

I am sorry, I can not see the comments regarding the docs. I added a bit of documentation to the capture function now.

Also, I included a github action so it gets built on PRs and Pushes.
It should look like this once the PR is merged: https://github.com/dns2utf8/shotgun-1/actions

[9ary]

Nice, thanks for putting in so much work. Can you please avoid pushing commits one by one in quick succession though? I got an email for each of those.
Also, even though this was originally my project, I'm on Wayland now so I'll leave the reviewing to someone else, as I can't even test this PR.

[dns2utf8]

I do, however, setting up CI forces me to push every time I have to add a package to the job

[9ary]

That's a bit odd, shouldn't it consider pushes atomic?

[dns2utf8]

It does. Setting up CI is usually done in a separate branch because for every change one wants to test in the CI a new commit + push is needed. This should hold for the time being, so you don't have to expect so many emails in the future.

On another note, do you have a similar project for wayland? Or plans to make it work with wayland?

[9ary]

On another note, do you have a similar project for wayland? Or plans to make it work with wayland?

I've been using grim which works pretty well, it's about as simple as shotgun so I don't really feel the need to port it over. The only missing feature, which I understand is a limitation of the current Wayland protocols, is the ability to capture a window's contents directly rather than a crop of the entire desktop.

The majority of shotgun is really only about getting an image from the X server, so Wayland support would involve rewriting most of it and maintaining both implementations in the same repo. Considering there's already a really good equivalent tool out there, I'd rather recommend that instead.

[dns2utf8]

That makes sense. When I switch to wayland I will look into wrapping grim from rust because I need the screenshot in my color blinder GUI application, preferably without having to save it to disk first.

Regarding this PR, I added a hint for crates.io to display the README.md in the listing.

[9ary]

For what it's worth I would accept a pull request for Wayland support, I'm just not really motivated to do it myself. There is definitely value in a single library crate with a unified API and multiple backends, it's just that for command line tools this simple, it's often easier to adapt your scripts to a new one than to make the program itself more complex.

[dns2utf8]

@pickfire What do you think?

[pickfire]

@pickfire What do you think?

I wrote some comments but you haven't replied to the new ones.

[dns2utf8]

I can not see any comments in the code:

Could you check if the "submit review" function worked on github?

Maybe there is an extension interfering with the function?

[pickfire]

It isn't comments but pending requests.

[dns2utf8]

Thank you for the screenshot. Your review comments are still pending until you click the "finish review" button on the top right.

I am working through the comments I can see in the screenshot now

[dns2utf8]

I refactored the code:

• the geometry parsing is now in the xwrap module
• it does not allow empty input and it has tests for it

This is all the time I have currently.

[pickfire]

I think we should preserve the original error, maybe keep X and Y separate since that is what was returned by the C function?

[pickfire]

Other than the mentioned comment other stuff looks good to me, did not test it locally.

Thanks for sending the patching and keeping up with my comments.

[Lonami]

This PR is really old and has a lot of conflicts, so I will be closing it.

(I haven't actually followed along what happened here, I'm just cleaning up as I don't think it makes sense to keep this open.)