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

selectionEdgeDestroy: misc cleanups #304

Merged
merged 3 commits into from
May 26, 2023

Conversation

N-R-K
Copy link
Collaborator

@N-R-K N-R-K commented May 21, 2023

  • xeventUnmap() did not check the union type before reading from it.
  • unmap event should always come, no need to sleep/retry 30 times.
  • remove xeventUnmap() and waitUnmapWindowNotify() entirely. the main logic is short enough to inline directly at the call site.
  • rather than unmapping the window, and then destroying it, destroy it directly and wait for the DestroyNotify event. (this may help with: scrot --select borders caught up in the screenshot #284)
  • compare window ID against None rather than 0. no functional difference, but using None is more idiomatic for X11 things.

@N-R-K
Copy link
Collaborator Author

N-R-K commented May 22, 2023

(this may help with: #284)

Just noticed that that bug happens on classic selection mode, not edge. I'll remove that part of the commit message before merging.

@daltomi
Copy link
Collaborator

daltomi commented May 25, 2023

This PR breaks what you want to avoid. That the selection line does not appear in the capture.

scrot --select=hide --line mode=edge,width=4,color=red

2023-05-25-110926_411x290_scrot

@N-R-K
Copy link
Collaborator Author

N-R-K commented May 25, 2023

This PR breaks what you want to avoid. That the selection line does not appear in the capture.

Weird, it's working fine on my end. The selection line doesn't appear on the shot:

scrot

@N-R-K N-R-K linked an issue May 25, 2023 that may be closed by this pull request
@N-R-K
Copy link
Collaborator Author

N-R-K commented May 25, 2023

@daltomi A couple questions

  1. Are you running a compositor?
  2. Does the follow patch make any difference for you?
diff --git a/src/selection_edge.c b/src/selection_edge.c
index f0cc3c7..94ac4d1 100644
--- a/src/selection_edge.c
+++ b/src/selection_edge.c
@@ -135,6 +135,7 @@ void selectionEdgeDestroy(void)
             if (ev.type == DestroyNotify && ev.xdestroywindow.window == pe->wndDraw)
                 break;
         }
+        nanosleep(&(struct timespec){ .tv_nsec = 120 * 1000L * 1000L }, NULL);
 
         XFree(pe->classHint);
     }

@N-R-K N-R-K marked this pull request as draft May 25, 2023 18:05
* xeventUnmap() did not check the union type before reading from it.
* unmap event should always come, no need to sleep/retry 30 times.
* remove xeventUnmap() and waitUnmapWindowNotify() entirely. the main
  logic is short enough to inline directly at the call site.
* rather than unmapping the window, and then destroying it, destroy it
  directly and wait for the DestroyNotify event.
* compare window ID against `None` rather than `0`. no functional
  difference, but using `None` is more idiomatic for X11 things.

this commit also fixes a regression introduced in b2d4358. because we
pull the event out now, the DestroyNotify event won't stick around in
the queue anymore.

Fixes: resurrecting-open-source-projects#319
@daltomi
Copy link
Collaborator

daltomi commented May 25, 2023

Yes with composer, but it also fails without composer: if you select on a window that plays video, the drag is shown:

2023-05-25-190928_700x494_scrot

The master branch works fine. (I originally implemented the delay to avoid these issues, answering your second question)

@N-R-K
Copy link
Collaborator Author

N-R-K commented May 26, 2023

I originally implemented the delay to avoid these issues, answering your second question

I see, I think I understand why this is happening a bit better now. Although we received a DestroyNotify event, the frame still might not have been updated. Or a compositor might be buffering frames.

I pushed a new commit adding the delay back. I think it should fix the issue. (I've also added a comment, because it's not obvious why there's a sleep there).

@N-R-K N-R-K marked this pull request as ready for review May 26, 2023 05:55
Copy link
Collaborator

@daltomi daltomi left a comment

Choose a reason for hiding this comment

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

Now it works well.

@N-R-K N-R-K merged commit 1a9964a into resurrecting-open-source-projects:master May 26, 2023
N-R-K added a commit that referenced this pull request May 26, 2023
@N-R-K N-R-K deleted the unmap_ev branch May 26, 2023 14:38
@N-R-K
Copy link
Collaborator Author

N-R-K commented May 26, 2023

Now it works well.

Thanks for confirming. Looks like my understanding was correct.

I've merged the sleep for now, but I think we'd want to see if there's a more reliable way to detect repaint. I'll open a separate issue for it soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

scrot: received DestroyNotify event
2 participants