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

continued refactoring of app.rs for simplicity #25

Open
16 tasks
joshka opened this issue Feb 11, 2024 · 1 comment
Open
16 tasks

continued refactoring of app.rs for simplicity #25

joshka opened this issue Feb 11, 2024 · 1 comment

Comments

@joshka
Copy link
Member

joshka commented Feb 11, 2024

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.

@joshka
Copy link
Member Author

joshka commented Feb 11, 2024

For context:

image image image image

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

No branches or pull requests

1 participant