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

Updated look & feel #19

Merged
merged 5 commits into from
Aug 25, 2012
Merged

Updated look & feel #19

merged 5 commits into from
Aug 25, 2012

Conversation

c-alpha
Copy link
Contributor

@c-alpha c-alpha commented Aug 24, 2012

Visible changes:

  • Look & feel is now exactly as was Apple's old battery gauge
  • Using hi-res artwork to draw the capacity bar

Under the hood:

  • Images are loaded only once at startup and cached

Images are loaded only once at startup and cached.
Images are loaded only once at startup and cached.
@c-alpha
Copy link
Contributor Author

c-alpha commented Aug 24, 2012

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
Copy link
Owner

codler commented Aug 25, 2012

There is some points I would like to say.

aside from that it was good improvements you made

@c-alpha
Copy link
Contributor Author

c-alpha commented Aug 25, 2012

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
Copy link
Owner

codler commented Aug 25, 2012

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.


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.

Added setting for switching parentheses around time remaining on and off.
Moved all settings (incl. "advanced") to a submenu.
Added German and French localizations for new menu items.
TODO: Swedish and Dutch localizations missing.
@c-alpha
Copy link
Contributor Author

c-alpha commented Aug 25, 2012

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
Copy link
Owner

codler commented Aug 25, 2012

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
Copy link
Contributor Author

c-alpha commented Aug 25, 2012

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
Copy link
Owner

codler commented Aug 25, 2012

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 added a commit that referenced this pull request Aug 25, 2012
@codler codler merged commit d42f37d into codler:master Aug 25, 2012
@codler
Copy link
Owner

codler commented Aug 25, 2012

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
Copy link
Owner

codler commented Aug 26, 2012

I refactor your code :P 9102c77

@c-alpha
Copy link
Contributor Author

c-alpha commented Aug 27, 2012

  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.

osigge pushed a commit to osigge/Battery-Time-Remaining that referenced this pull request Sep 11, 2012
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