feat!: Refactor #4

Dronakurl · 2024-12-30T11:16:30Z

It is such a great project to learn rust.
I refactored it to allow any kind of modules as long as the implement Display.
I am not mad when you decline this PR, it was a great learning about polymorphisms.

BTW: I don't think, the threads are necessary: tmux already runs commands in the background and everything but the CPU stuff is really fast (1ms). But it might speed things up for future modules

- Added documentation comments for `Color` enum and `Style` struct in `colors/mod.rs` - Documented `Display` implementation for `Style` and `Color` - Added documentation for `foreground_color`, `background_color`, and `bold` functions - Updated `config.rs` to use new modules and styles - Enhanced `icons` module with detailed documentation - Refactored `main.rs` to remove unnecessary threading - Updated `README.md` with detailed instructions for adding custom modules - Added `battery.rs` module for battery status - Enhanced `datetime.rs` module with date and time functions - Refactored `systemstats.rs` to use `fmt::Display` instead of `Show` - Improved `tmux.rs` with detailed documentation and standard styling - Removed unused `conditional_insert.rs` and `system` utilities - Updated `utils/strings.rs` with detailed documentation - Updated `modules/README.md` with contributing guidelines - Refactored `modules/mod.rs` to include new modules

Dlurak · 2024-12-30T21:41:12Z

I'm gonna review it in a few days at the earliest, thank you so much for your efforts :)

As you've been adding new commits to the pr it would be good to know when you think that it is ready from your site, feel free to ping me once you are happy with the pr so I know that reviewing makes sense :)

Dronakurl · 2025-01-17T20:14:19Z

I'm gonna review it in a few days at the earliest, thank you so much for your efforts :)

As you've been adding new commits to the pr it would be good to know when you think that it is ready from your site, feel free to ping me once you are happy with the pr so I know that reviewing makes sense :)

I think, I am happy with the result now

Dlurak

Sorry that it took me so long to review your pr, I worked on some things and then forgot about it, I'm really sorry.
Wow that is really some quite impressive work! 👍
But there are a few general small issues that need optimization:

Can you please create a new traits to convert the actual modules like Memory or Nvidia to the Module struct.
Something like that might be a good:

trait ModuleStyles {
    fn style(&self) -> Style;
    fn icon(&self) -> Option<Icon>;
}

Module should get a method to create a new method from something that implements ModuleStyles. Module::default() should also use the new trait when possible.

Many modules instantly return a Box<Module<Self>>.
To make them more versatile they could return Self or in some cases Option<Self>
Using the new trait it's easy to create a Box<Module> from it, just something like one of the two lines would be enough:

Module::new_box(Memory::default());
Box::new(Module::default_styled(Memory::default()));

It's ok when a module is optional or can fail, for example the low battery warning should be this: Option<Module<String>>.
get_modules() could use the .chain method for that.

Also I left you some comments on the code which need to be addressed :)

Dlurak · 2025-02-20T19:03:56Z

src/colors/mod.rs

I know that I already did it like that but can you please move that file to src/color.rs. It's kind of unnecessary to have a new directory for only one file

Dlurak · 2025-02-20T19:05:25Z

src/icons/mod.rs

Same as for the colors/mod.rs it would be better to move the file to src/icons.rs

Dlurak · 2025-02-20T19:13:03Z

src/utils/mod.rs

This entire utils module now only offers the PrettyDuration struct. I think we can delete the directory and move src/utils/string.rs to something like src/pretty_duration.rs

Dlurak · 2025-02-20T19:49:41Z

src/modules/high_cpu.rs

