Skip to content

snackbar: Handle connectivity changes and show appropriate SnackBar#465 #598

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

Conversation

satyam372
Copy link

@satyam372 satyam372 commented Mar 28, 2024

PR for issue #465
This commit introduces a SnackBarPage widget that manages changes in connectivity status and displays corresponding SnackBars. The SnackBarPage widget listens to connectivity changes using the Connectivity package and updates the UI to show a SnackBar indicating either "No Internet Connection" or "Connecting" based on the current connectivity status and the value of the isStale property.

The initState method initializes the connectivity status when the widget is first created, while the didChangeDependencies method subscribes to connectivity changes to update the UI accordingly. To prevent memory leaks, the dispose method cancels the subscription when the widget is disposed.

The showSnackBar method is responsible for displaying the appropriate SnackBar based on the current connectivity status and the value of the isStale property. It utilizes ScaffoldMessenger to manage SnackBar display and dismissal.
2
1

@satyam372 satyam372 marked this pull request as ready for review March 28, 2024 19:26
@gnprice
Copy link
Member

gnprice commented Mar 28, 2024

When a PR is meant to fix an issue, please refer to the issue in both the commit and the PR description. This is mentioned in the Zulip Git style guide, and in more detail here:
https://github.com/zulip/zulip-mobile/blob/main/docs/style.md#mention-fixed-issue

Similarly, when there are previous closely related PRs, mention those too. For example (and I'm mentioning these now in order to get the cross-references): previous iterations of this PR were #586, #584, #583.

Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@satyam372 It looks like this PR doesn't yet have any logic to address the issue #465, which is the issue where we want a banner or snack bar resembling this one. See in particular my last comment below.

I'm marking this PR as draft; feel free to mark as ready again when it has logic that addresses the issue.

The other comments will also be relevant for a revised version of this PR (and for other PRs in general).

Comment on lines 1 to 6
//
// import 'dart:async';
// import 'package:flutter/material.dart';
// import 'package:connectivity_plus/connectivity_plus.dart';
//
// class SnackBarPage extends StatefulWidget {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't check in commented-out code, except where there's some specific useful purpose it serves. Here, it's not clear what the reader is meant to do with this commented-out version of your code. It looks pretty similar to the live version, and it'd be a fair amount of work for the reader to pinpoint the differences.

@@ -38,6 +38,7 @@ dependencies:
sdk: flutter
flutter_localizations:
sdk: flutter
connectivity_plus: ^5.0.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dependency should be added in its own commit. See git log --stat --full-diff pubspec.* for examples.

Comment on lines 182 to 188
style: TextStyle(color: Colors.white),
),
],
),
duration: Duration(seconds: 3),
),
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We write most close-parens in a more compact style than this, which helps the reader see more code at a time. Browse around lib/widgets/ for examples.

Comment on lines 170 to 181
if (isConnected) {
_scaffoldMessengerState.showSnackBar(
const SnackBar(
content: Row(
children: [
Icon(
Icons.sync,
color: Colors.white,
),
SizedBox(width: 8),
Text(
'Connecting',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The naming here doesn't fit together very well — the variable says the app "is connected", but the text says it's "connecting". Those don't logically agree.

When the concepts haven't been given crisp names, that makes it hard to think through what the logic is really doing and whether it matches what's desired. That in turn makes it easy to have bugs.

void didChangeDependencies() {
super.didChangeDependencies();
// Subscribe to connectivity changes
_connectivitySubscription = Connectivity().onConnectivityChanged.listen((ConnectivityResult result) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this PR is meant to be for #465. The title of that issue says:

Show "Connecting…" banner (or equivalent) when server data is stale

But I don't see any logic here that relates to whether the server data is stale. Instead, it seems to be all about whether the device believes it has an internet connection.

Knowing whether the device thinks it has an internet connection can be helpful for choosing the exact warning message to show. But it 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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thankyou for your feedback

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach :-

  1. Checking Internet Connection: connectivity_plus package to determine if the device has
    an active internet connection. This ensures that your app can communicate with the server.

             2) State Management: state management using libraries like Provider or Riverpod. Store 
                 timestamps or flags with your data to track when it was last updated.
    
             3) getting real-time events from GET https://yourZulipDomain.zulipchat.com/api/v1/events get
                   updates about changes in your data. 
    
             4) Timestamps: Attaching  time stamp to  data when  fetching it from the server and compare . 
    

@gnprice this was my approach to the issue #465 . Please correct me if I am going wrong.

Copy link
Author

@satyam372 satyam372 Mar 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sample Code for my logic

` void _refreshData() async {
try {
var response = await http.get(Uri.parse('https://jsonplaceholder.typicode.com/posts'));
// Replace the url with https://yourZulipDomain.zulipchat.com/api/v1/events

  if (response.statusCode == 200) {
    setState(() {
      _isDataStale = false;
    });
    _showSnackbar('Data refreshed successfully');
  } else {
    setState(() {
      _isDataStale = true;
    });
    _showSnackbar('Failed to refresh data. Status code: ${response.statusCode}');
  }
} catch (error) {
  setState(() {
    _isDataStale = true;
  });
  _showSnackbar('An error occurred: $error');
}

}

void _showSnackbar(String message) {
ScaffoldMessenger.of(context).showSnackBar(
SnackBar(
content: Text(message),
duration: Duration(seconds: 3),
),
);
}
}`

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gnprice gnprice marked this pull request as draft March 29, 2024 00:04
@satyam372 satyam372 changed the title snackbar: Handle connectivity changes and show appropriate SnackBar snackbar: Handle connectivity changes and show appropriate SnackBar#465 Apr 3, 2024
@satyam372 satyam372 marked this pull request as ready for review April 3, 2024 20:22
@chrisbobbe
Copy link
Collaborator

Please disable your autoformatter and undo the unrelated formatting changes, so that we can do a deeper code review without distractions. The project's approach to formatting is covered in the README: https://github.com/zulip/zulip-flutter?tab=readme-ov-file#code-formatting

@chrisbobbe
Copy link
Collaborator

Also, please organize your commits: https://github.com/zulip/zulip-flutter?tab=readme-ov-file#submitting-a-pull-request

@satyam372 satyam372 marked this pull request as draft April 4, 2024 04:40
@satyam372
Copy link
Author

If I am understanding your feedback correctly, @chrisbobbe, do I have to make small commits for my feature instead of adding all edited files into a single commit? Please correct me if I am wrong.

@satyam372 satyam372 closed this Apr 5, 2024
@satyam372 satyam372 reopened this Apr 5, 2024
@satyam372 satyam372 force-pushed the 465-connecting-snackbar branch from bb32850 to 3aaf30b Compare April 5, 2024 20:00
@satyam372 satyam372 closed this Apr 5, 2024
@satyam372 satyam372 deleted the 465-connecting-snackbar branch April 5, 2024 20:39
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.

3 participants