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

Fix issues with watching CSS reloading #2128

Merged
merged 9 commits into from
Mar 28, 2023
Merged

Fix issues with watching CSS reloading #2128

merged 9 commits into from
Mar 28, 2023

Conversation

rodrigogiraoserrao
Copy link
Contributor

@rodrigogiraoserrao rodrigogiraoserrao commented Mar 23, 2023

This will close #2063.

The only reason I am a bit confident in this fix is because all the tests pass.
I am not 100% sure why this works, but I have a conjecture: the call to reset_styles was flagging the need to repaint a widget too soon and it was either preventing the latter refresh from happening or it was keeping some outdated information around.

I got to this conclusion because setting the border property programmatically works, and the code that runs in that case is just the __set__ in the descriptor and the layout refresh, as opposed to what happens when the CSS is reloaded, which runs many more lines of code, so I figured that the CSS reloading was “doing too much work”.

Note: Playing with the code and running the tests suggests we can actually get rid of the method reset_styles altogether.

@rodrigogiraoserrao rodrigogiraoserrao marked this pull request as ready for review March 23, 2023 17:42
@willmcgugan
Copy link
Collaborator

That may be right. I'm assuming you've pushed the hot reloading quite hard and can't break it.

If that method isn't called anywhere, then I'd go ahead and remove it.

@rodrigogiraoserrao
Copy link
Contributor Author

I'm assuming you've pushed the hot reloading quite hard and can't break it.

I'm going to go ahead and add tests for the hot reloading.

@willmcgugan
Copy link
Collaborator

@rodrigogiraoserrao Still working on those tests?

@rodrigogiraoserrao
Copy link
Contributor Author

@rodrigogiraoserrao Still working on those tests?

Yeah, there wasn't a good way to test the hot reloading and take a snapshot test.
Wrapping up now.

@rodrigogiraoserrao
Copy link
Contributor Author

@willmcgugan now I'm happy.

@rodrigogiraoserrao rodrigogiraoserrao merged commit 0189918 into main Mar 28, 2023
@rodrigogiraoserrao rodrigogiraoserrao deleted the fix-2063 branch March 28, 2023 15:01
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.

Watching CSS for border changes renders misplaced content
2 participants