cacao icon indicating copy to clipboard operation
cacao copied to clipboard

fix: `'static` lifetime elision for examples

Open EstebanBorai opened this issue 1 year ago • 2 comments

Takes advantage of lifetime elision on some examples where the rule applies.

Refer: https://doc.rust-lang.org/reference/lifetime-elision.html#static-lifetime-elision


I also noticed that cargo fmt applied changes on src which I didn't expected to be honest. I can take those back if needed.

Thanks so much for such a great crate!

EstebanBorai avatar Aug 26 '22 02:08 EstebanBorai

Cool! The fmt oddities are probably due to not running with nightly - could you re-run with that?

ryanmcgrath avatar Aug 26 '22 02:08 ryanmcgrath

Cool! The fmt oddities are probably due to not running with nightly - could you re-run with that?

Thanks for sharing @ryanmcgrath, I've run cargo fmt on nightly channel and that fixed the fmt undesired changes.

EstebanBorai avatar Aug 26 '22 14:08 EstebanBorai

So the one thing on this particular PR is that the monospacedSystemFontOfSize:weight: API is macOS 10.15+. For things like this I've generally done a check to see the OS version and then done a reasonable fallback... in this particular case, could you just return systemFontOfSize:weight: for macOS 10.14 and lower?

(e.g https://github.com/ryanmcgrath/cacao/blob/1ad51887a4fd34c8a8a35d85b94d3319dfb79db7/src/color/appkit_dynamic_color.rs#L50)

This would provide a reasonable dev experience, I think - it'd be apparent (for those who care to ship that far back) that monospaced fonts aren't working, but it's not going to crash on a random unsupported selector.

ryanmcgrath avatar Oct 12 '22 18:10 ryanmcgrath

So the one thing on this particular PR is that the monospacedSystemFontOfSize:weight: API is macOS 10.15+. For things like this I've generally done a check to see the OS version and then done a reasonable fallback... in this particular case, could you just return systemFontOfSize:weight: for macOS 10.14 and lower?

(e.g

https://github.com/ryanmcgrath/cacao/blob/1ad51887a4fd34c8a8a35d85b94d3319dfb79db7/src/color/appkit_dynamic_color.rs#L50

) This would provide a reasonable dev experience, I think - it'd be apparent (for those who care to ship that far back) that monospaced fonts aren't working, but it's not going to crash on a random unsupported selector.

Thanks for reviewing @ryanmcgrath!

I will refer this comment in the corresponding PR so I dont forget, Im pretty busy right now but I think I can take a look in the next days.

EstebanBorai avatar Oct 13 '22 04:10 EstebanBorai

Ah! Damn, I totally wrote that for the wrong PR. My mistake, thanks for catching.

ryanmcgrath avatar Oct 16 '22 21:10 ryanmcgrath

Ah! Damn, I totally wrote that for the wrong PR. My mistake, thanks for catching.

No worries!

EstebanBorai avatar Oct 16 '22 23:10 EstebanBorai