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

RichLog fixes #4978

Merged
merged 18 commits into from
Sep 11, 2024
Merged

RichLog fixes #4978

merged 18 commits into from
Sep 11, 2024

Conversation

darrenburns
Copy link
Member

@darrenburns darrenburns commented Sep 9, 2024

Please review the following checklist.

  • Docstrings on all new or modified functions / classes
  • Updated documentation
  • Updated CHANGELOG.md (where appropriate)

@darrenburns darrenburns linked an issue Sep 10, 2024 that may be closed by this pull request
Copy link
Member Author

Choose a reason for hiding this comment

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

By deferring the initial render until the width is available, we can now render the content that was added via write in compose at the correct width, rather than doing the often wrong default of min_width.

@darrenburns darrenburns changed the title Rich log width fix RichLog fixes Sep 10, 2024
@willmcgugan
Copy link
Collaborator

willmcgugan commented Sep 10, 2024

Apologies if you have considered this. Just catching up.

There is a danger here that if you defer writing that the renderable could change between the time you call write and the time it is rendered.

That shouldn't necessarily be a deal breaker. It is probably more surprising if a renderable doesn't fit.

Perhaps deferred writes should attempt to copy the renderable at the point of calling write?

@darrenburns
Copy link
Member Author

I'd prefer to document that writes may be deferred until the widget is fully mounted rather than attempting copying, since renderables could be any object at all. They may not support copying or it may be expensive.

Copy link
Member Author

Choose a reason for hiding this comment

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

This shows the fix for width not being respected in write in action:

Copy link
Member Author

Choose a reason for hiding this comment

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

Previously, due to #4980, this would incorrectly shrink down to RichLog.min_width rather than the actual available content width.

@darrenburns darrenburns marked this pull request as ready for review September 10, 2024 14:25
Copy link
Collaborator

@willmcgugan willmcgugan left a comment

Choose a reason for hiding this comment

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

LGTM.

Suggest we copy Text renderables, since they have a copy method.

@willmcgugan willmcgugan merged commit 5867dcf into main Sep 11, 2024
20 checks passed
@willmcgugan willmcgugan deleted the rich-log-width-fix branch September 11, 2024 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants