Introduce MVVM #147
Draft
Introduce MVVM #147
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ivinjabraham
force-pushed
the
refactor/introduce-mvvm
branch
3 times, most recently
from
d24496f to
4b10584
Compare
The `App` struct serves as the model in the MVC pattern. The proposed MVVM refactor will continue using it as the primary Model. Rename `App` to `Model` to better reflect it's purpose. Signed-off-by: Ivin Joel Abraham <[email protected]>
The `View` is a crucial part of MVVM that will handle the UI. Rename the struct from `CurrentScreen`, which conveys logic, to simply `View` that will reflect it's purpose in the architecture. Signed-off-by: Ivin Joel Abraham <[email protected]>
The MVVM refactor will require a trait for viewmodels to establish a reliable interface. Introduce a basic implementation of the trait with no usage. Signed-off-by: Ivin Joel Abraham <[email protected]>
ivinjabraham
force-pushed
the
refactor/introduce-mvvm
branch
from
4b10584 to
47c0e91
Compare
To use as a key in a hashmap, a type must implement `Eq` and `Hash`. Additionally, `Copy` is required as well to be used in the .entry() method of Hashmap. Derive `Eq`, `Copy` and `Hash` on View to use it as a key in Hashmaps. Signed-off-by: Ivin Joel Abraham <[email protected]> Signed-off-by: Ivin Joel Abraham <[email protected]>
ivinjabraham
force-pushed
the
refactor/introduce-mvvm
branch
from
47c0e91 to
3c044a9
Compare
The View component in MVVM handles UI layout and construction. Though it is currently in the model directory, the `View` enum will be our View component. Future commits will move this to it's own directory. Add a stub implementation of a method that can draw the current screen. Signed-off-by: Ivin Joel Abraham <[email protected]>
ivinjabraham
force-pushed
the
refactor/introduce-mvvm
branch
from
9d5fda0 to
68dac46
Compare
The previous implementation, `key_handling`, used to match `KeyEvent` and not `Event`. Correct the mistake in the new version. Additionally, remove the comments that were getting outdated quickly due to rapid development. Signed-off-by: Ivin Joel Abraham <[email protected]>
The multiple components in the proposed MVVM architecture require a central manager to orchestrate event handling, controlling and orchestration. Introduce an `App` to be the central manager with draft implementations for methods. Signed-off-by: Ivin Joel Abraham <[email protected]>
ivinjabraham
force-pushed
the
refactor/introduce-mvvm
branch
from
446f445 to
0fd1aaa
Compare
The `ui` directory contains the functions that build the screen which belongs to the View component in the MVVM architecture. Replace the old name with `views`. Signed-off-by: Ivin Joel Abraham <[email protected]>
Drawing a screen would require information about the screen's state, which shouldn't be available in the View itself. Pass state to the `draw_screen` function. Signed-off-by: Ivin Joel Abraham <[email protected]>
Signed-off-by: Ivin Joel Abraham <[email protected]>
ivinjabraham
force-pushed
the
refactor/introduce-mvvm
branch
from
f44b580 to
3a76e8f
Compare
The `View` variant `MailingListSelection` is slightly unconventional as other variants that include a similar selection list such as `LatestPatchsets` does not emphasize on the "Selection" aspect of the View. Rename the variant to better match with the others. Signed-off-by: Ivin Joel Abraham <[email protected]>
`draw_screen` should only be reading the state when rendering and any changes should be done through the viewmodel i.e ownership should belong to the viewmodel. Signed-off-by: Ivin Joel Abraham <[email protected]>
The ratatui Terminal instance is ultimately required to draw on the screen. Hence, pass it to the View when required. Signed-off-by: Ivin Joel Abraham <[email protected]>
Port the previous MVC architecture's View of MailingList to the new MVVM architecture. Keep previous implementations to ensure `patch-hub` does not break. This implementation is not currently used, it is being introduced ahead of main loop integration to avoid breaking compilation during the transition. Signed-off-by: Ivin Joel Abraham <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR address #139. I cannot foresee all the necessary steps at the moment but I will change this body when I can see the plan taking a more concrete form. Ideally, I will be able to integrate it without causing regressions i.e have the new architecture work alongside the old. But it is unclear how to go about that at the moment.
My local version of
clippy(v0.1.88) seems to have added a new check and is now throwing errors for string interpolation syntax across the codebase. I haven't fixed them in this PR since it seems logically out of place. Though I think someone should raise an issue if they can confirm that it's not just on my machine.