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

gui: Add fix for wxGUI Data import Python error #4504

Merged
merged 7 commits into from
Oct 18, 2024

Conversation

ww2406
Copy link
Contributor

@ww2406 ww2406 commented Oct 13, 2024

This fixes #648 by adding a command tracker queue.

Currently, a user can trigger an error if they manually close the dialog box before the commands. This is due to the command completion callbacks depending on state from the UI dialog which is not accessible if the dialog is destroyed.

To address this, I added a command tracking queue to keep track of the number of commands executing. This PR then modifies the close event handler so that If this queue is not empty, the close command is interrupted and does not execute.

One opportunity might be to consolidate the command dispatch mechanism into one function so future maintainers do not need to explicitly invoke the proposed _addToCommandQueue function, but I wasn't sure if that was optimizing more than needed at the moment.

Look forward to any feedback you might have!

This fixes OSGeo#648 by adding a command tracker queue. When the queue is not empty, closing the dialog is prevented to ensure the completion callback can still access UI elements.
@github-actions github-actions bot added GUI wxGUI related Python Related code is in Python labels Oct 13, 2024
@ww2406 ww2406 changed the title Add fix for #648 with error message during import gui: Add fix for #648 with error message during import Oct 13, 2024
@echoix
Copy link
Member

echoix commented Oct 13, 2024

Great to see some progress on an older issue!

@neteler neteler changed the title gui: Add fix for #648 with error message during import gui: Add fix for wxGUI Data import Python error Oct 13, 2024
@petrasovaa
Copy link
Contributor

@ww2406 thank you for looking into this. I think this makes sense, I will leave just couple notes below.

@ww2406
Copy link
Contributor Author

ww2406 commented Oct 18, 2024

Thanks for the notes, @petrasovaa! I think I've got everything taken care of. Is there anything else needed from me at all?

Copy link
Contributor

@petrasovaa petrasovaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thank you for your contribution!

@petrasovaa petrasovaa added this to the 8.5.0 milestone Oct 18, 2024
@petrasovaa petrasovaa enabled auto-merge (squash) October 18, 2024 13:59
@petrasovaa petrasovaa merged commit 62b001b into OSGeo:main Oct 18, 2024
26 checks passed
@a0x8o a0x8o mentioned this pull request Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GUI wxGUI related Python Related code is in Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] wxGUI Data import Python error
3 participants