Skip to content

WIP new option list #5510

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

Merged
merged 21 commits into from
Feb 12, 2025
Merged

WIP new option list #5510

merged 21 commits into from
Feb 12, 2025

Conversation

willmcgugan
Copy link
Collaborator

@willmcgugan willmcgugan commented Feb 8, 2025

A rewrite of OptionList internals, mostly preserving the original interface. The original was brittle, small changes would break everything. There was also some misguided attempts at doing things lazily, which resulted in doing work multiple times, particularly when first adding the widget. The original would also not be able to report its size accurately, until two or three frames later. An earlier pass partially fixed that, but it still took a frame before the OL could report its size.

@willmcgugan willmcgugan marked this pull request as draft February 8, 2025 17:52
@TomJGooding
Copy link
Contributor

TomJGooding commented Feb 11, 2025

I hope you don't mind me sticking my nose in (as usual!), especially since this is still a WiP.

Is it necessary to remove the Separator for this improved version of the OptionList, or is this a design choice? Specifying a separator with None is obviously less intuitive, and it would be a shame to introduce a breaking change if it can be avoided.

@willmcgugan
Copy link
Collaborator Author

I would disagree it is less intuitive. The OptionList intuitively takes a list of options. But a "Separator" is not an option, nor anything that is present in the option list. It is just a way of marking the end of a logical group. I think None does a better job of dividing the options into such groups.

Internally, it does simplify things. There is no need to distinguish between a real option and this special Separator case.

@willmcgugan willmcgugan marked this pull request as ready for review February 12, 2025 11:03
@willmcgugan willmcgugan merged commit d10791d into main Feb 12, 2025
23 checks passed
@willmcgugan willmcgugan deleted the new-option-list branch February 12, 2025 14:13
Comment on lines +3496 to +3497
def test_option_list_wrapping(snap_compare):
"""You should see a 40 cell wide Option list with a single line, ending in an ellipsis."""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I didn't spot this before it was merged, from the snapshot it looks like the OptionList height is based on the full height of the prompt rather than the truncated version with nowrap. Is that expected?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thanks.

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