Add screenshot for single screen #32

SushiWaUmai · 2022-07-23T04:46:09Z

Resolves #30

SushiWaUmai · 2022-07-23T04:47:00Z

Sorry for the diffs, my formatter did that automatically.

Lonami

Assuming it builds changes look fine, although perhaps we should separate the format and changes into two commits.

pickfire · 2022-07-23T07:08:14Z

src/main.rs

@@ -1,4 +1,4 @@
-// This Source Code Form is subject to the terms of the Mozilla Public
+// This Source Code Form is subject to the terms of the Mozilla& Public


I don't think this change is necessary.

pickfire · 2022-07-23T07:09:57Z

It doesn't seem to update the docs, other people would have to look at the code to know.

9ary · 2022-07-23T07:13:31Z

It doesn't seem to update the docs, other people would have to look at the code to know.

~~They did add help text for the new flag, but it should indeed be copied over to the README.~~
I'm blind. This doesn't seem to add a new flag at all?

bb010g · 2022-07-23T07:20:21Z

Sorry for the diffs, my formatter did that automatically.

Please split the reformatting out into a separate, dedicated commit.

9ary · 2022-07-23T07:23:02Z

src/main.rs

@@ -32,7 +32,7 @@ fn run() -> i32 {

    let mut opts = Options::new();
    opts.optopt("i", "id", "Window to capture", "ID");
-    opts.optopt("g", "geometry", "Area to capture", "WxH+X+Y");
+    opts.optflagopt("g", "geometry", "Area to capture", "WxH+X+Y");


Ok, I see what you did here. This means that -g without specifying a region would capture the screen where the cursor is. This isn't good, please add a separate flag instead, and document it properly.

bb010g

I agree with @9ary. Please do not introduce options that optionally take values to this CLI. The functionality looks fine, if implemented differently.

SushiWaUmai · 2022-07-23T08:52:51Z

Hi, I appreciate the reviews, my suggestion would be to revert the commit once and

separate the commits from functionality and formatting
introduce the new flag
add more documentation

Unfortunately I do not have much time, and therefore I cannot implement it right away.

This reverts commit 5bfc2df.

9ary

Looking pretty good overall.

Another concern I have is you don't seem to handle passing both -g and -s gracefully. Currently, -s will override -g unconditionally. I would prefer to either switch that around, or take the intersection.

9ary · 2022-07-25T07:18:32Z

src/main.rs

-    let sel = match matches.opt_str("g") {
+
+    let mut sel: util::Rect;
+    sel = match matches.opt_str("g") {


I don't think splitting this line in two or even specifying the type is strictly necessary here. Is it?

9ary · 2022-07-25T07:27:49Z

src/main.rs

+            }
+        };
+
+        let screen_rects = match display.get_screen_rects(display.get_default_root()) {


This introduces an additional call to get_screen_rects which is a little inefficient. It should be cached, or better yet lazily initialized so it only gets called when needed. std::Lazy is currently experimental, so we'll need to find a crate for this.
Since get_screen_rects returns an iterator, you'll need to collect it into a vector, and then make the masking step reuse that. Maybe removing the iterator altogether would be a good idea. I believe I originally wrote it this way to avoid superfluous allocations, but that's already out of the window, so there's no point in keeping it.

We also already cache the result of get_default_root during initialization. You should definitely use that.

As for this part, I think it should be fine if we cache it for now using a vector, I think we can still change that later on.

Yes, do that, and adapt the masking step to reuse the same vector. Lazy evaluation might not even be necessary considering that masking always happens so forget about that.

9ary · 2022-07-25T07:34:58Z

src/main.rs

+        sel = match screen_rects.enumerate().find(|(_, r)| r.contains(cursor)) {
+            Some((_, r)) => r,
+            None => {
+                eprintln!("Failed to find screen containing cursor");
+                return 1;
+            }
+        }


Alright so under normal circumstances this shouldn't fail. That said, maybe we should only bother to mutate sel if something is found? Honestly not sure what the best strategy is here. This seems sound enough all things considered.

src/util.rs

9ary · 2022-07-25T07:39:35Z

src/xwrap.rs

+    pub fn get_cursor_position(&self) -> Option<util::Pos> {
+        let mut x = 0;
+        let mut y = 0;
+
+        let (mut win_x, mut win_y, mut mask) = (0, 0, 0);
+        let mut root_win = self.get_default_root();
+
+        unsafe {
+            xlib::XQueryPointer(
+                self.handle,
+                root_win,
+                &mut root_win,
+                &mut root_win,
+                &mut x,
+                &mut y,
+                &mut win_x,
+                &mut win_y,
+                &mut mask,
+            );
+        }
+
+
+        Some(util::Pos { x, y })
+    }


This function returns an Option, but it doesn't appear to handle the failure case. If XQueryPointer is fallible, then it should return None on failure. Otherwise, get rid of the Option.

Also I see another call to get_default_root. This method should probably be implemented for xlib::Window instead, or take the root window as another parameter.

src/main.rs

bb010g · 2022-07-25T08:07:19Z

Another concern I have is you don't seem to handle passing both -g and -s gracefully. Currently, -s will override -g unconditionally. I would prefer to either switch that around, or take the intersection.

On this note, I would highly recommend at least reading the Command Line Arguments section of Fuchsia's Command-line Tools Rubric. Here's a relevant section from it:

Mutually Exclusive Options

Some options don't make sense with other options. We call the options mutually exclusive.

Passing mutually exclusive options is considered a user error. When this occurs the tool will do one of the following:

Write an error message explaining the issue and exit with a non-zero result code; doing no work (i.e. there was no data changed as a result of the call). This is the expected handling, so no further documentation or notes are required.

Prioritize one option over another. E.g. "passing -z will override -y". In this case the handling will be documented in the --help output.

Other handling is possible (first takes precedence or last takes precedence or something else) though this is discouraged. In this case the handling will be documented in the Description, Options, and Notes; though "See Notes" may be used in Description and Options with the full write-up in Notes.

9ary · 2022-07-25T08:11:35Z

Erroring out is also an acceptable solution yes. Either way. the conflict behavior should be documented.

9ary

I think -s should also conflict with -i. -g + -i makes sense, and we just take the intersection there, but that wouldn't make sense with -s.

9ary · 2022-07-26T11:20:35Z

src/xwrap.rs

+    pub fn get_cursor_position(&self, mut root_win: xlib::Window) -> util::Point {
+        let mut x = 0;
+        let mut y = 0;
+
+        let (mut win_x, mut win_y, mut mask) = (0, 0, 0);
+
+        unsafe {
+            xlib::XQueryPointer(
+                self.handle,
+                root_win,
+                &mut root_win,
+                &mut root_win,
+                &mut x,
+                &mut y,
+                &mut win_x,
+                &mut win_y,
+                &mut mask,
+            );
+        }
+
+        util::Point { x, y }
+    }
 }


So according to the documentation, XQueryPointer is fallible. https://tronche.com/gui/x/xlib/window-information/XQueryPointer.html

The first failure mode is a BadWindow error if you pass in an invalid window. For our use that would be a programming error so you can just make that a panic. (actually xlib will very likely do that itself so maybe it's not necessary, try passing it some random integer and see what happens)

It looks like the other failure case is related to having multiple X screens (not the same as multiple monitors). That is a very uncommon configuration that we shouldn't bother supporting. Regardless, you should re-introduce the Option and the error message for when XQueryPointer returns false.

Lastly, the root_win argument should just be called window, and it shouldn't be mutable. This is a big C-ism that we don't have to deal with in Rust. If you want to use the values XQueryPointer puts in there, you should put them in the return struct. It's unnecessary in this case though, so you should just throw root_return and child_return away.

I do wonder, what happens if you pass 0 as the window argument? If it works, then we should still get the cursor position relative to the root window and you can get rid of the window argument. Otherwise, win_x and win_y are the values you should be returning, as they are relative to the window you're passing in.

Ok, I'll make sure that I get that done soon, unfortunately this is the first project I worked on with rust so I'll have to take a closer look to the error handing part.

pickfire · 2022-07-27T11:41:59Z

src/main.rs

@@ -133,20 +133,20 @@ fn run() -> i32 {
        },
    };

+    let screen_rects: Vec<util::Rect> = match display.get_screen_rects(root) {
+        Some(r) => r.map(|r| r.clone()).collect(),


Suggested change

Some(r) => r.map(|r| r.clone()).collect(),

Some(r) => r.cloned().collect(),

9ary · 2022-07-27T18:00:25Z

src/main.rs

+        Some(r) => r.map(|r| r.clone()).collect(),
+        None => {
+            eprintln!("Failed to get screen rects");
+            return 1;


This means that shotgun will always bail out if screens can't be enumerated, even when it's not strictly required. XRandR should be available on pretty much anything these days, do we care enough to support the failure case?

SushiWaUmai · 2022-07-30T10:59:15Z

Alright I feel like I fixed all the stuff mentioned, did I miss anything?

9ary · 2022-07-30T11:11:18Z

I'm still not sure the usage of XQueryPointer is correct, but I currently don't have the energy to continue the back and forth on this pull request (for reasons unrelated). If it works in its current state then I think it's okay to merge this as it is and we can fix that wrapper later.

I'll leave it up to someone who can actually test this to merge the PR and prepare a new release.

Lonami

There's still some formatting changes but I don't think that's worth fighting over. Implementation seems fine if it gets the job done, Most if not all concerns have also been fixed so with @9ary's approval I'm going to go ahead and merge it. Thanks!

Lonami

(I meant to approve, not request changes.)

Lonami approved these changes Jul 23, 2022

View reviewed changes

add screenshot of single screen

5bfc2df

pickfire reviewed Jul 23, 2022

View reviewed changes

9ary reviewed Jul 23, 2022

View reviewed changes

bb010g requested changes Jul 23, 2022

View reviewed changes

SushiWaUmai added 4 commits July 25, 2022 06:00

Revert "Revert commit for better code quality"

a87c62a

This reverts commit 5bfc2df.

add screenshot functionality for single screen using -s flag

22d90bf

add flag to README file

ed3f59b

format code

26f3063

9ary requested changes Jul 25, 2022

View reviewed changes

9ary requested changes Jul 26, 2022

View reviewed changes

SushiWaUmai added 6 commits July 26, 2022 13:52

Rename the struct Pos to Point

70fc700

remove option return value and take the root window as a parameter

a25fcaa

add error message for mutually exclusive options -g and -s

de01f92

update the optionflag for single screen capture

15f50f0

use the cache for get_default_root

dc0d790

cache screen_rects

98d6ab3

pickfire reviewed Jul 27, 2022

View reviewed changes

9ary reviewed Jul 27, 2022

View reviewed changes

SushiWaUmai added 4 commits July 28, 2022 12:04

remove redundant clone

05795a5

pass 0s to XQueryPointer and remove redundant variables

2ebfad1

return option in get_cursor_position

5434ffe

make -i conflict with -s

06a8c5c

format code

ccac5ac

9ary approved these changes Jul 30, 2022

View reviewed changes

Lonami requested changes Jul 30, 2022

View reviewed changes

Lonami approved these changes Jul 30, 2022

View reviewed changes

Lonami merged commit 97ecf32 into neXromancers:master Jul 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add screenshot for single screen #32

Add screenshot for single screen #32

SushiWaUmai commented Jul 23, 2022

SushiWaUmai commented Jul 23, 2022

Lonami left a comment

pickfire Jul 23, 2022

pickfire commented Jul 23, 2022 •

edited

Loading

9ary commented Jul 23, 2022 •

edited

Loading

bb010g commented Jul 23, 2022

9ary Jul 23, 2022

bb010g left a comment

SushiWaUmai commented Jul 23, 2022

9ary left a comment

9ary Jul 25, 2022

9ary Jul 25, 2022

SushiWaUmai Jul 26, 2022

9ary Jul 26, 2022

9ary Jul 25, 2022

9ary Jul 25, 2022

9ary Jul 25, 2022

bb010g commented Jul 25, 2022

Mutually Exclusive Options

9ary commented Jul 25, 2022

9ary left a comment

9ary Jul 26, 2022

SushiWaUmai Jul 27, 2022

pickfire Jul 27, 2022

9ary Jul 27, 2022

SushiWaUmai commented Jul 30, 2022

9ary commented Jul 30, 2022

Lonami left a comment

Lonami left a comment

		@@ -1,4 +1,4 @@
		// This Source Code Form is subject to the terms of the Mozilla Public
		// This Source Code Form is subject to the terms of the Mozilla& Public

	Some(r) => r.map(\|r\| r.clone()).collect(),
	Some(r) => r.cloned().collect(),

Add screenshot for single screen #32

Add screenshot for single screen #32

Conversation

SushiWaUmai commented Jul 23, 2022

SushiWaUmai commented Jul 23, 2022

Lonami left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pickfire commented Jul 23, 2022 • edited Loading

9ary commented Jul 23, 2022 • edited Loading

bb010g commented Jul 23, 2022

Choose a reason for hiding this comment

bb010g left a comment

Choose a reason for hiding this comment

SushiWaUmai commented Jul 23, 2022

9ary left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bb010g commented Jul 25, 2022

Mutually Exclusive Options

9ary commented Jul 25, 2022

9ary left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SushiWaUmai commented Jul 30, 2022

9ary commented Jul 30, 2022

Lonami left a comment

Choose a reason for hiding this comment

Lonami left a comment

Choose a reason for hiding this comment

pickfire commented Jul 23, 2022 •

edited

Loading

9ary commented Jul 23, 2022 •

edited

Loading