Issue with bindings due to KVO changes... #475
Comments
|
The issue is that it's saying that setKeys:triggerChangeNotificationsForDependentKey: is not recognized on the class object. :/ |
|
|
|
Good catch @gcasa. Your code seems to be using the old We can look into adding support for that. In the meantime you could do one of these to work around:
|
|
@triplef I'm not sure I see a clear reason why the two implementations need to be incompatible. This is a breaking change that causes bindings to be broken. The code in question is in NSTreeController and NSObjectController, which has been functional up until now. Here is what Apple Documentation says on this... So the deprecated method you mentioned should still work. The general approach on this project is not to deprecate methods unless necessary. Even given what Apple is saying, as a developer, I would expect the setKeys:... method to still work. |
|
You’re right there’s no reason. The new KVO implementation is a complete rewrite, because the old one had many issues and the new one was based on code from WinObjC, and the omission of that deprecated method was just not caught during implementation, reviews, or tests. |
|
It's understandable, as it's a huge effort. I'm very thankful for the improved implementation. I hope it's better and more efficient. We need to have another file, perhaps NSKVODeprecated.m or something to hold the implementations that are out of date. |
|
Or NSKVOLegacy.m... whatever. :) |
|
Bear in mind that, because the new implementation really can't work with the gnu runtime, the GUI library can't depend on features exclusive to it and will need to be tested with both GCC/gnu and Clang/ng :-(
…
On 10 Dec 2024 at 16:00, Gregory Casamento ***@***.***> wrote:
Or NSKVOLegacy.m... whatever. :)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>
|
No one is advocating that gui depend on any features exclusive to GCC. My point is this... Apple supports the old method by implementing it in terms of the new one. We should be able to do the same. Simple. I will change GUI to use the new method as it seems more efficient. My concern, however, is that there may be other applications or tools used by other users that this change breaks. |
|
@triplef one note... I realize Algorridim doesn't use the GUI, but you still need to be careful not to break it. |
They are not. I just opted not to reintroduce a long (since 10.5) deprecated API. Given that the new KVO implementation is tied to libobjc2, I am tempted to say that most code bases affected never used, or have long migrated. |
Difficult to do, if there is no CI. |
They are not compatible by demonstration, per the breakage in the GUI. However, as the previous implementation (which appears to be more general since it works on both GCC and Clang) contains both, I believe adding this method is achievable.
As I said above in my previous reply, this is not the policy on this project. Generally, APIs are kept. We still have APIs from OPENSTEP that have been dropped from Cocoa. Also... while this API is deprecated it is STILL PRESENT in Cocoa which makes my point even stronger that it should still be there. I will make this easy on you guys... I'll add the implementation in libs-base myself. sigh |
You are not limited to CI for your testing. Given that this change affects bindings, that should have been included in the things that needed to be tested. The bindings implementation has been in GUI for a while. Also, I could swear that this was the subject of our last discussion when there was breakage then as well. Just to underscore the difference here, @hmelder is correct... this likely doesn't have as wide an impact, so this doesn't fall into the same category as that issue did. |
@rfm I want to be extra clear. The bindings code has been in GUI for the last few years, so this is NOT NEW CODE as @triplef seemed to imply. So by @hmelder "opting not to implement" the deprecated method it is this change that is at fault. This change is what made things in libs-base dependent on a specific runtime. I said it once before when the NSDate code broke GUI, the project needs a test to cover cases like this. This:
When you write a "new version" of something what you don't have the option to do is leave out methods YOU PERSONALLY think aren't needed or aren't FUN. You must fully implement the API PERIOD. To @triplef's point if this was a problem it should have been caught by the process... and it was NOT. We are not a big company so we don't have the resources to catch everything so we rely on people to do the right thing when committing code. I am going to add some tests to make sure at least this case is caught in the future. The tests need to be expanded to cover all of the methods to make sure that Again... I am extremely glad you're part of the community. None of the above is meant to piss anyone off, but please remember what I said above. What upsets me right now is that instead of working on getting bindings to work in GUI... which also is not super fun... I am sitting fixing this issue. :/ |
|
You misunderstood me. My comment was about the new KVO implementation.
Since it's runtime dependent we must keep the old implementation too, and the two are always going to behave slightly differently (unless someone with the expertise can fix the bugs in the old one), so I was pointing out that your bindings implementation will eventually need to cope with either/both of the KVO implementations, so needs to be tested with both.
I understand that you are angry to find your work broken by the new KVO, but your anger may have affected your reading of Hugo's comments: my reading is that he told you how to use the configure time option to keep on working with the old KVO (so no need to rush to any action), and said he'd implement the missing method in the new KVO. Those comments mean there's no actual reason for you to be stuck working on the KVO issue!
Only in a subsequent comment did he try to explain the reason for his mistake (thinking the method was obsolete), and yes I agree that we want to keep old methods around, and that Hugo, needs to bear that in mind more (and that I should have checked for missing methods), but we *have* found the problem now, in good time before any new release.
By telling Hugo you are working on the missing method rather than asking about his plans to implement it, you may have put him off working on it. If you want his help/expertise it might make sense to focus on asking for that.
As a PS ... It's best to catch things early; the more CI we have the less people need to remember to do, and the closer tests are to the code, the sooner we fix issues. A lot of KVO tests have been added to base, but you might want to add more for features that bindings depend upon.
I was hoping to do a base release before Xmas, but that's probably not happening (and certainly not without GUI/back working with it).
…
On 12 Dec 2024 at 03:34, Gregory Casamento ***@***.***> wrote:
>
>
> Bear in mind that, because the new implementation really can't work with the gnu runtime, the GUI library can't depend on features exclusive to it and will need to be tested with both GCC/gnu and Clang/ng :-(
>
>
@rfm I want to be extra clear. The bindings code has been in GUI for the last few years, so this is NOT NEW CODE as @triplef seemed to imply. So by @hmelder "opting not to implement" the deprecated method it is this change that is at fault. This change is what made things in libs-base dependent on a specific runtime. I said it once before when the NSDate code broke GUI, the project needs a test to cover cases like this. This:
Breaks the rules of the project
Breaks existing code that currently works I don't care if "most code has moved on" in @hmelder 's opinion...
When you write a "new version" of something what you don't have the option to do is leave out methods YOU PERSONALLY think aren't needed or aren't FUN. You must fully implement the API PERIOD. To @triplef's point if this was a problem it should have been caught by the process... and it was NOT.
We are not a big company so we don't have the resources to catch everything so we rely on people to do the right thing when committing code. I am going to add some tests to make sure at least this case is caught in the future. The tests need to be expanded to cover all of the methods to make sure that
Again... I am extremely glad you're part of the community. None of the above is meant to piss anyone off, but please remember what I said above. What upsets me right now is that instead of working on getting bindings to work in GUI... which also is not super fun... I am sitting fixing this issue. :/
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
Regarding catching things early: as it seems like @gcasa you have some additional apps to test changes with, would it be ok if we add you as a reviewer on (larger) pull requests in the future? Unless/until we manage to integrate CI tests for all the code in apps like the ones you linked above that seems like a good way to increase our chances of catching things early. |
Yes, I fully understand this. GUI should absolutely be tested with both. My issue is that the "new" KVO implementation is INCOMPLETE and thus not compatible with the old one. This bothers me on a FUNDAMENTAL level as this should have been implemented.
I didn't see the last part. If he's going to implement it that would be perfect. I apologize for my frustration. I simply didn't see the point in "switching back". The way I read it is that he had opted not to implement it and that most codebases had moved on.
Understood.
@hmelder Please implement it as you said. I apologize for being upset. Your work is valued. I am simply very passionate (and a bit overprotective) about this project sometimes. Thank you again for working so hard on this. As I said, please go ahead and implement this. I didn't mean to put you off of working on it as I didn't see that part of your comment.
Yes, indeed.
There are too many changes that we need to stabilize. I agree. |
For every feature I have added I have a test. They are in my repo. They usually follow the pattern of CLASSNAME_FEATURE_test. Where the first two, obviously, are replaced. And, yes, please it would be fine to add me to such PRs. I would appreciate it. |
Are these tests also in libs-gui, or if not could you add them there @gcasa? We should then integrate the libs-gui tests into the libs-base CI runs to expand test coverage of libs-base changes. It will probably make CI runs quite a bit longer, but it seems like it might be worth it given that two recent libs-base changes caused issues in libs-gui (not sure though if they would have been caught by libs-gui tests?). |
I had a discussion with @hmelder earlier about this. They are in my personal repo under my account here in github. I wasn't sure how to integrate some of them into the automated tests. It would be nice to have them there. Most are interactive tests of given functionality. This weekend I will see if I can do some of that or at least put them into a directory where they can be easily accessed in libs-gui. |
|
@hmelder I have deleted the branch associated with this and remove myself from this issue. Please keep me informed about the implementation of the method discussed. I will be glad to test. Thanks so much. |
https://github.com/gcasa/NSTreeController_test
https://github.com/gcasa/NSBrowser_bindings_test
Botn of these test NOW FAIL due to the latest changes. I am trying to fix this, but it is confusing as to why it breaks as the method appears to be defined. FRUSTRATED WITH CONSTANT BREAKAGE
The text was updated successfully, but these errors were encountered: