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

Feature: Add timeout option to interactive confirm printer. #568

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

KarolosLykos
Copy link
Contributor

@KarolosLykos KarolosLykos commented Aug 30, 2023

Description

Added a timeout feature to the interactive confirm printer. Now, if a timeout is specified and no answer is provided, the default value is automatically selected. This enhancement improves the user experience by allowing the confirmation prompt to automatically proceed with the default choice when a response isn't provided within the defined timeframe.

Scope

interactive_confirm_printer

  • Bug Fix
  • New Feature
  • Documentation
  • Other

Related Issue

Fixes #543

To-Do Checklist

  • I tested my changes
  • I have commented every method that I created/changed
  • I updated the examples to fit with my changes
  • I have added tests for my newly created methods

@MarvinJWendt
Copy link
Member

Hi @KarolosLykos, looks good so far! The only thing is, that it's not visible, that the ConfirmPrinter will automatically choose a value, after a given time.

Could you add a countdown to the output, that only shows if Timeout > 0?

Something like this:
image

Where the [5s] goes down second by second.

This could be solved by using an area, but I would recommend a \r here, like in progressbar and spinner (which deletes the current line).

Also, normally this shows, when a user chooses a value:
image

I think we could show something like that, when the timeout is reached:
image

@KarolosLykos
Copy link
Contributor Author

KarolosLykos commented Sep 3, 2023

Hi @KarolosLykos, looks good so far! The only thing is, that it's not visible, that the ConfirmPrinter will automatically choose a value, after a given time.

Could you add a countdown to the output, that only shows if Timeout > 0?

Something like this: image

Where the [5s] goes down second by second.

This could be solved by using an area, but I would recommend a \r here, like in progressbar and spinner (which deletes the current line).

Also, normally this shows, when a user chooses a value: image

I think we could show something like that, when the timeout is reached: image

Yes, that make sense. To ensure I grasp your point correctly, you're suggesting refactoring the show function and its associated methods to utilise a writer, similar to the other printing functions. Is that the essence of your suggestion?

@KarolosLykos
Copy link
Contributor Author

@MarvinJWendt Hi, I've made the requested changes, but I'm encountering issues with passing the checks, especially on the Windows platform. I'm currently using Arch Linux and lack access to a Windows machine. Could you please review the changes and provide feedback? Additionally, could you advise on what needs to be modified for the Windows platform tests, as they seem to hang on tests I didn't modify?

@KarolosLykos KarolosLykos marked this pull request as ready for review September 14, 2023 13:23
@MarvinJWendt
Copy link
Member

Hi @KarolosLykos, I will look into it. Weird that windows times out.

@MarvinJWendt
Copy link
Member

Hi @KarolosLykos, I spent some time trying to find the bug here. I guess that some keyboard event is pushed and never read, meaning that all following events are out of sync (which would explain the next tests to fail).

We want to rewrite the whole testing system in the future anyway, so I think you can remove the tests that you introduced for this printer.

In the future, we want to test all printers separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Request to add a timeout for InteractiveConfirm
2 participants