Add nonblocking open support to Dialog #552
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #552 +/- ##
==========================================
- Coverage 70.75% 70.74% -0.01%
==========================================
Files 287 287
Lines 25638 25637 -1
Branches 4678 4680 +2
==========================================
- Hits 18139 18136 -3
Misses 6386 6386
- Partials 1113 1115 +2 |
|
@sccolbert I am interested in the historical reasons you had not to provide such a mode initially. |
My guess is that open wasn't available when enaml on QT was being made but, this is pure speculation. |
|
I'm pretty sure you can call `.show()` on a dialog and it will open and
return immediately. However, it won't be modal.
…On Wed, May 22, 2024 at 10:32 AM Will McCall ***@***.***> wrote:
@sccolbert <https://github.com/sccolbert> I am interested in the
historical reasons you had not to provide such a mode initially.
My guess is that open wasn't available when enaml on QT was being made
but, this is pure speculation.
image.png (view on web)
<https://github.com/nucleic/enaml/assets/13341241/3dc5c75b-5a52-42f8-8e18-5395f9b9639c>
—
Reply to this email directly, view it on GitHub
<#552 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABBQSLPRZA4RGCI62MJX23ZDS3AXAVCNFSM6AAAAABIDUFD7GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMRVGA4TKNRZG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Mostly I'm just exposing the api that QT recommends instead of |
|
I'm not arguing against the addition, just pointing out an option that
already exists in case it worked for you.
Historically, the Dialog class was written when we also supported the wx
backend, so that Qt method may have been overlooked for compatibility
reasons.
…On Wed, May 22, 2024 at 11:45 AM Will McCall ***@***.***> wrote:
I'm pretty sure you can call .show() on a dialog and it will open and
return immediately. However, it won't be modal.
show does exist on Dialog by virtue of it existing for all enaml widgets
(which I am leery on, but that is a different conversation). However,
losing the modal behavior is a significant loss given that this is a dialog
and exec_ has the modal behavior we are trying to separate from the
blocking.
Mostly I'm just exposing the api that QT recommends instead of exec (see
https://doc.qt.io/qt-6/qdialog.html#exec, specifically the note) as it
has the closest semantics to exec.
—
Reply to this email directly, view it on GitHub
<#552 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABBQSJPQNNVNSAM737PIVTZDTDQ7AVCNFSM6AAAAABIDUFD7GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMRVGI4DENZYGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
For what it's worth, I took a quick perusal of some other toolkits to see how they handle dialogs. GTK3 has a similar blocking/non-blocking split to QT (with the default falling on the non-blocking side) Win32 supports both blocking and non-blocking. AppKit supports both blocking and non-blocking Note that I'm not a developer in any of these so, it is very possible that I have gotten something wrong or misunderstood the docs. That being said, the world looks to be moving towards non-blocking dialogs exclusively for newer toolkits, which lends credence to this change or something in the same vein. |
|
Thanks for the detailed explanation. Given it means aligning with most toolkit, this addition looks fine to me. Could you add some tests and an entry to the changelog ? |
|
I've written a test but no clue if it works though. Unfortunately, I have not been able to get enaml building on my machine due to dependency issues so, I'm unable to run the test to check if it works. |
|
Ok, this should be ready to go. I would personally squash this down b/c there are intermediate commits due to debugging difficulties. Some notes:
|
|
Do other windows automatically remove themselves from the set, or is it
something in their "destroy" method (or another signal handler), that
removes them.
IIRC, that list is not keeping weakrefs.
…On Wed, Jun 19, 2024 at 11:30 AM Will McCall ***@***.***> wrote:
Ok, this should be ready to go. I would personally squash this down b/c
there are intermediate commits due to debugging difficulties.
Some notes:
- I only wrote a test over the new async Dialog path. This is b/c I'm
not sure how best to test the sync Dialog given that it blocks on the show.
This should be fine as the sync path is untouched by my changes.
- There should also changes made to the stdlib dialogs to expose the
async interface for them as well. I'll leave that as an exercise for later.
- I encountered some weirdness that I'm not sure if I can class as a
bug or not. Basically, when we close the Dialog, it sticks around in the
windows set and does not remove itself. I'm not sure if this is intended
but, once we not longer have a live window for the dialog and all the
references to it are gone, I would expect it to remove itself from the set.
—
Reply to this email directly, view it on GitHub
<#552 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABBQSJNOEIUZZGZW2LY753ZIGW3PAVCNFSM6AAAAABIDUFD7GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNZZGA4TENBXGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
The addition and removal to and from the windows set is done in the initialize and destroy methods. So I guess the issue is that we are not fully executing the lifecycle of the Dialog with the way I have done it here (which is creating a dialog object outside of the tree). This raises the question of what is the proper way to create and show a Dialog given that it is often done in a callback from some user initiated action like a button. |
There was a problem hiding this comment.
Since you did not touch the closing logic I fail to see why the dialog is not removed from the set. What may happen is that Qt does not process all the required events for the closing to complete. They are some testing utilities to process events till a window is closed, could you try using them ?
| @@ -68,6 +71,16 @@ def exec_(self): | |||
| self.activate_proxy() | |||
| return self.proxy.exec_() | |||
|
|
|||
| def open(self): | |||
| """ Launch the dialog as a modal window. | |||
There was a problem hiding this comment.
You could specify that this call is non blocking to make the difference with exec_
|
That was the ticket it seems; I needed wait the QT loop for a bit since the callback to do the destroy was deferred on it. |
|
Tests failures are only Codecov failing to retrieve the upload_token. So all good. Could you add an entry into the changelog and I will merge. |
Add a non-blocking 'open' to Dialog so that applications may continue to run their existing event loops while the dialog is open.
Applications that wish to receive results from the Dialog can do so via the existing 'finished' event.