-
Notifications
You must be signed in to change notification settings - Fork 199
Add fzf-based interactive selection to ros2cli commands #1151
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
base: rolling
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -16,10 +16,13 @@ | |||||||||||||
| import functools | ||||||||||||||
| import inspect | ||||||||||||||
| import os | ||||||||||||||
| import subprocess | ||||||||||||||
| import sys | ||||||||||||||
| import time | ||||||||||||||
|
|
||||||||||||||
| from typing import Dict | ||||||||||||||
| from typing import List | ||||||||||||||
| from typing import Optional | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| def get_ros_domain_id(): | ||||||||||||||
|
|
@@ -133,3 +136,61 @@ def get_rmw_additional_env(rmw_implementation: str) -> Dict[str, str]: | |||||||||||||
| return { | ||||||||||||||
| 'RMW_IMPLEMENTATION': rmw_implementation, | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| def interactive_select( | ||||||||||||||
| items: List[str], | ||||||||||||||
| prompt: str = 'Select an item:' | ||||||||||||||
| ) -> Optional[str]: | ||||||||||||||
| """ | ||||||||||||||
| Launch interactive fuzzy search using fzf to select from a list of items. | ||||||||||||||
|
|
||||||||||||||
| :param items: List of items to select from | ||||||||||||||
| :param prompt: Prompt message to display in fzf | ||||||||||||||
| :return: Selected item or None if user cancelled or fzf not available | ||||||||||||||
| """ | ||||||||||||||
| if not items: | ||||||||||||||
| print('No items available to select from.', file=sys.stderr) | ||||||||||||||
| return None | ||||||||||||||
|
|
||||||||||||||
| try: | ||||||||||||||
| # Check if fzf is available | ||||||||||||||
| result = subprocess.run( | ||||||||||||||
| ['fzf', '--version'], | ||||||||||||||
| capture_output=True, | ||||||||||||||
| text=True, | ||||||||||||||
| timeout=1 | ||||||||||||||
| ) | ||||||||||||||
| if result.returncode != 0: | ||||||||||||||
| raise FileNotFoundError() | ||||||||||||||
| except (FileNotFoundError, subprocess.TimeoutExpired): | ||||||||||||||
| print( | ||||||||||||||
| 'Error: fzf is not installed but is a dependency for this package. You can install it with rosdep', | ||||||||||||||
| file=sys.stderr | ||||||||||||||
| ) | ||||||||||||||
| return None | ||||||||||||||
|
Comment on lines
+156
to
+171
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i believe this is redundant. i would do the following instead to check import shutil
if shutil.which('fzf') is None:
print('Error: fzf is not installed...', file=sys.stderr)
return None |
||||||||||||||
|
|
||||||||||||||
| try: | ||||||||||||||
| # Launch fzf with items as input - using direct TTY access | ||||||||||||||
| process = subprocess.Popen( | ||||||||||||||
| ['fzf', '--prompt', prompt + ' ', '--height', '40%', '--reverse'], | ||||||||||||||
| stdin=subprocess.PIPE, | ||||||||||||||
| stdout=subprocess.PIPE, | ||||||||||||||
| text=True | ||||||||||||||
| ) | ||||||||||||||
|
|
||||||||||||||
| # Send items to fzf | ||||||||||||||
| stdout, _ = process.communicate(input='\n'.join(items)) | ||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We do not need to have any signal control here when opening the child process? If the user presses Ctrl+C, this may leave the terminal in a bad state or not clean up properly... maybe we can add import signal
try:
stdout, _ = process.communicate(input='\n'.join(items))
except KeyboardInterrupt:
process.terminate()
process.wait()
return None
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. besides that, i would add the timeout just in case if fzf hangs for some reason, the entire command hangs indefinitely. |
||||||||||||||
|
|
||||||||||||||
| # Check if user cancelled (Ctrl-C or ESC) | ||||||||||||||
| if process.returncode != 0: | ||||||||||||||
| return None | ||||||||||||||
|
|
||||||||||||||
| # Return selected item (strip newline) | ||||||||||||||
| selected = stdout.strip() | ||||||||||||||
| return selected if selected else None | ||||||||||||||
|
|
||||||||||||||
| except Exception as e: | ||||||||||||||
| print(f'Error during interactive selection: {e}', file=sys.stderr) | ||||||||||||||
| return None | ||||||||||||||
|
Comment on lines
+193
to
+195
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Catching all exceptions hides bugs and makes debugging harder.
Suggested change
|
||||||||||||||
|
|
||||||||||||||
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.
does it miss tty detection? no check for whether stdin/stdout is a tty? i think this will cause scripts and CI/CD pipelines to hang, if it meets this case. related to this issue, fzf reads keyboard input from /dev/tty, not stdin. however, when ros2cli commands are piped or redirected, fzf may not be able to access the tty properly?
the followings are pseudo code,
and