Bug: CPTableView row caching is wrong #509

Closed
klaaspieter opened this Issue Feb 26, 2010 · 9 comments

Comments

Projects
None yet
6 participants
Contributor

klaaspieter commented Feb 26, 2010

First, the row caching in general is one big magic chunk of code of which I'm not sure if everything is even used.

Second there is a bug in CPTableColumn _newDataViewForRow. The code there asks the tableview for it's cached dataviews and always returns the last dataView. Effectively always reversing the dataviews used for each row. Example:

Row 0: dataview 1
Row 1: dataview 2
Row 2: dataView 3

After reloadData this becomes:

Row 0: dataview 3
Row 1: dataview 2
Row 2: dataview 1

I've tried fixing this by removing the first item of the cachedDataViews but it doesn't work. Like I said at the start there is just to much caching stuff going on in various places for any fix (except a rewrite) to be reliable.

Member

boucher commented Feb 26, 2010

I don't quite see how the caching thing is a bug. The order the cached views are used in doesn't seem particularly important.

Contributor

cacaodev commented Feb 26, 2010

[edit]
_cachedDataViews is not really the equivalent of a dataview prototype. I'm removing my comment because i don't want to add confusion to this already complex debate. Hope it's OK.

Member

boucher commented Feb 26, 2010

We'll never be able to magically cache the views returned from such a delegate method. The solution will have to be caching by identifier, like in the iPhone table view world.

Contributor

appden commented Feb 26, 2010

This definitely causes a bug because if you go into editing mode on a cell, then resize the entire window, you'll see the editing cell jump up and down as you resize because of the reordering issue. We need to at least lock the editing cell from being recycled, this is important for me especially because I'm using variable row heights and growing a row while editing it causes it to jump also.

Contributor

klaaspieter commented Feb 26, 2010

This is also an issue with dataview that contains controls.

If the textfield in the last row is first responder and data is reloaded the dataview moves to row 0 because the dataview still contains the same textfield the first responder state will appear to have jumped (assuming that the textfield is an instance variable or ephemeral subview).

This is also an issue because caching isn't very useful if on every reload a different view is returned. The tableview will change the object value of the view and the view will have to redraw and relayout reducing the effectiveness of caching all together.

Like I said in the original ticket the caching behavior of CPTableView is a bit of a mess currently. There are a lot of different places where cells are created / cached.

I believe a complete rewrite of the caching system by a single person or a couple of persons communicating heavily is the best solution right now. Before that we should probably discuss what should be cached, when it should be cached and where it should be cached. Shall I open a discussion on the mailing list for this?

cappbot commented May 9, 2012

Milestone: 1.0. Labels: #accepted, #needs-patch, #needs-reduction, AppKit, bug. What's next?

  • This issue needs a volunteer to write and submit code to address it.
  • A minimal test app should be created which demonstrates the concern of this issue in isolation.
Contributor

ahankinson commented Dec 1, 2014

Browsing through the commit logs I can see that _newDataViewForRow:column has been rewritten in the interim.

If there is a specific performance problem with the new implementation then a new issue should be opened. For now, I'll close this.

+#fixed

cappbot commented Dec 1, 2014

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

cappbot closed this Dec 1, 2014

cappbot commented Dec 1, 2014

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

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