Possible Regression in CPTextField?

[Mike Fellows]

Hi,

I'm having a problem with a recent change to master, specifically:

https://github.com/280north/cappuccino/commit/918f6fbb8f6de92550e9940f21df377fadc0aa02

I'm certain that that specific change introduced some problems in one of
my apps with editable cells in a CPTableView. It only seems to occur
when the enter/return key is used to finish the edit of the textfield.
Tabbing out of the textield or clicking elsewhere in the window doesn't
cause the problem and editing the textfield is successful.

When the enter/return key is pressed to finish the edit the
tableView:setObjectValue:forTableColumn:row:row callback is called
twice, the first time with the expected new value for the edited cell
and the second time with the 0th data value for the column as the object
value. In my case the 0th data value is the column header so the edited
cell ends up with the column header instead of what was entered during
the edit.

Is this expected behavior? If not any idea why the setObjectValue
callback is being called twice? I noticed that the commit from
Aparajita reworked the insertNewline... callbacks in CPTextField
significantly.

I can put together a test case if need be.

Regards,
Mike

[Alexander Ljungberg]

Hi,

If you put a breakpoint in tableView:setObjectValue:forTableColumn:row, who is the caller the second time? Normally the table view hooks itself up as the action receiver of the editing text field, and reacts to the action firing by triggering the update. But the action shouldn't fire twice.

Alexander

> --
> You received this message because you are subscribed to the Google Groups "Cappuccino & Objective-J Development List" group.
> To post to this group, send email to objecti...@googlegroups.com.
> To unsubscribe from this group, send email to objectivej-de...@googlegroups.com.
> For more options, visit this group at http://groups.google.com/group/objectivej-dev?hl=en.
>

[Mike Fellows]

I wasn't able to get the Chrome debugger to break or trace execution
properly (I haven't used it in a couple of Chrome versions and it
doesn't seem to work well with my Cappuccino apps anymore). I traced
executing the old fashioned way though.

It seems that the CPTextField in the table cell is calling it's action
callback twice. I think this occurs in all cases that the table cell
editing is finished off by pressing the return/enter key. But for most
apps it will be invisible as it makes the same change twice. The reason
why it stood out for me is because our
tableView:setObjectValue:forTableColumn:row:row callback sorts the table
column and reloads the data - so the second callback clobbered the data
that was moved into the old location for the cell that was edited.

I think this is probably a regression and have a fairly good
understanding of the the order of events. But I'm not sure what should
be fixed. Here is what happens:

1. Edit a cell in the table and press enter.

2. You end up in CPTextField's insertNewline function. This function
was rearranged by the fix I noted originally - and the changes cause the
second callback to occur.

3. In the new insertNewline function line 781 is reached:

if (![self action] || [self sendAction:[self action] to:[self target]])

the [self sendAction:... callback is made. It will make the call out to
your tableview:setObjectValue... function but after, along the way,
something will call the CPTextField's resignFirstResponder function.

4. Once in resignFirstResponder it will hit lines 628 to 635:

if (_isEditing)
{
_isEditing = NO;
[self textDidEndEditing:[CPNotification
notificationWithName:CPControlTextDidEndEditingNotification object:self
userInfo:nil]];

if ([self sendsActionOnEndEditing])
[self sendAction:[self action] to:[self target]];
}

Which calls the [self sendAction: callback again.

The reason why this used to work is that the old insertNewline function
had a check before the first call to [self sendAction: that would flip
the _isEditing flag from YES to NO. So when the clause shown in step 4
above was hit _isEditing was always NO and the extra call to [self
sendAction: was not made.

Apologies for the long drawn out description but it is not clear why the
changes were made to insertNewline and I didn't want to submit a patch
without first discussing it.

Any insight would be appreciated.

Regards,
Mike

On 11-07-14 03:42 PM, Alexander Ljungberg wrote:
> Hi,
>
> If you put a breakpoint in tableView:setObjectValue:forTableColumn:row, who is the caller the second time? Normally the table view hooks itself up as the action receiver of the editing text field, and reacts to the action firing by triggering the update. But the action shouldn't fire twice.
>
> Alexander
>
> On 14 Jul 2011, at 18:19, Mike Fellows wrote:
>
>> Hi,
>>
>> I'm having a problem with a recent change to master, specifically:
>>
>> https://github.com/280north/cappuccino/commit/918f6fbb8f6de92550e9940f21df377fadc0aa02
>>
>> I'm certain that that specific change introduced some problems in one of my apps with editable cells in a CPTableView. It only seems to occur when the enter/return key is used to finish the edit of the textfield. Tabbing out of the textield or clicking elsewhere in the window doesn't cause the problem and editing the textfield is successful.
>>
>> When the enter/return key is pressed to finish the edit the tableView:setObjectValue:forTableColumn:row:row callback is called twice, the first time with the expected new value for the edited cell and the second time with the 0th data value for the column as the object value. In my case the 0th data value is the column header so the edited cell ends up with the column header instead of what was entered during the edit.
>>
>> Is this expected behavior? If not any idea why the setObjectValue callback is being called twice? I noticed that the commit from Aparajita reworked the insertNewline... callbacks in CPTextField significantly.
>>
>> I can put together a test case if need be.
>>
>> Regards,
>> Mike
>>
>> --

>> You received this message because you are subscribed to the Google Groups "Cappuccino& Objective-J Development List" group.

[Mike Fellows]

I've poked around a bit more and can describe the problem more
succinctly now.

In CPTextField the textfield's target action is called in two places, in
resignFirstResponder and in insertNewline. So if the target action
happens to change the responder from the textfield to something else
then the target action gets called twice. The second time from the
resignFirstResponder method - and in that method the _idEditing flag is
flipped to NO which breaks out of what would otherwise be an endless loop.

In the case of an editable textfield in a CPTableView the
_commitDataViewObjectValue method is set as the textfield's target
action. And the CPTableView is set to be the first responder at the end
of that function causing the double callback just after it calls the
table view's setObjectValue: delegate.

I'll put together a simple test case and propose a patch to fix this
(more than likely I'll just revert the change to insertNewline).

Regards,
Mike

[Mike Fellows]

I've added a pull request with my proposed fix:

https://github.com/280north/cappuccino/pull/1320

Mike

[Aparajita Fishman]

> I've added a pull request with my proposed fix:

Thanks for tracking it down, I had not thought to test with CPTableView.

Regards,

Aparajita
www.aparajitaworld.com

"If you dare to fail, you are bound to succeed."
- Sri Chinmoy | www.srichinmoy.org

[Alexander Ljungberg]

I have merged your changes, thanks. Fantastic manual test by the way, very clear and to the point.

Alexander

> You received this message because you are subscribed to the Google Groups "Cappuccino & Objective-J Development List" group.