CPTableView formatting and fixes [+1] #1892

Merged
merged 41 commits into from Feb 6, 2015

Conversation

Projects
None yet
8 participants
Contributor

cacaodev commented Apr 2, 2013

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.

cacaodev added some commits Mar 30, 2013

@cacaodev cacaodev Fixed: In CPTrace, fixed error when an argument was not a cappuccino …
…object.

Arguments such as CGPoint, CGRect and functions are now correctly
printed to the console.
377ec37
@cacaodev cacaodev New: -enumerateAvailableViewsUsingBlock: and private -_enumerateViews…
…InRows:columns:usingBlock: and -_enumerateViewsInRows:tableColumns:usingBlock: methods.

New: preparedViewAtColumn:row:

These methods allow to enumerate visible data views or data views in specified columns and rows.
-enumerateAvailableViewsUsingBlock: is the counterpart of cocoa's -enumerateAvailableRowViewsUsingBlock: except that it enumerates data views instead of CPTableRowView and the block has an additional column parameter.

This commit reverses the way views are stored and accessed in the table view: rows>columns instead of columns>rows.
Applied the second method where it is relevant, making the code more compact and readable.
714da0a
@cacaodev cacaodev Fixed: -reloadDataForRowsIndexes:columnIndexes: behavior.
Fixed: Column dragging performance, content binding performance.

After this commit, -reloadDataForRowsIndexes:columnIndexes: reloads the data and the data only.
The -reloadData method no longer tries to reuse the views, instead it just reloads the data for visible views.
To flush and reload the views cache for visible rows, use _reloadDataViews.
To internaly layout the views geometry, use _layouViewsForRowIndexes:columnIndexes:

In CPTableView dragging column code, relayout the views whose frame changed instead of reloading everything.
a6c143a
@cacaodev cacaodev Fixed: After drag, views were cached.
This commit reverts #1478. It appears it is a bad idea to enqueue
visible views. The views generally stay in the cache forever and they
are repeatedly asked to be removed from the table at each load, causing
a performance penalty.
7c6765b
@cacaodev cacaodev Fixed: editingColumn and editingRow return the correct index in view …
…based tables

Fixed: An edited view no longer send its action (view-based) or commit its object value (cell-based) when unexposed.
Fixed: Cell based tables support for editing any control, not just textfield and buttons.
eae9a10
@cacaodev cacaodev Fixed: noteHeightOfRowsWithIndexesChanged: was not showing immediatly…
… row height changes.

Todo: instead of reloading everything we should be able to just
relayout frames for visible views and then tile.
690ae1a
@cacaodev cacaodev Fixed: selectColumnIndexes:byExtendingSelection: returns early when t…
…here is no change in columns selection

Before this change, rows could be unselected even if the columns
selection did not change.
a4bf6c1
@cacaodev cacaodev Fixed: -moveColumn:to: now preserve selected columns
Fixed: Starting a column drag is now faster.
Fixed: When dragging a selected column, selection is now drawn on the dragging view and the cursor is the closed hand.
Fixed: When dragging a table column, underlying columns were sliding according to the tracking location instead of the column lateral edges.
Fixed: In CPTableView, the drop indicator for rows could appear when dragging a column.

This commit creates directly the dragging column instead of relying on
built-in drag&drop. Also fixes a bug where the drop indicator would appear when
dragging a column if some rows were previously drag&dropped.
984c7a6
@cacaodev cacaodev Fixed CPOutlineView -reloadData and _layoutViewsForRowIndexes:columnI…
…ndexes: subclassing.
be7f6ae
@cacaodev cacaodev Fixed CPOutlineView subclassing when settting CPThemeStateSelectedDat…
…aView
ba3c602
@cacaodev cacaodev Fixed CPOutlineView dragging column subclassing 44b6136

cappbot commented Apr 2, 2013

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

Contributor

ahankinson commented Apr 2, 2013

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 commented Apr 2, 2013

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.

cacaodev added some commits Apr 4, 2013

