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.

Sign up for GitHub

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

Jump to bottom

Handle selection items overflowing several times #190

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

Handle selection items overflowing several times #190

wants to merge 4 commits into from

Conversation

grunweg
Copy link
Contributor

@grunweg grunweg commented May 6, 2022

PR #14 was good, but not general enough: there were in fact two different bugs hiding. This PR

  • documents the logic of the dix
  • fixes an off-by-two error in the logic (which is easy to overlook; I only found that during manual testing),
  • fixes clear_preserve_prompt to handle items overflowing several times
  • cleans up the code slightly using iterators (happy to drop this commit if you prefer)
    See individual commits for details.

grunweg added 4 commits May 5, 2022 20:31
@grunweg
fix/refactor: rewrite size_vec collection code using iterators.
6cad8ae 
Handle multi-line items in sort and fuzzy_select.
Select and multi-select already handle these.
@grunweg
fix: handle very long items in clear_preserve_prompt correctly.
13f5631 
The previous code would (correctly) handle the case of an item
spanning between one and two lines, but failed to consider the case
of the output spanning more than two. Extend the logic to cover that.
@grunweg
doc: document and comment clear_preserve_prompt().
4039d45 
Record the logic of the commit which introduced it.
The current logic is not quite correct; we'll fix that in the next commits.
@grunweg
fix: fix off-by-two errors when determining item width in selection p…
26dd942 
…rompts.

Formatting each item adds two additional characters,
which the code forgot to take into account.
@grunweg
Copy link
Contributor Author

grunweg commented May 6, 2022

There's a deeper issue underlying this: for any output line which overflows the terminal (i.e. is longer than the terminal width), we only increase the height by one --- even if the line can span several lines on the terminal. When clearing the terminal, we only take the height into account, i.e. will not clear enough output.
This bug also exists is TermThemeRender::clear.

A deeper fix would be to record the length of each line of output, and take this into account when clearing the terminal. (I have prototyped this; it seems workable, if slightly fiddly.)

Counter-arguments I can think of

  1. Not every terminal has a size (see console::TermTarget::ReadWritePair).
    Solution: when no size is available, just use the default size. That's no worse than the current state.

  2. This doesn't handle terminal resizing: if we just check the size at clearing time, that can result in both over- or underclearing, depending on whether the terminal was resized in the meantime.
    Solution: don't record the number of additional lines, but the lengths of inputs. Compute the height to clear when clearing. Is not hard; we just store some additional data for each line we wrote.

  3. Do we need this? This issue is particularly pronounced for selection prompts (as the selection items need to be drawn over and over, potentially). It's just not as bad for the other prompts.

@psunkara (or other maintainers). Would you prefer to see a "more right" fix instead?

@grunweg grunweg mentioned this pull request May 6, 2022
Open
@Gordon01
Copy link
Contributor

Gordon01 commented Sep 4, 2023

In #104 @pksunkara mentioned textwrap. The number of lines it returns can be used to calculate the number of rows to clear.
I've experimented with it and it works fine.

I believe, with textwrap this PR is not needed. Am I correct?

@grunweg
Copy link
Contributor Author

grunweg commented Sep 4, 2023

Looks like it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@grunweg @Gordon01