-
-
Notifications
You must be signed in to change notification settings - Fork 35
Add "memory" and "cpu" modules #75
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When adding new features, please document them by providing examples in the template config file. Also, missing sign-off
} | ||
} | ||
ButtonType::Memory(memory_mode) => { | ||
let memory_usage = get_memory_usage(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make a common function for "button with percentage and optional icon", there seems to be a lot of duplication for Battery/Memory/Cpu
|
||
fn new_memory(action: Key, memory_mode: String, theme: Option<impl AsRef<str>>) -> Button { | ||
let image = Self::load_image("memory", theme.as_ref()); | ||
let memory_mode = match memory_mode.as_str() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Common parser for all of them? also all panic messages refer to battery
} | ||
|
||
fn new_memory(action: Key, memory_mode: String, theme: Option<impl AsRef<str>>) -> Button { | ||
let image = Self::load_image("memory", theme.as_ref()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about using the "memory" icon for cpu, and "memory alt" for memory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't find a memory alt icon in material design icons
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
freedesktop-icons = "0.4.0" | ||
chrono = { version = "0.4", features = ["unstable-locales"] } | ||
udev = "0.9" | ||
sysinfo = "0.36.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are linux-only, and this is not expected to change ever, so why not just read /proc directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, that would take extra work, especially for the CPU, but I could do that
fn get_cpu_usage() -> u32 { | ||
let mut s = System::new(); | ||
s.refresh_cpu_usage(); | ||
std::thread::sleep(sysinfo::MINIMUM_CPU_UPDATE_INTERVAL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this probably will be changed for other reasons, but in general - absolutely not. The app is structured around an event loop, and you should not do blocking sleeps
Changes:
Caveats: