search

codler / Battery-Time-Remaining

Show battery time remaining in Mac OS X 10.8+ Mountain Lion and Mavericks
http://yap.nu/battery-time-remaining/
Other
412 stars 72 forks source link

Updated look & feel #19

Closed c-alpha closed 12 years ago

c-alpha commented 12 years ago

Visible changes:

Under the hood:

c-alpha commented 12 years ago

Sorry for the confusion about the commits after the pull request. I had forgotten to change the limit value for drawing a red capacity bar back to 15% from my debugging value.

codler commented 12 years ago

There is some points I would like to say.

aside from that it was good improvements you made

c-alpha commented 12 years ago

First of all, thanks a lot for the quick review and kind comments!

I think paranthesis around the time are not needed

That was the way Apple's pre-ML battery gauge did it. I also felt that the parentheses helped me to visually discern the battery time from the clock, as it has the same composition of digits and the colon (":"). Which might be why Apple put them there in the first place? Your mileage may vary, of course. Maybe a candidate for a configurable option? I'll look into making it into one.

and also "Not charging"-text. When it is fully charged it will change battery icon (v1.5 have a bug which dont change the icon, but this is already fixed for next release). This will also save space in menubar which most people want.

Good point. In Apple's pre-ML gauge it would display the "not charging" for the couple of seconds it takes to switch the power source and put the battery to charging mode, when you connect the charger. Re-reading the code I realise that it will show "not charging" also when fully charged. To get the pre-ML behaviour, the FSM would need to be extended; like which I don't feel at the moment. So I rest my case re. the "not charging".

I saw you removed [image setTemplate:YES];. This should not be removed as it fixes white drop shadow. #5

Hm. With the setTemplate: call, the red capacity bar would be rendered dithered in grey. The documentation of setTemplate: hints to this:

(...) Images you mark as template images should consist of only black and clear colors. You can use the alpha channel in the image to adjust the opacity of black content, however. (...)

But maybe it's a consequence of how I compose the image. I'll look for a way of having a red capacity bar with setTemplate: in place.

The icon is more blocky in retina display. If you dont have retina display you can still test on your computer. [...]

Thanks for the pointer! I'll check this out.

codler commented 12 years ago

I agree that parenthesis in pre-ML might be to distinguish battery time and clock time, but I don't think it is needed in our case, because you can distinguish by:

I want to try keep the app as simple as possible for users.

What is FSM?

oh, I didn't notice setTemplate would make the red color grey. Thanks for pointing it out! this is a bug in 1.5.1 then.

c-alpha commented 12 years ago

Ok, I believe to have implemented all of your comments (for which thanks), except the setTemplate: issue. But see more below.

I agree that parenthesis in pre-ML might be to distinguish battery time and clock time, but I don't think it is needed in our case, because you can distinguish by: As third party app this battery time can only be on the left side of the menubar and the clock always on the right side. The font is smaller. I want to try keep the app as simple as possible for users.

Agreed. I made it a configurable option, and the default is "off".

What is FSM?

Finite state machine. See here. This is used to model the behaviour of the system. E.g. when you connect the charger, it goes from "charging" to "not charging" (the electronics takes a few seconds to switch power and start charging the battery), and then to "charging".

oh, I didn't notice setTemplate would make the red color grey. Thanks for pointing it out! this is a bug in 1.5.1 then.

Hm, not sure whether it's a bug for your repo. I changed the way the capacity bar is drawn, which may well affect the colour rendering. I will need to dig more to understand why this happens with setTemplate:.

codler commented 12 years ago

It looks good now :)

Just one thing. I get this in my log when I compile

2012-08-25 16:12:39.185 Battery Time Remaining[3830:303] It does not make sense to draw an image when [NSGraphicsContext currentContext] is nil. This is a programming error. Break on void _NSWarnForDrawingImageWithNoCurrentContext() to debug. This will be logged only once. This may break in the future.

c-alpha commented 12 years ago

It looks good now :)

Glad you like it. ;-))

Just one thing. I get this in my log when I compile [...]

I get the same message when I run it. But not always. I have googled for the error message both on the Interwebs, and the Apple developer forums. There are some others who have seen the same message in various contexts. It seems that on most occasions, the issue is linked to legacy code using lockFocus and unlockFocus being deployed on ML. Although this is officially supported (when using drawInRect:fromRect:operation:fraction:), it seems that there are situations where this doesn't play with the new, multi-threaded drawing architecture in ML. Plus we are in a special situation being an NSStatusItem where we don't have an NSView to get around said issues.

It seems that when I start/stop our app very often in a short time, the message is more likely to occur. After the message has shown up, if you inspect the graphics, you will find that there are rendering errors (e.g. a red segment used at the end of the capacity bar instead of a black one). Also if I wait a few minutes, the hit "Run" again, without touching anything else, it just works without the message.

So I suspect that it is rather an issue with the environment where our status bar item is hosted. And that we can do very little about it. I guess for NSStatusItems the resources are more limited than for general apps. So I wouldn't be surprised if it were for example not possible to have a video playing in a status bar image. ;-)

As for setTemplate it seems that it is was conceived for toolbar buttons (see here), which reduces my hopes that a template image would be capable of displaying colour. This seems to imply that to get the white shadow, and colour, we would have to roll our own drawEtchedInRect: method, juggling with masks etc.

codler commented 12 years ago

I c, ye I also only get that msg sometimes which makes it harder to debug.

I notice that the native battery icon don't have drop shadow when it shows the red color.

What do this line do? Is it necessary?

[[NSGraphicsContext currentContext] setImageInterpolation:NSImageInterpolationHigh]

I tested to remove that line and everything seem to still work.

codler commented 12 years ago

I found out why we got "_NSWarnForDrawingImageWithNoCurrentContext" in the log. It has to do with this code

NSDrawThreePartImage(NSMakeRect(capBarLeftOffset, capBarTopOffset, capBarLength, capBarHeight),
                        batteryLevelLeft, batteryLevelMiddle, batteryLevelRight,

When capBarLength is shorter than batteryLevelLeft+batteryLevelRight together it will leave no room to render batteryLevelMiddle, thats why we got the output in log. So it wasn't random when the output came, It only came when we had low battery percent left :P Ill fix this tomorrow.

codler commented 12 years ago

I refactor your code :P https://github.com/codler/Battery-Time-Remaining/commit/9102c77a9490c90a822e2c56761da46e6703aad2

c-alpha commented 12 years ago

1) Thanks a lot for pulling in my code! Glad you liked it.

2) Also thanks for the clean-up. I like it.

3) <Thumbs up!> for discovering the NSDrawThreePartImage() issue. I like your subtle fix, too. ;-))

4) I can confirm your observation of Apple's battery indicator not having the white shadow when in red state. I can also confirm, that even Apple are using setTemplate: themselves to get the white shadow. With a low battery level (say 5%), when you plug/unplug the charger, Apples battery icon is not updated during the "calculating..." phase. After you unplug the charger, they also immediately switch to the capacity bar, but it's grey (i.e. they apply setTemplate on the red capacity bar). Only after the "calculating..." state is left, Apple updates their icon (white shadow is removed and capacity bar becomes red). I have added a fe small updates to adopt this scheme, and even improve on it (capacity bar is rendered as red - when applicable - even during calculating state). See commit commit ff6259daf3 in my fork.