Skip to content

Added support for using NSSort Descriptors & NSPredicate & Sections #12

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
wants to merge 5 commits into from
Closed

Conversation

pmodernme
Copy link

@mcdonamp These are the changes I mentioned in my Issue I sent you Friday.

NSSortDescriptors are used in the FirebaseArray to sort the results client-side. When using this method, it is most useful to use FQuery for initially filtering results, not sorting them.
e.g.:
[[ref queryOrderedByChild:@"hidden"] queryEqualToValue:@NO];

Then add the NSSortDescriptor to the firebase array when initializing your data source.

[[NSSortDescriptor alloc] initWithKey:@"value.origin_date" ascending:NO]]

NSSortDescriptors are added as an array. Results are sorted first by the sortDescriptor at index 0, then index 1, etc...

You can add an NSPredicate to the FirebaseArray to filter the results on the client-side.

Both NSSortDescriptors and NSPredicate filter based on the FDataSnapshots received by the Array.

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

@pmodernme pmodernme changed the title Added support for using NSSort Descriptors Added support for using NSSort Descriptors & NSPredicate Sep 28, 2015
@asciimike
Copy link
Contributor

@pmodernme awesome! I just gave it a glance and it looks pretty good 👍

I'll give it a more thorough review later this week. A few initial thoughts:

  1. The constructors are getting really long at this point (they already were). Might we want to do something like have a FirebaseSorted*ViewDataSource that would encapsulate the sort descriptors and predicate stuff and expose it in a smaller package? Unsure of the answer here and would love some feedback.
  2. I was planning on implementing this with a second array (one for all the underlying data, one that just has the sorted/filtered data). The current implementation makes event raising very easy, but means that the descriptor can't be changed (all the events will have to be re-raised when the datasource is destroyed and re-initialized). Not terribly expensive if everything is already cached on the client (which it is, since we're not doing server side filtering). That said, it means in order to change the predicate/sort descriptor, we need to tear down the datasource and create a new one. Alternatively, I guess we could just override the default setters for predicate and sortDescriptor to kill and re-initialize the listeners when someone sets them to something different (though I'm not sure we would handle the deletion events properly unless we also purged the current list and started over, or built in diffing logic, which starts to get awfully close to the client itself).

Also, if you'd like to do some Java, we can get close to feature parity on the Android version by implementing custom comparators for sorting and then using something like Guava for predicate support.

@asciimike asciimike mentioned this pull request Sep 29, 2015
@pmodernme
Copy link
Author

Hi Michael. Thanks for all the feedback. I really appreciate it.

  1. I think it would be cleaner if the datasource could be configured then
    activated, instead of it all happening at once.

FirebaseTableViewDataSource * datasource = [FirebaseTableViewDataSource
new];
datasource.cellClass = ...
datasource.predicate = ...
datasource.ref = ...

And then maybe setting the ref activates it, or if the user calls
initObservers.

  1. What if we did purge the dataSource, but the array gets passed to the
    new datasource. The observers are removed, the current array is
    filtered/sorted, and then the re-initialized observers fill in the gaps?

I haven't programmed in Java in a decade, so I can't help much there. If my
current (3 year) project ever gets finished, I'll probably attempt to port
it, then I'll have to learn all over again.

I'm currently attempting to implement sectioning. I'm going to work on it a bit more
tomorrow.

--Zoe

@googlebot
Copy link

CLAs look good, thanks!

@pmodernme
Copy link
Author

@mcdonamp I added sectioning. It works pretty slowly for large datasets (over 200 snapshots). I eliminated most of the begin / endUpdates calls. The data also appears more uniformly at initial display.

Any thoughts on thread safety? It would be nice to be able to put Firebase on a background thread and still play nice with UIKit.

@asciimike
Copy link
Contributor

Can we split out sectioning into it's own PR? It'll be easier to pull them in independently (even if the sectioning PR is pending this one).

The hard part about threading is that all UI updates have to happen on the main thread, so even if we're able to raise events in the background (which we already are, I believe), it might not have a significant impact.

I'll poke around and see though. The "large" datasets problem is likely to still exist, and the short answer is that we might have to do end up querying things and doing a paginated approach (though Firebase doesn't have great support for that yet).

