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

Temperature Over Time #1366

Open
1 task done
Freyert opened this issue Dec 30, 2023 · 5 comments
Open
1 task done

Temperature Over Time #1366

Freyert opened this issue Dec 30, 2023 · 5 comments
Labels
feature Requests for a new feature.

Comments

@Freyert
Copy link

Freyert commented Dec 30, 2023

Checklist

Describe the feature request

My motivation is that I have issues on my computer where it overheats, and crashes. I'm actually not sure it's overheating though.

I was looking at the code and readme which caused me to notice that there are essentially Widgets with Graphs and Widgets without Graphs.

I also noticed you doing a larger rewrite of the widget system to improve extensibility #374

I think the core issue why I can't graph temperatures is related to this lack of extensibility.

From my personal exploration:

My expectations would be that I could convert either one of these metrics into an instantaneous table or a graph as suits me. Though I can also understand particular widgets like the process table being different...

@Freyert Freyert added the feature Requests for a new feature. label Dec 30, 2023
@ClementTsang
Copy link
Owner

ClementTsang commented Dec 31, 2023

Yeah, it's something I wanted to do (I made a comment a while ago in #205), but haven't had time with other stuff in life.

@Freyert
Copy link
Author

Freyert commented Apr 3, 2024

@ClementTsang I see! That's helpful to get more context from those other issues.

I think I'll fork and make a mess, but try some things out. If I come up with anything I'll share it back here for your consideration.

@Freyert
Copy link
Author

Freyert commented Apr 4, 2024

Ok so my main focus area is the fact that all of these widgets for graphs have to implement a different draw function depending on the data they receive.

flowchart TD
    subgraph src/canvas.rs
    A[struct Painter] --> B[impl Painter.draw_data]
    end

    subgraph src/canvas/widgets/cpu_graph.rs
    B --> C[impl Painter.draw_cpu]
    end

    subgraph src/canvas/widgets/mem_graph.rs
    B --> D[impl Painter.draw_memory_graph]
    end

    subgraph src/canvas/widgets/network_graph.rs
    B --> E[impl Painter.draw_network]
    end
Loading

@Freyert
Copy link
Author

Freyert commented Apr 4, 2024

My plan is to invert control here a bit. Instead of a CPU Widget or a Network Widget... We can have a Graph Widget or a Table Widget.

So then where do we get our data from? From things that are Graphable or Tableable. Which now I think is trapped in the widget state.

This is already kind of how it works. crate::canvas::components::{TimeGraph, GraphData} take care of this.

Which really helps me hone in on: "how do I remove all these draw_x functions and just wind up with a draw.

My current hunch is that it's just a poor separation of concerns: Painter should not be figuring out the legends and labels for the data. That should come from the State.


This is tricky. So there's App which has AppWidgetStates. AppWidgetStates just does some real minimal basic stuff. The actual data resides inside the App.converted_data.

From an ergonomics perspective I think I would enjoy having App not be in charge of so much itself, but instead delegate it out to the widgets.

So my near target is to have the widgets own their converted_data somehow, so they can use it to return labels.

I think it would be much easier to get down to a single draw method without much switching if the WidgetState were responsible for the data being displayed in that widget and sending all necessary config to Painter for drawing.

TLDR; there's too much state in my paint!

@Freyert
Copy link
Author

Freyert commented Apr 7, 2024

I've been exploring more and it seems like separation of concerns is a bit difficult. There are good reasons for everything to be scoped the way it is, but there are ways to separate concerns with a little more added complexity.


One of my big goals is to have the state objects (TempState, NetState) own the data that their widgets need to render.

If we're talking about a Model View Controller framework (which bottom uses?) then the state objects should own all data that the application uses.


However, we also have the concept of the Collector. The Collector runs in a separate thread to collect system metrics on a defined interval.

I first thought it would be good to separate collection to each Widget so that each Widget would have a State and a Collector. However this isn't ideal because Collector collects sysinfo for example and then delivers it to several other widgets.

It would be inefficient at this moment to have each widget have a thread for collecting their info because they would all make redundant calls with sysinfo.


This pattern could be handled by Channels. However we would now need a thread to pull data into the state objects.

collect -> channel_n -> Vec<i32>.

Downside: need thread to pull data out of the channel.
Upside: it probably already exists.


Goal: give state ownership of a data channel that the collector can send to and give the collector a reference to transmit to. Just for Temperature, but hopefully if the pattern works well refactor other states to follow.

In the end this will give the states higher authority over their data and we can potentially move the farmer responsibility into the state.

States being responsible for their data allows us to then focus on making widgets be Views instead of a whole MVC architecture themselves.

Models: Battery, Temperature, Network, CPU, Disk
Views: Graph, Table

BatteryModel + GraphView = BatteryGraphWidget
TemperatureModel + GraphView = TemperatureGraphWidget
TemperatureModel + TableView = TemperatureTableWidget

etc.

I would probably also do away with the term Widget all together in favor of Views. Highlighting an architectural pattern in the naming choices will make it much easier to navigate and construct well. Or saying that a widget is a combination of a Model and a View. That could work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Requests for a new feature.
Projects
None yet
Development

No branches or pull requests

2 participants