Conversation
gcasa
force-pushed
the
NSDiffableDataSource_branch
branch
from
df2da0f to
90a8419
Compare
gcasa
force-pushed
the
NSDiffableDataSource_branch
branch
from
926a924 to
1ac2073
Compare
gcasa
changed the title
|
Ready for review. |
Co-authored-by: gcasa <27943+gcasa@users.noreply.github.com>
Fix NSCollectionView drawRect to avoid modifying parent view
…-gui into NSDiffableDataSource_branch
| } | ||
| } | ||
|
|
||
| - (NSCollectionViewItem *) makeItemWithIdentifier: (NSUserInterfaceItemIdentifier)identifier |
There was a problem hiding this comment.
It looks strange that the identifier is never used anywhere.
There was a problem hiding this comment.
I believe that on macOS, the identifier is used to cache the cell or view so that it can't be re-used. That logic isn't implemented here (or even in table/outline view) on GNUstep. For now it's here for API compatibility. Let me know if you disagree with this.
There was a problem hiding this comment.
As I understand it the identifier should be used to decide which item or NIB to use. This behavior is currently missing, but could be added later on. It would be great to have a comment about this in the code to manage the expectations of people using this code.
There was a problem hiding this comment.
The current code was written by TestPlant / Keysight and (partly) by me. Currently, it appears to rely on the convention that the class name and the nib file name (or identifier) will be the same. This is an incorrect assumption. I think it is important that this is corrected, but I am not sure if it's right to do it in this PR.
| NSUInteger sectionIndex = 0; | ||
| NSUInteger itemIndex = 0; | ||
|
|
||
| if ([indexPath respondsToSelector: @selector(length)] && [indexPath length] >= 2) |
There was a problem hiding this comment.
Which AI did generate this code? It looks really strange. I really would prefer if you would review the generated code first and only after that (and another review by Copilot) hand it over to me.
I already have to review badly written AI code at work, I don't want to do this in my spare time as well.
There was a problem hiding this comment.
I have been testing AI to see if it produced functional code. This part was produced by Claude Sonnet 4. I will take a look and clean it up if it looks strange.
There was a problem hiding this comment.
I have reviewed the code a couple of times from top to bottom to get rid of any weirdness.
There was a problem hiding this comment.
A lot of my colleagues at work use Claude Sonnet 4.5 and that usually give better results. Either it is not trained that well for Objective-C or you should switch to the newer version. I will leave you another week to clean up, before I look again.
There was a problem hiding this comment.
I value your technical input, but the tone of your message came across as disrespectful. We can have more productive conversations if we focus on technical solutions rather than ultimatums. Waiting another week when it took my all of 15 mins to clean everything up is entirely too long. Additionally, the delay invites the very issue you complained about before (making changes after requesting review).
rfm
left a comment
rfm
left a comment
There was a problem hiding this comment.
I notice lots of enumerator based while loops.
Should we expose the FOR_IN macro from GSFastEnumeration.h in the base library, so that GUI code can use fast enumeration irrespective of what compiler is in use?
Not only would itrun those loops faster, it would also make the code shorter and more readable/maintainable.
I have replaced every instance of this with FOR_IN(...). |
The test for this is in https://github.com/gcasa/NSDiffableDataSource_test.git
The test on the mac: