You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
kd: do you have more simplifications in mind for crates-tui?
joshka: I think app is still doing too many things. Try reading it top to bottom and writing them out.
Split mode into keybinding focused selector (not sure the right name for this) and tab selection.
Rationale: The App has a 9 modes, but the app really just has 3 tabs of note (summary, search, and almost there crate info), these need to have a more prominent role in how they control the app. Showing help and popup are things that can happen on any mode and which overlay the mode. Common is there not because it's an actual mode, but to add a global keybinding profile
Push the methods on Mode and the methods that call them down into the search page.
Rationale: impl Mode: contains stuff which is used in app to select which part of the search page to render. The search page should handle how to render itself, and these checks should be moved there instead
Remove the excess help and popup modes. Make the rendering of these a stack where the main mode is rendered and the help or popup are rendered over the top without having to track these as a mode
Rationale: Last mode is only a thing because there's help and popup modes rather than there being some sort more useful stack of what's rendered (i.e. render the help popup over the summary / search mode )
Store which page is currently focused in a way that makes it easier to push the keys to the right place in the one method
Rationale: Handle key event should not know about Search or filter - it shoud just forward the event to the search page and let it handle it if the search page is the one currently shown.
Make the code to handle events / commands to actions a method call on some keybindings type (may need to refactor the config types to make that work?)
Rationale: in handle_key_events_from_config: the code that gets the command from the key binding is overly complex and belongs in some keybindings type instead of the app and elsewhere
Split the actions into actions for each page and global actions (perhaps Action::SearchAction(SearchAction) or similar)
Rationale: in handle_action: There's still a mismash of things which are search responsibilities and app responsibilities stemming from the action type being a global enum that has everything that can happen in the app. This means to understans the search page, you have to understand how the application as a whole interacts with it. The enum should only have actions related to the app (quit, tick, resize, show/hide help/popup, switch tabs, open urls (maybe), copy clipboard (maybe))
Simplify the event -> command -> action approach
Rationale: The event -> command -> action conversion is overly complex. My guess is that Command might be better off as a set of constants in a module rather than an enum? Not enitrely sure about how to fix this one. Perhaps there's just one too many concepts, or they fit badly for the usage?
create a type / module for handling keyboard chords properly
Rationale: Key refresh tick handling should be done outside of the app with a more clearly defined and testable mechanism for holding key events that turn into chords (or can be bypassed when the user is in a place where they expect to just be able to type normally)
Simplify Quit
Rationale: Looking at how it ends up interacting with the entire app, I think quit makes more sense as exit: bool than as a mode. It does't have any behavior / keys that make sense, so it's really just a simple flag
Move scroll related methods into the various widgets and handle pushing the events or actions to the widgets instead of handling calling individual methods
Rationale: the scroll methods are there because the events / actions are not handled by the visible / focused widget properly. Push the logic down
Simplify the switch mode method
Rationale: Switch mode has some recent modifications to support pushing the search mode down into the pages, but that was step 1, step 2 is making it so the search page is responsible for handling search / filter mode and the main app doesn't care about it
Fix how crate info showing is handled - move the loading parts of that into a crate info module instead of search module
Rationale: update_current_selection_crate_info / show_full_crate_details should be pulled out and converted to something that allows a tab for the crate info to be shown. This would allow pressing enter on the summary screen and pressing enter on the search results to do the same thing. If you don't want this as a tab, at least make it a high level concept which the app pushes to the selected tab to render (but a tab is better)
Move crate counting down to search
Rationale: store_total_number_of_crates belongs in search, not the app
Make the open url / clipboard copy a parameterized action
Rationale: Open url / clipyboard should accept a url and be something any page can trigger (push don't pull)
Move the part of cursor setting that calls search out, and replace with something more generic
Rationale: update_cursor knows too much about search - it should perhaps just be shared mutable state for whichever view / page is visible to set (or a call to send a SetCursor action, whichever makes sense)
move the events widget into its own type / module
Rationale: This is mostly self contained behavior / related to the key chord handling mentioned above (perhaps they belong together?)
Currently App is ~600 LoC in a 650 LoC file. I'm guessing a once all the things that are separate responsibilities are extracted it would be closer to half that.
With a more general perspective on this once these tasks are done, look at the two most unrelated methods in each type and try to explain the relationship between them. The language that you use to describe how this works is a good source of missing concepts that might help model the application in a way that aligns the code with how we talk about the code.
The text was updated successfully, but these errors were encountered:
kd: do you have more simplifications in mind for crates-tui?
joshka: I think app is still doing too many things. Try reading it top to bottom and writing them out.
Split mode into keybinding focused selector (not sure the right name for this) and tab selection.
Rationale: The App has a 9 modes, but the app really just has 3 tabs of note (summary, search, and almost there crate info), these need to have a more prominent role in how they control the app. Showing help and popup are things that can happen on any mode and which overlay the mode. Common is there not because it's an actual mode, but to add a global keybinding profile
Push the methods on
Mode
and the methods that call them down into the search page.Rationale:
impl Mode
: contains stuff which is used in app to select which part of the search page to render. The search page should handle how to render itself, and these checks should be moved there insteadRemove the excess help and popup modes. Make the rendering of these a stack where the main mode is rendered and the help or popup are rendered over the top without having to track these as a mode
Rationale: Last mode is only a thing because there's help and popup modes rather than there being some sort more useful stack of what's rendered (i.e. render the help popup over the summary / search mode )
Store which page is currently focused in a way that makes it easier to push the keys to the right place in the one method
Rationale: Handle key event should not know about Search or filter - it shoud just forward the event to the search page and let it handle it if the search page is the one currently shown.
Make the code to handle events / commands to actions a method call on some keybindings type (may need to refactor the config types to make that work?)
Rationale: in handle_key_events_from_config: the code that gets the command from the key binding is overly complex and belongs in some keybindings type instead of the app and elsewhere
Split the actions into actions for each page and global actions (perhaps
Action::SearchAction(SearchAction)
or similar)Rationale: in handle_action: There's still a mismash of things which are search responsibilities and app responsibilities stemming from the action type being a global enum that has everything that can happen in the app. This means to understans the search page, you have to understand how the application as a whole interacts with it. The enum should only have actions related to the app (quit, tick, resize, show/hide help/popup, switch tabs, open urls (maybe), copy clipboard (maybe))
Simplify the event -> command -> action approach
Rationale: The event -> command -> action conversion is overly complex. My guess is that Command might be better off as a set of constants in a module rather than an enum? Not enitrely sure about how to fix this one. Perhaps there's just one too many concepts, or they fit badly for the usage?
create a type / module for handling keyboard chords properly
Rationale: Key refresh tick handling should be done outside of the app with a more clearly defined and testable mechanism for holding key events that turn into chords (or can be bypassed when the user is in a place where they expect to just be able to type normally)
Simplify Quit
Rationale: Looking at how it ends up interacting with the entire app, I think quit makes more sense as
exit: bool
than as a mode. It does't have any behavior / keys that make sense, so it's really just a simple flagMove
scroll
related methods into the various widgets and handle pushing the events or actions to the widgets instead of handling calling individual methodsRationale: the scroll methods are there because the events / actions are not handled by the visible / focused widget properly. Push the logic down
Simplify the switch mode method
Rationale: Switch mode has some recent modifications to support pushing the search mode down into the pages, but that was step 1, step 2 is making it so the search page is responsible for handling search / filter mode and the main app doesn't care about it
Fix how crate info showing is handled - move the loading parts of that into a crate info module instead of search module
Rationale: update_current_selection_crate_info / show_full_crate_details should be pulled out and converted to something that allows a tab for the crate info to be shown. This would allow pressing enter on the summary screen and pressing enter on the search results to do the same thing. If you don't want this as a tab, at least make it a high level concept which the app pushes to the selected tab to render (but a tab is better)
Move crate counting down to search
Rationale: store_total_number_of_crates belongs in search, not the app
Make the open url / clipboard copy a parameterized action
Rationale: Open url / clipyboard should accept a url and be something any page can trigger (push don't pull)
Move the part of cursor setting that calls search out, and replace with something more generic
Rationale: update_cursor knows too much about search - it should perhaps just be shared mutable state for whichever view / page is visible to set (or a call to send a SetCursor action, whichever makes sense)
move the events widget into its own type / module
Rationale: This is mostly self contained behavior / related to the key chord handling mentioned above (perhaps they belong together?)
Currently App is ~600 LoC in a 650 LoC file. I'm guessing a once all the things that are separate responsibilities are extracted it would be closer to half that.
With a more general perspective on this once these tasks are done, look at the two most unrelated methods in each type and try to explain the relationship between them. The language that you use to describe how this works is a good source of missing concepts that might help model the application in a way that aligns the code with how we talk about the code.
The text was updated successfully, but these errors were encountered: