Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Throw assertion when attempting to inject section changes on AppKit #95

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Throw assertion when attempting to inject section changes on AppKit #95

wants to merge 1 commit into from

Conversation

helje5
Copy link

@helje5 helje5 commented Jan 20, 2020

Section changes are not supported yet on AppKit (neither
NSCollectionView nor NSTableView, see issues #93/#94).

Checklist

Description

Just added a single assertion to both extensions:

            assert(changeset.sectionChangeCount < 1,
                   "NSCollectionView sections are not yet supported, sorry.")

Related Issue

Motivation and Context

Without the assertion, the extensions silently drop changes / do the wrong thing. This way we can at least catch them in debug mode.

Impact on Existing Code

No impact.

Screenshots (if appropriate)

Screenshot 2020-01-20 at 16 02 07

Section changes are not supported yet on AppKit (neither
NSCollectionView nor NSTableView, see issues #93/#94).
@ra1028
Copy link
Owner

ra1028 commented Mar 31, 2020

I prefer NSCollectionView to support sections instead of asserts.
On the other hand, NSTableView can't support sections, but I don't think it needs assertions.

@ra1028
Copy link
Owner

ra1028 commented Jun 9, 2020

@helje5
Please let me know if you disagree with my thought.

@helje5
Copy link
Author

helje5 commented Jun 9, 2020

To be honest I don't understand what you are saying / trying to say :-)

I prefer NSCollectionView to support sections instead of asserts.

You mean you prefer DifferenceKit to support NSCollectionView sections? Yeah, I'd prefer that too, but while that isn't available, asserts seem to be way to go? Otherwise people, like me, run into this and wonder why it isn't working.

On the other hand, NSTableView can't support sections, but I don't think it needs assertions.

You mean you prefer DifferenceKit to support NSTableView groups (same like sections, just w/ a different API)? Yeah, I'd prefer that too, but while that isn't available, asserts seem to be way to go? Otherwise people, like me, run into this and wonder why it isn't working.

@ra1028
Copy link
Owner

ra1028 commented Jun 10, 2020

@helje5
Ah sorry, I realized that I had written a rather nonsensical comment as you say.
I mean that NSCollectionView should actually be able to support sections, so it's better to implement it than to make assertions.
Does this PR help you?
And then, in my understanding, NSTableView doesn't have an API to support sections.
I agree that it's reasonable to make assertions so that users can see why it's not working at this point.
However, it's also important to avoid crashing the app if something goes wrong, so we need to carefully determine when and where to assert.
Assertions that are too sensitive will provide a painful API like the UICollectionView as we've been experiencing.
IMO, the natural thing to do in this case would be to keep working rather than crashing immediately. what do you think?

@helje5
Copy link
Author

helje5 commented Jun 10, 2020

I mean that NSCollectionView should actually be able to support sections, so it's better to implement it than to make assertions.

I agree! And until that works, have the assertion, a one line change.

Does this PR help you?

Don't have the time right now to check it out, maybe?

And then, in my understanding, NSTableView doesn't have an API to support sections.

Your understanding is wrong. NSTableView has groups which is the same thing like UIKit sections, it just has a (quite) different API for that. So the task here would be to either provide a fascade which makes it work, or have a version of the algorithm running against the tableview representation of the data.
IIRC I actually played with building that, but eventually stopped due to time constraints.

I agree that it's reasonable to make assertions so that users can see why it's not working at this point.
However, it's also important to avoid crashing the app if something goes wrong

Assertions only stop the app when building them in debug mode. They exist specifically to help during development and are inactive when building in release mode.

I think there is no reason not to enable them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants