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 error from new textualize from dismiss() in call_after_refresh. #412

Merged
merged 3 commits into from
Sep 27, 2024

Conversation

JoeZiminski
Copy link
Member

@JoeZiminski JoeZiminski commented Sep 24, 2024

This PR is a bug fix, which addresses a new error that seems to have been caused by a textualize update.

Because of the way the event loop works, as far as I can tell whenever a new Screen is made, it takes control of the application. This means if we are transferring data, if we want to show a 'Transferring' screen, then we need to pass a lot of options to a dialog class which itself runs the transfer. This approach seems a little strange, because the 'Transferring' window is just a lightweigh screen, whereas all options for the transfer are set and coordinated in the Transfer tab, so it makes sense for the transfer logic to be in the transfer-tab class.

To allow this, FinishTransferScreen provides an OK (proceed with transfer) or Cancel (dont transfer) button. If OK is pressed, previously is would change the message to 'Transferring' and call self.dismiss(True) within a call_after_refresh, that would (after GUI update) dismiss and trigger the callback function transfer_data. In recent versions, this reaises an error.

As a workaround, a new (slightly messier) workflow means FinishTransferScreen directly calls the transfer function, and the transfer function tears down FinishTransferScreen itself. This is documented in a docstring in this PR.

An alterantive approach would be:

  1. in the transfer tab, call FinishTransferScreen and wait for response with push_screen_wait
  2. if True, call a run_transfer_data function. This will create a RunTransferDialog class that shows 'Transferring', but will also require all options from the transfer tab to be passed to it, including the interface. Then the code to run the transfer will be run from this dialog window.
  3. This push screen has a callback which handles displaying if the transfer worked or not.

Overall I like this approach less because a lot of the core transfer code is moved into an obscure dialog, rather on the transfer tab, which has the function to coordinate and run the transfer.

See #431 for a much nicer solution to implement ASAP.

Copy link
Member

@niksirbi niksirbi left a comment

Choose a reason for hiding this comment

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

Approving this for the sake of the bugfix ahead of the course, but I'll admit I don't understand the textual TUI logic well enough to meaningfully review this.

datashuttle/tui/tabs/transfer.py Outdated Show resolved Hide resolved
Co-authored-by: Niko Sirmpilatze <[email protected]>
@JoeZiminski JoeZiminski merged commit c76f633 into main Sep 27, 2024
23 checks passed
@JoeZiminski JoeZiminski deleted the fix_dismiss_on_wait_until_refresh branch September 27, 2024 08:51
@JoeZiminski JoeZiminski changed the title Fix error from new textualize from dismiss in call_after_refresh. Fix error from new textualize from dismiss in call_after_refresh. Sep 27, 2024
@JoeZiminski JoeZiminski changed the title Fix error from new textualize from dismiss in call_after_refresh. Fix error from new textualize from dismiss() in call_after_refresh. Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants