greshake / i3status-rust

Very resourcefriendly and feature-rich replacement for i3status, written in pure Rust
GNU General Public License v3.0
2.88k stars 475 forks source link

net block calls bitrate/SSID update function regardless of config #768

Closed ammgws closed 4 years ago

ammgws commented 4 years ago

Noticed this bug when trying to test https://github.com/greshake/i3status-rust/pull/758.

This is probably the culprit: https://github.com/greshake/i3status-rust/blob/0d39c9b876dee2d0e599cd18fa5e1d6cb47f5e1a/src/blocks/net.rs#L861-L870

The functions are called regardless of whether bitrate etc have been specified in the format string. This can cause the block to fail since you need certain commands installed for some of them - for example, iw is needed for bitrate but if you only want to display SSID you shouldn't be required to install it in order for the block work.

@gurditsbedi Sorry to bother you again but could you please have a look at this? It should be the last thing to fix with respect to the new format option. IIRC before it would check the existence of the widget before calling the update function, so that logic will need to be reworked to work with the new format option.

gurditsbedi commented 4 years ago

So from what I have understood. I need to stop calling self.update_bitrate() if bitrate is not present in a format string? Should I do same for self.update_ssid()?;, self.update_signal_strength()?;, self.update_ip_addr()?; ?

ammgws commented 4 years ago

Yes that should do it.

gurditsbedi commented 4 years ago

Considering the definition of the mentioned functions. AFAIU, Each function first check whether the value is Some. If and only if the value is Some, then it goes to execute the shell command to retrieve the value.

I don't think anything is needed to be done.

ammgws commented 4 years ago

On mobile now, but I think before it checked whether the Widget was Some or not, but now it's a string so it's always Some.

gurditsbedi commented 4 years ago

I missed that point. Will proceed with the above mentioned changes and send the fix soon.

On Thu, Jul 9, 2020, 1:54 PM Jason notifications@github.com wrote:

On mobile now, but I think before it checked whether the Widget was Some or not, but now it's a string so it's always Some.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/greshake/i3status-rust/issues/768#issuecomment-655985304, or unsubscribe https://github.com/notifications/unsubscribe-auth/AECEHLFFNPOBNNUTDXA4PYLR2V5ELANCNFSM4OVGXZJQ .

gurditsbedi commented 4 years ago

I wanted to do this similar to the way it was previous, by specifying Some/None in new method of impl ConfigBlock for Net for each option. Is there a way to check whether a option is present in format string? AFAIU, there is no method to check for.

Though I can do format.contains("{bitrate}") to check the options existence but, it will break soon after completion of https://github.com/greshake/i3status-rust/pull/750

Do you think adding such a method to check for option will be helpful?

ammgws commented 4 years ago

I just added a quick fix for now.

Do you think adding such a method to check for option will be helpful?

Seems like we need one to go with #750