@cacaodev cacaodev Fixed: -reloadData: was not reloading views after a change in model r…
…ows count
0a1d215
@cacaodev cacaodev Remove old code. Adds Equality check in -setDataView: b42b412
@cacaodev cacaodev Fixed: data views were not reloaded after a CPTableColumn setDataView:
Cell-based table views with no identifier set for the table column:
Before this commit, the caching system was asking for a view identified
by the tableColumn UID. If the table column data view was changed
externaly, the previoulsy cached data views were loaded instead of the
new ones. The views are now identified by the -dataview UID.

Test: AppKit/TableTest -testLayout were a custom data view is set.
06391c9
@cacaodev cacaodev Fixed: in some circumstances, table views binded with the content bin…
…ding were not correctly reloading the data.

Test: AppKit/WithOrWithoutbinding
87acd9d
@cacaodev cacaodev Fixed: The data views were not loaded immediately
When reloading the table view, the actual loading (-load) is defered
until layout is needed (generally in the next run loop). This is an
advantage because it minimize reloads but in some case it is necessary
to force a reload, for example when we need to access data views, or
manually edit a view, or when we explicitely ask for a reload.
This commit adds _reloadDataViewsImmediately and make use of it when
necessary.

Tests: AppKit/CPTableViewTest -> -testEditCell
8d69c37
@cacaodev cacaodev Fixed: CPTableViewTest: -deselectAll should send 1 CPtableViewSelecti…
…onDidChangeNotification, not 2.

Checked in cocoa.
Moved a failing test to the end of the method so it does not shadow other tests.
7b1c696
@cacaodev cacaodev Simplified -numberOfRows
Added -_numberOfRows that computes the new number of rows. Removed the
nil check.

All tests in AppKit/CPTableViewTest are now passing with success.
43f0f2a
@cacaodev cacaodev Merge remote-tracking branch 'upstream/master' into CPTableView-enume…
…rateRows
548a557
@cacaodev cacaodev Remove check in -hitTest: that was preventing reverse set binding e53337b
Contributor

ahankinson commented Apr 14, 2013

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

cappbot commented Apr 14, 2013

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.

Me1000 was assigned Apr 21, 2013

@cacaodev cacaodev Fixed removeTableColumn: was not reloading views immediatly. Added te…
…st for issue 1913 in Manual/TableTest/TestTableColumn. Throws an exception when you click the button twice
4d8f7c6
Owner

aljungberg commented Jun 12, 2013

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 commented Jun 12, 2013

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 was assigned Jun 12, 2013

Owner

aljungberg commented Aug 12, 2013

-#ready-to-commit

cappbot commented Aug 12, 2013

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

cappbot commented Dec 9, 2013

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

@aljungberg aljungberg modified the milestone: 0.9.9, 0.9.8 Apr 17, 2014

cappbot commented Apr 17, 2014

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

Contributor

cacaodev commented Apr 17, 2014

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 added some commits Apr 18, 2014

@cacaodev cacaodev Merge remote-tracking branch 'upstream/master' into CPTableView-enume…
…rateRows

Possible regression from #fe260a8
Regression: -reloadData does not reload views any more even if the table is empty (see CPOutlineViewCibTest).
BUG: -removeTableColumn: error.

Conflicts:
	AppKit/CPOutlineView.j
	AppKit/CPTableHeaderView.j
	AppKit/CPTableView.j
3a97db8
@cacaodev cacaodev New: ojunit tests for CPTableView -reloadData & -removeTableColumn:
-removeTableColumn: is failing with an « index out of bounds » error.
57408d1
@cacaodev cacaodev Code style
Rename _numberOfRowsDidChange -> _dataViewsNeedReloadAfterContentChange.
Subclasses use this method to tell if a full view reloading is needed
when calling -reloadData.
Currently CPOutlineView returns YES - this is the previous behavior.

Added private - (void)_reloadDataForRowIndexes:(CPIndexSet)rowIndexes
columnIndexes:(CPIndexSet)columnIndexes
This is the internal method for reloading objectValues only.
45a59f9
Contributor

cacaodev commented Apr 19, 2014

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.
Owner

cacaodev commented on a4bf6c1 Apr 19, 2014

