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

Placeholder cycle per app #2590

Closed
willmcgugan opened this issue May 17, 2023 · 8 comments · Fixed by #2607
Closed

Placeholder cycle per app #2590

willmcgugan opened this issue May 17, 2023 · 8 comments · Fixed by #2607
Assignees
Labels
enhancement New feature or request

Comments

@willmcgugan
Copy link
Collaborator

The placeholder stores the current color in a classvar.

This means that a second app will start where the previous app left off. And may return a different color from the first iteration.

Not much of an issue in practice, but in the docs the colors change every screenshot. I think we need a separate cycle per app. Suggest we have an independent sequence for each app instance.

@rodrigogiraoserrao rodrigogiraoserrao self-assigned this May 18, 2023
@rodrigogiraoserrao rodrigogiraoserrao added the enhancement New feature or request label May 18, 2023
@rodrigogiraoserrao
Copy link
Contributor

We already have reset_color_cycle as an attempt to fix that, but I don't like it.
Can I get rid of reset_color_cycle while I'm at it?

@willmcgugan
Copy link
Collaborator Author

Sure

@willmcgugan
Copy link
Collaborator Author

You might be able to do this with a WeakKeySet where the keys are apps. Just a thought.

@rodrigogiraoserrao
Copy link
Contributor

You might be able to do this with a WeakKeySet where the keys are apps. Just a thought.

Thanks. I was doing it with a standard dictionary with app IDs as keys.

@willmcgugan
Copy link
Collaborator Author

That would work. Downside is that the app IDs wouldn't be removed when the app is deleted.

@rodrigogiraoserrao
Copy link
Contributor

But that's also a marginal issue, no?
How many apps will you create and destroy after loading the Placeholder..?

@willmcgugan
Copy link
Collaborator Author

It's unlikely to be an issue, sure. But its not much more work to do the weak dict. May as well?

@github-actions
Copy link

Don't forget to star the repository!

Follow @textualizeio for Textual updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants