-
Notifications
You must be signed in to change notification settings - Fork 164
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
Add interactive mode for message creation #753
base: rolling
Are you sure you want to change the base?
Conversation
Signed-off-by: Florian Vahl <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
While it is a nice idea, it adds too much to this feature. Signed-off-by: Chris Lalancette <[email protected]>
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.
I'm sorry for the long delay in getting to look at this. I do think this is a valuable feature to have, so thanks for getting it started.
That said, I have a couple of concerns with the implementation:
- The storing of temporary files to reuse next time is clever, but I think it is doing too much in one PR. I'd rather us just get the interactive feature in first, then we can think about adding in caching separately.
- I had to rebase this whole thing because the newly added 'auto' (for std_msgs/Header) and 'now' (for builtin_interfaces/Time) keywords touch the same code. I did that, and fixed the resulting problems, and also ended up removing the caching mechanism.
- The way that this feature interacts with 'auto' and 'now' isn't ideal. In particular, there is no way to specify them in interactive mode. Additionally, if you pass them on the command line, and also enter interactive mode, they don't populate properly. I'm not quite sure how to fix this, but it would be really good to have them integrated.
f27a29f
to
9c1f112
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.
Storage of the last send message of a given type, so large inputs can be easily reused
where is this come from? is this provided by python package?
I did that, and fixed the resulting problems, and also ended up removing the caching mechanism.
Ah, okay isee.
In particular, there is no way to specify them in interactive mode. Additionally, if you pass them on the command line, and also enter interactive mode
When we are using the interactive mode, can we just ask the user to fill in the contents via editor?
Let me show you an example where it wouldn't be ideal. Right now in Rolling, you can do this:
And you'lll get this output:
(that's the same as in Foxy, Galactic, and Humble). But in Rolling, you can also do this:
Which will get you this:
(notice that the time is increasing on every publish). If we only allow interactive mode to specify numbers for the timestamps, then we can get the previous behavior, but not the new one. It would be much nicer if this was integrated together so that it would work for users like they expect. |
As discussed in #742 this PR adds an interactive TUI to the
ros2 topic pub
command.The cli is mostly kept as it is, only an additional
-i
or--interactive
flag is added. This allows the old behavior for scripting etc. while enhancing the experience in a manual use case.Added dependencies
python3-prompt-toolkit
python3-pygments
Features
Other changes:
publish
methodCloses #742
The implementation for the
topic pub
command serves as a proof of concept. We can expand it to services and actions if the details are ironed out.Demo