CPTableView formatting and fixes [+1]

[cacaodev]

There might be a "breaking change" here, in commit be7f6ae. I didn't mention it in the commit message because i wanted your input first. Previously, -reloadData was reloading everything: the views, the data and performing various tasks needed to setup the view. Now it just fetches the data from the model and applies it to the view. Except when the number of rows changed: in this case we reload the views like we did before meaning that when calling -reloadData from the controller after a row insertion/removal, there should be no change. To be sure nothing breaks, i would like to know how you use -reloadData in your controllers and if you need the views to be reloaded even when the number of rows in the content did not change.

There is one situation I can think of where reloading the views might be useful but i don't think anyone ever tested it. It's when using view-based tables with the delegate for creating views. You may want to initiate a remote request to fetch some data (lazy loading) and then create a view in the delegate based on the content. In this case, you need to tell the table to reload one or more specific views when the data is received. If we do this, we will need a new implementation, not the previous reloadData (renamed _reloadDataViews here) . This method was not fetching the views from the delegate, instead, we were just enqueuing the views in the cache and redistributing them with new values and new frames, which is not enough.

[cappbot]

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

[ahankinson]

Looks like it fails the unit testing. Could you have a look at the Travis error and either verify the test, or fix the commit?

The Travis error is:

CPTableViewTest..
addFailure test=[CPTableViewTest testNumberOfRowsChangedSelectionNotification]: didChange notifications when selected rows disappear expected:<2> but was:<1>
Trace not implemented
.
addError test=[CPTableViewTest testEditCell]: Cannot read property "37802" from undefined
    at script(/home/travis/build/cappuccino/cappuccino/narwhal/packages/narwhal-lib/lib/narwhal/sandbox.js:118)
    at script(/home/travis/build/cappuccino/cappuccino/narwhal/packages/narwhal-lib/lib/narwhal/sandbox.js:241)
    at script.narwhal(/home/travis/build/cappuccino/cappuccino/narwhal/narwhal.js:290)
...

-#new +feature +AppKit +#needs-improvement

[cappbot]

Milestone: Someday. Labels: #needs-improvement, AppKit, feature. What's next? The code for this issue has problems with formatting or fails a capp_lint check, has bugs, or has non-optimal logic or algorithms. It should be improved upon.

[ahankinson]

-#needs-improvement +#accepted +#ready-to-commit

[cappbot]

Milestone: Someday. Labels: #accepted, #ready-to-commit, AppKit, feature. What's next? The changes for this issue are ready to be committed by a member of the core team.

[aljungberg]

This looks great. Any chance I could prevail on you to merge in the latest master (or rebase) so we can get a clean commit?

assignee=aljungberg milestone=0.9.7

[cappbot]

Assignee: aljungberg. Milestone: 0.9.7. Labels: #accepted, #ready-to-commit, AppKit, feature. What's next? The changes for this issue are ready to be committed by aljungberg.

[aljungberg]

-#ready-to-commit

[cappbot]

Assignee: aljungberg. Milestone: 0.9.7. Labels: #accepted, AppKit, feature. What's next? A reviewer should examine this issue.

[cappbot]

Assignee: aljungberg. Milestone: 0.9.8. Labels: #accepted, AppKit, feature. What's next? A reviewer should examine this issue.

[cappbot]

Assignee: aljungberg. Milestone: 0.9.9. Labels: #accepted, AppKit, feature. What's next? A reviewer should examine this issue.

[cacaodev]

This branch is no longer mergeable, mainly because of master changes in CPTableColumnHeader class

I will open individual issues for the fixes that have no dependencies.

[cacaodev]

It's mergeable after all. What's left :

• removeTableColumn: hard error. (Added a ojunit test for that : CPTableViewTableColumnTest.j).
• See what fe260a8 was supposed to fix and if there is a regression.

[Dogild]

Any news for this PR ?

[cacaodev]

I am not near my machine currently so I can't check it again with recent master changes. Last news was: everything works except removeTableColumn: hard error located in the -load method and related to the run loop (cf ojunit and manual test) I could use some help here because I am i bit a court d'idees.

