From cabc5942793c9417a626b581a66f1a0e3c9cf7f4 Mon Sep 17 00:00:00 2001 From: Clement Tsang <34804052+ClementTsang@users.noreply.github.com> Date: Sat, 25 Feb 2023 04:24:38 -0500 Subject: refactor: clean up some options code (#1029) * remove some dead code * use macros to help clean up clutter for binary flags * add test * group * fix using gpu feature --- src/app.rs | 4 +- src/app/data_harvester/temperature.rs | 2 +- src/clap.rs | 1 + src/options.rs | 439 ++++++++++------------------------ src/units/data_units.rs | 2 +- src/widgets/process_table.rs | 22 +- 6 files changed, 155 insertions(+), 315 deletions(-) (limited to 'src') diff --git a/src/app.rs b/src/app.rs index bbba3ea2..7ae3a2c8 100644 --- a/src/app.rs +++ b/src/app.rs @@ -31,7 +31,7 @@ pub mod states; use frozen_state::FrozenState; -#[derive(Debug, Clone)] +#[derive(Debug, Clone, Eq, PartialEq)] pub enum AxisScaling { Log, Linear, @@ -45,7 +45,7 @@ impl Default for AxisScaling { /// AppConfigFields is meant to cover basic fields that would normally be set /// by config files or launch options. -#[derive(Debug, Default)] +#[derive(Debug, Default, Eq, PartialEq)] pub struct AppConfigFields { pub update_rate_in_milliseconds: u64, pub temperature_type: temperature::TemperatureType, diff --git a/src/app/data_harvester/temperature.rs b/src/app/data_harvester/temperature.rs index 48d98ad2..a24da371 100644 --- a/src/app/data_harvester/temperature.rs +++ b/src/app/data_harvester/temperature.rs @@ -24,7 +24,7 @@ pub struct TempHarvest { pub temperature: f32, } -#[derive(Clone, Debug, Copy)] +#[derive(Clone, Debug, Copy, PartialEq, Eq)] pub enum TemperatureType { Celsius, Kelvin, diff --git a/src/clap.rs b/src/clap.rs index c7fa155b..b7610583 100644 --- a/src/clap.rs +++ b/src/clap.rs @@ -443,6 +443,7 @@ use CPU (3) as the default instead. app } + #[cfg(test)] mod test { use super::*; diff --git a/src/options.rs b/src/options.rs index c72aaa52..6e4f089d 100644 --- a/src/options.rs +++ b/src/options.rs @@ -43,18 +43,6 @@ pub struct Config { pub net_filter: Option, } -impl Config { - pub fn get_config_as_bytes(&self) -> anyhow::Result> { - let config_string: Vec> = vec![ - // Top level - CONFIG_TOP_HEAD.into(), - toml::to_string_pretty(self)?.into(), - ]; - - Ok(config_string.concat().as_bytes().to_vec()) - } -} - #[derive(Clone, Debug, Default, Deserialize, Serialize, TypedBuilder)] pub struct ConfigFlags { pub hide_avg_cpu: Option, @@ -81,7 +69,7 @@ pub struct ConfigFlags { pub battery: Option, pub disable_click: Option, pub no_write: Option, - // For built-in colour palettes. + /// For built-in colour palettes. pub color: Option, pub mem_as_value: Option, pub tree: Option, @@ -97,24 +85,6 @@ pub struct ConfigFlags { pub retention: Option, } -#[derive(Clone, Default, Debug, Deserialize, Serialize)] -pub struct WidgetIdEnabled { - id: u64, - enabled: bool, -} - -impl WidgetIdEnabled { - pub fn create_from_hashmap(hashmap: &HashMap) -> Vec { - hashmap - .iter() - .map(|(id, enabled)| WidgetIdEnabled { - id: *id, - enabled: *enabled, - }) - .collect() - } -} - #[derive(Clone, Debug, Default, Deserialize, Serialize)] pub struct ConfigColours { pub table_header_color: Option>, @@ -143,11 +113,10 @@ pub struct ConfigColours { } impl ConfigColours { + /// Returns `true` if there is a [`ConfigColours`] that is empty or there isn't one at all. pub fn is_empty(&self) -> bool { if let Ok(serialized_string) = toml::to_string(self) { - if !serialized_string.is_empty() { - return false; - } + return serialized_string.is_empty(); } true @@ -174,6 +143,28 @@ pub struct IgnoreList { pub whole_word: bool, } +macro_rules! is_flag_enabled { + ($flag_name:ident, $matches:expr, $config:expr) => { + if $matches.contains_id(stringify!($flag_name)) { + true + } else if let Some(flags) = &$config.flags { + flags.$flag_name.unwrap_or(false) + } else { + false + } + }; + + ($cmd_flag:literal, $cfg_flag:ident, $matches:expr, $config:expr) => { + if $matches.contains_id($cmd_flag) { + true + } else if let Some(flags) = &$config.flags { + flags.$cfg_flag.unwrap_or(false) + } else { + false + } + }; +} + pub fn build_app( matches: &ArgMatches, config: &mut Config, widget_layout: &BottomLayout, default_widget_id: u64, default_widget_type_option: &Option, @@ -183,16 +174,18 @@ pub fn build_app( let retention_ms = get_retention_ms(matches, config).context("Update `retention` in your config file.")?; - let autohide_time = get_autohide_time(matches, config); + let autohide_time = is_flag_enabled!(autohide_time, matches, config); let default_time_value = get_default_time_value(matches, config, retention_ms) .context("Update 'default_time_value' in your config file.")?; - let use_basic_mode = get_use_basic_mode(matches, config); + + let use_basic_mode = is_flag_enabled!(basic, matches, config); + let expanded_upon_startup = is_flag_enabled!(expanded_on_startup, matches, config); // For processes - let is_grouped = get_app_grouping(matches, config); - let is_case_sensitive = get_app_case_sensitive(matches, config); - let is_match_whole_word = get_app_match_whole_word(matches, config); - let is_use_regex = get_app_use_regex(matches, config); + let is_grouped = is_flag_enabled!("group", group_processes, matches, config); + let is_case_sensitive = is_flag_enabled!(case_sensitive, matches, config); + let is_match_whole_word = is_flag_enabled!(whole_word, matches, config); + let is_use_regex = is_flag_enabled!(regex, matches, config); let mut widget_map = HashMap::new(); let mut cpu_state_map: HashMap = HashMap::new(); @@ -214,14 +207,14 @@ pub fn build_app( let is_custom_layout = config.row.is_some(); let mut used_widget_set = HashSet::new(); - let show_memory_as_values = get_mem_as_value(matches, config); - let is_default_tree = get_is_default_tree(matches, config); - let is_default_command = get_is_default_process_command(matches, config); - let is_advanced_kill = !get_is_advanced_kill_disabled(matches, config); + let show_memory_as_values = is_flag_enabled!(mem_as_value, matches, config); + let is_default_tree = is_flag_enabled!(tree, matches, config); + let is_default_command = is_flag_enabled!(process_command, matches, config); + let is_advanced_kill = !(is_flag_enabled!(disable_advanced_kill, matches, config)); let network_unit_type = get_network_unit_type(matches, config); let network_scale_type = get_network_scale_type(matches, config); - let network_use_binary_prefix = get_network_use_binary_prefix(matches, config); + let network_use_binary_prefix = is_flag_enabled!(network_use_binary_prefix, matches, config); let app_config_fields = AppConfigFields { update_rate_in_milliseconds: get_update_rate_in_milliseconds(matches, config) @@ -229,21 +222,21 @@ pub fn build_app( temperature_type: get_temperature(matches, config) .context("Update 'temperature_type' in your config file.")?, show_average_cpu: get_show_average_cpu(matches, config), - use_dot: get_use_dot(matches, config), - left_legend: get_use_left_legend(matches, config), - use_current_cpu_total: get_use_current_cpu_total(matches, config), - unnormalized_cpu: get_unnormalized_cpu(matches, config), + use_dot: is_flag_enabled!(dot_marker, matches, config), + left_legend: is_flag_enabled!(left_legend, matches, config), + use_current_cpu_total: is_flag_enabled!(current_usage, matches, config), + unnormalized_cpu: is_flag_enabled!(unnormalized_cpu, matches, config), use_basic_mode, default_time_value, time_interval: get_time_interval(matches, config, retention_ms) .context("Update 'time_delta' in your config file.")?, - hide_time: get_hide_time(matches, config), + hide_time: is_flag_enabled!(hide_time, matches, config), autohide_time, - use_old_network_legend: get_use_old_network_legend(matches, config), - table_gap: u16::from(!get_hide_table_gap(matches, config)), - disable_click: get_disable_click(matches, config), + use_old_network_legend: is_flag_enabled!(use_old_network_legend, matches, config), + table_gap: u16::from(!(is_flag_enabled!(hide_table_gap, matches, config))), + disable_click: is_flag_enabled!(disable_click, matches, config), enable_gpu_memory: get_enable_gpu_memory(matches, config), - show_table_scroll_position: get_show_table_scroll_position(matches, config), + show_table_scroll_position: is_flag_enabled!(show_table_scroll_position, matches, config), is_advanced_kill, network_scale_type, network_unit_type, @@ -407,8 +400,6 @@ pub fn build_app( let net_filter = get_ignore_list(&config.net_filter).context("Update 'net_filter' in your config file")?; - let expanded_upon_startup = get_expanded_on_startup(matches, config); - Ok(App::builder() .app_config_fields(app_config_fields) .cpu_state(CpuState::init(cpu_state_map)) @@ -435,12 +426,13 @@ pub fn build_app( pub fn get_widget_layout( matches: &ArgMatches, config: &Config, ) -> error::Result<(BottomLayout, u64, Option)> { - let left_legend = get_use_left_legend(matches, config); + let left_legend = is_flag_enabled!(left_legend, matches, config); + let (default_widget_type, mut default_widget_count) = get_default_widget_and_count(matches, config)?; let mut default_widget_id = 1; - let bottom_layout = if get_use_basic_mode(matches, config) { + let bottom_layout = if is_flag_enabled!(basic, matches, config) { default_widget_id = DEFAULT_WIDGET_ID; BottomLayout::init_basic_default(get_use_battery(matches, config)) @@ -562,65 +554,6 @@ fn get_show_average_cpu(matches: &ArgMatches, config: &Config) -> bool { true } -fn get_use_dot(matches: &ArgMatches, config: &Config) -> bool { - if matches.contains_id("dot_marker") { - return true; - } else if let Some(flags) = &config.flags { - if let Some(dot_marker) = flags.dot_marker { - return dot_marker; - } - } - false -} - -fn get_use_left_legend(matches: &ArgMatches, config: &Config) -> bool { - if matches.contains_id("left_legend") { - return true; - } else if let Some(flags) = &config.flags { - if let Some(left_legend) = flags.left_legend { - return left_legend; - } - } - - false -} - -fn get_use_current_cpu_total(matches: &ArgMatches, config: &Config) -> bool { - if matches.contains_id("current_usage") { - return true; - } else if let Some(flags) = &config.flags { - if let Some(current_usage) = flags.current_usage { - return current_usage; - } - } - - false -} - -fn get_unnormalized_cpu(matches: &ArgMatches, config: &Config) -> bool { - if matches.contains_id("unnormalized_cpu") { - return true; - } else if let Some(flags) = &config.flags { - if let Some(unnormalized_cpu) = flags.unnormalized_cpu { - return unnormalized_cpu; - } - } - - false -} - -fn get_use_basic_mode(matches: &ArgMatches, config: &Config) -> bool { - if matches.contains_id("basic") { - return true; - } else if let Some(flags) = &config.flags { - if let Some(basic) = flags.basic { - return basic; - } - } - - false -} - /// FIXME: Let this accept human times. fn get_default_time_value( matches: &ArgMatches, config: &Config, retention_ms: u64, @@ -689,82 +622,6 @@ fn get_time_interval( Ok(time_interval) } -pub fn get_app_grouping(matches: &ArgMatches, config: &Config) -> bool { - if matches.contains_id("group") { - return true; - } else if let Some(flags) = &config.flags { - if let Some(grouping) = flags.group_processes { - return grouping; - } - } - false -} - -pub fn get_app_case_sensitive(matches: &ArgMatches, config: &Config) -> bool { - if matches.contains_id("case_sensitive") { - return true; - } else if let Some(flags) = &config.flags { - if let Some(case_sensitive) = flags.case_sensitive { - return case_sensitive; - } - } - false -} - -pub fn get_app_match_whole_word(matches: &ArgMatches, config: &Config) -> bool { - if matches.contains_id("whole_word") { - return true; - } else if let Some(flags) = &config.flags { - if let Some(whole_word) = flags.whole_word { - return whole_word; - } - } - false -} - -pub fn get_app_use_regex(matches: &ArgMatches, config: &Config) -> bool { - if matches.contains_id("regex") { - return true; - } else if let Some(flags) = &config.flags { - if let Some(regex) = flags.regex { - return regex; - } - } - false -} - -fn get_hide_time(matches: &ArgMatches, config: &Config) -> bool { - if matches.contains_id("hide_time") { - return true; - } else if let Some(flags) = &config.flags { - if let Some(hide_time) = flags.hide_time { - return hide_time; - } - } - false -} - -fn get_autohide_time(matches: &ArgMatches, config: &Config) -> bool { - if matches.contains_id("autohide_time") { - return true; - } else if let Some(flags) = &config.flags { - if let Some(autohide_time) = flags.autohide_time { - return autohide_time; - } - } - - false -} - -fn get_expanded_on_startup(matches: &ArgMatches, config: &Config) -> bool { - matches.contains_id("expanded_on_startup") - || config - .flags - .as_ref() - .and_then(|x| x.expanded_on_startup) - .unwrap_or(false) -} - fn get_default_widget_and_count( matches: &ArgMatches, config: &Config, ) -> error::Result<(Option, u64)> { @@ -816,50 +673,18 @@ fn get_default_widget_and_count( } } -fn get_disable_click(matches: &ArgMatches, config: &Config) -> bool { - if matches.contains_id("disable_click") { - return true; - } else if let Some(flags) = &config.flags { - if let Some(disable_click) = flags.disable_click { - return disable_click; - } - } - false -} - -fn get_use_old_network_legend(matches: &ArgMatches, config: &Config) -> bool { - if matches.contains_id("use_old_network_legend") { - return true; - } else if let Some(flags) = &config.flags { - if let Some(use_old_network_legend) = flags.use_old_network_legend { - return use_old_network_legend; - } - } - false -} - -fn get_hide_table_gap(matches: &ArgMatches, config: &Config) -> bool { - if matches.contains_id("hide_table_gap") { - return true; - } else if let Some(flags) = &config.flags { - if let Some(hide_table_gap) = flags.hide_table_gap { - return hide_table_gap; - } - } - false -} - +#[allow(unused_variables)] fn get_use_battery(matches: &ArgMatches, config: &Config) -> bool { #[cfg(feature = "battery")] - if let Ok(battery_manager) = Manager::new() { - if let Ok(batteries) = battery_manager.batteries() { - if batteries.count() == 0 { - return false; + { + if let Ok(battery_manager) = Manager::new() { + if let Ok(batteries) = battery_manager.batteries() { + if batteries.count() == 0 { + return false; + } } } - } - if cfg!(feature = "battery") { if matches.contains_id("battery") { return true; } else if let Some(flags) = &config.flags { @@ -868,11 +693,14 @@ fn get_use_battery(matches: &ArgMatches, config: &Config) -> bool { } } } + false } +#[allow(unused_variables)] fn get_enable_gpu_memory(matches: &ArgMatches, config: &Config) -> bool { - if cfg!(feature = "gpu") { + #[cfg(feature = "gpu")] + { if matches.contains_id("enable_gpu_memory") { return true; } else if let Some(flags) = &config.flags { @@ -881,18 +709,7 @@ fn get_enable_gpu_memory(matches: &ArgMatches, config: &Config) -> bool { } } } - false -} -#[allow(dead_code)] -fn get_no_write(matches: &ArgMatches, config: &Config) -> bool { - if matches.contains_id("no_write") { - return true; - } else if let Some(flags) = &config.flags { - if let Some(no_write) = flags.no_write { - return no_write; - } - } false } @@ -958,61 +775,6 @@ pub fn get_color_scheme(matches: &ArgMatches, config: &Config) -> error::Result< Ok(ColourScheme::Default) } -fn get_mem_as_value(matches: &ArgMatches, config: &Config) -> bool { - if matches.contains_id("mem_as_value") { - return true; - } else if let Some(flags) = &config.flags { - if let Some(mem_as_value) = flags.mem_as_value { - return mem_as_value; - } - } - false -} - -fn get_is_default_tree(matches: &ArgMatches, config: &Config) -> bool { - if matches.contains_id("tree") { - return true; - } else if let Some(flags) = &config.flags { - if let Some(tree) = flags.tree { - return tree; - } - } - false -} - -fn get_show_table_scroll_position(matches: &ArgMatches, config: &Config) -> bool { - if matches.contains_id("show_table_scroll_position") { - return true; - } else if let Some(flags) = &config.flags { - if let Some(show_table_scroll_position) = flags.show_table_scroll_position { - return show_table_scroll_position; - } - } - false -} - -fn get_is_default_process_command(matches: &ArgMatches, config: &Config) -> bool { - if matches.contains_id("process_command") { - return true; - } else if let Some(flags) = &config.flags { - if let Some(process_command) = flags.process_command { - return process_command; - } - } - false -} - -fn get_is_advanced_kill_disabled(matches: &ArgMatches, config: &Config) -> bool { - if matches.contains_id("disable_advanced_kill") { - return true; - } else if let Some(flags) = &config.flags { - if let Some(disable_advanced_kill) = flags.disable_advanced_kill { - return disable_advanced_kill; - } - } - false -} - fn get_network_unit_type(matches: &ArgMatches, config: &Config) -> DataUnit { if matches.contains_id("network_use_bytes") { return DataUnit::Byte; @@ -1041,17 +803,6 @@ fn get_network_scale_type(matches: &ArgMatches, config: &Config) -> AxisScaling AxisScaling::Linear } -fn get_network_use_binary_prefix(matches: &ArgMatches, config: &Config) -> bool { - if matches.contains_id("network_use_binary_prefix") { - return true; - } else if let Some(flags) = &config.flags { - if let Some(network_use_binary_prefix) = flags.network_use_binary_prefix { - return network_use_binary_prefix; - } - } - false -} - fn get_retention_ms(matches: &ArgMatches, config: &Config) -> error::Result { const DEFAULT_RETENTION_MS: u64 = 600 * 1000; // Keep 10 minutes of data. @@ -1069,3 +820,71 @@ fn get_retention_ms(matches: &ArgMatches, config: &Config) -> error::Result Ok(DEFAULT_RETENTION_MS) } } + +#[cfg(test)] +mod test { + + use clap::ArgMatches; + + use crate::{app::App, canvas::canvas_styling::CanvasColours}; + + use super::{get_color_scheme, get_widget_layout, Config}; + + fn create_app(mut config: Config, matches: ArgMatches) -> App { + let (layout, id, ty) = get_widget_layout(&matches, &config).unwrap(); + let colours = + CanvasColours::new(get_color_scheme(&matches, &config).unwrap(), &config).unwrap(); + + super::build_app(&matches, &mut config, &layout, id, &ty, &colours).unwrap() + } + + // TODO: There's probably a better way to create clap options AND unify together to avoid the possibility of + // typos/mixing up. Use macros! + #[test] + fn verify_cli_options_build() { + let app = crate::clap::build_app(); + + let default_app = { + let app = app.clone(); + let config = Config::default(); + let matches = app.get_matches_from([""]); + + create_app(config, matches) + }; + + // Skip battery since it's tricky to test depending on the platform testing. + let skip = ["help", "version", "celsius", "battery"]; + + for arg in app.get_arguments().collect::>() { + let arg_name = arg + .get_long_and_visible_aliases() + .unwrap() + .first() + .unwrap() + .to_owned(); + + if !arg.is_takes_value_set() && !skip.contains(&arg_name) { + let arg = format!("--{arg_name}"); + + let arguments = vec!["btm", &arg]; + let app = app.clone(); + let config = Config::default(); + let matches = app.get_matches_from(arguments); + + let testing_app = create_app(config, matches); + + if (default_app.app_config_fields == testing_app.app_config_fields) + && default_app.is_expanded == testing_app.is_expanded + && default_app + .proc_state + .widget_states + .iter() + .zip(testing_app.proc_state.widget_states.iter()) + .all(|(a, b)| (a.1.test_equality(b.1))) + { + panic!("failed on {arg_name}"); + } + } + } + } +} diff --git a/src/units/data_units.rs b/src/units/data_units.rs index 5f7092bd..ff9e8ebb 100644 --- a/src/units/data_units.rs +++ b/src/units/data_units.rs @@ -1,4 +1,4 @@ -#[derive(Debug, Clone)] +#[derive(Debug, Clone, Eq, PartialEq)] pub enum DataUnit { Byte, Bit, diff --git a/src/widgets/process_table.rs b/src/widgets/process_table.rs index c5871848..12af6bc3 100644 --- a/src/widgets/process_table.rs +++ b/src/widgets/process_table.rs @@ -61,7 +61,7 @@ impl ProcessSearchState { } } -#[derive(Clone, Debug)] +#[derive(Clone, Debug, Eq, PartialEq)] pub enum ProcWidgetMode { Tree { collapsed_pids: FxHashSet }, Grouped, @@ -814,6 +814,26 @@ impl ProcWidgetState { self.is_sort_open = false; self.force_rerender_and_update(); } + + #[cfg(test)] + pub(crate) fn test_equality(&self, other: &Self) -> bool { + self.mode == other.mode + && self.proc_search.is_ignoring_case == other.proc_search.is_ignoring_case + && self.proc_search.is_searching_whole_word == other.proc_search.is_searching_whole_word + && self.proc_search.is_searching_with_regex == other.proc_search.is_searching_with_regex + && self + .table + .columns + .iter() + .map(|c| c.header()) + .collect::>() + == other + .table + .columns + .iter() + .map(|c| c.header()) + .collect::>() + } } #[inline] -- cgit v1.2.3