From 14f494a25d1417e8093f2e82cd1a29ea18672369 Mon Sep 17 00:00:00 2001 From: Klaas Pieter Annema Date: Thu, 16 Sep 2010 16:16:41 +0200 Subject: [PATCH 1/9] Properly set multiple value placeholders on textfields. Previously CPKeyValueBinding would just set the binding marker placeholder values as the stringValue of the textfield. This commit fixes that and properly sets those values as the placeholder value. --- AppKit/CPKeyValueBinding.j | 62 +++++++++++++++++++++++++++++--------- 1 file changed, 47 insertions(+), 15 deletions(-) diff --git a/AppKit/CPKeyValueBinding.j b/AppKit/CPKeyValueBinding.j index 1ea547e5d..b16f9579a 100644 --- a/AppKit/CPKeyValueBinding.j +++ b/AppKit/CPKeyValueBinding.j @@ -148,8 +148,47 @@ var CPBindingOperationAnd = 0, options = [_info objectForKey:CPOptionsKey], newValue = [destination valueForKeyPath:keyPath]; - newValue = [self transformValue:newValue withOptions:options]; - [_source setValue:newValue forKey:aBinding]; + if (CPIsControllerMarker(newValue)) + { + var valueIsPlaceholder = YES; + + switch (newValue) + { + default: + valueIsPlaceholder = NO; + break; + + case CPMultipleValuesMarker: + newValue = [options objectForKey:CPMultipleValuesPlaceholderBindingOption] || @"Multiple Values"; + break; + + case CPNoSelectionMarker: + newValue = [options objectForKey:CPNoSelectionPlaceholderBindingOption] || @"No Selection"; + break; + + case CPNotApplicableMarker: + if ([options objectForKey:CPRaisesForNotApplicableKeysBindingOption]) + [CPException raise:CPGenericException reason:@"can't transform non applicable key on: "+_source+" value: "+newValue]; + + newValue = [options objectForKey:CPNotApplicablePlaceholderBindingOption] || @"Not Applicable"; + break; + } + + if (valueIsPlaceholder && + [_source respondsToSelector:@selector(setPlaceholderString:)] && + [_source respondsToSelector:@selector(setStringValue:)]) + { + [_source setStringValue:nil]; + [_source setPlaceholderString:newValue]; + } + else + [_source setValue:newValue forKey:aBinding]; + } + else + { + newValue = [self transformValue:newValue withOptions:options]; + [_source setValue:newValue forKey:aBinding]; + } } - (void)reverseSetValueFor:(CPString)aBinding @@ -200,20 +239,9 @@ var CPBindingOperationAnd = 0, if (valueTransformer) aValue = [valueTransformer transformedValue:aValue]; - switch (aValue) - { - case CPMultipleValuesMarker: return [options objectForKey:CPMultipleValuesPlaceholderBindingOption] || @"Multiple Values"; - case CPNoSelectionMarker: return [options objectForKey:CPNoSelectionPlaceholderBindingOption] || @"No Selection"; - - case CPNotApplicableMarker: if ([options objectForKey:CPRaisesForNotApplicableKeysBindingOption]) - [CPException raise:CPGenericException reason:@"can't transform non applicable key on: "+_source+" value: "+aValue]; - - return [options objectForKey:CPNotApplicablePlaceholderBindingOption] || @"Not Applicable"; - - case nil: - case undefined: return [options objectForKey:CPNullPlaceholderBindingOption] || nil; - } + if (aValue === undefined || aValue === nil) + aValue = [options objectForKey:CPNullPlaceholderBindingOption] || nil; return aValue; } @@ -447,3 +475,7 @@ CPValidatesImmediatelyBindingOption = @"CPValidatesImmediatelyBi CPValueTransformerNameBindingOption = @"CPValueTransformerNameBindingOption"; CPValueTransformerBindingOption = @"CPValueTransformerBindingOption"; +CPIsControllerMarker = function(/*id*/anObject) +{ + return anObject === CPMultipleValuesMarker || anObject === CPNoSelectionMarker || anObject === CPNotApplicableMarker; +} \ No newline at end of file From afc5b72092b1299206f2c76ab4c7c0e03e9a774d Mon Sep 17 00:00:00 2001 From: Klaas Pieter Annema Date: Tue, 21 Sep 2010 17:27:33 +0200 Subject: [PATCH 2/9] made setting binding placeholder values slightly less hacky --- AppKit/CPKeyValueBinding.j | 32 +++++++++++--------------------- 1 file changed, 11 insertions(+), 21 deletions(-) diff --git a/AppKit/CPKeyValueBinding.j b/AppKit/CPKeyValueBinding.j index b16f9579a..eda0dc8e3 100644 --- a/AppKit/CPKeyValueBinding.j +++ b/AppKit/CPKeyValueBinding.j @@ -146,18 +146,13 @@ var CPBindingOperationAnd = 0, var destination = [_info objectForKey:CPObservedObjectKey], keyPath = [_info objectForKey:CPObservedKeyPathKey], options = [_info objectForKey:CPOptionsKey], - newValue = [destination valueForKeyPath:keyPath]; + newValue = [destination valueForKeyPath:keyPath], + isPlaceholder = CPIsControllerMarker(newValue); - if (CPIsControllerMarker(newValue)) + if (isPlaceholder) { - var valueIsPlaceholder = YES; - switch (newValue) { - default: - valueIsPlaceholder = NO; - break; - case CPMultipleValuesMarker: newValue = [options objectForKey:CPMultipleValuesPlaceholderBindingOption] || @"Multiple Values"; break; @@ -173,22 +168,17 @@ var CPBindingOperationAnd = 0, newValue = [options objectForKey:CPNotApplicablePlaceholderBindingOption] || @"Not Applicable"; break; } - - if (valueIsPlaceholder && - [_source respondsToSelector:@selector(setPlaceholderString:)] && - [_source respondsToSelector:@selector(setStringValue:)]) - { - [_source setStringValue:nil]; - [_source setPlaceholderString:newValue]; - } - else - [_source setValue:newValue forKey:aBinding]; } else { + // Only transform the value if the current value is not a placeholder newValue = [self transformValue:newValue withOptions:options]; - [_source setValue:newValue forKey:aBinding]; } + + [_source setValue:newValue forKey:aBinding]; + + if ([_source respondsToSelector:@selector(_setCurrentValueIsPlaceholder:)]) + [_source _setCurrentValueIsPlaceholder:isPlaceholder]; } - (void)reverseSetValueFor:(CPString)aBinding @@ -276,7 +266,7 @@ var CPBindingOperationAnd = 0, var exposedBindings = [], theClass = [self class]; - while(theClass) + while (theClass) { var temp = [CPKeyValueBinding exposedBindingsForClass:theClass]; @@ -407,7 +397,7 @@ var invokeAction = function invokeAction(/*CPString*/targetKey, /*CPString*/argu var invocation = [CPInvocation invocationWithMethodSignature:[target methodSignatureForSelector:selector]]; [invocation setSelector:selector]; - var bindingName = argumentKey + var bindingName = argumentKey, count = 1; while (theBinding = [bindings objectForKey:bindingName]) From 098131e25243ae2e2f319aed7c8a051562a60fa5 Mon Sep 17 00:00:00 2001 From: Klaas Pieter Annema Date: Wed, 22 Sep 2010 11:44:41 +0200 Subject: [PATCH 3/9] coding guideline improvements for CPArrayController --- AppKit/CPArrayController.j | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/AppKit/CPArrayController.j b/AppKit/CPArrayController.j index 934314707..fe1d14419 100644 --- a/AppKit/CPArrayController.j +++ b/AppKit/CPArrayController.j @@ -111,7 +111,7 @@ return self; } --(void)prepareContent +- (void)prepareContent { [self _setContentArray:[[self newObject]]]; } @@ -148,7 +148,7 @@ - (void)setContent:(id)value { - if(![value isKindOfClass:[CPArray class]]) + if (![value isKindOfClass:[CPArray class]]) value = [value]; var oldSelectedObjects = nil, @@ -178,7 +178,7 @@ // We need to be in control of when notifications fire. _contentObject = value; - if(_clearsFilterPredicateOnInsertion) + if (_clearsFilterPredicateOnInsertion) [self __setFilterPredicate:nil]; // Causes a _rearrangeObjects. else [self _rearrangeObjects]; @@ -365,17 +365,17 @@ if (![indexes count]) { - if(_avoidsEmptySelection && [[self arrangedObjects] count]) + if (_avoidsEmptySelection && [[self arrangedObjects] count]) indexes = [CPIndexSet indexSetWithIndex:0]; } else { var objectsCount = [[self arrangedObjects] count]; // Remove out of bounds indexes. - [indexes removeIndexesInRange:CPMakeRange(objectsCount, [indexes lastIndex]+1)]; + [indexes removeIndexesInRange:CPMakeRange(objectsCount, [indexes lastIndex] + 1)]; // When avoiding empty selection and the deleted selection was at the bottom, select the last item. - if(![indexes count] && _avoidsEmptySelection && objectsCount) - indexes = [CPIndexSet indexSetWithIndex:objectsCount-1]; + if (![indexes count] && _avoidsEmptySelection && objectsCount) + indexes = [CPIndexSet indexSetWithIndex:objectsCount - 1]; } if ([_selectionIndexes isEqualToIndexSet:indexes]) @@ -418,7 +418,7 @@ count = [objects count], arrangedObjects = [self arrangedObjects]; - for (var i=0; i 0 } --(void)selectPrevious:(id)sender +- (void)selectPrevious:(id)sender { var index = [[self selectionIndexes] firstIndex] - 1; @@ -545,9 +545,9 @@ [self didChangeValueForKey:@"content"]; } --(void)add:(id)sender +- (void)add:(id)sender { - if(![self canAdd]) + if (![self canAdd]) return; [self insert:sender]; @@ -555,7 +555,7 @@ - (void)insert:(id)sender { - if(![self canInsert]) + if (![self canInsert]) return; var newObject = [self automaticallyPreparesContent] ? [self newObject] : [self _defaultNewObject]; @@ -576,13 +576,13 @@ - (void)addObjects:(CPArray)objects { - if(![self canAdd]) + if (![self canAdd]) return; var contentArray = [self contentArray], count = [objects count]; - for (var i=0; i Date: Wed, 22 Sep 2010 11:45:13 +0200 Subject: [PATCH 4/9] call selectionWill and didChange when setting selection indexes --- AppKit/CPArrayController.j | 2 ++ 1 file changed, 2 insertions(+) diff --git a/AppKit/CPArrayController.j b/AppKit/CPArrayController.j index fe1d14419..b4f3365e3 100644 --- a/AppKit/CPArrayController.j +++ b/AppKit/CPArrayController.j @@ -342,7 +342,9 @@ - (BOOL)setSelectionIndexes:(CPIndexSet)indexes { + [self _selectionWillChange] [self __setSelectionIndexes:indexes]; + [self _selectionDidChange]; } /* From 2fba03dc757925ccd3d3e07cdf7af344e7ed3135 Mon Sep 17 00:00:00 2001 From: Klaas Pieter Annema Date: Wed, 22 Sep 2010 13:10:48 +0200 Subject: [PATCH 5/9] fix style issues in CPTextField and CPKeyValueBindingTest --- AppKit/CPTextField.j | 16 ++++++++-------- Tests/AppKit/CPKeyValueBindingTest.j | 6 +++--- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/AppKit/CPTextField.j b/AppKit/CPTextField.j index cf5ec9465..3afc353b1 100644 --- a/AppKit/CPTextField.j +++ b/AppKit/CPTextField.j @@ -279,7 +279,7 @@ CPTextFieldStatePlaceholder = CPThemeState("placeholder"); _isEditable = shouldBeEditable; - if(shouldBeEditable) + if (shouldBeEditable) _isSelectable = YES; // We only allow first responder status if the field is editable and enabled. @@ -695,8 +695,8 @@ CPTextFieldStatePlaceholder = CPThemeState("placeholder"); - (void)textDidBlur:(CPNotification)note { - //this looks to prevent false propagation of notifications for other objects - if([note object] != self) + // this looks to prevent false propagation of notifications for other objects + if ([note object] != self) return; [[CPNotificationCenter defaultCenter] postNotification:note]; @@ -704,8 +704,8 @@ CPTextFieldStatePlaceholder = CPThemeState("placeholder"); - (void)textDidFocus:(CPNotification)note { - //this looks to prevent false propagation of notifications for other objects - if([note object] != self) + // this looks to prevent false propagation of notifications for other objects + if ([note object] != self) return; [[CPNotificationCenter defaultCenter] postNotification:note]; @@ -757,7 +757,7 @@ CPTextFieldStatePlaceholder = CPThemeState("placeholder"); Sets a placeholder string for the receiver. The placeholder is displayed until editing begins, and after editing ends, if the text field has an empty string value */ --(void)setPlaceholderString:(CPString)aStringValue +- (void)setPlaceholderString:(CPString)aStringValue { if (_placeholderString === aStringValue) return; @@ -881,7 +881,7 @@ CPTextFieldStatePlaceholder = CPThemeState("placeholder"); newValue = [stringValue stringByReplacingCharactersInRange:selectedRange withString:pasteString]; [self setStringValue:newValue]; - [self setSelectedRange:CPMakeRange(selectedRange.location+pasteString.length, 0)]; + [self setSelectedRange:CPMakeRange(selectedRange.location + pasteString.length, 0)]; } } @@ -1158,7 +1158,7 @@ var secureStringForString = function(aString) if (!aString) return ""; - return Array(aString.length+1).join(CPSecureTextFieldCharacter); + return Array(aString.length + 1).join(CPSecureTextFieldCharacter); } diff --git a/Tests/AppKit/CPKeyValueBindingTest.j b/Tests/AppKit/CPKeyValueBindingTest.j index f9fe62368..5face5a7a 100644 --- a/Tests/AppKit/CPKeyValueBindingTest.j +++ b/Tests/AppKit/CPKeyValueBindingTest.j @@ -39,7 +39,7 @@ [binder setCheese:@"banana"]; - [self assertTrue:[self valueForKey:@"FOO"]==="banana" message:"Bound value should have been updated to banana, was "+FOO]; + [self assertTrue:[self valueForKey:@"FOO"] === "banana" message:"Bound value should have been updated to banana, was " + FOO]; } - (void)testBindOptions @@ -164,7 +164,7 @@ [tableColumn bind:@"value" toObject:arrayController withKeyPath:@"arrangedObjects.valueA" options:nil]; // Reset these if they were read during initialization. - for(var i=0; i<[content count];i++) + for (var i = 0; i < [content count]; i++) [content[i] setAccesses:0]; var testView = [DataViewTester new]; [tableColumn prepareDataView:testView forRow:0]; @@ -242,7 +242,7 @@ CPNumber accesses @accessors; } -+ (AccessCounter) counterWithValueA:aValue valueB:anotherValue ++ (AccessCounter)counterWithValueA:aValue valueB:anotherValue { r = [self new]; [r setValueA:aValue]; From 8fe67b73dad4ec0f3792ae84ed64c1762d7b8daa Mon Sep 17 00:00:00 2001 From: Klaas Pieter Annema Date: Wed, 22 Sep 2010 13:11:32 +0200 Subject: [PATCH 6/9] implement _setCurrentValueIsPlaceholder in CPTextField CPKeyValueBinding will sent _setCurrentValueIsPlaceholder: to indicate to an object that current value is a placeholder and should be displayed as such. This commit implements the method for CPTextField and adds a unit test to test the behavior. --- AppKit/CPTextField.j | 26 ++++++++++++++++++++++++ Tests/AppKit/CPKeyValueBindingTest.j | 30 ++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/AppKit/CPTextField.j b/AppKit/CPTextField.j index 3afc353b1..0f5b00ebc 100644 --- a/AppKit/CPTextField.j +++ b/AppKit/CPTextField.j @@ -86,6 +86,8 @@ CPTextFieldStatePlaceholder = CPThemeState("placeholder"); CPColor _textFieldBackgroundColor; id _placeholderString; + id _originalPlaceholderString; + BOOL _currentValueIsPlaceholder; id _delegate; @@ -780,6 +782,30 @@ CPTextFieldStatePlaceholder = CPThemeState("placeholder"); return _placeholderString; } +- (void)_setCurrentValueIsPlaceholder:(BOOL)isPlaceholder +{ + if (isPlaceholder) + { + // Save the original placeholder value so we can restore it later + // Only do this if the placeholder is not already overridden because the bindings logic might call this method + // several times and we don't want the bindings placeholder to ever become the original placeholder + if (!_currentValueIsPlaceholder) + _originalPlaceholderString = [self placeholderString]; + + // Set the current string value as the current placeholder and clear the string value + [self setPlaceholderString:[self stringValue]]; + [self setStringValue:@""]; + } + else + { + // Restore the original placeholder, the actual textfield value is already correct + // because it was set using setValue:forKey: + [self setPlaceholderString:_originalPlaceholderString]; + } + + _currentValueIsPlaceholder = isPlaceholder; +} + /*! Size to fit has two behavior, depending on if the receiver is an editable text field or not. diff --git a/Tests/AppKit/CPKeyValueBindingTest.j b/Tests/AppKit/CPKeyValueBindingTest.j index 5face5a7a..5055cbad0 100644 --- a/Tests/AppKit/CPKeyValueBindingTest.j +++ b/Tests/AppKit/CPKeyValueBindingTest.j @@ -190,6 +190,29 @@ [self assert:'value' equals:testView.lastKey]; } +- (void)testTextField +{ + var textField = [[CPTextField alloc] initWithFrame:CGRectMakeZero()]; + [textField setPlaceholderString:@"cheese"]; + + + content = [ + [BindingTester testerWithCheese:@"yellow"], + [BindingTester testerWithCheese:@"green"] + ]; + arrayController = [[CPArrayController alloc] initWithContent:content]; + + [arrayController setSelectionIndexes:[CPIndexSet indexSetWithIndexesInRange:CPMakeRange(0, 2)]]; + + var options = [CPDictionary dictionaryWithJSObject:{CPMultipleValuesPlaceholderBindingOption:@"Multiple Values"}]; + [textField bind:@"value" toObject:arrayController withKeyPath:@"selection.cheese" options:options]; + + [self assert:@"Multiple Values" equals:[textField placeholderString]]; + + [arrayController setSelectionIndex:0]; + [self assert:@"cheese" equals:[textField placeholderString]]; +} + - (void)observeValueForKeyPath:(CPString)aKeyPath ofObject:(id)anObject change:(CPDictionary)changes context:(id)aContext { CPLog(@"here: "+aKeyPath+" value: "+[anObject valueForKey:aKeyPath]); @@ -202,6 +225,13 @@ id cheese; } ++ (id)testerWithCheese:(id)aCheese +{ + var tester = [[self alloc] init]; + [tester setCheese:aCheese]; + return tester; +} + - (void)setCheese:(id)aCheese { cheese = aCheese; From e59f20668bc4aeb418eb827f70527609c8d36367 Mon Sep 17 00:00:00 2001 From: Klaas Pieter Annema Date: Wed, 22 Sep 2010 13:35:36 +0200 Subject: [PATCH 7/9] don't set objectValue in resignFirstResponder if it didn't change This change makes sure that the textfield doesn't sent change notifications if the value didn't actually changed when resigning first responder. --- AppKit/CPTextField.j | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/AppKit/CPTextField.j b/AppKit/CPTextField.j index 0f5b00ebc..c51eddf18 100644 --- a/AppKit/CPTextField.j +++ b/AppKit/CPTextField.j @@ -545,7 +545,8 @@ CPTextFieldStatePlaceholder = CPThemeState("placeholder"); var element = [self _inputElement]; - [self setObjectValue:element.value]; + if ([self stringValue] !== element.value) + [self _setStringValue:element.value]; CPTextFieldInputResigning = YES; element.blur(); From d8c0d2da8e0a0e88f3ebc9027a1ff43718f01445 Mon Sep 17 00:00:00 2001 From: Klaas Pieter Annema Date: Thu, 23 Sep 2010 10:24:11 +0200 Subject: [PATCH 8/9] don't reverse set binding if textfield is showing a bindings placeholder --- AppKit/CPTextField.j | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/AppKit/CPTextField.j b/AppKit/CPTextField.j index c51eddf18..76086d325 100644 --- a/AppKit/CPTextField.j +++ b/AppKit/CPTextField.j @@ -714,6 +714,15 @@ CPTextFieldStatePlaceholder = CPThemeState("placeholder"); [[CPNotificationCenter defaultCenter] postNotification:note]; } +- (void)sendAction:(SEL)anAction to:(id)anObject +{ + // Don't reverse set our empty value + if (!_currentValueIsPlaceholder) + [self _reverseSetBinding]; + + [CPApp sendAction:anAction to:anObject from:self]; +} + /*! Returns the string the text field. */ From c9e0ab654fd4a132fc4fd21b33df8ff9f04cb5b1 Mon Sep 17 00:00:00 2001 From: Klaas Pieter Annema Date: Thu, 23 Sep 2010 10:58:20 +0200 Subject: [PATCH 9/9] test observations during setSelectionIndexes --- Tests/AppKit/CPArrayControllerTest.j | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/Tests/AppKit/CPArrayControllerTest.j b/Tests/AppKit/CPArrayControllerTest.j index 1b9e47160..dd6c574e8 100644 --- a/Tests/AppKit/CPArrayControllerTest.j +++ b/Tests/AppKit/CPArrayControllerTest.j @@ -245,6 +245,19 @@ [self assert:[CPIndexSet indexSetWithIndex:0] equals:observation.newValue message:"new selected index should be 0"]; } +- (void)testObservationDuringSetSelectionIndexes +{ + var arrayController = [self arrayController], + newContent = [self setupObservationFixture]; + + var newSelection = [CPIndexSet indexSetWithIndex:2]; + [arrayController setSelectionIndexes:newSelection]; + + [self assertNotNull:[arrayController selection] message:@"a selection was made, selection proxy should be defined"]; + [self assert:2 equals:[observations count] message:@"exactly 2 change notifications should be sent for new selection indexes"]; + [self assert:newSelection equals:[arrayController selectionIndexes] message:@"selection was not set properly"]; +} + - (void)observeValueForKeyPath:keyPath ofObject:anActivity change:change