[Dogild]

I'll take a look this week ;)

[Dogild]

In this part of the code :

- (void)removeTableColumn:(CPTableColumn)aTableColumn
{
    if ([aTableColumn tableView] !== self)
        return;

    var index = [_tableColumns indexOfObjectIdenticalTo:aTableColumn];

    if (index === CPNotFound)
        return;

    // we defer the actual removal until the end of the runloop in order to keep a reference to the column.
    [_differedColumnDataToRemove addObject:{"column":aTableColumn, "shouldBeHidden": [aTableColumn isHidden]}];

    [aTableColumn setHidden:YES];
    [aTableColumn setTableView:nil];

    var tableColumnUID = [aTableColumn UID];

    if (_objectValues[tableColumnUID])
        _objectValues[tableColumnUID] = nil;

    if (_dirtyTableColumnRangeIndex < 0)
        _dirtyTableColumnRangeIndex = index;
    else
        _dirtyTableColumnRangeIndex = MIN(index, _dirtyTableColumnRangeIndex);

    [_tableColumns removeObject:aTableColumn];
    [self _reloadDataViewsImmediately];
}

We call [_tableColumns removeObject:aTableColumn];. We should call this method at this end of the method - (void)load, otherwise the indexes of the array of tableColum is totally messed up when unloading and reloading the dataViews.

I would suggest to do that here (load method) :

if (removeCount)
    {
        var removeIndexes = [CPIndexSet indexSet];

        for (var i = 0; i < removeCount; i++)
        {
            var data = _differedColumnDataToRemove[i],
                tableColumn = data.column,
                columnIdx = [_tableColumns indexOfObject:tableColumn];

            if (columnIdx !== CPNotFound)
                [removeIndexes addIndex:columnIdx];
        }

        // FIXME: do we also need to remove non exposed rows that may stay in the cache ?
        [self _unloadDataViewsInRows:_exposedRows columns:removeIndexes];

        // Here we can loop on the removeIndexes and call the method - (void)removeTableColumn:(CPTableColumn)aTableColumn

        [_differedColumnDataToRemove removeAllObjects];
    }

What do you think ?

[cacaodev]

Hi Dogild, thanks for this, i will take a look asap.

[cacaodev]

This PR is almost done, finally. Thanks doglid for your help. -removeTableColumn now works like before.

What's left in my opinion:

• [x] Test the method -getColumn:row:forView for speed perf and infinite loops because it is now used in multiple places. We are passing it the FR which can be anything.
• [x] A test is failing in CPTableViewTest.j : this is because the dataView prototype is changed after the tablecolumns were binded. It's a regression but is it important to allow the dataView to change after setup ?
• [ ] -reloadDataForRows:columns: still not really implemented. It seems obvious but it's not.
• [ ] -reloadData reloads the data AND the views (like before, like cocoa) BUT the views come from the cache, they are not fresh views.

[Dogild]

Sounds good, the tests pass again ! Let me know when you think it's ready so I can try it on our app ;)

[cacaodev]

For me it's ready, at least to be reviewed. The only thing i'd like to improve is the last item in my previous message: a user call to -reloadData should recreate the views (currently they are mixed). This could be useful in a situation where views are dynamically created based on a criteria and this criteria changes, for example a table with remote data.

It's not a regression so imo we can do this later.

[Dogild]

I tried it on our application, I didn't have any bugs yet.

[Dogild]

Any chance to have the feature to have selectable CPTextField (non-editable) in a cell-based CPTableView when selecting a row is not allowed (like in iMessage, to copy and past messages) ?

[Dogild]

My bad, it works as in cocoa already !

[Dogild]

Well actually not really, you have to select the row and then you can select the text

[Dogild]

Fixed here : https://github.com/cappuccino/cappuccino/pull/2231

[daboe01]

+#ready-to-commit

[cappbot]

