Bug: CPOutlineView overrides tableview delegate and datasource #437

Open
klaaspieter opened this Issue Feb 1, 2010 · 16 comments

Comments

Projects
None yet
8 participants
Contributor

klaaspieter commented Feb 1, 2010

In my opinion overriding the delegate and datasource methods to return another instance variable is bad practice. If the tableview calls [self delegate] anywhere it could potentially receive the outline view it's delegate in stead.

Member

boucher commented Feb 2, 2010

This is how NSOutlineView behaves. I agree it's pretty stupid, but I'm not sure we should change it.

Member

boucher commented Feb 2, 2010

OK, update. They are actually the same object. So the mistake is ours. We should be using the same delegate as both the table view and data source delegate. (and datasource). Patch welcome.

Contributor

klaaspieter commented Feb 3, 2010

I do not have time to implement this, but I do have some ideas about how to implement it, so whoever does have the time, please contact me :)

Contributor

cacaodev commented Mar 25, 2010

Here is a patch that moves all delegate&datasource methods into private instance methods in both table & outline. All these methods are in the same order at the end of both impl. with //unimplemented comments when needed.

18005e204bba1d2913f8896305b3c4383b9dae29

[edit] I rebased, should merge without conflicts

Member

boucher commented Mar 25, 2010

You can double check Cocoa, but I believe it's supposed to send both sets of messages, the table ones and the outline view ones, if both are implemented.

Contributor

cacaodev commented Mar 25, 2010

I don't see this in cocoa. I tried with 2 methods that would conflict like

  • (BOOL)outlineView:shouldSelectItem: and - (BOOL)tableView:shouldSelectRow:
    , with 2 similar methods that register automatically the delegate, and tried with only the tableView delegate method: each time only the outline view delegate method is being called. But maybe i missed something ...
Member

boucher commented Mar 25, 2010

You're probably right then, I was working off memory from a few weeks ago.

Contributor

cacaodev commented May 12, 2010

New patch for delegate & datasource calls wrapped into instance methods based on current master:

http://github.com/cacaodev/cappuccino/commit/90527e4be5d5faf13cbd3b5e1693d28d79d5aac9

[edit 05-18-10: commit back online and updated]

Contributor

cacaodev commented Sep 7, 2010

I'd like to know what is the status of this ?

Do we still agree we need to remove this private delegate/dataSource in CPOutlineView and that the way to achieve this is to wrap all delegate/ds calls into instance methods in CPTableView AND CPOutlineView when they're used in both ?

I can rework my patch but i need a definitive answer.

Contributor

Me1000 commented Sep 7, 2010

I think we agree that we deviate from cocoa here, but I think our current implementation is pretty elegant…
That said I'm not opposed to moving to the cocoa API, but I think it will make our delegation and data source methods a bit messier.

If you're willing to patch it (with tests :) ) I'm willing to merge it in.
but I'm not willing to put too much time in it, because it seems unnecessary at this point.

Member

boucher commented Sep 7, 2010

I think the Cocoa implementation makes much more sense, and I'd advocate doing that.

cappbot commented May 9, 2012

Milestone: 1.0. Labels: #needs-review, AppKit, bug. What's next? This issue is pending an architectural or implementation design decision and should be discussed or voted on.

@tolmasky tolmasky pushed a commit to tolmasky/cappuccino that referenced this issue Jan 4, 2013

@Me1000 Me1000 Fire selectionIsChanging when selecting column. closes #437 76443a5
Contributor

aparajita commented Feb 16, 2013

@cacaodev If you want to take another look at this after we merge view-based tables, I'll be happy to look at it.

Contributor

daboe01 commented Mar 13, 2015

we now assign specific protocols to _outlineViewDataSource and _outlineViewDelegate. This would not be possible with the original suggestion from @klaaspieter.
@cacaodev @Dogild @aparajita any objections against closing this one?

Contributor

cacaodev commented Mar 13, 2015

@daboe01 : For me the steps for fixing the original issue from @klaaspieter were:
1- In CPTableView, wrap all datasource/delegate calls into instance methods. This seems to be done, (and thanks to the one who did it !).
2- In CPOutlineView, subclass these new methods when necessary and redirect to the outline view delegate/datasource.
3- Get rid of the dummy delegate object and the [super setDelegate:] call.

There is just one thing we need to check: what if you want the outline view delegate to also support some methods from the CPTableViewDelegate : is it allowed in cocoa ? what methods are irrelevant and maybe ignored, how conflicts between the 2 protocols are resolved.

Is there something with the new protocols preventing us from completing steps 2-3 ?

Member

Dogild commented Mar 13, 2015

The delegate of a CPOutlineViewDelegate must conform to the CPOutlineViewDelegate protocol. It won't work with the delegate of a CPTableViewDelegate as CPOutlineViewDelegate does not conform to CPTableViewDelegate. We can't have tableViews delegate methods for the outlineView delegate. It's like this in Cocoa (just tried with the selection delegate) !

So yes, we should override the datasource/delegate calls and removed these lines (there are not in NSOutlineView.j"

id <CPOutlineViewDataSource>    _outlineViewDataSource;
id <CPOutlineViewDelegate>      _outlineViewDelegate;

and the classes _CPOutlineViewTableViewDelegate and _CPOutlineViewTableViewDataSource. It should work easily normally.

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