search

realm / realm-tasks

To Do app built with Realm, inspired by Clear for iOS
Other
368 stars 73 forks source link

Integrate Fine-Grained Notifications in iOS app #352

Closed jpsim closed 7 years ago

jpsim commented 7 years ago

Don't merge until realm/realm-cocoa#4253 is released. Otherwise ready for review.

This really improves the look and feel of the iOS app, for both local and synchronized changes.

If you're looking to help, this could definitely use more QA testing, although after a few rounds of bug fixes, I wasn't able to crash the app in any conflicting scenario I could come up with.

One notable functional change I made to simplify things is to keep a write transaction open for the duration of editing a cell. This blocks a large number of complex state changes that could modify the UI coming in from sync that would require quite a bit more complexity in the app to be resilient against.

Porting the macOS app to use changeset notifications is turning out to be a bit more involved, so I'll keep that for another PR.

/cc @TimOliver @tgoyne @stel

bigfish24 commented 7 years ago

🎉🎉🎉🎉

stel commented 7 years ago

Also I can take care of macOS version later this week if needed.

jpsim commented 7 years ago

Also I can take care of macOS version later this week if needed.

I'd appreciate that! IMHO we should replace the editing/animating/needsReloadTableView state variables by keeping write transactions open across those boundaries. That's what I did when editing cells in the iOS app in this PR, and I believe that approach to be more robust.

jpsim commented 7 years ago

Thanks for the feedback, @stel. It helped me improve a few parts of the code. 🙏

stel commented 7 years ago

The idea with open write transaction is absolutely great and works perfect in macOS app too! It should be added to some list of best-practices with RMP 👍