Fixed: font cell of a CPTableView comes black when editing another cell

[Dogild]

The font of a default dataView came black when editing another cell of the tableView. Now it stays black.

Fixed #2130

[cappbot]

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

[cacaodev]

thanks man ! I beleive you mean it stays white.

[aljungberg]

At a glance this looks incorrect: wouldn't this cause the text to be white when the table is not actually the first responder? This is what we just fixed in a different issue recently.

[aljungberg]

Yes, this patch will not work. This patch causes a regression with regards to https://github.com/cappuccino/cappuccino/pull/2009.

Before:

With this pull request:

The text is no longer read-able.

+AppKit +#needs-patch

[cappbot]

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

[Dogild]

Can you share your example Alex please :)

[aljungberg]

Already did. :) It's this one: https://github.com/cappuccino/cappuccino/tree/master/Tests/Manual/ThemeKitchenSink

1. Click on a row in the top table.
2. Click on the bottom table so it becomes the new first responder, but the window is still key.

[Dogild]

Perfect got it.

Well, it's more complex that I have expected. The cells can exactly have the same themeState ("keyWindow+selectedTableDataView+tableDataView") for two cases ;

• The one from this issue, cell in a row where another cell is editing (the row is selected)
• The one Alex explained.

Solution could be to add another themeState ? Other idea ? The themeState of the textField seems good to me for the two cases.

There is another issue as well, select a row in the top table, begin to edit and then click on the bottom table, the font is white again (the themeState firstResponder is still set to the textField of the cell).

[aljungberg]

Yeah so the issue is that when the cell is edited, that cell becomes the first responder so the table 'loses' its first responder state.

I think in this case we should perhaps special case it. Even that the table view is not the first responder anymore, it should keep the first responder theme state since it 'hosts' the first responder in an obvious way.

So in CPTableView's editColumn, change this:

    [[self window] makeFirstResponder:view];

to this:

    [[self window] makeFirstResponder:view];
    [self _notifyViewDidBecomeFirstResponder]

This will add the first responder state back to the table view as a whole (and all its subviews) even that it's technically not the first responder.

Since it happens before any redraws, it shouldn't be visible to the user.

However this will only work if the table is view based so not sure this is the best solution.

We do want the table to know it's still a 'main control' from the user's point of view, even that a cell is being edited.

[Dogild]

I thought to add a new ThemeState named CPThemeStateChildFirstResponder, or something like that.

[Dogild]

Your solution works ;

It needs to add line 3702 and 1682 :

[self _notifyViewDidBecomeFirstResponder];

Do we want that or the new themeState ?

With this solution, we still have an issue :

• Start to edit a cell, click in the other tableview , the cells of the first tableView will still have the firstResponder attribute and the font will be white.

[aljungberg]

I thought to add a new ThemeState named CPThemeStateChildFirstResponder, or something like that. But the other cells aren't children of the first responder either, so they still wouldn't know. They're siblings.

Maybe a state like CPThemeStateFirstResponderLike. But there'd still be no automatic way to install that so we'd end up with special code in TableView anyhow, wouldn't we?

The other issue sounds like an unrelated bug -- something somehow not properly removing the first responder state in this particular transfer.

[Dogild]

Yeh I think it's going to be manual. I'll add the new themeState CPThemeStateFirstResponderLike and set it manually for the CPTableView

[Dogild]

I found a better way actually.

Let me know what you think

[primalmotion]

@aljungberg is that good enough for you?

[aljungberg]

What's different about this change versus what we discussed? :)

You know I just thought of another way that might look cleaner since it'd only be one area of change.

In CPTableView we can simply override _notifyViewDidResignFirstResponder and not call super if _isFocused is still true.

[Dogild]

Nothing much, I thought it's a lot of new code for a simple things finally (the new themeState), and maybe not the most optimized solution.

