snackbar: show connecting snackbar depending on bool isstale

[satyam372]

issue #465

This branch adds a variable bool isstale variable and a function in Class Peraccountstore which is present in Store.dart file . This variable is updated to true when an exception is caught related to BAD_EVENT_QUEUE or Networkerror . When the variable becomes true a connecting snackbar is triggered which provides feedback to the user regarding the staleness of their data.

This Branch also adds a snackbar.dart file which is responsible for showing the connecting snackbar depending on the status of bool isstale.

[satyam372]

Previous PR for this issue were #598,#586,#584,#583

[chrisbobbe]

Thanks, @satyam372!

As Greg said at #598 (comment),

Knowing whether the device thinks it has an internet connection […] doesn't in itself do anything to resolve the original issue: in the main situation we're interested in, there is a perfectly good internet connection.

For this PR, let's remove all the code that's about knowing whether the device thinks it has an Internet connection, and focus for now on the original issue, #465. So in particular, let's not add or use the connectivity_plus dependency at this point.

It might be helpful to use connectivity_plus to change the exact message we show to the user, but let's leave that for a followup in a later PR, to simplify the work we're doing here.

From a quick read of the code in this revision, it looks like it has a bug: as long as the device continues to believe it has an Internet connection, the user won't get any feedback if the server expires the event queue (causing ZulipApiException(code: 'BAD_EVENT_QUEUE_ID')), or if a polling request has a network error. (Or if the server acknowledges and reports a server error, with a 5xx HTTP status code, which is another way a polling request could fail, causing us to lose our real-time stream of events.)

[satyam372]

thanks @chrisbobbe for you feedback !
After reading your feedback, what I have understood is that we need to inform the user about the actual reason for the data being stale, as described in issue #555. Additionally, we should temporarily remove the connectivity_plus dependency and code related to it .
Am I right?

[chrisbobbe]

[…] we need to inform the user about the actual reason for the data being stale, as described in issue #555.

No, let's not do #555 now. This PR should only focus on the original issue, #465. The snackbar can simply say "Connecting…" for now, without giving more details.

Additionally, we should temporarily remove the connectivity_plus dependency and code related to it .

That's right. These things are not necessary to fix the original issue, #465.

[chrisbobbe]

Hi @satyam372, it looks like you've updated the PR branch. Is it ready for another review? When you push updates and it's ready for another review, it's helpful if you post a comment saying so, for explicitness. That way reviewers don't have to guess. 🙂

This revision looks like probably not what you intend to be reviewed—GitHub shows me that you're proposing 27 commits, which make changes across 20 files, and many of those changes are unexplained and not related to #465.

I see you've asked for help in #git help in the Zulip development community. Is that about your work on this PR, or something else?

[satyam372]

Hi @chrisbobbe,

Yes, I have updated the branch, but it is not ready for review. Actually, it has gotten messed up. This was likely due to using merge instead of rebase. Then, my local main got diverged, which I have mentioned in the Git Help stream. Due to not having a history of the Git commands I ran, I have made it hard for the mentors to resolve this issue.

Now, I am planning to make a new copy of the project and create a new PR. However, this will lead to many branches for a single issue. But anyhow, the previous branches contain all the wrong logic, which is of no use. Due to all the mess in the branch and in my local project, I don't think there is any other option.

[satyam372]

@chrisbobbe , about the bug you mentioned in the previous conversation bug: as long as the device continues to believe it has an Internet connection, the user won't get any feedback if the server expires the event queue , I addressed the bug by setting isstale=true when encountering the exception (causing ZulipApiException(code: 'BAD_EVENT_QUEUE_ID'). I tested this solution by simulating a scenario where the server expires the event queue. I used 'queue_id': RawParameter('wrong' + queueId) for testing, as mentioned in the commits of PR#561. The solution worked effectively.

before using 'queue_id': RawParameter('wrong' + queueId), for testing , as i use my physical device for debugging I connected my physical device to the wifi and and then explicitly blocked internent access of my wifi keeping the wifi on (at this time the data isstale because wifi has no internet connection) but no error msg was seen on the logs of android studio. Generally it shows backing of then... will try so do we need to add another exception to handle it ?

[gnprice]

Now, I am planning to make a new copy of the project and create a new PR. However, this will lead to many branches for a single issue.

If you want to make a new local copy of the project (with git clone), that's fine; but (as we previously discussed in a chat thread) please update the existing PR rather than open a new one. To do this, you push to the existing branch on your GitHub fork (https://github.com/satyam372/zulip-flutter), exactly the same way as you could from your existing local copy of the project. See our Git guide:
https://zulip.readthedocs.io/en/latest/git/pull-requests.html#update-a-pull-request

