Skip to content

Fix fan speed for T2 MacBooks #1

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

mrousavy
Copy link

On newer MacBooks (T2 chips) the fan info was throwing an error. Also, I've put the temps in the front and sorted by them.

Screenshot 2020-05-12 at 21 48 18

@mrousavy
Copy link
Author

mrousavy commented May 13, 2020

I also aligned the descriptions/labels left and values on the right. Personal preference which is better, but I think it's easier to read. Also since it's sorted from highest temp to lowest, the most important information is easily readable (because it's first, and on the far right)

Demo:
demo

Currently I hardcoded the menu width to 250, don't know if there is a better way to do this.

Maybe apply text styles in a separate loop after creating the labels, and just use the menu.size.width (which now will be the width of the longest text, without spaces) and add some extra width (~20?) to create spacing for the longest text.

See: 15db3fa#diff-95951b3083d0f544bd9afb833c854babR46-R51

@aisk
Copy link
Owner

aisk commented May 13, 2020

@mrousavy Great job!

But I'm a little busy these days, I'll review it this weekend, sorry!

@aisk aisk self-requested a review May 13, 2020 16:34
for item in data.items {
let m = NSMenuItem()
m.title = "\(item.0) \(item.1)"

Copy link
Owner

Choose a reason for hiding this comment

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

Looks like some of the lines which you committed is indent by tab, I think we should using space instead. Otherwise the codes looks wired like in this PR now.

Copy link
Owner

Choose a reason for hiding this comment

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

You can configure your XCode to using four space as you tap the tab key.

README.md Outdated
@@ -21,3 +40,4 @@ MIT lisence with exceptions:
- The app icon is designed by [nervouna](https://github.com/nervouna) under MIT license.
- The menu bar icon is copied from Google's Material Design under Apache 2.0 license.
- The SMC.swift is copied from [SMCKit project](https://github.com/beltex/SMCKit) under [MIT license](https://github.com/beltex/SMCKit/blob/master/LICENSE).
- Forked and adjusted by [mrousavy](https://github.com/mrousavy)
Copy link
Owner

Choose a reason for hiding this comment

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

Hi I'd like to add any contributors name in the README file, but this section is for the codes (or assets) which is copied from other project's LICENSE, so I think this change here is not suitable.

I think we can add another section to show contributor's name.

And I have another project, is using a service to the contributor's name like this: https://github.com/aisk/rust-memcache/blob/master/README.md#contributors , I think you can take a look it's source code and copy it here.

@@ -6,6 +6,9 @@
//
// Copyright (C) 2014-2017 beltex <https://beltex.github.io>
//
// Updated to work with T2 MacBooks using code from
Copy link
Owner

Choose a reason for hiding this comment

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

These three lines shoule put below next section.

@aisk
Copy link
Owner

aisk commented May 15, 2020

Hi @mrousavy I like you changes but I think there are some minor changes need to resolve which I have commented. Greet job, many thanks!

@mrousavy
Copy link
Author

Oh, my bad! Didn't notice Xcode wasn't using spaces, should've double checked that.

Also, the README changes shouldn't be in here as well, will remove them right away.

@aisk
Copy link
Owner

aisk commented May 15, 2020

LGTM, I'll try to test the changes on my machine, wait a minutes.

@aisk
Copy link
Owner

aisk commented May 15, 2020

Looks like the fan speed does not worked my my machine (MacBook Pro Mid 2015, no T2 chips):

image

@mrousavy
Copy link
Author

Hmm that's weird, maybe we should check for device type and then decide between the methods to use fan speed, legacy on non T2 chips and the new one on T2 chips..
unfortunately I don't have a device to test it..

@aisk
Copy link
Owner

aisk commented May 15, 2020

Good idea, I'll have a try tomorrow.

@aisk
Copy link
Owner

aisk commented Jul 5, 2020

@mrousavy Hi readly sorry for the late of reply. I just took a look at the original PR from SMCKit, and found they have a do ... catch mechanism, for read the data as it is T2 chip model and then fallback to read data like before.

I copied the change and it works fine. I think in this PR. we can copy the total SCMKit.swift file from the original PR from here: https://github.com/beltex/SMCKit/blob/ff231124ef2c4e7924cefa388bbca8d43c1fcb63/SMCKit/SMC.swift /

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.

2 participants