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

Add CPU usage for every core #1250

Merged
merged 3 commits into from
Sep 18, 2021
Merged

Add CPU usage for every core #1250

merged 3 commits into from
Sep 18, 2021

Conversation

Darkclainer
Copy link
Contributor

@Darkclainer Darkclainer commented Sep 17, 2021

This MR adds usage and icon for every CPU core. Use in format like {usage0}, {usage1} and etc. You can build nice graph for CPU with it:

image

P.S.
Sorry, I haven't write in C++ for years, so code can very bad.

UPD:
Found that there is relevant feature request: #746

@Alexays
Copy link
Owner

Alexays commented Sep 17, 2021

Very nice!
Can you update man? :)

@Darkclainer
Copy link
Contributor Author

Very nice!
Can you update man? :)

Done.

@Darkclainer
Copy link
Contributor Author

Darkclainer commented Sep 17, 2021

Also, I have found eerie comment:

// TODO: as creating dynamic fmt::arg arrays is buggy we have to calc both

@Alexays, do you know what it was about?

@Alexays
Copy link
Owner

Alexays commented Sep 18, 2021

Also, I have found eerie comment:

// TODO: as creating dynamic fmt::arg arrays is buggy we have to calc both

@Alexays, do you know what it was about?

It was for an older version of fmt I think.
It should be ok now.

@Alexays
Copy link
Owner

Alexays commented Sep 18, 2021

Thanks!

@Alexays Alexays merged commit 8489646 into Alexays:master Sep 18, 2021
@Alexays
Copy link
Owner

Alexays commented Sep 18, 2021

Rollbacked as CI failed :/

@Darkclainer
Copy link
Contributor Author

Rollbacked as CI failed :/

It didn't build on Fedora. I've checked tests of fmt lib and including fmt/args.h seems legit (https://github.com/fmtlib/fmt/blob/master/test/args-test.cc#L8). The problem seems to be, that waybar uses system's version of fmt.

I can try to create PR that will add fmt as dependency. Will you merge such PR, @Alexays?

@alebastr
Copy link
Contributor

It didn't built as Fedora (and a few more distributions according to repology) ships fmt 7.x in the stable releases. I'm pretty sure that if CI had reached Debian it would fail in the same way.

As a Fedora maintainer of waybar package, I'd prefer if we can keep compatibility with older fmt. There are policies that discourage us from having third party libraries bundled in the package source, so I'll have to deal with reverting the change downstream in Fedora.

As far as I can see, 7.x has required classes in the <fmt/core.h>. Maybe conditionally checking the fmt version and including the correct file for the installed fmt version would work for you (same way as it's already done around a few breaking fmt changes)?

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.

3 participants