NSBrowser bindings changes and general binding improvements

[gcasa]

This branch is to implement bindings in NSBrowser to NSTreeController and to fix and improve our use of bindings.

[gcasa]

@fredkiefer Converted to draft. Sorry. :)

[gcasa]

It's now working, I am reviewing how it's working because I had to do something similar to what I did in NSOutlineView, which I am not sure of.

[gcasa]

@fredkiefer These changes currently function as intended. I am still looking for a way to simplify them to your satisfaction. I am unconvinced, however, that, given the complexity of the data structure, that KVB can do it as simply as we would like. Ultimately, I'm wondering if it even matters if it is simple on our side as long as it works and is simple for the developer/user.

[fredkiefer]

Sorry, I might be wrong with some of the comments, it is just I don't understand the intention behind all this.

[gcasa]

Sorry, I might be wrong with some of the comments, it is just I don't understand the intention behind all this.

No worries. I'll do my best to explain the intentions behind each change. There were honestly some things in the macOS implementation that I didn't understand myself.

[gcasa]

With your permission, I will go ahead and merge this unless you have any overriding concerns. @fredkiefer

[gcasa]

@fredkiefer I have made further changes to the tests that indicate that the objectValue is set to the string of the node on Cocoa. I feel as though I have explored every possibility. The only solution is to maintain some parallel data structure. If I could store it in the cell itself, that would be feasible.

[fredkiefer]

Let's go back a step and look at all this from the outside. An NSBrowser has different ways to get its values for display. This could be via different types of delegates, that come either as item based delegate, passive delegate or a delegate that creates the rows for each column itself.
What we want to achieve here is to extend this mechanism to bindings. In my understanding having a binding completely replaces the delegate behaviour. This seems not to be the case for your code as it will be mostly be active when an item based delegate is present. Otherwise there would be no need to change the _itemForColumn: method.
If this is correct, I would suggest that you have a separate branch inside all methods where bindings come into play. That would make the code easier to read and understand.
Your code also seems to mix the concepts of NSContentBinding and NSContentValueBinding. I would expect that this only works correctly when both basically reference the same objects, with that later just adding one more key to the path. In most cases this will hopefully be the case, but does the implementation really have to rely on that? Maybe we could ignore NSContentValueBinding for the start and only implement the description behaviour?

Could you please point me to the test code that you are using for this feature? Maybe I will be able to better understand what you are trying to handle with your changes.

[gcasa]

@fredkiefer its here:

https://github.com/gcasa/NSBrowser_binding_test

I though I had linked this before. My convention is to name the test similar to the branch name so I can keep it straight. My apologies.

[gcasa]

I will make further comments in the morning. I am not feeling well at the moment. I will explain why it contains and uses NSContentBinding and NSValueBinding the way it does.

[gcasa]

Let's go back a step and look at all this from the outside. An NSBrowser has different ways to get its values for display. This could be via different types of delegates, that come either as item based delegate, passive delegate or a delegate that creates the rows for each column itself. What we want to achieve here is to extend this mechanism to bindings.

The code as I have implemented it does this. It gets the relevant data from the bindings rather than from the delegate.

In my understanding having a binding completely replaces the delegate behaviour. This seems not to be the case for your code as it will be mostly be active when an item based delegate is present.

The bindings behave very much like the item based delegate in how the data is pulled from each node.

Otherwise there would be no need to change the _itemForColumn: method. If this is correct, I would suggest that you have a separate branch inside all methods where bindings come into play.

Is that not what is being done? Every method that pulls the data has different conditionals for when a delegate is used and when a binding is used.

That would make the code easier to read and understand. Your code also seems to mix the concepts of NSContentBinding and NSContentValueBinding.

This difference is called for in the documentation...
https://developer.apple.com/library/archive/documentation/Cocoa/Reference/CocoaBindingsRef/BindingsText/NSBrowser.html

See under content... it says that "Unless contentValues is also bound, the titles of the items in the NSBrowser are derived by invoking invoking the descriptionmethod for each of the content objects."

I would expect that this only works correctly when both basically reference the same objects, with that later just adding one more key to the path. In most cases this will hopefully be the case, but does the implementation really have to rely on that? Maybe we could ignore NSContentValueBinding for the start and only implement the description behaviour?

See above.

