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

Extend and tweak documention in select.rs and theme.rs. #177

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

grunweg
Copy link
Contributor

@grunweg grunweg commented Feb 17, 2022

Trying to understand the selection prompt and theme rendering a bit better, I touched up the docs a bit to make them easier to follow for me. Hence, this PR is a mixture of various small changes --- feel free to accept or decline as you wish.

I'm not sure about all of the members of TermThemeRenderer, especially prompt_preserve_height. Related to that, I don't understand the distinction of clear in contrast to clear_preserve_prompt yet. @pksunkara Can you help me with that? Even a pointer to where prompt_preserve_height is actually used would help.

@pksunkara
Copy link
Collaborator

I am actually unsure. I was confused by this earlier this week too. You have my blessing to refactor it to your heart's content. I was planning to do that before 0.11.0 anyway (after around 2 months).

@grunweg
Copy link
Contributor Author

grunweg commented Feb 22, 2022

Thanks for the trust ❤️
That certainly sounds like an interesting small project; I'll see if/when I get to it. (This is a spare time project for me.)

@grunweg
Copy link
Contributor Author

grunweg commented May 5, 2022

I found some time to look into this today and just pushed a couple more commits with further doc improvements. Let me know if I you prefer me to squash some commits (such as all those just changing indicative to imperative tense).

@grunweg
Copy link
Contributor Author

grunweg commented May 5, 2022

I think I figured out what prompt_reset_height does.

If prompts_reset_height is true,

  • prompt_height denotes the height (number of lines) of all input until the last prompt string. (If no prompt was displayed so far, prompt_height is zero.)
  • height is the number of lines after the current prompt: right after a prompt, that's zero.

If prompts_reset_height is false, prompt_height is always zero and height counts all lines of input (both prompts and other ouput).

The password prompt uses prompts_reset_height=false; all other prompts use prompts_reset_height=true.

@psunkara Does that sound plausible to you?
I'm not sure yet how to clarify this in the best way: if doc comments will do or some other refactoring is useful. Will ponder that...

grunweg added 8 commits May 5, 2022 17:37
I'm not sure about the grammar or format of the comments, but open to changing it.
The comment of default() was misleading: if a default value is set,
the user can still change the selected answer. They merely have the
*additional* option of confirming the default choice.
@grunweg
Copy link
Contributor Author

grunweg commented May 5, 2022

Looking into the password prompt closely, I don't see how prompts_reset_height=false makes a difference for its execution.
In interact_on, terminal output is produced the following ways:

  • prompt_password: produces "normal output" (counting as height, regardless of prompts_reset_height)
  • error: also produces normal output
  • password_prompt_selection: this produces "prompt output"; this is the only place where prompts_reset_height comes in.

However, the way the code is structured,
- password_prompt_selection is always called last (right before interact_on returns),
- before called password_prompt_selection, the terminal is cleared
- hence, height (and prompt_height) are always 0 when password_prompt_selection is called.
Actually, that's not relevant: password_prompt_selection writes a line of output, so the difference is whether self.height=1 and self.prompt_height=0 (with false) or the other way round. But that doesn't matter: see next comment.

grunweg added 2 commits May 6, 2022 00:51
Setting it to false doesn't make a difference in practice.

The distinction between self.height and self.prompt_height only matters
when calling TermThemeRenderer::clear_preserve_prompt(): that method preserves
prompts and only clears input below the most recent prompt.
However, the password prompt only calls TermThemeRenderer::clear(),
which treats height and prompt_height in the same way.

The function set_prompts_reset_height is now unused, hence is removed.
@grunweg
Copy link
Contributor Author

grunweg commented May 6, 2022

Setting prompts_reset_heights=true in the password prompt doesn't make a difference in practice.

The distinction between self.height and self.prompt_height only matters when calling TermThemeRenderer::clear_preserve_prompt(): that method preserves prompts and only clears input below the most recent prompt. However, the password prompt only calls TermThemeRenderer::clear(), which treats height and prompt_height in the same way. (And I don't see why that would need to change.)

@grunweg
Copy link
Contributor Author

grunweg commented May 6, 2022

@mitsuhiko You introduced prompts_reset_height in 489a065. Do you remember why you specifically set this to false for password prompts? I don't see a reason right now, but maybe I'm just overlooking something.

In case that reason doesn't exist anymore, I just pushed two commits removing prompts_reset_height (always setting it to true). I also pushed a commit documenting the meaning of height and prompt_height: with prompts_reset_height removed, that is crystal clear.

I purposefully did not include docstrings for clear() and clear_preserve_prompt to avoid conflicts with in-flight PRs #190 and #191. These fix some pre-existing bugs in those methods and document the correct behaviour.

@grunweg
Copy link
Contributor Author

grunweg commented May 6, 2022

Sorry for all the PRs right now. I'm done for the moment - take as much time pondering this as you need or prefer!

@mitsuhiko
Copy link
Collaborator

I do not remember sadly.

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.

3 participants