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

[rwt_app_chooser] support app args #117

Merged
merged 3 commits into from
May 30, 2023

Conversation

sktometometo
Copy link
Contributor

Support args when start an app.

@CLAassistant
Copy link

CLAassistant commented Jan 18, 2022

CLA assistant check
All committers have signed the CLA.

@Affonso-Gui
Copy link
Contributor

I am generally against increasing the number of prompts. I believe that one of the main strengths of the rwt_app_chooser is its ability to be used on mobile devices, and I don't think that emulating a text-heavy CLI interface is doing any good for that.

Besides that, this implementation also has several points that need to be more thoroughly discussed:

  1. How is the user supposed to know what type of arguments and value ranges are supported?
  2. What should be the default behavior on parse errors? Ignore the prompt and launch with default values anyways does not seem appropriate.
  3. Is this safe regarding injection and other security leaks?

Normally, I would suggest to implement different app instances with different argument values.
If this is impractical due to a wide range of possible values, then I guess we should consider implementing more extensive arg support or consider other client interfaces for the app_manager server.

@sktometometo
Copy link
Contributor Author

It is impractical to implement different app instances for different args because some app can take args with variable length. The number of applications will be a number of combinations.

Main reason of "text-heavy CLI interfaces" for app args is that args field of StartApp service is already extensive and there is still no interface for app_manager apps to tell app_args list to clients of app_manager. So client interface of app_args to a user have to be implemented to express a dictionary which has strings as values and has strings as keys. (I know my implementation is more ambiguous than this.)
And KeyValue message definition comes from roslaunch's args specification. They do not have types and limitations for arg values.
In order to add types and value ranges to app args, we need to re-consider app_args interface of app_manager's apps.

@Affonso-Gui
Copy link
Contributor

I do think that this is a good pretext for reconsidering the app_args interface (adding fields to list_apps message, or adding another channel of communication?)

@Affonso-Gui
Copy link
Contributor

@knorth55

@k-okada
Copy link
Member

k-okada commented May 28, 2023

@sktometometo please fix test

@sktometometo
Copy link
Contributor Author

@k-okada CI passed

@k-okada k-okada merged commit 548cc8c into tork-a:kinetic-devel May 30, 2023
1 check passed
@sktometometo sktometometo deleted the PR/support-app-args branch May 30, 2023 04:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants