Skip to content
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

feature: show hostname in title #999

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

RaresCon
Copy link
Contributor

@RaresCon RaresCon commented Jan 23, 2023

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.

Windows example

WSL/Ubuntu example

Arch example

Option example

Hostname example

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)

RaresCon and others added 8 commits January 11, 2023 14:59
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.
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
Copy link
Owner

ClementTsang commented Jan 23, 2023

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
Copy link
Contributor Author

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
Copy link
Owner

ClementTsang commented Jan 23, 2023

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
Copy link
Contributor Author

RaresCon commented Jan 23, 2023

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
Copy link

codecov-commenter commented Jan 23, 2023

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
Copy link
Owner

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

@RaresCon
Copy link
Contributor Author

RaresCon commented Jan 24, 2023

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
Copy link
Owner

ClementTsang commented Jan 24, 2023

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
Copy link
Contributor Author

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 ClementTsang changed the title Custom terminal title feature: custom terminal title Jan 24, 2023
Copy link
Owner

@ClementTsang ClementTsang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/clap.rs Outdated
Comment on lines 341 to 346
let title = Arg::new("title")
.long("title")
.takes_value(true)
.value_name("Title")
.help("Sets the title of the current terminal.");

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
src/options.rs Outdated
Comment on lines 737 to 756
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(""))
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

RaresCon commented Feb 17, 2023

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

Copy link
Owner

@ClementTsang ClementTsang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: rename to match the option.

@@ -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>,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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") {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@@ -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() {
Copy link
Owner

@ClementTsang ClementTsang Feb 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}

@@ -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> {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ClementTsang ClementTsang changed the title feature: custom terminal title feature: show hostname in title Feb 18, 2023
@ClementTsang ClementTsang self-assigned this Mar 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants