Support global prefix + global indentation #82
Conversation
hampuslidin
changed the title
|
Implemented global indentation as well as a field to the |
| if self.display_mode == PasswordDisplayMode::Hidden { | ||
| self.input.clear(); | ||
| } |
| @@ -127,6 +127,18 @@ pub struct RenderConfig { | |||
| /// a separator from the prefix. | |||
| pub option: StyleSheet, | |||
|
|
|||
| /// Global prefix used for all prompt lines. | |||
| /// | |||
| /// Note: all lines produced by any non-fullscreen prompt will be prefixed | |||
There was a problem hiding this comment.
Maybe the PR will still give me the context, but what would "non-fullscreen" prompt mean here?
I think the doc should be more explicit so any new user can immediately understand what's this option used for, and how it related to global_indentation.
For example, from reading both docs it's still a bit ambiguous to me:
- Should I use only global_prefix if I just want to add like a ">" char before everything?
- Should I use the prefix and the indentation together?
Indentation is pretty obvious by itself I guess, it's just not clear whether prefix needs the indentation too
There was a problem hiding this comment.
Good point, the docs here could definitely be improved. I can’t remember what I meant by ”non-fullscreen prompt”, so I guess that needs to be reworked as well :D
| fn clear_until_new_line(&mut self) -> Result<()> { | ||
| // `console` has no API for clearing until new lines, so we print the ANSI code explicitly | ||
| // here. | ||
| self.term.write_str("\r\x1b[0K") |
| if left >= len { | ||
| cur_pos.col = cur_pos.col.saturating_add(len); | ||
| } else { | ||
| cur_pos.row = cur_pos.row.saturating_add(1); | ||
| cur_pos.col = len; | ||
| cur_pos.col = self.render_config.global_indentation + len; | ||
| } |
There was a problem hiding this comment.
There's a slight risk of edge cases where the terminal width is too small to accomodate (indentation + char length), if the indentation turns out to be too long and the terminal not too much.
This could break all functionality instead of just making things look ugly. But it does seem like a rare case and in these scenarios I think terminal users just assume things break if you have tiny terminals.
Do you think we should worry about this edge case or is it too much? Fine either way, just wanted to bring it up.
| } | ||
|
|
||
| /// Sets the global indentation for all prompt lines. | ||
| pub fn with_global_indentation(mut self, global_indentation: u16) -> Self { |
There was a problem hiding this comment.
(arbitrary comment unrelated to this line)
Can you add these changes to the changelog?
|
Also, sorry the month-long wait. |
| $self | ||
| .terminal | ||
| .cursor_move_to_column($self.render_config.global_indentation)?; |
There was a problem hiding this comment.
Found a tiny bug* where we would leave the cursor at the end of the prompt in column 0, not respecting the global_indentation.
* It isn't really a bug, but a decision we'll have to make, take a look at the example modifications I made at this commit 43da960
Which one of the following behaviors do we want a "println!" after the prompt to have?
╔══════════════════════════════════════════════════════╗
║ > Description: mikael
║ Your transaction has been successfully recorded.
or
╔══════════════════════════════════════════════════════╗
║ > Description: mikael
Your transaction has been successfully recorded.
In other words, should we leave the cursor at column 0 or an indented value after the prompt exits?
Your PR is currently leaving it at position 0, I did a little refactoring on 43da960 that left it at the indented column, what do you think?
| let __converted = newline_converter::unix2dos(&__content); | ||
|
|
||
| for $line_ident in __converted.split_inclusive("\r\n") { | ||
| if $self.is_new_line { |
There was a problem hiding this comment.
I was coming up with a way to remove the additional flag and managed to reduce a little bit of the code at 43da960, wdyt?
|
Sorry for not giving a response for a very long time, I have not had the time to look into this any further. Given the age of the PR, and the fact that I don't actually need this feature very much anymore, maybe we should close this and give this another go if someone else feels like they need the feature? |
|
There was a bug fix for this I saw, so maybe that work can be salvaged into a separate PR though. |
|
Sorry on my end as well that it didn't work out, I'm closing for the sole purpose of clean-up (and the age of the PR), let me know if you'd still like to see this implemented upstream |
This PR implements a new field in the render config called a "global prefix", which will be interpreted by the prompt backend as a prefix that is rendered before each line written out to the terminal.
This will be a breaking change, since the
RenderConfigtype was altered to allow the user to infer a lifetime other than the'staticlifetime. A non-static lifetime was only added for the global prefix field, but for consistencies sake, the PR can be updated to support the other fields just as easy if needed. Alternatively, the non-staticRenderConfigcan also be broken out into a separate PR, if it is desired to be handled separately.Additionally, a bug fix for hidden password prompts is also included in the PR (see commit history). Again, this can be handled in a separate PR if desired.
Feel free to come back with suggestions if the feature and/or implementation is appropriate!
Closes #81