The commit message is not very clear.
The test is:

  • A row is selected (all columns are unselected)
  • -selectColumnIndexes:[CPIndexSet indexSet] byExtendingSelection:NO was deselecting the selected row and it should not.
Member

Dogild commented Jun 26, 2014

Any news for this PR ?

Contributor

cacaodev commented Jun 27, 2014

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.

Member

Dogild commented Jun 30, 2014

I'll take a look this week ;)

Member

Dogild commented Jul 23, 2014

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 ?

Contributor

cacaodev commented Jul 26, 2014

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

@cacaodev cacaodev Fixed: -removeTableColumn: now works without error. OJTest in AppKit/…
…CPTableColumnTest.j, manual test in TableTest/TestTanleColumn/

Fixed: Added an out of bounds check to _unloadDataViews:...
Revert: revert -reloadData to the previous behavior where views & data were reloaded, not only data. That's what cocoa does for view based tables.
3e8afae
Contributor

cacaodev commented Sep 12, 2014

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

What's left in my opinion:

  • 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.
  • 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.

cacaodev added some commits Sep 18, 2014

@cacaodev cacaodev Merge remote-tracking branch 'upstream/master' into CPTableView-enume…
…rateRows
b775cbb
@cacaodev cacaodev NEW: ojtest for method - (void)getColumn:(Function)columnRef row:(Fun…
…ction)rowRef forView:(CPView)aView

This internal method is used for editing and public methods rowForView: and columnForView
2dccd2f
@cacaodev cacaodev CPTableView: -_reloadDataViewsImmediately -> -reloadData
CPTableColumn: binder:-setValueFor:  is a simple data reload.
f820991
@cacaodev cacaodev view-based table: always use the proto UID as view identifier
-setDataView: was not working because the caching system was picking
views cached with the column identifier which is persistent.

Test: CPTableViewTest -testLayout
1a414ac
Member

Dogild commented Sep 23, 2014

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

Contributor

cacaodev commented Sep 23, 2014

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.

Member

Dogild commented Sep 24, 2014

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

Member

Dogild commented Oct 24, 2014

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) ?

Member

Dogild commented Oct 24, 2014

My bad, it works as in cocoa already !

Member

Dogild commented Oct 24, 2014

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

Member

Dogild commented Oct 24, 2014

Fixed here : #2231

Contributor

daboe01 commented Oct 25, 2014

+#ready-to-commit

cappbot commented Oct 25, 2014

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.

Owner

primalmotion commented Nov 17, 2014

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

Contributor

cacaodev commented Nov 18, 2014

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).

Member

Dogild commented Nov 18, 2014

@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.

Contributor

cacaodev commented Nov 18, 2014

@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.

Member

Dogild commented Nov 18, 2014

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

Owner

primalmotion commented Jan 9, 2015

@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 commented Jan 9, 2015

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.

Contributor

daboe01 commented Jan 16, 2015

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

cappbot commented Jan 16, 2015

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.

cappbot changed the title from CPTableView formatting and fixes to CPTableView formatting and fixes [+1] Jan 16, 2015

Contributor

cacaodev commented Jan 16, 2015

@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.

Contributor

cacaodev commented Jan 27, 2015

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

Member

Dogild commented Jan 29, 2015

Let me try that asap ;)

Owner

primalmotion commented Jan 29, 2015

@cacaodev awesome

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

Member

Dogild commented Jan 29, 2015

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

Member

Dogild commented Jan 29, 2015

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

Owner

primalmotion commented Jan 29, 2015

@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.

Contributor

cacaodev commented Jan 30, 2015

@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 primalmotion added a commit that referenced this pull request Feb 6, 2015

@primalmotion primalmotion Merge pull request #1892 from cacaodev/CPTableView-enumerateRows
CPTableView formatting and fixes [+1]
f9ce1d1

@primalmotion primalmotion merged commit f9ce1d1 into cappuccino:master Feb 6, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
Owner

primalmotion commented Feb 6, 2015

finally merged!

Thanks!

+#fixed

cappbot commented Feb 6, 2015

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

Contributor

cacaodev commented Feb 7, 2015

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.

cacaodev deleted the cacaodev:CPTableView-enumerateRows branch Feb 18, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment