realm / realm-swift

Realm is a mobile database: a replacement for Core Data & SQLite
https://realm.io
Apache License 2.0
16.18k stars 2.14k forks source link

Collection Change Handlers: AppKit-Compatible Indexes #7980

Open bdkjones opened 1 year ago

bdkjones commented 1 year ago

Problem

After filing #7979 as a bug and discovering the design was intentional, I'm filing this as a feature request.

In collection change handlers, the modifications array of indexes references the previous collection. That seems to be appropriate for UITableView on iOS, which has a performBatchUpdates() method. However, it does not work for NSTableView on macOS, which has no such API.

On macOS, the indexes for modified objects are useless if any insertions and/or deletions have occurred in the same change notification. We must reload the entire table because there is no way to translate the "old" modified-object-indexes to the CURRENT indexes of those objects. We need the current indexes because we've deleted rows and then inserted new rows into the table, which moved the modified rows around—they no longer occupy the slots that modifications says they do!

Solution

The easiest solution I see is to provide two modifications arrays: one referencing the previous collection version and one referencing the latest collection version:

switch changes
{
    case .initial:
        break

    case .update(_, let deletions, let insertions, let modifications, let currentModifications):
        // "currentModifications" is [Int] referencing the current state of the Realm Collection.

Other Options:

  1. Have modifications reference the current state of the collection if the application is linked against AppKit instead of UIKit
  2. Allow the developer to specify which kind of modifications he wants (old collection state or current collection state) in the .observe() call. The default would be as-is behavior for backward compatibility. This is the most elegant, foolproof solution, I think.

Either way, it would be useful if Mac apps didn't have to resort to a full-reload of NSTableView when handling changesets.

Just Use SwifUI!

Unfortunately, SwiftUI simply isn't ready to lift the kind of UI loads that AppKit can. Many Mac apps have much more complex UIs than mobile apps and it'll be many years before SwiftUI's performance is ready to handle all of that complexity. There are still many cases where AppKit is absolutely needed. For example, 32-column, 8,000-row TableViews.

How important is this improvement for you?

Would be a major improvement

bdkjones commented 2 months ago

Nothing on this proposal? Realm does list macOS as an official platform, but this API is essentially UIKit-only. It would be nice if Realm were a proper fit on all the platforms it supports.

Screenshot 2024-05-01 at 01 17 49

bdkjones commented 2 months ago

The problem is more extensive than just modifications && (insertions || deletions). The problem can manifest anytime there are BOTH insertions and deletions in a changeset.

This happens, for example, if an NSTableView is sorted by a column, foo, and the user changes the value of that column in one row such that the order of the rows is affected. A changeset will happen with 1 deletion and 1 insertion, but again, once the deletions are handled, the rows for insertions are now wrong—they reference the OLD state of the table's data.

It's easily reproducible if you modify the table cell's value in such a way that its row moves to the bottom of the table. The proposed insertion index is out of bounds and the row is never inserted during the change-handler.

Bottom Line

If you're a Mac developer and you use NSTableView with a Realm Results object, you can't follow the docs. If either of these things is true, you must force-reload the entire table:

1) modifications is non-zero and EITHER deletions or insertions is non-zero.

2) both deletions and insertions are non-zero, regardless of the value of modifications.

nirinchev commented 2 months ago

The main blocker for shipping this is that it'd actually be a breaking change due to the new associated value we'd need to add to the .update case. I've added it as a discussion topic for next week's roundtable and I'll report back on whether we come up with a solution that doesn't require a major version bump.

bdkjones commented 2 months ago

@nirinchev wouldn't it be non-breaking to simply add an option to the collection change subscription that controls how the indexes are calculated? The default would be the current behavior (indexes relative to the "old" collection, suitable for UIKit's -batchUpdates()) and AppKit-based developers could specify an option that would give them the indexes they need. That wouldn't change anything for existing implementations?

nirinchev commented 2 months ago

Yes, that's one approach we're considering but I'd like to see if there's anything smart we could do that exposes both sets of indexes, like we do in other SDKs.