search

nucleic / enaml

Declarative User Interfaces for Python
http://enaml.readthedocs.io/en/latest/
Other
1.52k stars 130 forks source link

Add nonblocking open support to Dialog #552

Open yajaru opened 1 month ago

yajaru commented 1 month ago

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.

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 55.55556% with 4 lines in your changes missing coverage. Please review.

Project coverage is 70.74%. Comparing base (ca6cf4a) to head (58412b0).

:exclamation: Current head 58412b0 differs from pull request most recent head ec2c616

Please upload reports for the commit ec2c616 to get more accurate results.

Additional details and impacted files ```diff @@ 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 ```
MatthieuDartiailh commented 1 month ago

@sccolbert I am interested in the historical reasons you had not to provide such a mode initially.

yajaru commented 1 month ago

@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

sccolbert commented 1 month ago

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 https://github.com/nucleic/enaml/pull/552#issuecomment-2125095697, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABBQSLPRZA4RGCI62MJX23ZDS3AXAVCNFSM6AAAAABIDUFD7GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMRVGA4TKNRZG4 . You are receiving this because you were mentioned.Message ID: @.***>

yajaru commented 1 month ago

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.

sccolbert commented 1 month ago

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 https://github.com/nucleic/enaml/pull/552#issuecomment-2125282782, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABBQSJPQNNVNSAM737PIVTZDTDQ7AVCNFSM6AAAAABIDUFD7GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMRVGI4DENZYGI . You are receiving this because you were mentioned.Message ID: @.***>

yajaru commented 1 month ago

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) GTK4 drops the blocking API and only leaves the non-blocking version.

Win32 supports both blocking and non-blocking. WinUI 3 only supports non-blocking

AppKit supports both blocking and non-blocking SwiftUI only supports 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.

MatthieuDartiailh commented 1 month ago

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 ?

yajaru commented 1 month ago

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. If there are any dumb errors, then squash will have to do for it.

yajaru commented 3 weeks ago

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:

sccolbert commented 3 weeks ago

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 https://github.com/nucleic/enaml/pull/552#issuecomment-2179092471, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABBQSJNOEIUZZGZW2LY753ZIGW3PAVCNFSM6AAAAABIDUFD7GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNZZGA4TENBXGE . You are receiving this because you were mentioned.Message ID: @.***>

yajaru commented 3 weeks ago

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). https://github.com/nucleic/enaml/blob/ca6cf4a6d80eab455cbe8201f9251b46bd185fa7/enaml/widgets/window.py#L152C5-L168C37

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.

yajaru commented 3 weeks ago

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.

MatthieuDartiailh commented 3 weeks ago

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.