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

config: add rfd crate & when input is rfd use file dialog #1852

Closed
wants to merge 1 commit into from
Closed

Conversation

rzmk
Copy link
Collaborator

@rzmk rzmk commented May 31, 2024

Description

Using the rfd crate this PR attempts to add file dialog capability for users to:

  • Select an input using a file dialog when using rfd as their file path parameter instead of a file path string
  • Do the above but for multiple file inputs
  • Use --output or -o with rfd as the output path to choose a file to overwrite or create a new file using the file dialog UI

qsv-rfd-demo

Potential improvements

  • Refactoring for the default file input implementation and the rfd input implementation
  • Better error handling (e.g., dialog opens but user doesn't select a file). Currently this is a panic. Any ideas? @jqnatividad
  • Only tested this on a Windows machine with qsvlite.exe
  • Having issues with stats and frequency (potentially other untested commands too, dialog keeps reopening when you select a file and command never runs). Looks like multiple commands that don't use Config::new right away take rfd as the input(s) which I think could be the issue since without it in the current implementation the args.arg_input value is rfd rather than the path provided by the result of Config::new. Any ideas? @jqnatividad (see below comment)

@rzmk
Copy link
Collaborator Author

rzmk commented Jun 1, 2024

For stats.rs something like this works:

pub fn run(argv: &[&str]) -> CliResult<()> {
    let mut args: Args = util::get_args(USAGE, argv)?;
    if args.arg_input == Some("rfd".to_string()) {
        if let Some(path) = FileDialog::new().set_directory("/").pick_file() {
            args.arg_input = Some(path.to_str().unwrap().to_string());
        } else {
            return fail_clierror!("Could not get path from rfd. Did you select a file?");
        }
    }
    ...

However this could be refactored somehow as there are multiple commands that may have this issue where they use get_args but then attempt to get args.arg_input from it without having ran the file dialog.

@jqnatividad
Copy link
Collaborator

jqnatividad commented Jun 1, 2024

This is awesome @rzmk !

However, we need to maintain qsv's command-line composability and figure out a way to still be able to use stdin reliably.

A lot of qsv commands switch to reading from stdin when the input file is not specified. Actually, there is a little bit of inconsistency in some commands like sqlp where the user needs to specify - to indicate reading from stdin.

Also, commands like stats have a bit involved input handling as its used by other commands and has some extended caching logic.

On the subject of composability using command line redirection, pipes, etc, perhaps the best way to incorporate rfd is to add a new command called prompt?

qsv prompt | qsv stats | qsv prompt --output stats.csv

where we "prompt" for the input CSV, send it to stats, and then we once again "prompt" for the output file.

This would be a less radical way of incorporating rfd without having to refactor input processing in qsv

WDYT?

@rzmk
Copy link
Collaborator Author

rzmk commented Jun 3, 2024

How would you write qsv prompt for commands with multiple inputs? For example in the following scenarios:

qsv headers <file1> <file2> <file3> -o <file3>
qsv validate <input> <json-schema> --valid-output <file>

And what if you only wanted to use qsv prompt for one or some of the inputs? For example using the file dialog only for qsv validate's <json-schema> and --valid-output's <file> while <input> is a specified file path.

@rzmk
Copy link
Collaborator Author

rzmk commented Jun 4, 2024

qsv prompt (#1860, #1861) is currently available, though it may not cover all of the cases discussed here and in #1860.

@rzmk rzmk closed this Jun 4, 2024
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.

2 participants