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

Refactoring: Remove state management API #445

Merged
merged 2 commits into from
Apr 8, 2019

Conversation

bryphe
Copy link
Member

@bryphe bryphe commented Apr 8, 2019

Revery included a very simple Redux-inspired state-management solution. However, there's no reason to force users into a certain state-management paradigm (we should let the user bring their own state management solution.

For Onivim 2, I'm experimenting with isolinear, which is an Elm-inspired approach that also handles effects.

@bryphe bryphe merged commit bf9deff into master Apr 8, 2019
@bryphe bryphe deleted the refactoring/remove-state-management branch April 8, 2019 21:22
@msvbg
Copy link
Contributor

msvbg commented Apr 13, 2019

This commit breaks my code (per git bisect). The window is completely black until I move the window, at which point rendering resumes.

@bryphe
Copy link
Member Author

bryphe commented Apr 13, 2019

Ah, sorry about that @msvbg !

I suspect it is due to the removal of the needsRender tracking variable - it's set to true for the first render, so it guarantees there will always be an initial render. I tested against this change against the example app and Onivim 2, but I suspect it worked in the former due to an animation (marking the window as dirty) and in the latter due to the separate state management we do that would trigger a render. I was able to reproduce with the example by changing the default example to a non-animatable one and removing the Window.setPos call (any change in position / size will mark the window as dirty).

We can add the flag back to ensure there is a first render. I'll put up a shortly. Thanks for reporting!

@bryphe
Copy link
Member Author

bryphe commented Apr 13, 2019

This would be a perfect case to have a tool like #156 to avoid regressions like this.

@msvbg
Copy link
Contributor

msvbg commented Apr 13, 2019

Indeed, considering how visual Revery is it would make a lot of sense to have that.

bryphe added a commit that referenced this pull request Apr 13, 2019
* Add back 'needsRender' in the more limited 'isFirstRender' form

* Formatting

* Switch from ref -> mutable to match other fields
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