With what I proposed, the tableView will get the themeState firstResponder immediately when one of its subviews will be firstResponder and lost it when something else will be firstResponder. The main benefits are that it works with our actual issues (the both described above) and then it will work with every mode of the tableView, viewBased or not.

IMHO the themeState involved a lot of new code and complexity.

[aljungberg]

Are you proposing to give the first responder theme state to superviews of the the subview that becomes the first responder? It could work, I considered it too, but it'd make every single superview all the way up to the window itself take on the "first responder" theme state. So you get a lot of things with this new theme state where you might not expect it, like the window content view.

But if we do go that way, I'd probably name it CPThemeStateContainsFirstResponder. And yeah it would work well with the Table View.

I'm okay with this idea if that's what you meant. It'll work, it's generic and won't require so many special cases.

[Dogild]

Do we need it for every views ? I don't think so, right now just the tableView need this things.

What I meant, it's for these issues, the tableView will have the themeState firstResponder as long that one of its subviews or itself are firstResponder. That's it

[aljungberg]

Right, but that's already the case with the current patch, like we discussed, isn't it? Just trying to understand if there was something else you were suggesting.

Even that I misunderstood you, I think this latest idea is probably good. It's a general solution which fixes the problem that the solution we're doing here can't be (easily) by others since it uses internal APIs that might change and people rightfully shouldn't use that stuff.

At the same time this general solution would work for CPBrowser, CPOutlineView and maybe CPTokenField when editing an individual token. Since the number of steps up from any widget to the topmost superview is not huge it shouldn't be incredibly expensive.

Refining the idea we could at the same time rename CPThemeStateFirstResponder to CPThemeStateInFirstResponder. Then we have CPThemeStateContainsFirstResponder going upwards and CPThemeStateInFirstResponder going downwards (including the actual first responder). Very cleanly themable, easy to understand.

[Dogild]

I just tried to implement it and it doesn't work for our problem here.

When one of the texField comes firstResponder, it's going to tell to all of the superviews that they contain the current firstResponder. The problem is that the other textField of the same row are not one of the superview of the current editing textField, so they don't get the themeState CPThemeStateContainsFirstResponder.

[Dogild]

In case you wanna try

In CPView.j

- (BOOL)becomeFirstResponder
{
    var r = [super becomeFirstResponder];

    if (r)
    {
        [self _notifyViewDidBecomeFirstResponder];
        [self _notifySuperviewDidBecomeFirstResponder];
    }

    return r;
}

- (void)_notifySuperviewDidBecomeFirstResponder
{
    var superview = [self superview];

    while (superview)
    {
        [superview setThemeState:CPThemeStateContainsFirstResponder];
        superview = [superview superview];
    }
}

- (void)_notifyViewDidBecomeFirstResponder
{
    [self setThemeState:CPThemeStateInFirstResponder];

    var count = [_subviews count];

    while (count--)
        [_subviews[count] _notifyViewDidBecomeFirstResponder];
}

- (BOOL)resignFirstResponder
{
    var r = [super resignFirstResponder];

    if (r)
    {
        [self _notifyViewDidResignFirstResponder];
        [self _notifySuperviewDidResignFirstResponder]
    }

    return r;
}

- (void)_notifyViewDidResignFirstResponder
{
    [self unsetThemeState:CPThemeStateInFirstResponder];

    var count = [_subviews count];

    while (count--)
        [_subviews[count] _notifyViewDidResignFirstResponder];
}

- (void)_notifySuperviewDidResignFirstResponder
{
    var superview = [self superview];

    while (superview)
    {
        [superview unsetThemeState:CPThemeStateContainsFirstResponder];
        superview = [superview superview];
    }
}

[aljungberg]

Right, good point.

[Dogild]

It sounds like my last commit was the good one finally

[aljungberg]

Yep it's good. Merged. Thanks!

+#fixed assignee=aljungberg milestone=0.9.8

[cappbot]

Milestone: 0.9.8. Labels: #fixed, AppKit. What's next? This issue is considered successfully resolved.