feature: show hostname in title #999

RaresCon · 2023-01-23T16:49:30Z

Description

A description of the change, what it does, and why it was made. If relevant (such as any change that modifies the UI), please provide screenshots of the changes:

Added an option to change the terminal name. The user can use --title, followed by the desired name for the terminal, or use title_to_hostname field (true/false) in the config file to force the name of the terminal to be the name of the host machine. Otherwise, if no option is used, the default name of the terminal will be now "Bottom". It also works while using SSH.

This PR was also discussed at:

UPB-CS-OpenSourceUpstream#4

Issue

If applicable, what issue does this address?

#277

Testing

If relevant, please state how this was tested. All changes must be tested to work:

Tested it on multiple platforms. I also used SSH on all of them.

If this is a code change, please also indicate which platforms were tested:

Windows
macOS
Linux

Checklist

If relevant, ensure the following have been met:

Areas your change affects have been linted using rustfmt (cargo fmt)
The change has been tested and doesn't appear to cause any unintended breakage
Documentation has been added/updated if needed (README.md, help menu, doc pages, etc.)
The pull request passes the provided CI pipeline
There are no merge conflicts
If relevant, new tests were added (don't worry too much about coverage)

For bottom to know that there are no batteries on the system, I added the battery::Manager to the options.rs file because here is the first moment bottom verifies battery configuration by reading the config file, which may or may not contain the battery field, but for a better UX, it doesn't matter what bottom finds in the config file now, if it doesn't retrieve battery data, it just ignores the battery widget all together. If needed, it can be adjusted so that if the config file contains the battery field, it will still show the widget.

I guarded the options.rs in two places for battery module that can be missing in the feature list.

Dynamic battery widget

Revert "Dynamic battery widget"

I added a new option in terminal and in the config file to be able to change the terminal title with any custom one. The user can now add `--title` and the custom title after it or add the `title_to_hostname` field in the config file to set the terminal's name to hostname. If there is no option found, then the name of the terminal will be set to "Bottom".

ClementTsang · 2023-01-23T17:17:27Z

Hmmm... My first impression is that I'm not a fan of changing the default behaviour. I would personally rather it do nothing if nothing is set.

Also while it does make sense to have a command-line flag for setting the title to a custom string, if we do keep that, I'm surprised there isn't a config file equivalent. What if I want to permanently set it? (EDIT: That said, I'm not even sure if I want this part, as it seems a bit unnecessary IMO.)

As for setting to grab the hostname - does it make sense to just set it as the hostname? Or should we still label what the application is (e.g. btm (PiCluster52))? I'm not sure here, just spitballing.

Would also be nice to clean up that merge history a bit (why does it have commits from the dynamic battery PR?), but that's a nitpick and I'll likely just squash.

RaresCon · 2023-01-23T17:25:17Z

I will add a config file option for setting the title and also remove the "Bottom" standard name if nothing is set.

The idea about including the name of Bottom along the hostname is a good one, didn't think of it.

After that, if everything is ok, I can squash the commits.

ClementTsang · 2023-01-23T17:28:53Z

Just to save you the effort, I added an edit to my earlier comment, but you can hold off on adding the option for setting the title in the config if you want - I'm not sure if I want to add that feature in general at the moment.

RaresCon · 2023-01-23T17:55:36Z

Just to save you the effort, I added an edit to my earlier comment, but you can hold off on adding the option for setting the title in the config if you want - I'm not sure if I want to add that feature in general at the moment.

I will not add that then, but I will rework the feature a bit to match your ideas.

In case the title is set and there is an error because of gethostname (as in the hostname can not be retrieved) or if the user doesn't use the command-line flag correctly, should I set the name of the terminal to some error message? I ask this because the current function can not set the name to the old name of the terminal (the path in this case) if an error happens during its execution.

EDIT: I will just use a tuple to return the name and a boolean if there were no errors.

the name of the terminal, it will be set as before to its path. This behaviour will happen when an error happens inside the `get_use_terminal_name` function as well.

codecov-commenter · 2023-01-23T18:30:23Z

Codecov Report

Base: 20.71% // Head: 20.74% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (2c3811b) compared to base (00d60d8).
Patch coverage: 59.45% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #999      +/-   ##
==========================================
+ Coverage   20.71%   20.74%   +0.03%     
==========================================
  Files          75       75              
  Lines       14680    14706      +26     
==========================================
+ Hits         3041     3051      +10     
- Misses      11639    11655      +16

Impacted Files	Coverage Δ
src/lib.rs	`6.21% <ø> (ø)`
src/bin/main.rs	`37.01% <40.00%> (+0.43%)`	⬆️
src/options.rs	`41.14% <56.45%> (-0.73%)`	⬇️
src/clap.rs	`100.00% <100.00%> (ø)`
src/app/query.rs	`0.00% <0.00%> (ø)`
src/app/data_harvester.rs	`0.00% <0.00%> (ø)`
src/app/data_harvester/processes.rs	`0.00% <0.00%> (ø)`
src/widgets/process_table/proc_widget_column.rs	`13.92% <0.00%> (ø)`
src/widgets/process_table/proc_widget_data.rs	`9.58% <0.00%> (+0.06%)`	⬆️
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

ClementTsang · 2023-01-24T00:30:34Z

Just a heads up, your CI is probably failing because your Cargo.lock is... kinda weird.

RaresCon · 2023-01-24T09:18:16Z

Just a heads up, your CI is probably failing because your Cargo.lock is... kinda weird.

I've added the gethostname crate and it needs windows crate, version 0.4.3, but the original version in Cargo.lock is 0.4.4. Is this the weird thing about it? (I'm quite new to how Cargo.toml/Cargo.lock work, so any advice would be appreciated)

Also, CI runs with --locked, so I guess this is the reason they fail.

ClementTsang · 2023-01-24T14:14:30Z

If you just build normally on your machine, Cargo.lock should update automatically and you should commit that. The current one you have seems like it's manually edited or something? If I switch to your PR Branch and build, I immediately get a diff there regarding features and version.

I use --locked in CI because I expect a properly updated lock file to be committed alongside whenever Cargo.toml is updated.

RaresCon · 2023-01-24T16:36:18Z

I've rebuilt the project, thank you for explaining that to me!

I resolved some conflict with master manually before (mistakes were made, hehe).

ClementTsang

Sorry for taking so long, have some suggestions/fixes required.

ClementTsang · 2023-02-05T02:11:13Z

src/clap.rs

+    let title = Arg::new("title")
+        .long("title")
+        .takes_value(true)
+        .value_name("Title")
+        .help("Sets the title of the current terminal.");
+


I thought we were just doing the hostname idea and putting the custom title part aside for now. If so, change this to match the config option, and not take a value.

src/options.rs

ClementTsang · 2023-02-05T02:15:40Z

src/options.rs

+pub fn get_use_terminal_name(matches: &ArgMatches, config: &Config) -> (bool, String) {
+    if matches.is_present("title") {
+        return if let Some(custom_name) = matches.value_of("title") {
+            (true, String::from(custom_name))
+        } else {
+            (false, String::from(""))
+        };
+    } else if let Some(flags) = &config.flags {
+        if let Some(title_to_hostname) = flags.title_to_hostname {
+            if title_to_hostname {
+                return match gethostname().into_string() {
+                    Ok(hostname) => (title_to_hostname, format!("btm ({})", hostname)),
+                    Err(_) => (false, String::from("")),
+                };
+            }
+        }
+    }
+    (false, String::from(""))
+}
+


Some suggestions:

As mentioned above, remove the custom name part, as we're not doing that for now. Just use the hostname.

Why not just return an Option? e.g. if there is no change, return None.

I removed the custom title option for terminal and made it use the hostname when `--title` is used. The title always contains `btm` now. If `--title` or `title_has_hostname = true` (in config file) is not present, then the terminal has the previous behaviour.

RaresCon · 2023-02-17T18:20:23Z

Sorry for this late commit, I will resolve the deprecated is_present and value_of method as fast as possible.

ClementTsang

Some more nitpicks. As mentioned in one of them, it would be nice if you rebase to the latest master branch since most of the clap-related changes you made shouldn't be necessary.

ClementTsang · 2023-02-18T04:32:36Z

src/clap.rs

+    let title = Arg::new("title")
+        .long("title")
+        .help("Sets the title of the current terminal to \"btm ($hostname)\".");
+


nit: rename to match the option.

ClementTsang · 2023-02-18T04:33:23Z

src/options.rs

@@ -92,6 +93,7 @@ pub struct ConfigFlags {
    pub network_use_log: Option<bool>,
    pub network_use_binary_prefix: Option<bool>,
    pub enable_gpu_memory: Option<bool>,
+    pub title_has_hostname: Option<bool>,


nit: Sorry for suggesting something else, but maybe title_shows_hostname? Change the config file and clap option as well.

ClementTsang · 2023-02-18T04:34:16Z

src/options.rs

@@ -498,7 +500,7 @@ pub fn get_widget_layout(
 }

 fn get_update_rate_in_milliseconds(matches: &ArgMatches, config: &Config) -> error::Result<u64> {
-    let update_rate_in_milliseconds = if let Some(update_rate) = matches.value_of("rate") {
+    let update_rate_in_milliseconds = if let Some(update_rate) = matches.get_one::<String>("rate") {


Hm, wonder why it's showing that you had to make a change here (and the rest of this file for any clap issues). If you want, it might be nice to rebase and clean up the commit history (e.g. those weird battery commits).

ClementTsang · 2023-02-18T04:35:44Z

src/options.rs

@@ -732,8 +735,26 @@ pub fn get_app_use_regex(matches: &ArgMatches, config: &Config) -> bool {
    false
 }

+pub fn get_use_terminal_name(matches: &ArgMatches, config: &Config) -> Option<String> {
+    match gethostname().into_string() {


nit: IMO invert this - that is, check if the match or config have the corresponding flag first before running gethostname.

So maybe for example:

if matches.contains_id("title") || &config.flags.and_then(|f| f.title_has_hostname).unwrap_or(false) { match gethostname().into_string() { Ok(hostname) => Some(format!("btm ({hostname})")), Err(_) => None, } } else { None }

ClementTsang · 2023-02-18T04:39:13Z

src/options.rs

@@ -732,8 +735,26 @@ pub fn get_app_use_regex(matches: &ArgMatches, config: &Config) -> bool {
    false
 }

+pub fn get_use_terminal_name(matches: &ArgMatches, config: &Config) -> Option<String> {


Forgot to add - might want to rename this to something like get_show_hostname.

RaresCon and others added 8 commits January 11, 2023 14:59

CFG guarding for BATTERY module

41e1807

I guarded the options.rs in two places for battery module that can be missing in the feature list.

Merge pull request #1 from RaresCon/dynamic_battery_widget

519d35e

Dynamic battery widget

Revert "Dynamic battery widget"

0b8ff0e

Merge pull request #2 from RaresCon/revert-1-dynamic_battery_widget

71d0452

Revert "Dynamic battery widget"

Merge branch 'master' into custom_terminal_title

948ecb0

Merge branch 'master' into custom_terminal_title

bf21a16

RaresCon mentioned this pull request Jan 23, 2023

Custom terminal title for Bottom UPB-CS-OpenSourceUpstream/bottom#4

Closed

9 tasks

Reworked the feature, now if nothing is used to set

4d6c136

the name of the terminal, it will be set as before to its path. This behaviour will happen when an error happens inside the `get_use_terminal_name` function as well.

Rebuilt to get Cargo.lock generated again

8ced3d1

ClementTsang changed the title ~~Custom terminal title~~ feature: custom terminal title Jan 24, 2023

ClementTsang requested changes Feb 5, 2023

View reviewed changes

Resolved some deprecated methods for arg_matches

2c3811b

ClementTsang requested changes Feb 18, 2023

View reviewed changes

ClementTsang reviewed Feb 18, 2023

View reviewed changes

ClementTsang changed the title ~~feature: custom terminal title~~ feature: show hostname in title Feb 18, 2023

ClementTsang self-assigned this Mar 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: show hostname in title #999

feature: show hostname in title #999

RaresCon commented Jan 23, 2023 •

edited

ClementTsang commented Jan 23, 2023 •

edited

RaresCon commented Jan 23, 2023

ClementTsang commented Jan 23, 2023 •

edited

RaresCon commented Jan 23, 2023 •

edited

codecov-commenter commented Jan 23, 2023 •

edited by codecov bot

ClementTsang commented Jan 24, 2023

RaresCon commented Jan 24, 2023 •

edited

ClementTsang commented Jan 24, 2023 •

edited

RaresCon commented Jan 24, 2023

ClementTsang left a comment

ClementTsang Feb 5, 2023

ClementTsang Feb 5, 2023

RaresCon commented Feb 17, 2023 •

edited

ClementTsang left a comment

ClementTsang Feb 18, 2023

ClementTsang Feb 18, 2023

ClementTsang Feb 18, 2023

ClementTsang Feb 18, 2023 •

edited

ClementTsang Feb 18, 2023

feature: show hostname in title #999

Are you sure you want to change the base?

feature: show hostname in title #999

Conversation

RaresCon commented Jan 23, 2023 • edited

Description

Issue

Testing

Checklist

ClementTsang commented Jan 23, 2023 • edited

RaresCon commented Jan 23, 2023

ClementTsang commented Jan 23, 2023 • edited

RaresCon commented Jan 23, 2023 • edited

codecov-commenter commented Jan 23, 2023 • edited by codecov bot

Codecov Report

ClementTsang commented Jan 24, 2023

RaresCon commented Jan 24, 2023 • edited

ClementTsang commented Jan 24, 2023 • edited

RaresCon commented Jan 24, 2023

ClementTsang left a comment

Choose a reason for hiding this comment

ClementTsang Feb 5, 2023

Choose a reason for hiding this comment

ClementTsang Feb 5, 2023

Choose a reason for hiding this comment

RaresCon commented Feb 17, 2023 • edited

ClementTsang left a comment

Choose a reason for hiding this comment

ClementTsang Feb 18, 2023

Choose a reason for hiding this comment

ClementTsang Feb 18, 2023

Choose a reason for hiding this comment

ClementTsang Feb 18, 2023

Choose a reason for hiding this comment

ClementTsang Feb 18, 2023 • edited

Choose a reason for hiding this comment

ClementTsang Feb 18, 2023

Choose a reason for hiding this comment

RaresCon commented Jan 23, 2023 •

edited

ClementTsang commented Jan 23, 2023 •

edited

ClementTsang commented Jan 23, 2023 •

edited

RaresCon commented Jan 23, 2023 •

edited

codecov-commenter commented Jan 23, 2023 •

edited by codecov bot

RaresCon commented Jan 24, 2023 •

edited

ClementTsang commented Jan 24, 2023 •

edited

RaresCon commented Feb 17, 2023 •

edited

ClementTsang Feb 18, 2023 •

edited