Could you please point me to the test code that you are using for this feature? Maybe I will be able to better understand what you are trying to handle with your changes.

Previous comment does this.

https://github.com/gcasa/NSBrowser_binding_test

Per the test, the behavior as I have implemented it is consistent with the documentation and tests on macOS Ventura. Please let me know if you can think of a cleaner way to implement this as I am at a loss.

[gcasa]

I updated the test mentioned in the earlier comment to have a delegate. The result on macOS is this...

-[NSTextFieldCell setLeaf:]: unrecognized selector sent to instance 0x6000039a0780
(
	0   CoreFoundation                      0x00007ff80880a6d6 __exceptionPreprocess + 242
	1   libobjc.A.dylib                     0x00007ff8082f1c00 objc_exception_throw + 62
	2   CoreFoundation                      0x00007ff8088b215f -[NSObject(NSObject) __retain_OA] + 0
	3   CoreFoundation                      0x00007ff808779edf ___forwarding___ + 1379
	4   CoreFoundation                      0x00007ff8087798e8 _CF_forwarding_prep_0 + 120
	5   AppKit                              0x00007ff80c6c86bf -[NSBrowserBinder browser:willDisplayCell:atRow:column:] + 171
	6   AppKit                              0x00007ff80ccf5827 -[_NSBindingAdaptor browser:willDisplayCell:atRow:column:] + 296
	7   AppKit                              0x00007ff80c6a91ad -[NSBrowser _sendDelegateWillDisplayCell:atRow:column:] + 75
	8   AppKit                              0x00007ff80c3fcb07 -[NSTableView _delegateWillDisplayCell:forColumn:row:] + 103
	9   AppKit                              0x00007ff80c347db7 -[NSTableView _sendDelegateWillDisplayCell:forColumn:row:] + 88
	10  AppKit                              0x00007ff80c347676 -[NSTableView preparedCellAtColumn:row:] + 1799
	11  AppKit                              0x00007ff80c346e2e -[NSTableView _drawContentsAtRow:column:withCellFrame:] + 42
	12  AppKit                              0x00007ff80c346a08 -[NSTableView drawRow:clipRect:] + 1707
	13  AppKit                              0x00007ff80c3460ff -[NSTableView drawRowIndexes:clipRect:] + 1024
	14  AppKit                              0x00007ff80c2d79a5 -[NSTableView drawRect:] + 1693
	15  AppKit                              0x00007ff80c20eb4f _NSViewDrawRect + 142
	16  AppKit                              0x00007ff80cca0f40 -[NSView _recursive:displayRectIgnoringOpacity:inContext:stopAtLayerBackedViews:] + 1891
	17  AppKit                              0x00007ff80c20e401 -[NSView(NSLayerKitGlue) _drawViewBackingLayer:inContext:drawingHandler:] + 762
	18  AppKit                              0x00007ff80c8daaee -[NSViewBackingLayer drawInContext:] + 64
	19  AppKit                              0x00007ff80c48e2a1 _swift_stdlib_malloc_size + 6513
	20  AppKit                              0x00007ff80c53f740 __getCGDisplayListGetHashSymbolLoc_block_invoke + 15297
	21  AppKit                              0x00007ff80c53aabd _NSCGDisplayListGetHash + 1647
	22  AppKit                              0x00007ff80c53f240 __getCGDisplayListGetHashSymbolLoc_block_invoke + 14017
	23  AppKit                              0x00007ff80c53cb96 __getCGDisplayListGetHashSymbolLoc_block_invoke + 4119
	24  AppKit                              0x00007ff80c598f59 block_destroy_helper.34 + 10697
	25  AppKit                              0x00007ff80c59952b block_destroy_helper.34 + 12187
	26  AppKit                              0x00007ff80c8da710 -[NSViewBackingLayer display] + 1763
	27  AppKit                              0x00007ff80c19104c __37+[NSDisplayCycle currentDisplayCycle]_block_invoke.18 + 811
	28  QuartzCore                          0x00007ff811608199 _ZN2CA11Transaction19run_commit_handlersE18CATransactionPhase + 95
	29  QuartzCore                          0x00007ff8117c763c _ZN2CA7Context18commit_transactionEPNS_11TransactionEdPd + 1090
	30  QuartzCore                          0x00007ff811606d83 _ZN2CA11Transaction6commitEv + 725
	31  AppKit                              0x00007ff80c21f531 __62+[CATransaction(NSCATransaction) NS_setFlushesWithDisplayLink]_block_invoke + 289
	32  AppKit                              0x00007ff80ccebca3 ___NSRunLoopObserverCreateWithHandler_block_invoke + 41
	33  CoreFoundation                      0x00007ff808799029 __CFRUNLOOP_IS_CALLING_OUT_TO_AN_OBSERVER_CALLBACK_FUNCTION__ + 23
	34  CoreFoundation                      0x00007ff808798f4c __CFRunLoopDoObservers + 503
	35  CoreFoundation                      0x00007ff8087984e5 __CFRunLoopRun + 905
	36  CoreFoundation                      0x00007ff808797b6c CFRunLoopRunSpecific + 536
	37  HIToolbox                           0x00007ff813e76fe3 RunCurrentEventLoopInMode + 292
	38  HIToolbox                           0x00007ff813e7c807 ReceiveNextEventCommon + 201
	39  HIToolbox                           0x00007ff813e7cb62 _BlockUntilNextEventMatchingListInModeWithFilter + 66
	40  AppKit                              0x00007ff80c0cad8b _DPSNextEvent + 902
	41  AppKit                              0x00007ff80cae74c8 -[NSApplication(NSEventRouting) _nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 1290
	42  AppKit                              0x00007ff80c0bbeb7 -[NSApplication run] + 610
	43  AppKit                              0x00007ff80c08f234 NSApplicationMain + 823
	44  NSBrowser_binding_test              0x000000010887dd0f main + 47
	45  dyld                                0x00007ff8083262cd start + 1805
)

The result on GNUstep is that the delegate is simply ignored if the binding is present. I believe this behavior can be deemed as undefined since this is not covered in the documentation.

[gcasa]

We get the following on the dual_controller branch which tests the idea that we can support different controllers for content and contentValues bindings. This doesn't appear to work, per the following...

[<TreeNode 0x600002c6b3c0> valueForUndefinedKey:]: this class is not key value coding-compliant for the key .
(
	0   CoreFoundation                      0x00007ff80880a6d6 __exceptionPreprocess + 242
	1   libobjc.A.dylib                     0x00007ff8082f1c00 objc_exception_throw + 62
	2   CoreFoundation                      0x00007ff80882d654 -[NSException raise] + 9
	3   Foundation                          0x00007ff80a05d4de -[NSObject(NSKeyValueCoding) valueForUndefinedKey:] + 220
	4   Foundation                          0x00007ff809790997 -[NSObject(NSKeyValueCoding) valueForKey:] + 267
	5   AppKit                              0x00007ff80c3e4ee7 -[NSBinder valueForBinding:atIndexPath:resolveMarkersToPlaceholders:] + 362
	6   AppKit                              0x00007ff80c6c8692 -[NSBrowserBinder browser:willDisplayCell:atRow:column:] + 126
	7   AppKit                              0x00007ff80ccf5827 -[_NSBindingAdaptor browser:willDisplayCell:atRow:column:] + 296
	8   AppKit                              0x00007ff80c6a91ad -[NSBrowser _sendDelegateWillDisplayCell:atRow:column:] + 75
	9   AppKit                              0x00007ff80c6a9084 -[NSBrowser _loadCell:atRow:col:inMatrix:] + 680
	10  AppKit                              0x00007ff80c3fff7a -[NSMatrix drawRect:] + 906
	11  AppKit                              0x00007ff80c20eb4f _NSViewDrawRect + 142
	12  AppKit                              0x00007ff80cca0f40 -[NSView _recursive:displayRectIgnoringOpacity:inContext:stopAtLayerBackedViews:] + 1891
	13  AppKit                              0x00007ff80c20e401 -[NSView(NSLayerKitGlue) _drawViewBackingLayer:inContext:drawingHandler:] + 762
	14  AppKit                              0x00007ff80c8daaee -[NSViewBackingLayer drawInContext:] + 64
	15  AppKit                              0x00007ff80c48e2a1 _swift_stdlib_malloc_size + 6513
	16  AppKit                              0x00007ff80c53f740 __getCGDisplayListGetHashSymbolLoc_block_invoke + 15297
	17  AppKit                              0x00007ff80c53aabd _NSCGDisplayListGetHash + 1647
	18  AppKit                              0x00007ff80c53f240 __getCGDisplayListGetHashSymbolLoc_block_invoke + 14017
	19  AppKit                              0x00007ff80c53cb96 __getCGDisplayListGetHashSymbolLoc_block_invoke + 4119
	20  AppKit                              0x00007ff80c598f59 block_destroy_helper.34 + 10697
	21  AppKit                              0x00007ff80c59952b block_destroy_helper.34 + 12187
	22  AppKit                              0x00007ff80c8da710 -[NSViewBackingLayer display] + 1763
	23  QuartzCore                          0x00007ff811626172 _ZN2CA5Layer17display_if_neededEPNS_11TransactionE + 856
	24  QuartzCore                          0x00007ff8117c74f8 _ZN2CA7Context18commit_transactionEPNS_11TransactionEdPd + 766
	25  QuartzCore                          0x00007ff811606d83 _ZN2CA11Transaction6commitEv + 725
	26  AppKit                              0x00007ff80c21f531 __62+[CATransaction(NSCATransaction) NS_setFlushesWithDisplayLink]_block_invoke + 289
	27  AppKit                              0x00007ff80ccebca3 ___NSRunLoopObserverCreateWithHandler_block_invoke + 41
	28  CoreFoundation                      0x00007ff808799029 __CFRUNLOOP_IS_CALLING_OUT_TO_AN_OBSERVER_CALLBACK_FUNCTION__ + 23
	29  CoreFoundation                      0x00007ff808798f4c __CFRunLoopDoObservers + 503
	30  CoreFoundation                      0x00007ff8087984e5 __CFRunLoopRun + 905
	31  CoreFoundation                      0x00007ff808797b6c CFRunLoopRunSpecific + 536
	32  HIToolbox                           0x00007ff813e76fe3 RunCurrentEventLoopInMode + 292
	33  HIToolbox                           0x00007ff813e7c807 ReceiveNextEventCommon + 201
	34  HIToolbox                           0x00007ff813e7cb62 _BlockUntilNextEventMatchingListInModeWithFilter + 66
	35  AppKit                              0x00007ff80c0cad8b _DPSNextEvent + 902
	36  AppKit                              0x00007ff80cae74c8 -[NSApplication(NSEventRouting) _nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 1290
	37  AppKit                              0x00007ff80c0bbeb7 -[NSApplication run] + 610
	38  AppKit                              0x00007ff80c08f234 NSApplicationMain + 823
	39  NSBrowser_binding_test              0x000000010bb308df main + 47
	40  dyld                                0x00007ff8083262cd start + 1805
)

Please let me know if you have any other concerns. The GNUstep code in this case seems to recurse or loop, but as this is an undefined case I believe that doesn't matter.

[gcasa]

I believe that all issues with this change have been adequately addressed. The above tests address the issue where we have a delegate in addition to the binding (a case that crashes macOS) and the case where we have separate bindings for NSObjectValue and NSContentValue (a case that also crashes macOS). GNUstep handles them both. Also, the tests match the functionality as expected on macOS. Please make any final comments.

[gcasa]

@fredkiefer Please review. :)

[fredkiefer]

Hi Greg,
there is nothing left for me here to review. You already know that in principle I think that this code is working. It is just horribly structured and that I won't approve it that way. If you want to merge it anyway just go ahead with it.

Judging from the stack traces from MacOS you provided above, Apple went a completely different way to implement this. They seem to have introduced an adopter that maps the binding behaviour onto a delegate. When done properly this could be a very clean solution.

[gcasa]

Hi Greg,

there is nothing left for me here to review. You already know that in principle I think that this code is working. It is just horribly structured and that I won't approve it that way. If you want to merge it anyway just go ahead with it.

Judging from the stack traces from MacOS you provided above, Apple went a completely different way to implement this. They seem to have introduced an adopter that maps the binding behaviour onto a delegate. When done properly this could be a very clean solution.

I agree that it can be done better and I made the same observation with respect to the adapter as I have seen it before in back traces but I have no idea how it functions. For now I will merge this. I will revisit it and the outline code when I have a better idea about the bindings adapter.

Thanks for all of your help and feedback.