-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
OSX migrate to NSCell based renderer #24004
base: master
Are you sure you want to change the base?
Conversation
the old HITheme renderer does not support dark mode for the disclosure button, we should move to use NSCells for rendering in the future
unfortunately a huge diff, but the rearrangements also happen if I do the change by hand on the existing project …
Stefan, is this ready to be merged? If so, I'd like to (squash) merge it removing the old files to get the correct diffs for this commit. Please let me know if I should do this, TIA! |
@vadz it's not ready, I still have a focus-clear-redraw issue I have to investigate, and I thought I'd post to wx-users to get more testing, but if this looks like it could hinder things, I could introduce a #ifdef internally to have the older code active and allow for testers to enable it on master |
IME asking people to test things really works, unfortunately. Other than those directly involved in the PR/bug report, few people test new code until it's merged into master. So if you're confident that this is the right way to do it and can fix the bugs you see, I'd merge it first and ask for testing afterwards. |
For what it's worth, we have this same experience with KiCad and thus also have adopted a "merge early" philosophy |
Stefan, I've converted this to draft for now, please make it "Ready for review" when you consider it in an acceptable (even if not perfect) state. TIA! |
Thanks, although you are not fond of them, I'm really thinking about adding #ifdefs with the old code - to support better A/B testing |
If we do this, we should at least enable the new version by default. But I'd indeed prefer to avoid having them, if only because it would be better if this appeared as a change of the existing code (which will be the case if it's replaced by the new version) instead of addition of another copy of it and then removal of the original version. |
agreed on that
Yes, this would be better. But the problem is: the objective-c++ needs the .mm ending. |
Just to be sure (I think you already know this, but, again, just in case): we can move the file, Git doesn't care about its path (nor extension) and history would still look correct in this case. |
yes, but I realized that I have started the wrong way in the branch, with the duplicate , I'll create a new one and setup this with the correct history |
the old HITheme renderer does not support dark mode for the disclosure button, we should move to use NSCells for rendering in the future, see #24002