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.

Sign up for GitHub

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

Jump to bottom

Add interactive mode for message creation #753

Open
wants to merge 3 commits into
base: rolling
Choose a base branch
from
Open

Add interactive mode for message creation #753

wants to merge 3 commits into from

Conversation

Flova
Copy link

@Flova Flova commented Sep 1, 2022 •
edited
Loading

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

  • Interactive editing of the message in simple terminal editor
  • YAML syntax highlighting
  • Mouse support in the editor
  • Storage of the last send message of a given type, so large inputs can be easily reused

Other changes:

  • Slight internal restructure
  • Removes parsing from the publish method
  • Updates doc strings

Closes #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

asciicast

@clalancette clalancette self-assigned this Sep 22, 2022
Flova and others added 3 commits October 12, 2022 20:31
@Flova @clalancette
Add tui (interactive mode) for message creation
a7883ec 
Signed-off-by: Florian Vahl <[email protected]>
@clalancette
Fixes so it works again.
64f6544 
Signed-off-by: Chris Lalancette <[email protected]>
@clalancette
Remove the storing of previous content.
9c1f112 
While it is a nice idea, it adds too much to this feature.

Signed-off-by: Chris Lalancette <[email protected]>
clalancette
clalancette requested changes Oct 12, 2022
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a 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:

  1. 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.
  2. 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.
  3. 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.

fujitatomoya
fujitatomoya reviewed Oct 14, 2022
View reviewed changes
Copy link
Collaborator

@fujitatomoya fujitatomoya left a 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?

@clalancette
Copy link
Contributor

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:

$ ros2 topic pub /foo nav_msgs/msg/Path '{header: {stamp: {sec: 25, nanosec: 30}}}'

And you'lll get this output:

publisher: beginning loop
publishing #1: nav_msgs.msg.Path(header=std_msgs.msg.Header(stamp=builtin_interfaces.msg.Time(sec=25, nanosec=30), frame_id=''), poses=[])

publishing #2: nav_msgs.msg.Path(header=std_msgs.msg.Header(stamp=builtin_interfaces.msg.Time(sec=25, nanosec=30), frame_id=''), poses=[])

(that's the same as in Foxy, Galactic, and Humble).

But in Rolling, you can also do this:

$ ros2 topic pub /foo nav_msgs/msg/Path '{header: {stamp: now}}'

Which will get you this:

publisher: beginning loop
publishing #1: nav_msgs.msg.Path(header=std_msgs.msg.Header(stamp=builtin_interfaces.msg.Time(sec=1666012466, nanosec=830824864), frame_id=''), poses=[])

publishing #2: nav_msgs.msg.Path(header=std_msgs.msg.Header(stamp=builtin_interfaces.msg.Time(sec=1666012467, nanosec=831668171), frame_id=''), poses=[])

(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.

@ymd-stella ymd-stella mentioned this pull request Jul 16, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fujitatomoya fujitatomoya fujitatomoya left review comments

@clalancette clalancette clalancette requested changes

Assignees

@clalancette clalancette

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Message template autocompletion broken
3 participants
@Flova @clalancette @fujitatomoya