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

refactor: state refactor #571

Closed
wants to merge 42 commits into from
Closed

refactor: state refactor #571

wants to merge 42 commits into from

Conversation

ClementTsang
Copy link
Owner

@ClementTsang ClementTsang commented Aug 23, 2021

Description

A description of the change and what it does. If relevant (such as any change that modifies the UI), please provide screenshots of the change:

Alright, let's take another crack at this.


The goals of this refactor

  • Throw out how we used to manage per-widget state; use a more generic approach.
  • Throw out how we used to do user input management - since most inputs are widget-specific, we instead bind the handling to the Widgets or Components themselves, barring global shortcuts, which will be handled separately.
  • Throw out the old way we do layout management and widget selection. The logic I've switched to is more akin to that of i3wm - instead of manually computing on startup where to move/reflect movement actions to based on computed widget size (which was horrible), there's an easier way to do it on-the-fly with a row/column system. Note this does mean the behaviour is now different and thus breaking. Going to go back to the old behaviour, but with the new layout system.
  • Refactor how drawing works; we dropped the old "trait" system (which made zero sense in hindsight) and modified it to work with our new Layout and Widget system.

Changes

  • Removed default_widget_count and default_widget_type from configs and flags.
  • Switching to tree mode no longer automatically sets the sort type to PID.

To do

  • Move net over to new drawing system
  • Move processes over to new drawing system
  • Write TextInput component
  • Write sort menu logic.
  • Write Carousel component
  • Move basic mode to new drawing system
  • Move battery over to new drawing system
  • Widget labels
  • Scroll positions
  • Full screen Esc. prompt in the title
  • Help dialog
  • Process kill dialog
  • Add back search error
  • Add back search conditions
  • CPU load avg in header
  • Fix movement
  • Make basic mem % not shift?
  • Port over all global shortcuts that are left - this just needs to be double-checked!
  • Port over all local (per-widget) shortcuts/mouse bindings if any are still missing - this just needs process killing/trees to be done, and to be double-checked!
  • Get rid of all old unused code - this is mostly done, with just a bit left!
  • Fix merge conflicts
  • Clippy pass
  • Write new internal unit tests, mainly for movement and layout
  • Document alllll the things (at least any pub fn for now).
  • Test!

Issue

If applicable, what issue does this address?

Closes: #234
Closes: #374
Closes: #562 (due to bumping crossterm version)

Testing

If relevant, please state how this was tested. All changes must be tested to work:

Furthermore, mark which platforms this change was tested on. All platforms directly affected by the change must be tested

  • Windows
  • macOS
  • Linux

Checklist

If relevant, ensure the following have been met:

  • Areas your change affects have been linted using rustfmt (cargo fmt)
  • The change has been tested and doesn't appear to cause any unintended breakage
  • Documentation has been added/updated if needed (README.md, help menu, etc.)
  • The pull request passes the provided CI pipeline
  • There are no merge conflicts

@ClementTsang
Copy link
Owner Author

Note to self: don't squash this.

Write some glue code to transition from the old code to the new one.
Even more glue code to help glue together our layout options to our new
layout system!

Note that this PR will most likely likely break the two options:
- default_widget_type
- default_widget_count
and as such, they'll probably be deleted in a later commit.

As for why, it's since they're kinda a pain to support and don't work
well. Users can still enable default widget selection through the
layout system (which will also see a revamp in the future).
This rips out this weird trait system I previously used for drawing
widgets, where I implemented a trait onto the Painter struct that did
the drawing.  I have no idea what I was thinking back then.
In particular, moving over table-style widgets
Things working as of now:
- Actually drawing
- Interpolation
- Styling
Slightly move around the ideas of EventResult, ReturnSignalResult,
and how they all work.

The gist of it is that we now have widgets returning EventResults (and
renamed to WidgetEventResult), and the main app event handler returns
ReturnSignalResult (now named EventResult).

Also add a new signal to handle re-updating data inputs! This is needed
for the process, and any sortable/configurable widget.
There were three bugs:

1. The click bounds calculation was incorrect. I did the silly mistake
   of checking for <= bounds for the bottom and right sections of a
   Rect when checking if the mouse intersected - this is WRONG.

   For example, let's say you want to calculate if an x value of 5 falls
   between something that starts at 0 and is 5 long.  It shouldn't,
   right?  Because it draws from 0 to 4?  But if you just did <=
   Rect.right(), you would get a hit - because it just does (start +
   width), so you get 5, and 5 <= 5!

   So, easy fix, change all far bounds checks to <.

2. The second bug is a mistake where I accidentally did not include
   bounds sets for my memory and net widgets. Instead, they set their
   bounds to the underlying graph representation, which is WRONG, since
   that bound gets updated on draw, and gets set to a slightly smaller
   rect due to borders!

3. A slightly sneakier one. This broke my bounds checks for the CPU
   widget - and it would have broken my process widget too.

   The problem lies in the concept of widgets that handle multiple
   "sub"-blocks internally, and how I was doing click detection
   internally - I would check if the bounds of the internal Components
   were hit.  Say, the CPU, I would check if the internal graph was hit,
   then if the internal table was hit.

   But wait! I said in point 2 that a graph gets its borders updated on
   draw to something slightly smaller, due to borders!  And there's the
   problem - it affected tables too.  I was setting the bounds of
   components to that of the *internal* representation - without borders
   - but my click detection *needed* borders included!

   Solution?  Add another trait function to check bordered bounds, and
   make the default implementation just check the existing bounds. For
   cases like internal Components that may need it, I add a separate
   implementation.

   I also switched over all border bounds checks for Widgets to that,
   since it's a bit more consistent.
ClementTsang and others added 6 commits September 5, 2021 19:09
Because writing your own layout system and management is just *so much
fun*. Totally.

-------------------------------------------------------------------

Moves the basic mode system over to the new drawing/widget system. In
the process, it has forced me to completely redo how we do layouts...
again. This is because basic mode has widgets that control their own
height - this means the height of the columns and rows that wrap it
are also affected by the widget's height.

The previous system, using a constraint tree and splitting draw Rects
via tui-rs' built-in constraint solver, did not support this concept
very well. It was not simple to propagate up the widths/heights
towards parents while also using tui-rs' built-in constraint solver.
In the end, it was easier to just rewrite it using another algorithm.

We now follow a process very similar to Flutter's layout system.
Relevant links to the Flutter docs are found in the code or below:

- https://flutter.dev/docs/development/ui/layout/constraints
- https://flutter.dev/docs/resources/inside-flutter#sublinear-layouts

The gist of it, however, is that we now instead a few new options for
any element in the layout tree. A node can either:

- Grow to fill remaining space
- Take up as much room as its children
- Be a specific length

Technically right now, it's not perfect, in that leaf nodes can be as
large as their children (which makes no sense), though in that case it
just treats it as an expand.
@ClementTsang ClementTsang force-pushed the state_refactor branch 2 times, most recently from 368aa2c to 81fd703 Compare September 7, 2021 04:48
Also has a few clippy fixes and bug fixes:
- Fix redundant rerendering on scroll on time graphs.
- Fix being off by one cell during rendering for no-battery situation
  on the battery widget.
- Fix having empty columns for
  rtl column width calculations (as otherwise it goes to the wrong column).
- Fix rendering issue on small windows with text tables.

We also now ensure that the CPU legend has enough room to draw!
@ClementTsang ClementTsang force-pushed the state_refactor branch 2 times, most recently from 7a8bbb7 to 89f54c1 Compare September 26, 2021 05:26
Mostly previously re-added files during the merge conflict resolution,
and a lot of unused code.

Still more to delete after I finish rewriting the process kill dialog.
@ClementTsang ClementTsang self-assigned this Oct 11, 2021
@ClementTsang ClementTsang added the refactor For refactoring issues. Generally you won't need to use this tag in your report. label Oct 11, 2021
@ClementTsang ClementTsang changed the title State Refactor refactor: state refactor Oct 11, 2021
ClementTsang and others added 2 commits October 31, 2021 01:48
Mouse control on collapse is not working yet, need to do some work
internally first.
@ClementTsang ClementTsang mentioned this pull request May 16, 2022
15 tasks
@ClementTsang ClementTsang deleted the state_refactor branch January 20, 2023 02:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor For refactoring issues. Generally you won't need to use this tag in your report.
Projects
No open projects
Archived in project
1 participant