furkansarihan / firestore_collection

Firestore extension with cache-first pagination and stream builder support.
MIT License
9 stars 5 forks source link

displayCompare not work as expected #3

Open azazadev opened 3 years ago

azazadev commented 3 years ago

Hi @furkansarihan

I'm using firestore_collection on some views with queryOrder based on timestamp and it's work as expected Now I'm trying to extend this package to other view, this new view contain the posts order by likeCount ( most liked posts should be on the top of List ), but this is not work as expected, I have try to override displayCompare and compare post timestamp if likeCount field is the same for 2 documents

FirestoreCollection fireCollection = FirestoreCollection(
  collection: FirebaseFirestore.instance.collection('posts'),
  initializeOnStart: true,
  // first page will fetched immediately
  offset: 15,
  // page size
  serverOnly: true,
  // cache first
  live: true,
  // notifies to newest documents
  query: FirebaseFirestore.instance.collection('posts'),
  queryOrder:
      QueryOrder(orderField: 'likeCount', displayCompare: comparePosts),
);

How to reproduce :

  1. import the main file attached
  2. add some posts ( LikeCount = 0 for all posts added ) --> new posts added
  3. restart the app
  4. add new posts ---> nothing happened --> the new post is not added

Can you please check why new documents are not added

NOTE : I'm using the last version 0.0.4

main file to reproduce :


import 'package:cached_network_image/cached_network_image.dart';
import 'package:cloud_firestore/cloud_firestore.dart';
import 'package:firebase_core/firebase_core.dart';
import 'package:firestore_collection/firestore_collection.dart';
import 'package:flutter/cupertino.dart';
import "package:flutter/material.dart";
import 'package:flutter_icons/flutter_icons.dart';

void main() async {
  WidgetsFlutterBinding.ensureInitialized();
  await Firebase.initializeApp();
  runApp(App());
}

class App extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      theme: ThemeData(),
      title: 'My Flutter App',
      home: Home(),
    );
  }
}

class Home extends StatefulWidget {
  @override
  State<StatefulWidget> createState() {
    return _HomeState();
  }
}

class _HomeState extends State<Home> {
  int _currentIndex = 0;
  final List<Widget> _children = [HomePage(), SearchPage(), ProfilePage()];

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: _children[_currentIndex],
      bottomNavigationBar: BottomNavigationBar(
        onTap: onTabTapped,
        currentIndex: _currentIndex,
        items: [
          BottomNavigationBarItem(
            icon: Icon(Icons.home),
            title: Text('Home'),
          ),
          BottomNavigationBarItem(
            icon: Icon(Icons.search),
            title: Text('Search'),
          ),
          BottomNavigationBarItem(
            icon: Icon(Icons.person),
            title: Text('Profile'),
          ),
        ],
      ),
    );
  }

  void onTabTapped(int index) {
    setState(() {
      _currentIndex = index;
    });
  }
}

class HomePage extends StatelessWidget {
  HomePage();

  @override
  Widget build(BuildContext context) {
    FirestoreCollection fireCollection = FirestoreCollection(
      collection: FirebaseFirestore.instance.collection('posts'),
      initializeOnStart: true,
      // first page will fetched immediately
      offset: 15,
      // page size
      serverOnly: true,
      // cache first
      live: true,
      // notifies to newest documents
      query: FirebaseFirestore.instance.collection('posts'),
      queryOrder:
          QueryOrder(orderField: 'likeCount', displayCompare: comparePosts),
    );

    int count = 0;
    return Scaffold(
      backgroundColor: Colors.white,
      // No appbar provided to the Scaffold, only a body with a
      // CustomScrollView.
      body: CustomScrollView(
        physics: const BouncingScrollPhysics(
            parent: AlwaysScrollableScrollPhysics()),
        slivers: <Widget>[
          // Add the app bar to the CustomScrollView.
          SliverAppBar(
            actions: <Widget>[
              IconButton(
                iconSize: 30.0,
                icon: const Icon(CupertinoIcons.plus),
                onPressed: () async {
                  count++;
                  await FirebaseFirestore.instance.collection('posts').add({
                    'fullName': 'user' + count.toString(),
                    'likeCount': 0,
                    'userPhotoUrl':
                        'https://static.thenounproject.com/png/15724-200.png',
                    'timestamp': Timestamp.now()
                  });
                },
              ),
            ],
            // Provide a standard title.
            expandedHeight: 100.0,
            floating: true,
          ),
          StreamBuilder(
            stream: fireCollection.stream,
            builder: (BuildContext context,
                AsyncSnapshot<List<DocumentSnapshot>> snapshot) {
              return SliverList(
                delegate: SliverChildBuilderDelegate(
                  (context, index) {
                    return ListTile(
                        leading: InkWell(
                          onTap: () => {print('tap...')},
                          child: Ink(
                            height: 50,
                            width: 50,
                            child: CachedNetworkImage(
                              imageUrl:
                                  snapshot.data[index].data()['userPhotoUrl'],
                              imageBuilder: (context, imageProvider) =>
                                  Container(
                                width: 50,
                                height: 50,
                                decoration: BoxDecoration(
                                  shape: BoxShape.circle,
                                  image: DecorationImage(
                                      image: imageProvider, fit: BoxFit.cover),
                                ),
                              ),
                              placeholder: (context, url) =>
                                  CircularProgressIndicator(),
                              errorWidget: (context, url, error) => Icon(
                                FontAwesome5.user,
                                color: Colors.black12,
                              ),
                            ),
                          ),
                        ),
                        title: Text(snapshot.data[index].data()['fullName']));
                  },
                  childCount: snapshot.hasData ? snapshot.data.length : 0,
                ),
              );
            },
          ),
        ],
      ),
    );
  }

