KarthikRIyer / swiftplot

Swift library for Data Visualization :bar_chart:
Apache License 2.0
400 stars 39 forks source link

bfix: Explicitly set font smoothing to true in Quartz #63

Closed odmir closed 4 years ago

odmir commented 4 years ago

This is so that the output doesn't change when the user uses a different the toggle in Settings in macOS. I ended up needing to also explicitly allow font smoothing and for some reason I need to have it right when text is going to be drawn, I tried doing it at the init phase but for some reason I don't understand, it wouldn't work.

karwa commented 4 years ago

I think this should be done when we create the context - so in the initialiser (but not the one which takes an external context), and when we change the image size. Also, it might be good to forward this as a property from the QuartzRenderer in case users want to toggle it.

Apple actually recommend to disable font smoothing when rendering to a bitmap (which is what the non-external-context things are used for), so we might flip the default to false at some point. I tried it out, and it does seem to produce nicer text without smoothing.

There are cases, such as rendering to a bitmap, when font smoothing is not appropriate and should be disabled.

I don't mind if you want to flip the default and update the reference images in this PR or another one.

odmir commented 4 years ago

Ok I had tried with it in the correct initialiser, but I didn't realise that the size setter created a new context so the settings in the init were always being overridden afterwards, which explains why it wasn't working before. I will add the property and update the reference images!

odmir commented 4 years ago

Actually now that I look at it again, setAllowsFontSmoothing should always be given a value of true right?

odmir commented 4 years ago

The drawGraphAndOutput method on the renderer always ends up calling the imageSize didSet observer consequently calling setAllowsFontSmoothing, so should the fontSmoothing computed property be just a normal property? Also I just realised I could have used a normal property with a didSet as well instead of a computed property.

odmir commented 4 years ago

I changed the fontSmoothing property from a computed property to a normal one with a didSet observer, even though today I think setAllowsFontSmoothing will always be called after setting this property to any value, in imageSize's didSet. In the future something might change where this is no longer true so I think this is the right approach.

karwa commented 4 years ago

I think a computed property is still best - the underlying context is the source of truth, after all. But we should set it to a default in the initialiser and save/restore it when we re-make the context.

odmir commented 4 years ago

Hum I think I get what you are saying and I completely agree with having one source of truth, but is there a way to get the "should smooth font" value out of the context?

odmir commented 4 years ago

Or would we only allow setting the property?

karwa commented 4 years ago

Oh, I didn't know there wasn't a getter for the font smoothing property 😕

Well, then I guess I can't really think of a better way to do it than this. I don't think you need to set allowsFontSmoothing to true, but other than that it seems fine.

odmir commented 4 years ago

If I don't set setAllowsFontSmoothing to true I don't seem to be able to force it to use font smoothing just with the setShouldSmoothFonts when the system preferences has it turned off.

odmir commented 4 years ago

Everything else works. Its just that when It is turned off in the system preferences then I can't force it to be on within SwifPlot without also setting the other option to true 😔

karwa commented 4 years ago

Ok. well if it's needed, it's needed. Looks good to me, then. 👍

odmir commented 4 years ago

Ok, @KarthikRIyer if you want this PR is ready to be merged!

KarthikRIyer commented 4 years ago

Thanks! Merging.