A binding with 'NSNegateBoolean' transformer will not handle 'nil' correct

[mrcarlberg]

I have no fix for this as I have a hard time figuring out how to solve this. I have only done the test case. Any help in doing this should be highly appreciated.

The corresponding working Objective-C test case is here:

- (void)testTransformValueBinding {
    NSButton *control = [[NSButton alloc] initWithFrame:CGRectZero];
    [control setButtonType:NSSwitchButton];

    NSArray *content =
    @[
     [@{ @"state": @YES } mutableCopy],
     [@{ @"state": @NO } mutableCopy],
     [@{} mutableCopy]
     ];

    NSArrayController *arrayController = [[NSArrayController alloc] initWithContent:content];

    [control bind:NSValueBinding toObject:arrayController withKeyPath:@"selection.state" options:@{ NSValueTransformerNameBindingOption: @"NSNegateBoolean" }];

    [arrayController setSelectionIndexes:[NSIndexSet indexSetWithIndex:0]];
    STAssertEqualObjects([control objectValue], @(NSOffState), @"content[0] is negated");
    [arrayController setSelectionIndexes:[NSIndexSet indexSetWithIndex:1]];
    STAssertEqualObjects([control objectValue], @(NSOnState), @"content[1] is negated");
    [arrayController setSelectionIndexes:[NSIndexSet indexSetWithIndex:2]];
    STAssertEqualObjects([control objectValue], @(NSOnState), @"content[2] is negated");

    [arrayController setSelectionIndexes:[NSIndexSet indexSetWithIndex:1]];
    [control performClick:nil];
    STAssertEqualObjects([control objectValue], @(NSOffState), @"value was changed");
    STAssertEqualObjects([content[1] valueForKey:@"state"], @YES, @"content[1] was negated after change");

    [arrayController setSelectionIndexes:[NSIndexSet indexSetWithIndex:2]];
    [control performClick:nil];
    STAssertEqualObjects([control objectValue], @(NSOffState), @"value was changed");
    STAssertEqualObjects([content[2] valueForKey:@"state"], @YES, @"content[2] was negated after change");
}

[cappbot]

Milestone: Someday. Label: #new. What's next? A reviewer should examine this issue.

[aljungberg]

Shouldn't NSIsNotNilTransformer be used for a value that can be nil?

[mrcarlberg]

Yes, it can. But now I have a bool value that I need to negate. If it is null it should be counted as false and after the transformer it should be true. This doesn't not work in Cappuccino. It does work in Cocoa as the test case shows.

[mrcarlberg]

I can add that the reason for not working is that when the value is null it will be a 'null marker' and transformers are not applied when the value is a 'marker'.

[ahankinson]

-#new
+#needs-patch

[cappbot]

Milestone: Someday. Label: #needs-patch. What's next? This issue needs a volunteer to write and submit code to address it.

[FrankReh]

Does Cappucinno add the key and value to the object like Cocoa does? I used the code above to create a small objc file and compiled with NSLog sprinkled in showing the value of the second NSArray item. It shows the key/value pair "state"/1 has been added to the empty dictionary by the simulated mouse click.

[arrayController setSelectionIndexes:[NSIndexSet indexSetWithIndex:2]];
// content[2] === { }
[control performClick:nil];
// content[2] === { state = 1; }

[FrankReh]

This change to the dictionary is not due to the option @{ NSValueTransformerNameBindingOption: @"NSNegateBoolean" }. When no option is passed to the binding creation, the simulated mouse click still adds the missing property to the object that the array controller is binding to at the time.

[FrankReh]

Yes, Cappuccino changes the empty object to { state = "1" } according to NSLog.

[mrcarlberg]

Ok, are you saying that this is working now in the current version of Cappuccino?

[FrankReh]

No, sorry. I was originally confused by the length of the test case, thinking it was the last test that fails. Now I see it is the test immediately after the index:2 selection, where the error message is @"content[2] is negated". The object bound is still empty so the binding check should pull out a nil and that should be interpreted as false and therefore the button should go to true. And as your investigation years ago showed you, the binding does not cause the button to be flipped to true while the object that is bound to is empty.

Still it is interesting that the binding is allowed to put a key and value into the object after a mouse click even though one didn't exist before. Same as Cocoa.

[mrcarlberg]

You can look at it as the key and value is already there as specified by the binding, except that the value is nil. Due to the implementation of [NS|CP]Dictionary you can't have a key and value pair with the value nil. So the dictionary just looks empty. 😄

[FrankReh]

