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

Fix infinite polling for usb devices after the "add to whitelist" dia… #7486

Merged

Conversation

amaiorano
Copy link
Contributor

…log has been opened once

Problem is that USBDeviceAddToWhitelistDialog starts a timer once created to poll for devices every second. In Qt, closing a dialog doesn't delete it, so it keeps on polling. The fix is to tell QT to delete this dialog on close, so that the timer gets destroyed with it.

@amaiorano
Copy link
Contributor Author

Addresses: https://bugs.dolphin-emu.org/issues/11126

@Zexaron

@ghost
Copy link

ghost commented Oct 11, 2018

Tested, works fine.

@Antidote
Copy link
Contributor

Antidote commented Oct 12, 2018

Does that dialog really need to be allocated on the heap? It's a modal dialog box, so it blocks input to other windows until it's closed. Making it a stack variable would have the same effect as setting that attribute.

@Zexaron @amaiorano

…log has been opened once

Problem is that USBDeviceAddToWhitelistDialog starts a timer once created to poll for devices every second. In Qt, closing a heap-allocated dialog doesn't delete it, so it keeps on polling. This fix is to allocate dialog on the stack, then use "exec" to run it modally without returning. Once closed, the stack instance will get destroyed, thus killing the timer.
@amaiorano amaiorano force-pushed the fix-infinite-polling-for-usb-devices branch from 9899091 to 3dec84a Compare October 13, 2018 01:34
@amaiorano
Copy link
Contributor Author

amaiorano commented Oct 13, 2018

Does that dialog really need to be allocated on the heap? It's a modal dialog box, so it blocks input to other windows until it's closed. Making it a stack variable would have the same effect as setting that attribute.

@Zexaron @amaiorano

(Edit: @Zexaron) @Antidote is right; however, there are two ways to run a modal dialog in Qt. One way is to call setModal(true) + show(), which the code currently does, but this doesn't block. So in this case, we cannot allocate the dialog on the stack. The other way is to call exec(), which is a blocking call, and more common, actually. (See the section entitled "Modal Dialogs" in the Qt docs here). I'm not sure why the code used the less common approach; but I tried with exec(), and it seems to work just fine. This way, we don't need to set the WA_DeleteOnClose attribute. I've rewritten by commit with this new solution.

@Antidote
Copy link
Contributor

I was referring to it being a blocking call for the user, I forgot that show wasn't a blocking call for the application itself.

@amaiorano
Copy link
Contributor Author

I know you probably hadn't realized that show wasn't application blocking, but it's possible to make it so using exec, so that's what I did. Unless there's a good reason not to, should we go ahead with this version? Or go back to my first solution?

@Antidote
Copy link
Contributor

I think using exec was the proper solution from the start, dunno why show was used instead.

@amaiorano
Copy link
Contributor Author

If we're happy with this, can someone from Dolphin team approve this PR?

@Tilka Tilka merged commit 9a1f259 into dolphin-emu:master Oct 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants