-
-
Notifications
You must be signed in to change notification settings - Fork 566
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
Vacuum: Implement TUI for the manual mode #845
Conversation
023bce7
to
b4818ea
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi and thanks for the PR! The manual mode is indeed quite cumbersome and this looks like a great addition :-)
I added some comments inline, but it would also be great if you could display the key bindings (either on start as a dialog, or print them out to the screen?).
Also, please update the docs/vacuum.rst
to make it easier to find that such feature exists.
miio/vacuum_tui.py
Outdated
try: | ||
import curses | ||
except ImportError: | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When can this happen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What we import here as curses
is actually an extension module that must be backed by one of few C implementations of a curses
library. ImportError
occurs when none of the implementations are available on the system.
I do not think there is much we can do about it except for mentioning in docs/vacuum.rst
, that curses
lib is a requirement for TUI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. I thought it is part of the standard library and always available.
How about letting it bubble up and doing the import inside the tui()
command? It could be caught there to display a user-friendly error message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I prefer all the import
s to be grouped in one place - at the top of a module. They are easier to manage that way.
If all we want is a prettier error message, I would rather delay the exception until VacuumTUI
is instantiated:
try:
import curses
except ImportError:
curses = None
...
class VacuumTUI:
def __new__(cls, *args, **kwargs):
if curses is None:
raise ImportError("curses library is not available")
return super().__new__(cls)
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what I want is it not to simply crash silently in any case, so your suggested solution works fine, too! Maybe check for the Noneness inside the __init__
though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved the check to __init__
in a new commit.
Added fixes according to inline comments and usage printing before the main loop. Also updated docs to mention the feature. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to go, thanks again @rnovatorov! 🎉
* Vacuum: Export the manual mode constants * Vacuum: Implement TUI for the manual mode * Vacuum: Document manual mode * VacuumTUI: Handle unavailable curses
I recently discovered this project and would like to thank you - it is awesome :)
I played with it a bit and noticed that the vacuum manual mode is difficult to use interactively via CLI. One has to enter somewhat lengthy (more than a char) commands and arguments, which might feel clumsy if a real-time response from the device is desired.
This PR adds a tiny
curses
based TUI to allow for a real-time movement control. I have not thoroughly explored the repo, so the proposed solution might not be optimal.