+        let process_info = if output.status.success() {
+            let stdout = String::from_utf8_lossy(&output.stdout);
+            let mut process_map: HashMap<String, f32> = HashMap::new();
+
+            stdout.lines().skip(1).for_each(|line| {
+                let parts: Vec<&str> = line.split_whitespace().collect();
+                if let (Some(cpu_usage), Some(name)) = (parts.get(1), parts.get(2)) {
+                    let cpu_usage: f32 = cpu_usage.parse().unwrap_or(0.0);
+                    if cpu_usage > 0.001 && *name != "ps" {
+                        *process_map.entry(name.to_string()).or_insert(0.0) += cpu_usage;
+                    }
+                }
+            });


Rust has methods on Iterators and Options that allow for creating very short, declerative and (imho) easy to read code:

let process_map = stdout .lines() .skip(1) .filter_map(|line| { let mut parts = line.split_whitespace().skip(1); let cpu_usage: f32 = parts.next()?.parse().unwrap_or(0.0); let name = parts.next()?; (cpu_usage > 0.1 && name != "ps").then_some((name, cpu_usage)) }) .fold(HashMap::new(), |mut acc, (name, cpu_usage)| { *acc.entry(name).or_insert(0.0) += cpu_usage; acc });

Btw, the type of the HashMap does not need to be explicitly stated, as the line which adds/updates entries uses f32 and String the type can be inferred

Dlurak · 2025-02-20T20:10:53Z

src/icons/mod.rs

 pub enum Icon {
+    /// Default empty icon
+    #[default]
+    Empty,


We use Option<Icon> everywhere. It seems like None and Icon::Empty represent the same thing. I would prefer to remove that Empty variant. Do you have a good reason to keep it?

Dlurak · 2025-02-20T21:10:50Z

src/modules/battery.rs

+            Ok(Self {
+                percentages: 0,
+                is_charging: false,
+            })


I think a Ok(None) would be the best option here. In rust it is generally discouraged to use magic values like 0 to indicate something. In this case you use the special value of 0 to indicate that no battery exists, this would be better with a Option. Also consider removing/modifying the Default implementation
When changing this you would of course also need to modify all usages of that special value for example in the Display implementation.

Dlurak · 2025-02-20T21:15:34Z

src/modules/battery.rs

+    ///
+    /// # Returns
+    /// A boxed Module containing battery information with conditional styling
+    pub fn get_with_warning(bg: Option<Color>) -> Box<Module<String>> {


There is no advantage in coupling the warning to the battery, it makes it harder to style, impossible to use independently and limits how the two modules can be placed in the status bar.
For example someone might want to place a green battery on the far left and a red battery warning on the far right, that would need two independent modules.
Not too important but it would be great if the threshold weren't hardcoded to twenty but a parameter

Dlurak · 2025-02-20T21:19:03Z

src/modules/high_cpu.rs

+    /// # Returns
+    ///
+    /// A `Box<Module<HighCpuModule>>` containing the process information.
+    pub fn new(fg: Option<Color>, bg: Option<Color>) -> Box<Module<HighCpu>> {


Instead of having the errors as the process info this should return a Result. Then the caller can decide how to handle the errors, maybe they want to show an error text with a different style, maybe they want to ignore the error...

Dlurak · 2025-02-20T21:20:45Z

src/modules/mod.rs

-                let swap_usage_percent = (used_swap as f64 / total_swap as f64) * 100.0;
-                Ok(strings::round(swap_usage_percent, rounding))
-            }
+impl<T> Default for Module<T>


When T implements the new proposed trait the style should be read from the trait implementation.

Btw I love your idea with the new Module struct, great job 👍

Dlurak · 2025-02-20T21:25:49Z

src/modules/nvidia.rs

+    pub memory_used: String,
+    pub memory_total: String,


A Option of some number type should be better here. I don't have a nvidia card so I can't say which type but some number type

Dlurak · 2025-02-23T20:13:45Z

I've just refactored various parts myself.

Dronakurl · 2025-02-24T06:08:41Z

Thank you for looking into it! I started too, but I did not immediately get the idea with the new trait. Can I still help with this? Dlurak ***@***.***> schrieb am So. 23. Feb. 2025 um 21:14:

…

I've just refactored various parts myself. — Reply to this email directly, view it on GitHub <#4 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ASKZU4B42DPWD52GYWYXC6T2RITY5AVCNFSM6AAAAABUL4PWZSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNZXGA4TCOJZGE> . You are receiving this because you authored the thread.Message ID: ***@***.***> [image: Dlurak]*Dlurak* left a comment (Dlurak/muxbar#4) <#4 (comment)> I've just refactored various parts myself. — Reply to this email directly, view it on GitHub <#4 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ASKZU4B42DPWD52GYWYXC6T2RITY5AVCNFSM6AAAAABUL4PWZSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNZXGA4TCOJZGE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

Dlurak · 2025-02-24T16:12:19Z

If you want to you can port your new modules and documentation. Totally up to you :)

I've fundamentally changed how muxbar renders it's modules:

Previously tmux would run muxbar, wait a short moment and then run it again... After every fresh start muxbar had to recompute every single module.

Now each module specifies when it will be rerendered and also how to update itself.
Once a module needs to rerender muxbar rerenders the desired module while using all other modules from a cache.
Muxbar now runs indefinitely and just outputs new lines from time to time.
For this reason the ToModule trait got two new methods.

Dronakurl added 4 commits December 30, 2024 12:06

chore: Update crate for systemstats

034663b

feat: add NVIDIA module

234da9b

feat: percentage display for nvidia module

f24b88d

Dronakurl force-pushed the refactor branch from 8d16a67 to f24b88d Compare December 30, 2024 13:42

Dronakurl added 5 commits December 30, 2024 16:30

Change default config

bb95855

doc: improve README

619279f

feat: high CPU module

fb4b73a

backup high cpu

2e5b18c

finish high cpu module

f870a86

Update NVIDIA module

55fadd9

Dronakurl marked this pull request as draft December 31, 2024 07:21

Dronakurl added 4 commits January 1, 2025 23:55

fix: handle no battery

5660707

feat: Make highcpu colors configurable

dc36d46

feat: color options more configurable in module standards

660f5cb

fix: empty modules

7d2ec94

Dronakurl force-pushed the refactor branch from 1aa7366 to 7d2ec94 Compare January 1, 2025 23:37

Dronakurl added 2 commits January 2, 2025 11:35

doc: add clarification in README

d9095c0

Merge branch 'master' into refactor

ae690de

Dronakurl marked this pull request as ready for review January 17, 2025 20:12

Dlurak requested changes Feb 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: Refactor #4

feat!: Refactor #4

Dronakurl commented Dec 30, 2024

Dlurak commented Dec 30, 2024

Dronakurl commented Jan 17, 2025

Dlurak left a comment

Dlurak Feb 20, 2025

Dlurak Feb 20, 2025

Dlurak Feb 20, 2025

Dlurak Feb 20, 2025

Dlurak Feb 20, 2025

Dlurak Feb 20, 2025

Dlurak Feb 20, 2025

Dlurak Feb 20, 2025

Dlurak Feb 20, 2025

Dlurak Feb 20, 2025

Dlurak commented Feb 23, 2025

Dronakurl commented Feb 24, 2025 via email

Dlurak commented Feb 24, 2025

feat!: Refactor #4

Are you sure you want to change the base?

feat!: Refactor #4

Conversation

Dronakurl commented Dec 30, 2024

Dlurak commented Dec 30, 2024

Dronakurl commented Jan 17, 2025

Dlurak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dlurak commented Feb 23, 2025

Dronakurl commented Feb 24, 2025 via email

Dlurak commented Feb 24, 2025