-
-
Notifications
You must be signed in to change notification settings - Fork 16
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
fix #13 #14
fix #13 #14
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR as well as all the investigation! Could you address these 2 items? Otherwise I will merge it and update the parts by myself!
captureWindow.on("move", (event) => { | ||
updateMain(captureWindow.getPosition(), null); | ||
checkWindowBounds(captureWindow); | ||
determineScreenToCapture(); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think calling determineScreenToCapture()
frequently caused flickering on some systems which is why I moved this to the moved
event. But it seems this event is not available on linux. Could you add a timer that will trigger the function like this? I think we can then get rid of the moved
event altogether!
let moveTimer = undefined;
captureWindow.on("move", (event) => {
clearTimeout(moveTimer);
moveTimer = setTimeout(() => {
checkWindowBounds(captureWindow);
determineScreenToCapture();
}, 100);
updateMain(captureWindow.getPosition(), null);
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it - something like a debounce
ipcRenderer.on("update-screen-to-capture", async (event, display) => { | ||
cons.log(`>> update display: ${display.id}`) | ||
ipcRenderer.on("update-screen-to-capture", async (event, { display, sourceId }) => { | ||
cons.log(`>> update display: ${display.id}, ${sourceId}`); | ||
if (!currentDisplay || display.id !== currentDisplay.id) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be nice if we moved this condition to determineScreenToCapture()
since the query for available sources has moved there
Also, I am wondering about the display_id property that don't match - does that happen only on your linux machine or on all machines? I tested this on my local (Manjaro Linux) machine and it found the display_id correctly. I wonder if this is hardware (monitor) related or software related |
Another thing I noticed today when trying to use this version - at least in Linux Mint the capture window seems to capture the mouse click events, and you can't click or drag through the capture window. I'll want to look into fixing this as well. |
sorry about the close and re-open - 3-year-old wanted to push buttons on my keyboard 🤦 |
I am using i3, I can confirm clicking through does not really work. It seems to be alternating between letting mouse events through and not. These logs from
whereas it should get MotionNotify events if not trying to go through the capture window:
|
See #13 for details of the problem and the troubleshooting steps I took to arrive at this solution.
display_id
doesn't match"move"
event of capture window ("moved"
wasn't being called in my environment)This pull request is a draft until I can test on all platforms:
Testing complete; ready for review!