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

Refactors AppState initialisation during app startup #199

Merged
merged 1 commit into from
Dec 13, 2023

Conversation

gosuwachu
Copy link
Contributor

I have found previous version with the Option<> a bit weird, but it is probably a matter of taste. In the new version AppState is created at the start, but unfortunately we have to deal with incoming events which complicate things.

However, I am not sure if we need to process the events during the traversal. The app is not that usable in that state anyway - events are all processed in batches which causes the cursor jump around a bit, and the experience is not smooth at all.

Also, as an example ncdu doesn't allow any interaction during scanning. Thoughts?

@Byron Byron merged commit 238bc5f into Byron:main Dec 13, 2023
2 checks passed
@Byron
Copy link
Owner

Byron commented Dec 13, 2023

Thanks a lot, this looks so much better (and reduces the amount of code).

However, I am not sure if we need to process the events during the traversal. The app is not that usable in that state anyway - events are all processed in batches which causes the cursor jump around a bit, and the experience is not smooth at all.

It's an important feature, which also allows the user to quit out of the application early.
Even though it's not the most responsive right now, it's predictable, and I use early browsing all the time, even if only to keep me busy while waiting.
So this is definitely a keeper.

I may add that during my experiments with moonwalk I noticed that it's much more responsive with it. It will just take some time until the author completes it, but it will land eventually for performance and responsiveness.

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.

None yet

3 participants