search

mumble-voip / mumble

Mumble is an open-source, low-latency, high quality voice chat software.
https://www.mumble.info
Other
6.13k stars 1.09k forks source link

FIX(client): Improve handling of the GlobalShortcutButtons dialog #6443

Closed Hartmnt closed 4 weeks ago

Hartmnt commented 1 month ago

This commit makes the shortcut configuration dialog a modal window. It also explicitly searches for a parent QDialog, such that the window can be centered over the Mumble application instead of randomly out of bounds of the screen. (This is most likely only necessary because of a bug in Qt, but Qt is a pain to debug)

Fixes #5024

davidebeatrici commented 4 weeks ago

Why use a timer that starts immediately? I'm assuming it's to wait for the constructor to return, but I don't see any reason to do that.

Hartmnt commented 4 weeks ago

Why use a timer that starts immediately? I'm assuming it's to wait for the constructor to return, but I don't see any reason to do that.

That's simple: The move does not work, if it happens immediately. I have no idea what Qt is doing, but I guess this whole thing is simply so buggy because the QDialog here is used exclusively in a QStandarditemEditorCreator as the "edit method" of the shortcut item delegate thingy...

Hartmnt commented 4 weeks ago

Why use a timer that starts immediately? I'm assuming it's to wait for the constructor to return, but I don't see any reason to do that.

I tested this again for good measure: Without the timer the move has no effect. I fully blame Qt for this behavior.

Hartmnt commented 4 weeks ago

The only issue I can see is that effectiveParent could have been deleted at the tine the lambda is called. However, I would consider this highly unlikely, given the context and I also don't know of a good way of working around that issue. In peiciple we'd have to associate the timer with effectiveParent in order to prevent Qt from executing our code in case the object has been deleted. Not sure if that's possible though.

Fixed by calculating the position before the timer :)

Krzmbrzl commented 4 weeks ago

I fully blame Qt for this behavior.

Yeah - my experience has also been that the current cycle in the event loop has to be completed before a newly created widget can be interacted with properly :shrug:

Krzmbrzl commented 4 weeks ago

💚 All backports created successfully

Status Branch Result
✅ 1.5.x

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details

davidebeatrici commented 4 weeks ago

Makes sense. There should be no issues in regards to the timer because it is executed by the event loop and thus should be guaranteed to trigger only after the previous entries in the queue are processed.