bug: fix issues introduced with average CPU usage refactor (#1846)

* fix potential logic bug with generate points logic, but this isn't the actual issue

* update changelog

* why was that there

* okay let's not bother

* Revert "okay let's not bother"

This reverts commit 1ce8f5619f9455d52b4993ef57681d4a5b302d6f.

* Revert "why was that there"

This reverts commit 6a2a4da04a888867df8608558247951c8084ff3c.

* Revert "fix potential logic bug with generate points logic, but this isn't the actual issue"

This reverts commit 7f7de8055b360669da5284a11e70b2a8d1eb9ac3.

* Revert "bug/refactor: draw average CPU last, refactor CPU data code (#1804)"

This reverts commit 43e1b348995e9c8d7bcb694301371b2b4649ebc8.

* reverse as a lazy fix for avg

CHANGELOG.md

@@ -20,7 +20,7 @@ That said, these are more guidelines rather than hardset rules, though the proje
 
 ---
 
-## [0.11.3]/[0.12.0] - Unreleased
+## [0.11.3] - Unreleased
 
 ### Features
 
@@ -30,6 +30,7 @@ That said, these are more guidelines rather than hardset rules, though the proje
 
 - [#1833](https://github.com/ClementTsang/bottom/pull/1833): Sort disk I/O using actual value rather than string representation.
 - [#1812](https://github.com/ClementTsang/bottom/pull/1812): Fix ARC collection on FreeBSD.
+- [#1846](https://github.com/ClementTsang/bottom/pull/1846): Fix displayed average CPU value being wrong in graphs.
 
 ### Other
 

src/app/data/store.rs

@@ -29,7 +29,7 @@ pub struct StoredData {
     pub arc_harvest: Option<MemData>,
     #[cfg(feature = "gpu")]
     pub gpu_harvest: Vec<(String, MemData)>,
-    pub cpu_harvest: Vec<cpu::CpuData>,
+    pub cpu_harvest: cpu::CpuHarvest,
     pub load_avg_harvest: cpu::LoadAvgHarvest,
     pub process_data: ProcessData,
     /// TODO: (points_rework_v1) Might be a better way to do this without having to store here?
@@ -50,7 +50,7 @@ impl Default for StoredData {
             #[cfg(not(target_os = "windows"))]
             cache_harvest: None,
             swap_harvest: None,
-            cpu_harvest: Default::default(),
+            cpu_harvest: cpu::CpuHarvest::default(),
             load_avg_harvest: cpu::LoadAvgHarvest::default(),
             process_data: Default::default(),
             prev_io: Vec::default(),
@@ -113,25 +113,7 @@ impl StoredData {
         }
 
         if let Some(cpu) = data.cpu {
-            self.cpu_harvest.clear();
+            self.cpu_harvest = cpu;
-
-            if let Some(avg) = cpu.avg {
-                self.cpu_harvest.push(cpu::CpuData {
-                    data_type: cpu::CpuDataType::Avg,
-                    usage: avg,
-                });
-            }
-
-            self.cpu_harvest
-                .extend(
-                    cpu.cpus
-                        .into_iter()
-                        .enumerate()
-                        .map(|(core, usage)| cpu::CpuData {
-                            data_type: cpu::CpuDataType::Cpu(core as u32),
-                            usage,
-                        }),
-                );
         }
 
         if let Some(load_avg) = data.load_avg {

src/app/data/time_series.rs

@@ -38,9 +38,6 @@ pub struct TimeSeriesData {
     /// CPU data.
     pub cpu: Vec<Values>,
 
-    /// Average CPU data.
-    pub avg_cpu: Values,
-
     /// RAM memory data.
     pub ram: Values,
 
@@ -73,12 +70,10 @@ impl TimeSeriesData {
             self.tx.insert_break();
         }
 
-        if let Some(cpu_harvest) = &data.cpu {
+        if let Some(cpu) = &data.cpu {
-            let cpus = &cpu_harvest.cpus;
+            match self.cpu.len().cmp(&cpu.len()) {
-
-            match self.cpu.len().cmp(&cpus.len()) {
                 Ordering::Less => {
-                    let diff = cpus.len() - self.cpu.len();
+                    let diff = cpu.len() - self.cpu.len();
                     self.cpu.reserve_exact(diff);
 
                     for _ in 0..diff {
@@ -86,7 +81,7 @@ impl TimeSeriesData {
                     }
                 }
                 Ordering::Greater => {
-                    let diff = self.cpu.len() - cpus.len();
+                    let diff = self.cpu.len() - cpu.len();
                     let offset = self.cpu.len() - diff;
 
                     for curr in &mut self.cpu[offset..] {
@@ -96,20 +91,13 @@ impl TimeSeriesData {
                 Ordering::Equal => {}
             }
 
-            for (curr, new_data) in self.cpu.iter_mut().zip(cpus.iter()) {
+            for (curr, new_data) in self.cpu.iter_mut().zip(cpu.iter()) {
-                curr.push((*new_data).into());
+                curr.push(new_data.usage.into());
-            }
-
-            // If there isn't avg then we never had any to begin with.
-            if let Some(avg) = cpu_harvest.avg {
-                self.avg_cpu.push(avg.into());
             }
         } else {
             for c in &mut self.cpu {
                 c.insert_break();
             }
-
-            self.avg_cpu.insert_break();
         }
 
         if let Some(memory) = &data.memory {

src/canvas/widgets/cpu_basic.rs

@@ -158,8 +158,7 @@ impl Painter {
             CpuDataType::Avg => ("AVG".to_string(), self.styles.avg_cpu_colour),
             CpuDataType::Cpu(index) => (
                 format!("{index:<3}",),
-                self.styles.cpu_colour_styles
+                self.styles.cpu_colour_styles[index % self.styles.cpu_colour_styles.len()],
-                    [(index as usize) % self.styles.cpu_colour_styles.len()],
             ),
         };
 

src/canvas/widgets/cpu_graph.rs

@@ -119,6 +119,7 @@ impl Painter {
     fn generate_points<'a>(
         &self, cpu_widget_state: &'a CpuWidgetState, data: &'a StoredData, show_avg_cpu: bool,
     ) -> Vec<GraphData<'a>> {
+        let show_avg_offset = if show_avg_cpu { AVG_POSITION } else { 0 };
         let current_scroll_position = cpu_widget_state.table.state.current_index;
         let cpu_entries = &data.cpu_harvest;
         let cpu_points = &data.timeseries_data.cpu;
@@ -127,59 +128,40 @@ impl Painter {
         if current_scroll_position == ALL_POSITION {
             // This case ensures the other cases cannot have the position be equal to 0.
 
-            let capacity = if show_avg_cpu {
+            cpu_points
-                cpu_points.len() + 1
+                .iter()
+                .enumerate()
+                .map(|(itx, values)| {
+                    let style = if show_avg_cpu && itx == AVG_POSITION {
+                        self.styles.avg_cpu_colour
+                    } else if itx == ALL_POSITION {
+                        self.styles.all_cpu_colour
                     } else {
-                cpu_points.len()
+                        self.styles.cpu_colour_styles
+                            [(itx - show_avg_offset) % self.styles.cpu_colour_styles.len()]
                     };
-            let mut points = Vec::with_capacity(capacity);
-
-            points.extend(cpu_points.iter().enumerate().map(|(itx, values)| {
-                let style_index = itx % self.styles.cpu_colour_styles.len();
-                let style = self.styles.cpu_colour_styles[style_index];
 
                     GraphData::default().style(style).time(time).values(values)
-            }));
+                })
-
+                .rev()
-            // We draw avg last so it is drawn on top.
+                .collect()
-            if show_avg_cpu {
-                points.push(
-                    GraphData::default()
-                        .style(self.styles.avg_cpu_colour)
-                        .time(time)
-                        .values(&data.timeseries_data.avg_cpu),
-                );
-            }
-
-            points
         } else if let Some(CpuData { .. }) = cpu_entries.get(current_scroll_position - 1) {
             // We generally subtract one from current scroll position because of the all entry.
 
-            let show_avg_offset = if show_avg_cpu { AVG_POSITION } else { 0 };
+            let style = if show_avg_cpu && current_scroll_position == AVG_POSITION {
-            let is_avg = show_avg_cpu && current_scroll_position == AVG_POSITION;
-
-            let style = if is_avg {
                 self.styles.avg_cpu_colour
             } else {
-                self.styles.cpu_colour_styles[(current_scroll_position - 1 - show_avg_offset)
+                let offset_position = current_scroll_position - 1;
-                    % self.styles.cpu_colour_styles.len()]
+                self.styles.cpu_colour_styles
+                    [(offset_position - show_avg_offset) % self.styles.cpu_colour_styles.len()]
             };
 
-            if is_avg {
             vec![
                 GraphData::default()
                     .style(style)
                     .time(time)
-                        .values(&data.timeseries_data.avg_cpu),
+                    .values(&cpu_points[current_scroll_position - 1]),
             ]
-            } else {
-                vec![
-                    GraphData::default()
-                        .style(style)
-                        .time(time)
-                        .values(&cpu_points[current_scroll_position - 1 - show_avg_offset]),
-                ]
-            }
         } else {
             vec![]
         }

src/collection/cpu.rs

@@ -8,7 +8,7 @@ pub type LoadAvgHarvest = [f32; 3];
 #[derive(Debug, Clone, Copy)]
 pub enum CpuDataType {
     Avg,
-    Cpu(u32),
+    Cpu(usize),
 }
 
 #[derive(Debug, Clone)]
@@ -17,8 +17,4 @@ pub struct CpuData {
     pub usage: f32,
 }
 
-#[derive(Debug, Clone, Default)]
+pub type CpuHarvest = Vec<CpuData>;
-pub struct CpuHarvest {
-    pub avg: Option<f32>,
-    pub cpus: Vec<f32>,
-}

src/collection/cpu/sysinfo.rs

@@ -3,19 +3,31 @@
 
 use sysinfo::System;
 
-use super::CpuHarvest;
+use super::{CpuData, CpuDataType, CpuHarvest};
 use crate::collection::error::CollectionResult;
 
 pub fn get_cpu_data_list(sys: &System, show_average_cpu: bool) -> CollectionResult<CpuHarvest> {
-    let avg = show_average_cpu.then(|| sys.global_cpu_usage());
+    let mut cpus = vec![];
 
-    let cpus = sys
+    if show_average_cpu {
-        .cpus()
+        cpus.push(CpuData {
+            data_type: CpuDataType::Avg,
+            usage: sys.global_cpu_usage(),
+        })
+    }
+
+    cpus.extend(
+        sys.cpus()
             .iter()
-        .map(|cpu| cpu.cpu_usage())
+            .enumerate()
-        .collect::<Vec<_>>();
+            .map(|(i, cpu)| CpuData {
+                data_type: CpuDataType::Cpu(i),
+                usage: cpu.cpu_usage(),
+            })
+            .collect::<Vec<_>>(),
+    );
 
-    Ok(CpuHarvest { avg, cpus })
+    Ok(cpus)
 }
 
 #[cfg(target_family = "unix")]

src/widgets/cpu_graph.rs

@@ -102,7 +102,6 @@ impl DataToCell<CpuWidgetColumn> for CpuWidgetTableData {
             } => match data_type {
                 CpuDataType::Avg => painter.styles.avg_cpu_colour,
                 CpuDataType::Cpu(index) => {
-                    let index = *index as usize;
                     painter.styles.cpu_colour_styles[index % painter.styles.cpu_colour_styles.len()]
                 }
             },