Assignee: aljungberg. Milestone: 0.9.9. Labels: #accepted, #ready-to-commit, AppKit, feature. What's next? The changes for this issue are ready to be committed by aljungberg.

[primalmotion]

I think we need to get done with this one once for all.

@cacaodev can you make it mergeable, and ping me once ok, so I can merge it in? Thanks

[cacaodev]

I'll merge this but first I need to know @Dogild what behavior you expect with #2231 : 1- Select the row AND edit the text 2- Just edit the text (make the textfield FR) without selecting the row (selectedRow unchanged).

[Dogild]

@cacaodev In cocoa, when you set the selectionStyle to CPTableViewSelectionHighlightStyleNone you can still focus elements in a row.

Normally, to focus a CPTextField, there are two steps, the first one is to select the row (it becomes blue) and then you can focus a CPTextField. With the style set to None, you will focus the CPTextField (or another CPView) after the firstClick.

[cacaodev]

@Dogild OK but what I want to know is that if you want the row to be the selectedRow in the TableView when the text field if focused (even if it is not visually selected of course) or not.

[Dogild]

I don't know how cocoa reacts with this situation. We need to try...I guess it updates the selectedRow.

[primalmotion]

@cacaodev can we get rid of this one once for all :)

Can you make it mergeable and then tell me? I'll merge it right away

Thanks

+#needs-improvement -#ready-to-commit

[cappbot]

Assignee: aljungberg. Milestone: 0.9.9. Labels: #accepted, #needs-improvement, AppKit, feature. What's next? The code for this issue has problems with formatting or fails a capp_lint check, has bugs, or has non-optimal logic or algorithms. It should be improved upon.

[daboe01]

hi @cacaodev, i need this one badly ;-) +1

[cappbot]

Assignee: aljungberg. Milestone: 0.9.9. Vote: 1. Labels: #accepted, #needs-improvement, AppKit, feature. What's next? The code for this issue has problems with formatting or fails a capp_lint check, has bugs, or has non-optimal logic or algorithms. It should be improved upon.

[cacaodev]

@primalmotion @daboe01 : hey guys, sorry for the delay but I don't have much time currently to work on this. I'll have a look when I can. If someone want to finish the work, you can merge it, fix the conflicts, and then fix issues with highlighting. From what i remember, in some situations the highlighted state was not correctly set on the main cell view or subviews.

[cacaodev]

@Dogild : can you re-apply e56cc85e963a (i'm not sure what to check) and then i think that's it.

[Dogild]

Let me try that asap ;)

[primalmotion]

@cacaodev awesome

@Dogild I have no doubt you'll tell me when you 're done :)

[Dogild]

@primalmotion I'm kinda in a middle of a huge big thing ... :p

[Dogild]

@primalmotion @cacaodev Archipel and Nuage works and not crash with this PR. @cacaodev #e56cc85 is still good in the PR

[primalmotion]

@cacaodev can you sum up what this PR actually does? Thanks!

Antoine Mercadal

On Jan 29, 2015, at 13:09, Alexandre Wilhelm notifications@github.com wrote:

@primalmotion @cacaodev Archipel and Nuage works and not crash with this PR. @cacaodev #e56cc85 is still good in the PR

— Reply to this email directly or view it on GitHub.

[cacaodev]

@primalmotion New features are: new methods -enumerateAvailableViewsUsingblock: and -preparedViewAtColumn:row: Then there are a lot of bug fixes related to column d&d, minor fixes for content bindings and setDataView: Internally, the code is rearranged and simplified thanks to the new view enumeration order (row -> column) and editing for both view-based and cell-based is now unified.

[primalmotion]

finally merged!

Thanks!

+#fixed

[cappbot]

Assignee: aljungberg. Milestone: 0.9.9. Vote: 1. Labels: #fixed, AppKit, feature. What's next? This issue is considered successfully resolved.

[cacaodev]

There is a style issue with this commit. A lot of brackets are not correctly aligned. It may be because of a setting in my merging tool. Sorry for that. I believe the only solution is to manually fix this (capp_lint does not detect that). I'll do it.