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 app initialisation #214

Merged
merged 20 commits into from
Jan 9, 2024
Merged

Refactors app initialisation #214

merged 20 commits into from
Jan 9, 2024

Conversation

gosuwachu
Copy link
Contributor

@gosuwachu gosuwachu commented Jan 8, 2024

This PR refactors app initialisation so that:

  1. the app starts with an empty Traversal.
  2. the app then starts the traversal by creating a RunningTraversal.
  3. Traversal gets updated by processing events from the RunningTraversal inside the main event loop.

This should enable in future to implement refresh logic by creating another RunningTraversal and updating parts of the tree (TBD how to exactly do that).

Other than the above AppState and TerminalApp have been moved to separate files.

@gosuwachu gosuwachu marked this pull request as ready for review January 8, 2024 23:51
Copy link
Owner

@Byron Byron left a comment

Choose a reason for hiding this comment

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

Thanks a lot for making this happen - I think it's a great base for implementing refresh later.

@Byron Byron force-pushed the init branch 3 times, most recently from 1f1dc6f to 97a0340 Compare January 9, 2024 12:52
- avoid `app_` prefix in `app` module.
- cleanup root-index logic a little
@Byron
Copy link
Owner

Byron commented Jan 9, 2024

I really like that the application will be able to trigger the refresh itself by creating a new BackgroundTraversal with enough information to know how to integrate_traversal_events in order to insert entries under a new root. Refresh should be no problem with that and fall right into place.

It also seems that TerminalApp doesn't fulfil a purpose anymore and that its fields can be merged into AppState without losing anything. This might then be helpful when initiating a new walk (as it requires WalkOptions). But I leave that to you :).

@Byron Byron merged commit 1812227 into Byron:main Jan 9, 2024
2 checks passed
@gosuwachu
Copy link
Contributor Author

gosuwachu commented Jan 9, 2024

It also seems that TerminalApp doesn't fulfil a purpose anymore and that its fields can be merged into AppState without losing anything. This might then be helpful when initiating a new walk (as it requires WalkOptions). But I leave that to you :).

I have thought the same thing, and was the first thing that I have tried to do when working on this change. However I have ran into ownership / borrow checker issues. I may give it another go.

@gosuwachu gosuwachu deleted the init branch January 9, 2024 14:36
@gosuwachu
Copy link
Contributor Author

Ups... all my messy commit "wip" history ended up being merged into main - I don't mind if you squash when landing the PR, but I will also try to remember to squash all the commits before sending stuff for review.

@Byron
Copy link
Owner

Byron commented Jan 10, 2024

No worries about it - I was aware and chose to prefer the detailed step-by-step history over a single commit that hides the steps. Even though I appreciate more structure, it's by no means a requirement and … I have been doing the same for a long time.

StackedGit changed that to the better, but it comes with a little overhead that to me at least is worth it.

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.

3 participants