search

michelf / sim-daltonism

A color blindness simulator for Mac and iOS
256 stars 18 forks source link

Metal implementation (using CIFilter) + Simplified Chinese translation #20

Closed importRyan closed 2 years ago

importRyan commented 3 years ago

As discussed via email, I'll continue to look at CPU-side timing to boost performance for larger window capture areas from this starting point. Monitoring the preview window's screen membership to switch preferred Metal devices might also be better.

Finally, sorry re: using one commit for the Filter menu additions and Mandarin translations; hopefully it's still ok to sort through.

michelf commented 3 years ago

More thoughts about the FilterStore system of classes...

As explained above it appears to have a thread safety issue. I think it'd be better if all the stateful parts where stored within the same class that would encapsulate all the read and write operations on that state. That would make thread safety issues easier to deal with.

Another issue: the chosen filter is now stored as a global state. If the user creates multiple filter windows it is no longer possible to select a different filter for each one.

importRyan commented 3 years ago

Awesome lessons here for my first PR. 👍🏻 Thanks. I'll change up that system. Never thought of windows w/ separate filters. Neat use case. Thanks for the careful review.

importRyan commented 3 years ago

@michelf Michel, I've come back round to this. The latest commits should address the to-dos identified earlier.

Re: window bounds, the mouse monitor inset rect had a math error. I've fixed that and given it a few pixels more than prior.

michelf commented 3 years ago

Thank you for those changes. I should be able to review this next weekend.

importRyan commented 3 years ago

Most of your PR and the features it adds is quite good, that's why I'm putting extra efforts into reviewing it.

As a self-taught developer, I welcome all the feedback you've given. Thank you for all your comments. It's fun to contribute.

when trying to split a problem domain into its components: you sometime end up with too many components.

Agreed. Condensing them removed a lot of boilerplate. The only differences really were in the Vendors.

michelf commented 2 years ago

I've left this hanging for too long, sorry.

I think I'll accept this as is and perhaps make a few changes myself later if needed. Can you just remove or revert 7bf5c79?, I'm not sure this fix is correct and I'd rather address it separately. (It's not like you introduced that bug, it was there before too.)

importRyan commented 2 years ago

Reverted. I've also meant to come back to this. Thanks for the prompt.

michelf commented 2 years ago

Merged! Again, thank you. I look forward to other pull requests.

I made some changes (rationale in each commit message):

I also believe there's too much code in UserDefaults+WindowRestoration.swift for what it does, but I'm not sure what to do yet with this. I suppose migrating this to iOS might help figure out how to streamline this.