  int comparePosts(DocumentSnapshot a, DocumentSnapshot b) {
    dynamic fieldA = a?.data()["likeCount"];
    dynamic fieldB = b?.data()["likeCount"];
    if (fieldA == null) {
      return 1;
    }
    if (fieldB == null) {
      return -1;
    }
    int res = fieldB.compareTo(fieldA);
    print('compare likes res : ' + res.toString());
    if (res == 0) {
      dynamic fieldA = a?.data()["timestamp"];
      dynamic fieldB = b?.data()["timestamp"];

      if (fieldA == null) {
        return 1;
      }
      if (fieldB == null) {
        return -1;
      }
      int res = fieldB.compareTo(fieldA);
      print('compare timestamp res : ' + res.toString());
      return res;
    } else {
      // Descending compare
      return res;
    }
  }
}

class SearchPage extends StatelessWidget {
  SearchPage();

  @override
  Widget build(BuildContext context) {
    int count = 0;
    return Scaffold(
      backgroundColor: Colors.white,
      // No appbar provided to the Scaffold, only a body with a
      // CustomScrollView.
      body: Container(),
    );
  }
}

class ProfilePage extends StatelessWidget {
  ProfilePage();

  @override
  Widget build(BuildContext context) {
    int count = 0;
    return Scaffold(
      backgroundColor: Colors.white,
      // No appbar provided to the Scaffold, only a body with a
      // CustomScrollView.
      body: Container(),
    );
  }
}
furkansarihan commented 3 years ago

@azazadev Hi there,

Current design of listener implementation; New documents must have greater value than the ones in already fetched.

We can maybe change the behaviour with changing the query parameter to isGreaterThanOrEqualTo from isGreaterThan for your use case.

displayCompare doesn't related to query, It just changes display order. But it should work.


I just noticed some index jumps when multiple items has same order value. It should be fixed.

azazadev commented 3 years ago

Hi @furkansarihan

I have made a quick test :

  1. override FirestoreCollection and replace isGreaterThan by isGreaterThanOrEqualTo
  2. run app from Editor
  3. add some posts --> UI refreshed and posts added
  4. run app again from Editor --> add some posts --> UI refreshed and posts added
  5. kill app from Simulator --> add some posts --> UI not refreshed and posts not added in List

probably because I'm not calling dispose after closing app from Simulator ? if yes how we can do it ?

Now I'm facing another blocking issue, not related to this one issue : after logout and when I'm trying to logIn I got this error : Flutter : Bad state: Stream has already been listened to

I'm calling dispose before logout and using BroadcastStream but issue persist !, do you have face this issue ? any idea how I can fix it ?

Stream :

   StreamBuilder(
    stream: model.firestoreCollection.stream.asBroadcastStream(),
    builder: (BuildContext context,
        AsyncSnapshot<List<DocumentSnapshot>> snapshot) {
      return SliverList(
        delegate: SliverChildBuilderDelegate(
          (context, index) {
            return new MyModel(
                modelId: snapshot.data[index].id);
          },
          childCount: snapshot.hasData ? snapshot.data.length : 0,
        ),
      );
    },
  ),

