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

Draw selection preview around window #22

Merged
merged 6 commits into from
Apr 8, 2020

Conversation

pickfire
Copy link
Contributor

@pickfire pickfire commented Apr 4, 2020

Fix #8

@Lonami
Copy link
Member

Lonami commented Apr 4, 2020

Hm… these changes seem to regress rendering by a lot. The guides lag behind the mouse so much, at least on my machine.

Copy link
Member

@expectocode expectocode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this! I think the width / colour of the new lines warrant their own config flags, and maybe one for enabling/disabling them too.

src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/lib/mod.rs Outdated Show resolved Hide resolved
@Lonami
Copy link
Member

Lonami commented Apr 4, 2020

This does the trick for me:

diff --git a/src/main.rs b/src/main.rs
index edff83a..1eb0143 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -131,6 +131,10 @@ fn main() -> Result<(), String> {
     let mut in_selection = false;
     let mut ignore_next_release = false;
 
+    use std::time::{Instant, Duration};
+    let mut last_draw = Instant::now();
+    const DRAW_DELTA: Duration = Duration::from_millis(8);
+
     // TODO draw rectangle around window under cursor
     loop {
         let ev = conn
@@ -158,6 +162,12 @@ fn main() -> Result<(), String> {
                 return Err("Exiting due to ESC key press".into());
             }
             xcb::MOTION_NOTIFY => {
+                let now = Instant::now();
+                if now - last_draw < DRAW_DELTA {
+                    continue;
+                }
+                last_draw = now;
+
                 let motion: &xcb::MotionNotifyEvent = unsafe { xcb::cast_event(&ev) };
 
                 let (left_x, right_x) = min_max(motion.event_x(), start_pt.x());

Not that's the best code but it works.

@pickfire
Copy link
Contributor Author

pickfire commented Apr 4, 2020

@Lonami That should be in another patch from what I will say since it fixes another problem.

@expectocode
Copy link
Member

by the way @Lonami in your patch, your subtraction of Instants should be saturating_sub since I think they panic if they go negative

@Lonami
Copy link
Member

Lonami commented Apr 4, 2020

they panic if they go negative

They can't be negative because Instant is monotonic:

A measurement of a monotonically nondecreasing clock.

@pickfire
Copy link
Contributor Author

pickfire commented Apr 4, 2020

@Lonami But rust does not know that, bound checks might still be there.

@expectocode
Copy link
Member

they panic if they go negative

They can't be negative because Instant is monotonic:

A measurement of a monotonically nondecreasing clock.

Ah you're right, I was confusing it with a different situation

@Lonami
Copy link
Member

Lonami commented Apr 4, 2020

@Lonami That should be in another patch

Most likely because it affects previous code, I only mentioned it because it makes this change-set usable.

@Lonami
Copy link
Member

Lonami commented Apr 4, 2020

@Lonami But rust does not know that, bound checks might still be there.

That's micro-optimization compared to the rest of things we're doing :)

@expectocode
Copy link
Member

expectocode commented Apr 5, 2020

This TODO should be removed: https://github.com/neXromancers/hacksaw/pull/22/files#diff-639fbc4ef05b315af92b4d836c31b023R140 (// TODO draw rectangle around window under cursor)

Copy link
Member

@Lonami Lonami left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with merging these and then improving performance.

@pickfire pickfire requested a review from expectocode April 7, 2020 03:51
@pickfire
Copy link
Contributor Author

pickfire commented Apr 7, 2020

@Lonami That's what I think it's best, just don't release before we update the other parts, I would also like to do more refactorings, especially the state of current functions. Also, I will plan out a public API for libraries then if I have the time I can help out improving the performance.

Note that there is a bug here when keys are pressed, windows changes. The border still stick to the old changes, we should listen to keyboard events now too but propagate them, I am not sure how to do this yet.

@expectocode
Copy link
Member

@pickfire How about we create a branch to work on this, and merge it into master when it's performant and the bugs are worked out?

@9ary
Copy link
Member

9ary commented Apr 8, 2020

I like this idea personally.

@pickfire
Copy link
Contributor Author

pickfire commented Apr 8, 2020

@expectocode Of course, that's good.

@expectocode expectocode changed the base branch from master to window-selection-border April 8, 2020 13:35
@expectocode expectocode merged commit e75426c into neXromancers:window-selection-border Apr 8, 2020
@pickfire pickfire deleted the window branch April 8, 2020 13:50
@Lonami
Copy link
Member

Lonami commented Apr 8, 2020

The most debated PR on the history of neXromancers so far has been merged 🎉

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.

Draw a selection preview around windows
4 participants