search

ryanmcgrath / cacao

Rust bindings for AppKit (macOS) and UIKit (iOS/tvOS). Experimental, but working!
MIT License
1.8k stars 65 forks source link

fix: `'static` lifetime elision for examples #54

Closed EstebanBorai closed 1 year ago

EstebanBorai commented 1 year ago

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!

ryanmcgrath commented 1 year ago

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

EstebanBorai commented 1 year ago

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.

ryanmcgrath commented 1 year ago

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.

EstebanBorai commented 1 year ago

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.

ryanmcgrath commented 1 year ago

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

EstebanBorai commented 1 year ago

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

No worries!