-
Notifications
You must be signed in to change notification settings - Fork 687
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
feat: prevent Electron versions that are in use by some window from being removed #1395
base: main
Are you sure you want to change the base?
Conversation
116874c
to
4398a4e
Compare
I've been pondering it some more, and I think we should expand the usage of the locking as a general solution. There's two main things which need to be solved regarding versions and multiple windows:
The current state of this PR is solving 2, but I think we can solve 1 at the same time. Here's what I've been pondering:
|
4398a4e
to
96a196b
Compare
I did what you suggested and came up with a way to simulate a user trying to do something simultaneously in multiple windows. It's a bit convoluted but the results seem to be reliable, so here it goes: For the
const receiverBroadcastChannel = new BroadcastChannel('testLocks');
receiverBroadcastChannel.addEventListener('message', async () => {
// the version is actually removed if we don't wait for a bit, but I assume
// users doing it manually would be at least this slow anyway :)
await new Promise(resolve => setTimeout(resolve, 20))
console.log('trying to remove the version the other window just selected');
const v25_3_0_removeButton = [
...document.querySelectorAll('table .bp3-button'),
][1];
v25_3_0_removeButton.click();
});
// $0 is the button that selects v25.3.0
$0.click();
const emitterBroadcastChannel = new BroadcastChannel('testLocks');
emitterBroadcastChannel.postMessage('whatever'); The first window will simulate the user clicking on the "Remove" button for 25.3.0 just after that version is selected in the second window, and nothing happens: 2023-07-24_23-25-46.mp4For the simultaneous downloads case (video below):
const receiverBroadcastChannel = new BroadcastChannel('testLocks');
receiverBroadcastChannel.addEventListener('message', () => {
console.log('trying to download...');
document.querySelector('a[label="Not downloaded"]').click();
});
const emitterBroadcastChannel = new BroadcastChannel('testLocks');
emitterBroadcastChannel.postMessage('whatever'); Both windows will receive the broadcast at the same time. Only one of the windows will be granted the lock at first (the window on the right side in the video), and the other window will only get the lock when the download finishes in the first window. Even though the lock does its job, we still get an error and end up with an inconsistent state ("Checking status" in the download selector in the window that first got the lock, and a "dest already exists" error in the other window) when both windows try to download the same version in rapid succession (the same thing happens in the case you mentioned of the user opening a new window when a download is in progress), so maybe 2023-07-24_23-32-14.mp4I think this is mostly ready for review, but I'd like some feedback on this before I add some new tests 🙂 |
ec48598
to
fd6d0f4
Compare
Some initial work on #1394:
2023-07-09_22-25-22.mp4
A couple call-outs:
navigator.locks
@dsanders11 I used plain lifecycle methods to update the UI when a version is set as active in another window, so we might be able to do without another dependency here if that aligns with what you had in mind.
ElectronSettings
is unmounted because of the async state update (there's a comment in the code with more details).Fixes #1394.