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.

Sign up for GitHub

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

Jump to bottom

Inline Temperature #111

Merged
merged 1 commit into from
Nov 17, 2020
Merged

Inline Temperature #111

merged 1 commit into from
Nov 17, 2020

Conversation

cgzones
Copy link
Member

@cgzones cgzones commented Sep 10, 2020 •
edited
Loading

Alternative implementation of #93

Show the temperature inline as the CPU frequency.

temp1
temp2
temp3

@cgzones cgzones marked this pull request as ready for review September 11, 2020 10:55
@cgzones cgzones mentioned this pull request Sep 11, 2020
Closed
BenBE
BenBE reviewed Sep 11, 2020
View reviewed changes
CPUMeter.c Outdated Show resolved Hide resolved
BenBE
BenBE reviewed Sep 11, 2020
View reviewed changes
CPUMeter.c Outdated Show resolved Hide resolved
BenBE
BenBE reviewed Sep 11, 2020
View reviewed changes
Settings.c Outdated
this->updateProcessNames = false;
this->cpuCount = cpuCount;
this->showProgramPath = true;
this->highlightThreads = true;
this->degreeFahrenheit = false;
Copy link
Member

Choose a reason for hiding this comment

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

What about display in Kelvin? /joke

@cgzones cgzones added the RFE label Sep 12, 2020
@BenBE
Copy link
Member

BenBE commented Sep 13, 2020

Can you skip the blank after °C (before the closing ] in the bar view)?

If the temperature is usually integer degrees I'd suggest dropping the fraction part in the display.

BenBE
BenBE reviewed Sep 15, 2020
View reviewed changes
Copy link
Member

@BenBE BenBE left a comment

Choose a reason for hiding this comment

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

Patch LGTM.

@cgzones
Copy link
Member Author

cgzones commented Sep 15, 2020

Can you skip the blank after °C (before the closing ] in the bar view)?

That's not that simple. The ° character is not an ASCII 7-bit character and somehow the length computation changes.
So for now I think the bar is more readable with the blank than with omitting the degree sign (40C).

If the temperature is usually integer degrees I'd suggest dropping the fraction part in the display.

Changed the format to %d (Celsius) and %3d (Fahrenheit).

@BenBE
Copy link
Member

BenBE commented Sep 15, 2020

Why not %3d for both? Making them both align right.

@cgzones
Copy link
Member Author

cgzones commented Sep 15, 2020

Why not %3d for both? Making them both align right.

Cause degree Celsius are normally two digit numbers.

@BenBE
Copy link
Member

BenBE commented Sep 15, 2020

Why not %3d for both? Making them both align right.

Cause degree Celsius are normally two digit numbers.

Asking the other way around: Why than bother with making Fahrenheit values 3 digits wide, when they usually are 3 digits anyway?

Just asking for why introducing the difference of %d°C vs. %3dF when both should produce valid results with %d (probably better in our case) or %3d (if we wanted to enforce alignment to the right).

@cgzones
Copy link
Member Author

cgzones commented Sep 15, 2020

Asking the other way around: Why than bother with making Fahrenheit values 3 digits wide, when they usually are 3 digits anyway?

99°F is ~37°C, so I'd like to have a fixed position if the value hovers around that.

@DX37
Copy link

DX37 commented Sep 16, 2020

Since on systems with Hyper-Threading libsensors shows temperature of only physical cores, inline temperature in CPU meters will look like this:
изображение

@BenBE
Copy link
Member

BenBE commented Sep 16, 2020

How much work would it be to get the logical and physical cores linked; i.e. duplicate the phys core 0 temp also shown for the HT core of that?

Having half the values display as N/A looks strange.

@DX37
Copy link

DX37 commented Sep 16, 2020

What about this implementation, which has triggered this pull request to happen, @BenBE?

@DX37 DX37 mentioned this pull request Sep 17, 2020
Closed
@cgzones
Copy link
Member Author

cgzones commented Sep 18, 2020

Needs to be refactored for SMT/HT and multi-socket systems.

@cgzones cgzones marked this pull request as draft September 18, 2020 10:49
@BenBE
Copy link
Member

BenBE commented Sep 25, 2020

I'd somehow prefer the order Temp Freq Load in the display of the meter asthis causes the least change towards previous versions that only had the Load in the meter. This shouldn't cause too many issues regarding the °C issue either as there's a natural space guaranteed in that case.

Any other opinions?

@cgzones cgzones force-pushed the temperature_v2 branch 4 times, most recently from c445a5a to 74abb86 Compare September 28, 2020 16:25
@cgzones cgzones force-pushed the temperature_v2 branch 2 times, most recently from 603cd19 to d6aa2fa Compare September 29, 2020 09:07
@cgzones cgzones marked this pull request as ready for review October 3, 2020 17:29
fasterit
fasterit reviewed Oct 5, 2020
View reviewed changes
CRT.c Outdated
#ifdef HAVE_LIBNCURSESW
if (CRT_utf8)
return "\xc2\xb0";
#endif
Copy link
Member

Choose a reason for hiding this comment

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

With libcurses you can still use ° as it is within most non-UTF8 charsets (ISO-8859-x), even CP437, have it at 0x90.

Copy link
Member Author

Choose a reason for hiding this comment

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

@fasterit can you take a look at the added commit

@cgzones cgzones mentioned this pull request Oct 9, 2020
Closed
@cgzones cgzones force-pushed the temperature_v2 branch 2 times, most recently from 801b51b to 009f616 Compare October 16, 2020 18:03
@fasterit fasterit added the needs-rebase Pull request needs to be rebased and conflicts to be resolved label Oct 20, 2020
@fasterit
Copy link
Member

Without --enable-sensors the option in setup should not be available as it will always be "N/A".

@fasterit fasterit removed the needs-rebase Pull request needs to be rebased and conflicts to be resolved label Oct 20, 2020
@cgzones cgzones force-pushed the temperature_v2 branch 5 times, most recently from d02d891 to 548a32c Compare October 26, 2020 18:49
@cgzones cgzones requested a review from fasterit October 26, 2020 18:49
BenBE
BenBE reviewed Oct 26, 2020
View reviewed changes
CRT.c
#endif

static char buffer[4];
int r = snprintf(buffer, sizeof(buffer), "%lc", 176);
Copy link
Member

Choose a reason for hiding this comment

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

xSnprintf?

Copy link
Member Author

Choose a reason for hiding this comment

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

no (maybe i'll add a comment): this might actually fail on platforms not supporting wide characters, e.g. with POSIX locale (occurred on a raspberry pi)

Copy link
Member

Choose a reason for hiding this comment

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

I feared something like that. ;-) Comment will be appreciated.

@cgzones cgzones force-pushed the temperature_v2 branch 2 times, most recently from b1cdc27 to 1822aad Compare October 31, 2020 18:53
@cgzones
Show CPU temperature in CPU meter
1b225cd 
Show the CPU temperature in the CPU meter, like CPU frequency, instead
of using an extra Meter.
@fasterit fasterit closed this in fec9af4 Nov 17, 2020
@fasterit fasterit merged commit fec9af4 into htop-dev:master Nov 17, 2020
@cgzones cgzones deleted the temperature_v2 branch November 17, 2020 12:25
@aurelijusrozenas aurelijusrozenas mentioned this pull request Dec 2, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BenBE BenBE BenBE left review comments

@fasterit fasterit Awaiting requested review from fasterit

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cgzones @BenBE @DX37 @fasterit