@chrisbobbe , about the bug you mentioned […] so do we need to add another exception to handle it ?

To answer these questions effectively we'll need to read your code. It'll be a lot easier to do that after you push a revision with clear and coherent commits. So let's start there, and then we can get into questions about the logic.

[gnprice]

Chat thread where we've discussed this PR (and its previous incarnations):
https://chat.zulip.org/#narrow/stream/48-mobile/topic/.23F465.20Show.20.22Connecting.E2.80.A6.22.20banner.20.28or.20equivalent.29.20when.20serv.2E.2E.2E/near/1792447

It's not possible to effectively review this PR as it stands, because the commits don't make clear what's being changed and why. Please take a look at our guide on making commits clear and coherent, linked above:
https://zulip.readthedocs.io/en/latest/contributing/commit-discipline.html

Please also read through our guide on making a clear, reviewable PR, and then revise this PR with a self-review:
https://zulip.readthedocs.io/en/latest/contributing/reviewable-prs.html
The principles in that guide are fundamentally the same ones we've always applied in the Zulip project; but the text of the guide is new, and perhaps it will help you better understand what we're expecting.

I'll mark this PR as draft for now; please feel free to mark as ready for review again after you've revised it with a thorough self-review.

[satyam372]

making bool isstale false when we get event queue

[gnprice]

Please do a more careful self-review before requesting review again on this PR. Reviewers are busy, and it's important to make good use of our time.

---

It looks like what's changed in this revision consists only of some edits to the commit messages:

$ git range-diff origin pr/619@{{1,0}}
1:  6ae0efc58 ! 1:  fd9ddab48 bool isstale:Add bool isstale to store.dart
    @@ Metadata
     Author: satyam_45 <[email protected]>
     
      ## Commit message ##
    -    bool isstale:Add bool isstale to store.dart
    +    Store:Add bool isstale variable to store.dart
    +
    +    Initialized bool isstale variable , created func void setIsStale() to update the value of bool isstale variable and updated bool isstale to true when an exception is caught which in return will trigger the connecting snackbar
     
      ## lib/model/store.dart ##
     @@ lib/model/store.dart: class PerAccountStore extends ChangeNotifier with StreamStore {
2:  e83ba8898 ! 2:  514e6d214 Snackbar.dart:Add snackabrt.dart file to lib/widget
    @@ Metadata
      ## Commit message ##
         Snackbar.dart:Add snackabrt.dart file to lib/widget
     
    +    The code defines a Flutter widget, SnackBarPage, which displays a SnackBar with a "Connecting" message when bool isStale is true.
    +
      ## lib/widgets/snackbar.dart (new) ##
     @@
     +import 'package:flutter/material.dart';
3:  2ced2eb12 ! 3:  62ba1a21e Import:Import file snackbar.dart
    @@ Metadata
     Author: satyam_45 <[email protected]>
     
      ## Commit message ##
    -    Import:Import file snackbar.dart
    +    ui: Add SizedBox widget and call SnackBarPage
    +
    +    Added a SizedBox widget with a height of 40 units to provide spacing. Called SnackBarPage within the UI, passing the isStale parameter from the store to enable the "connecting" snackbar based on stale data.
     
      ## lib/widgets/app.dart ##
     @@ lib/widgets/app.dart: import 'recent_dm_conversations.dart';
4:  6f2023055 = 4:  be5ef53ef Snackbar_test:Add snackbar_test.dart to test/widget

The commit messages are one of the areas that needs revision. But the added text largely just restates the details that are in the code changes. That's not the sort of information that makes a commit message helpful:
https://zulip.readthedocs.io/en/latest/contributing/commit-discipline.html#commit-description

Apart from the commit messages, the organization of the commits themselves doesn't follow what's needed for making the changes clear:
https://zulip.readthedocs.io/en/latest/contributing/commit-discipline.html#each-commit-must-be-coherent

There are also no tests for the logic that controls isStale. That's a central part of what this PR is doing, so it needs tests.

[satyam372]

Fixes #465

[satyam372]

Ready for review! @gnprice , @chrisbobbe

[gnprice]

This still doesn't meet our guidelines. You'll need to revise it to meet our guidelines before we can review your work.

[satyam372]

Ready for another Review. @gnprice @chrisbobbe

[gnprice]

Your last three revisions have all been pretty small. As I said above at #619 (comment) , this PR needs a revision based on a thorough self-review before it will meet our guidelines for a reviewable PR.

I'm closing this PR, because it doesn't seem like it's going to converge to something we can merge.

You're welcome to come back next year, after you've gotten some more experience writing software and working with other people, and try again contributing a PR to Zulip then.