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

Fix NSTableView batch update 1234 -> 3412 #69

Closed
wants to merge 3 commits into from

Conversation

hannesoid
Copy link

I had an issue where NSTableView wouldn't execute the moves correctly for the scenario
01232301

I realized that the moves produced by extendedDiff were
[M(0,2), M(1,3)]

Applying them in order would mean:
0123 + M(0,2)1203
1203 + M(1,3)1032
This (1032) is also what NSTableView showed after applying the extended diff, but not what we wanted to achieve in the first place

Applying it in reversed order means
0123 + M(1,3)0231
0231 + M(0,2)2301 which is what we wanted

So reversing the moves fixes this problem for NSTableView.

UITableView seems to not have these problems, I think it sorts the moves by itself somehow.

Unfortunately the related test
0123 -> 3120, resulting in [M(3,0), M(0,3)] (or reversed depending on this PR)
is not animated by NSTableView at all. I think this is because it applies them in sequence and the two moves basically cancel each other out.
Is there something else we could use for NSTableView which would work sequentially?

Again, UITableView seems to not have these problems.

@hannesoid
Copy link
Author

Alternative fix for my scenario would be just in AppKit+Diff.swift:65

        - update.moves.forEach { moveRow(at: $0.from.item, to: $0.to.item) }
        + // patched by hannes:
        + // - fixes order of moves for macOS
        + // - example: diff from a,b,c,d -> c,d,a,b will result in MoveSteps 0->2, 1->3. For correct application they need to be executed in reversed order.
        + update.moves.reversed().forEach { moveRow(at: $0.from.item, to: $0.to.item) }

@hannesoid
Copy link
Author

hmm I still have more NSTableView issues, maybe it needs a different overall batch/move strategy

@hannesoid
Copy link
Author

ok don't merge this, I think a different strategy is needed, maybe using patches

@hannesoid
Copy link
Author

I'll do a new one, where I provide ExtendedPatch based methods on NSTableView

@hannesoid hannesoid closed this May 27, 2020
@tonyarnold
Copy link
Owner

Thanks for looking into this, @hannesoid — I'm happy to chat about, or review anything you come up with to fix this 👍

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.

None yet

2 participants