Skip to content

Memory management guidelines #216

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

Closed
crrobinson14 opened this issue Dec 9, 2016 · 9 comments
Closed

Memory management guidelines #216

crrobinson14 opened this issue Dec 9, 2016 · 9 comments

Comments

@crrobinson14
Copy link

The docs for FUITableDataSource aren't very clear about memory management. The docs for Firebase itself ARE, and there's even a best-practices blog post on it here:

https://firebase.googleblog.com/2015/10/best-practices-for-ios-uiviewcontroller_6.html

FUIArray has a dealloc handler that removes its listeners but it's not clear what the best UIViewController pattern is for making sure this gets fired. What I've settled on is creating the dataSource reference in viewWillAppear and setting it to nil in viewDidDisappear. If this is appropriate I'm happy to submit a PR to enhance the README.md to talk about memory management, but wanted to confirm first that this was correct.

@morganchen12
Copy link
Contributor

Yeah, doing that is fine. #186 adds some more control over the FUIArray's activity, so you can suspend their observers on viewDidDisappear: and re-attach them on viewDidAppear: But until that gets merged, adding a little note in the database readme seems reasonable.

@crrobinson14
Copy link
Author

Wow, #186 looks really valuable. Do you think it will get merged in the next week or two? Today I'm working on dealing with drag/drop handling and the overrideable behaviors in the new FUICollection would be handy there. I'd rather submit a PR to enhance the docs for the "good stuff" if it'll be in soon. ;)

I'd love your feedback on timeline, but let's close this ticket so it's not clogging up the pipes in the meantime.

@morganchen12
Copy link
Contributor

I personally would like to merge it ASAP, but it's pending some discussion about how loose we'd like to be about cross-platform feature parity across the FirebaseUI libraries and I haven't had those conversations yet.

@ugiacoman
Copy link

@crrobinson14 What drag/drop features are you handling? Is this related to #218 ?

@morganchen12
Copy link
Contributor

No, it's not related to #218; #186 just touches on a lot of shortcomings of the database library.

@crrobinson14
Copy link
Author

@ugiacoman Nothing I'm doing is related to #218 or #186 that I KNOW of. It's just that right now today I'm implementing drag/drop reorder handling in my app where the tableview is wired via FirebaseUI. It's not clear to me from the docs what the "best" way to handle this is. FUITableDataSource doesn't interfere with the UITableViewDelegate so currently I'm planning to implement moveRowAtIndexPath and do a Firebase update to the keys that control the element ordering (the query is ordered). That's how I did it in Realm, too, and it worked fine so I assume it's the best approach here.

I only mentioned this above because the new FUICollection in #186 is more sophisticated. All of this stuff is very fresh in my head from the "implementor's" perspective and I was going to submit a PR to enhance the README.md with more details and examples of how to do the things I'm dealing with right now. The sample project is fairly basic, and there's not a lot of other documentation / examples floating around the Internet. I figured my recent experience would help jump-start some others.

@ugiacoman
Copy link

ugiacoman commented Dec 9, 2016

@crrobinson14 Haha you moved to Firebase after leaving Realm too? I have a feeling you're going to run into the same issue as we're facing. We implemented the collectionView equivalent moveItemAt. I'd love to hear if you are able to implement this without any problems. We're going to attempt to implement a hacky solution for now :/

@crrobinson14
Copy link
Author

crrobinson14 commented Dec 9, 2016

You weren't specific about what issue you were facing. The way we implemented this in Realm we had items in a table sorted by a weight value, basically just an int ascending-sort key 0..N. In moveItemAt we calculated a new array of id -> weight pairs, then updated both the local database and the server (since Realm had no sync).

In Realm we still had some visual glitches with this method so after much analysis and StackOverflow prodding we ended up using a static array. When the server notified us there was a dataset update, we'd make a local mutable array copy. Then in the moveItemAt code we also had this to provide an immediate update for the tableView:

[interestsArray removeObjectAtIndex:(NSUInteger) sourceIndexPath.row];
[interestsArray insertObject:interest atIndex:(NSUInteger) destinationIndexPath.row];

UITableView really likes that - it expects this callback to update the local array immediately. By the time you return, it wants its datasource to match the "post-drop" ordering. It remains to be seen whether Firebase will require this step but if it does it's not a huge deal. We can't use FUITableViewDataSource here anyway, we have to use FUIArray, because we're also using FMMoveTableViewDataSource. Once you start using FUIArray directly you're only a hop/skip/jump away from anything else you want to do.

@crrobinson14
Copy link
Author

n.b. the ordering question is off-topic from the title and we should probably either change the ticket or move the conversation elsewhere to avoid confusion.

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

No branches or pull requests

3 participants