logout

Future signOut() async {
    setBusy(true);
    try {
      await _fireStoreService1.firestoreCollection.dispose();
      await _fireStoreService2.firestoreCollection.dispose();
      await _fireStoreService3.firestoreCollection.dispose();
      await _authenticationService.signOut();
      notifyListeners();
    } catch (e) {
      logger.e('Failed to signOut' + e.toString());
    }
    setBusy(false);
  }

Thanks again for your responsiveness and your application

azazadev commented 3 years ago

@furkansarihan forget the second issue 'Flutter : Bad state: Stream has already been listened to' I have found issue and the fix now :)

will sent merge request to you tomorrow

for the first one 'query order' it will be good if we can release fix for this soon

furkansarihan commented 3 years ago

@azazadev I don't think it's related to dispose. And sorry but I couldn't follow your first issue.

But I think If you read the code you can easily follow the flow and find a better solution to cover your issue too.

We can always extend the package with extra parameters.

For second issue; It would happen after building the widget again which contains the StreamBuilder. Stateless Widget may be the cause.

Finally, Contributions are welcomed! 😄

azazadev commented 3 years ago

ok, will try to debug the first issue later and I will create another issue for error "Flutter : Bad state: Stream has already been listened to"

azazadev commented 3 years ago

@furkansarihan any idea why you are not using startAfterDocument(_lastDocument) on the Listener ?

furkansarihan commented 3 years ago

@azazadev I have no idea. Check out the native implementation. Do you think it changes anything?

azazadev commented 3 years ago

no idea also for now, I'm trying to understand how realtime pagination work, and from official documentation https://firebase.google.com/docs/firestore/query-data/query-cursors#node.js_3 and all what I have found as package or tutorial we are using everytime this pattern

example : https://github.com/marciovalim/realtime_pagination/blob/master/lib/cubits/realtime_pagination_cubit.dart https://github.com/FilledStacks/flutter-tutorials/blob/master/045-paginated-realtime-firestore/lib/services/firestore_service.dart https://medium.com/flutterdevs/pagination-in-flutter-with-firebase-firestore-96d6cc11aef2 https://morioh.com/p/5f0c0714bc09

we will appreciate if you can check this, I'm trying to resolve/debug the pagination for order field like 'LikeCount' and I'm not convinced to change compactor isGreaterThan to isGreaterThanOrEqualTo because this can introduce regression in the package

furkansarihan commented 3 years ago

@azazadev The cause of the behavior is isLessThan parameter in the queries.

isLessThanOrEqualTo can be a solution. Bu it's not.

Because duplicate documents can be fetched as described in here.

Now, startAfterDocument() makes sense if we considered this problem.

So, I think we can find a proper solution with using startAfterDocument() related query parameters rather than lessThen, greaterThen etc.

azazadev commented 3 years ago

@azazadev The cause of the behavior is isLessThan parameter in the queries.

isLessThanOrEqualTo can be a solution. Bu it's not.

Because duplicate documents can be fetched as described in here.

Now, startAfterDocument() makes sense if we considered this problem.

So, I think we can find a proper solution with using startAfterDocument() related query parameters rather than lessThen, greaterThen etc.

we can use multiple orderby as described here to resolve duplicate documents

azazadev commented 3 years ago

@furkansarihan please let me know for any comments for this merge request https://github.com/furkansarihan/firestore_collection/pull/7 otherwise please merge it

azazadev commented 3 years ago

@azazadev The cause of the behavior is isLessThan parameter in the queries.

isLessThanOrEqualTo can be a solution. Bu it's not.

Because duplicate documents can be fetched as described in here.

Now, startAfterDocument() makes sense if we considered this problem.

So, I think we can find a proper solution with using startAfterDocument() related query parameters rather than lessThen, greaterThen etc.

Hi @furkansarihan any news or plans to have this feature ? I need this :)

furkansarihan commented 3 years ago

@azazadev Hi, sorry for late response.

I tried something with startAfterDocument() last week but I couldn’t achieve a generic solution.

It’s not my priority right now, So It would be good if you can implement this since It’s your use case :)

azazadev commented 3 years ago

Thanks @furkansarihan very busy also in my job :), and no much time to work on my project, we can sync in coming weeks, please share what you have achieve in separate branch that I can check when I got time