How about in the CPBinder implementation for setValueFor: replacing the value of 'newValue' for this special case? It seems like the CPNullMarker case should follow the case as if a nil had been pulled out of the dictionary. (But I don't have any experience with bindings. I looked at this bug as a chance to learn how bindings work better in the first place.) I haven't removed all my trace statements but this does seem to make the test case pass.

- (void)setValueFor:(CPString)theBinding
{
    var destination = [_info objectForKey:CPObservedObjectKey],
        keyPath = [_info objectForKey:CPObservedKeyPathKey],
        options = [_info objectForKey:CPOptionsKey],
        newValue = [destination valueForKeyPath:keyPath];
    
    if (newValue === CPNullMarker) // new
        newValue = nil;            // new

    if (CPIsControllerMarker(newValue))
    {
        [self raiseIfNotApplicable:newValue forKeyPath:keyPath options:options];

        var value = [self _placeholderForMarker:newValue];
        [self setPlaceholderValue:value withMarker:newValue forBinding:theBinding];
    }
    else
    {
        var value = [self transformValue:newValue withOptions:options];
        [self setValue:value forBinding:theBinding];
    }
}

[FrankReh]

Been looking at the marker business further and I have to say I don't like my suggestion. The Null marker is one of four special cases that was created long ago for use with the idea of having place holders for these special events. It just doesn't seem to be being used appropriately for the case where the binding gets a nil value back when a boolean was all that was needed. The button view should be able to use that nil value as a legitimate boolean id representing false, the fact that it is changed to a Null marker should not then mean the button has to interpret the value as true.

[FrankReh]

Wanted to learn about bindings. Now I know a little bit about placeholders too. There appears to be place in the binding init code where this could be taken care of but so far it is not. The options value is passed right from the original bind: call, and this same options is even passed to the mechanism that would override the default placeholder value for something like the null marker. But of course the options dictionary at the moment only contains the info for the transformer, not for the null placeholder.

If the negation transformer could be used to define a new value for the null placeholder and put this into the same options dictionary, then it appears the null marker would have the desired effect of setting the button to true. A workaround, if one were looking for a workaround, appears to be to manually add this override to the options passed to the bind: method. I'll try that too soon.

Here's the existing code with the options being passed down.

- (id)initWithBinding:(CPString)aBinding name:(CPString)aName to:(id)aDestination keyPath:(CPString)aKeyPath options:(CPDictionary)options from:(id)aSource
{
    // We use [self init] here because subclasses override init. We can't override this method
    // because their initialization has to occur before this method in this class is executed.
    self = [self init];

    if (self)
    {
        _source = aSource;
        _info = @{
                CPObservedObjectKey: aDestination,
                CPObservedKeyPathKey: aKeyPath,
            };
        _suppressedNotifications = {};
        _placeholderForMarker = {};

        if (options)
            [_info setObject:options forKey:CPOptionsKey];

        [self _updatePlaceholdersWithOptions:options forBinding:aName];
        ...

[FrankReh]

This is enough to make the test case pass. Adding the CPNullPlaceholderBindingOption:YES to the options.

[control bind:CPValueBinding 
  toObject:arrayController 
  withKeyPath:@"selection.state" 
  options:@{
    CPValueTransformerNameBindingOption: CPNegateBooleanTransformerName, // transformer option
    CPNullPlaceholderBindingOption:YES // placeholder option
    }];

But it's not what you would want, because the Cocoa framework doesn't require this. The solution is becoming evident but what Cocoa does seems a bit odd and might be undocumented. I've confirmed the syntax is correct in my Cocoa tests and they show that the transformer option is valid to pass to the binding and that the placeholder option is also valid to pass to the binding, and in fact both options can be passed to the binding at the same time but in that last case, the placeholder option is completely ignored by Cocoa; the button sees the result of the transformer working on the default null placeholder, not what might be passed, be it set to YES or NO. So go with duplicating the Cocoa behavior?

[mrcarlberg]

Great work on this!

It is always preferred to go with the Cocoa behavior unless we can find a real benefit for the Cappuccino framework. I can not see any reason so please go with the Cocoa behavior.

[FrankReh]

There is a unit test that failed with the solution I would have come up with a couple days ago. That led me down a rabbit hole and now I really think I understand transformers better, at least on the Cocoa side. I may put my Cocoa tests into a gist.

When a transformer is mapped to a binding, the transformer will be applied to all the selections (one or more), nil or not, and any resulting nil values are only then replaced with the null placeholder. The Cappuccino code does not try to apply the transformer to nil. More to come.

An earlier revelation is that of the four place holders, three have to with the meta selection state (no selection, multiple section, and invalid selection) and only one has to do with the selected object's data itself (the null placeholder). This deserves a little special treatment on the Cappuccino side too.