@pmodernme pmodernme changed the title Added support for using NSSort Descriptors & NSPredicate Added support for using NSSort Descriptors & NSPredicate & Sections Sep 29, 2015
@asciimike asciimike added this to the 0.4 milestone Nov 19, 2015
@Vortec4800
Copy link
Contributor

This is good stuff, but hasn't been talked about in a while. Will this still be merged in?

@chadpav
Copy link

chadpav commented Sep 12, 2016

I'm in need of something like this as well. I was about to implement something when I decided to come take a look at the issues list. I think @pmodernme's implementation is a better, more generic approach to what I was going to do.

I forked master and took a look at what it would take to merge her changes, which were based on 0.4, and it's quite a lot. At this point, her work seems to be far enough away that someone would need to reimplement support for Sorting, Filtering, and Sections just using her implementation as a guide.

Is anyone looking into this? 1) I might not have the skill to do it. I've been working in Swift now for over a year. 2) I don't want to invest the time if a Pull Request is just going to be allowed to die. 3) I might be forced to hack something more concrete for my use case due to time constraints on my side.

Thoughts?

@asciimike
Copy link
Contributor

@chadpav the changes were actually based on 0.3 (or maybe even 0.2), so quite a bit has changed--and yes, it's likely that it would need to be mostly re-implemented.

The short answer about how we evaluate PRs is basically twofold:

  • Does the PR promote a best practice that we (Firebase) want to promote?
  • Is the PR available on all relevant platforms (iOS, Android, Web)?

Usually, the former is fulfilled, and the latter isn't. The former is important because we don't want to create a poor experience for Firebase apps by providing bad implementations of things like Auth, or client side merging (an inefficient implementation might render in a less than realtime way, which is generally our concern for things like this. That said, it hasn't stopped us in the past, see firebase-util and QueryBase.

The latter is important because Firebase is cross platform, and we need a consistent feature set in FirebaseUI to assure that all platforms are equally powerful and easy to use. This is much harder in the open source community since typically people aren't willing/able to do two or three implementations of a feature, and we haven't done the best job of trying to coordinate community resources.

I've been mulling a re-write of the internals here, and it's been brought up by a few colleagues after firebase/FirebaseUI-Android#276 to build this feature in (FirebaseArray would support joining/intersecting multiple refs under the hood, then support predicate support for filtering on top), but nobody on our team has had time to implement or coordinate this on both platforms (web already has the tools above).

@chadpav
Copy link

chadpav commented Sep 12, 2016

@mcdonamp, that's a great response. I appreciate it.

You guys have a high bar for community contributions but they do make sense. As you assume, it's going to be too high for an individual contributor on a single platform to meet though.

It would be nice to document some of this discussion in the "Contributing to FirebaseUI" section of the README across all relevant platforms. Currently it sounds much more straight forward than what we are discussing here. I trust that if you see an Issue or Pull Request in this repo that it makes it to the right person to evaluate whether or not the request should be prioritized across all relevant platforms.

Thanks so much! There are a lot of really nice things about Firebase that I'm enjoying.

@asciimike
Copy link
Contributor

@chadpav not a problem.

We had this discussion a few months back, and it resulted in adding such language to the PR template on both platforms. I totally agree that we should add it to the respective contributing sections as well, since folks won't realize until after they've put in the effort that something may not exist. That being said, because things are open source, developers are always welcome to have additional use case specific features on their own forks :)

The overall goal is to make this project more transparent. We've started moving issues (including our roadmap) into the issue tracker, and are trying to put together practices that allow community members to grab bugs and work on them with the guarantee that as long as they meet the quality bar, they'll be accepted (since by being prioritized and mentioned as ready for work, they should be meet our contribution standards). If you have suggestions on how to best approach this (or things that would help you contribute), we're all ears.

@asciimike
Copy link
Contributor

@chadpav I've added relevant language to iOS and Android CONTRIBUTING.md, let me know if this covers what you're thinking.

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.

6 participants