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.

Sign up for GitHub

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

Jump to bottom

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 •
edited
Loading

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.

@ClementTsang
refactor: add glue to prep for the transition
88ebcab 
Write some glue code to transition from the old code to the new one.
@ClementTsang
refactor: more glue code to build layout
b5e6dea 
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).
@ClementTsang
refactor: rename files from mod to their directory names
189be96
@ClementTsang
refactor: rip out trait system for drawing widgets
dd7e183 
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.
@ClementTsang
refactor: start moving over drawing system
0afc371 
In particular, moving over table-style widgets
@ClementTsang
refactor: start moving over the event system
6b69e37
@ClementTsang
refactor: separate out sorted and non-sorted text tables
b72e76a
@ClementTsang
refactor: port over graph widgets
2bff04d 
Things working as of now:
- Actually drawing
- Interpolation
- Styling
@ClementTsang
refactor: add network drawing cache system
64d47d5
@ClementTsang
refactor: another pass on sorting and columns
74293aa
@ClementTsang
refactor: Add data updating to process widget
1ec203c
@ClementTsang
refactor: change up event handling logistics
48c572d 
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.
@ClementTsang
bug: fix bug causing click bounds to fail
3fa5060 
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
@ClementTsang
refactor: Add sort capabilities to processes
27736b7
@ClementTsang
refactor: add text input
b1889b0
@ClementTsang
refactor: add back grouping and command
204b4dc
@ClementTsang
refactor: move basic mode over
eddc9a1 
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
refactor: move over battery widget
fa00dec
@ClementTsang
refactor: delete a bunch of old unused code
18af6b0
@ClementTsang ClementTsang force-pushed the state_refactor branch 2 times, most recently from 368aa2c to 81fd703 Compare September 7, 2021 04:48
@ClementTsang
refactor: add back widget titles
9ef38cb 
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
@ClementTsang
refactor: delete more stuff
9089231 
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
@ClementTsang
Merge branch 'master' into state_refactor
5833cb8
@ClementTsang
refactor: mostly add back tree mode for process
cc66f1f 
Mouse control on collapse is not working yet, need to do some work
internally first.
@ClementTsang ClementTsang mentioned this pull request Feb 27, 2022
Closed
@ClementTsang ClementTsang mentioned this pull request May 16, 2022
Merged
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
Reviewers
No reviews
Assignees

@ClementTsang ClementTsang

Labels
refactor For refactoring issues. Generally you won't need to use this tag in your report.
Projects
No open projects
bottom
Archived in project
Milestone
No milestone
1 participant
@ClementTsang