feature: Capture read command contents without displaying it with the silent flag #3947
Conversation
src/builtin.cpp
Outdated
| bool split_null = false; | ||
|
|
||
| const wchar_t *short_options = L"ac:ghlm:n:p:suxzP:UR:"; | ||
| const wchar_t *short_options = L"ac:ghlm:n:p:siuxzP:UR:"; |
There was a problem hiding this comment.
Notice that we are trying to keep the flags sorted so adding the i between the hl pair is better.
P.S., Yes, there is lots of code elsewhere in fish that violates this best practice but Rome wasn't built in a day.
There was a problem hiding this comment.
Ok, next time I will know, thanks
| @@ -168,6 +168,8 @@ class reader_data_t { | |||
| bool suppress_autosuggestion; | |||
| /// Whether abbreviations are expanded. | |||
| bool expand_abbreviations; | |||
| /// Silent mode used for password input on the read command | |||
| bool silent; | |||
There was a problem hiding this comment.
I don't see where you are initializing this in the constructor for the class. Which means the initial value it undefined. See the /// Constructor comment a hundred lines below.
There was a problem hiding this comment.
I will check for it
src/reader.cpp
Outdated
| @@ -2044,6 +2051,8 @@ void reader_set_test_function(parser_test_error_bits_t (*f)(const wchar_t *)) { | |||
|
|
|||
| void reader_set_exit_on_interrupt(bool i) { data->exit_on_interrupt = i; } | |||
|
|
|||
| void reader_set_silent_status(bool f) { data->silent = f; } | |||
There was a problem hiding this comment.
Why f for the parameter name? Why not something like val?
There was a problem hiding this comment.
It was meant to be the abbreviation of flag, but if more meaning is needed i can write the whole word
src/reader.cpp
Outdated
| @@ -261,6 +261,7 @@ class reader_data_t { | |||
| : allow_autosuggestion(0), | |||
| suppress_autosuggestion(0), | |||
| expand_abbreviations(0), | |||
| silent(0), | |||
There was a problem hiding this comment.
s/0/false/ please and you might as well fix the other bool initializers while you're touching this block of code 😄 Functionally it doesn't matter but it does make the intent clearer to a human reader.
|
This works but has an unpleasant behavior. With the first character I type the cursor jumps to the start of the line. It needs to either remain at the end of the prompt or, ideally, a |
|
@Markcial: Do you see the problem with the location of the cursor I noted in my previous comment? Your change to the code looks fine to me so there appears to be an unexpected tight coupling between the functions you modified that results in the cursor not being at the expected position. |
|
Yeah, it's true, i need to dig a little more, maybe i find a way to get a more consistent behaviour, I will try to post an update for the weekend, thanks for your time on the review |
…char for the read command
|
I ended up using Do I parametrize the obfuscation char or use another char as the asterisk or the shebang? What do you think i should do? |
|
Trying to grasp the reason of the segmentation faults, will try to fix it if able. |
|
I haven't reviewed your latest changes but it now seems to work fine. I like the solid circle if the environment supports unicode code points but we should fallback to |
Description
This PR updates the reader object adding a
silentparameter.If this parameter is set to true, it doesn't displays the commandline contents when the readline command is called. Allowing silent prompts for password or discrete command entries, the command can be modified to use a offuscation char like '*', but at the moment instead of masking the contents it doesn't displays them.
Fixes issue #838
TODOs: