-
Notifications
You must be signed in to change notification settings - Fork 329
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
Offline support for Network page #8332
Open
hrajwade96
wants to merge
35
commits into
flutter:master
Choose a base branch
from
hrajwade96:network_screen_offline_support
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+1,087
−175
Open
Changes from 31 commits
Commits
Show all changes
35 commits
Select commit
Hold shift + click to select a range
fe11d5b
added initial implementation
hrajwade96 6c140b4
adding constants file
hrajwade96 0c1f414
updated NEXT_RELEASE_NOTES.md
hrajwade96 e7eeecb
Revert "updated NEXT_RELEASE_NOTES.md"
hrajwade96 c5ec6f6
used constants, added util for header size calc, added params
hrajwade96 131cde6
Merge branch 'master' into network_screen_offline_support
hrajwade96 2a9c4cb
param renamed, used _log
hrajwade96 4f473be
Merge remote-tracking branch 'origin/network_screen_offline_support' …
hrajwade96 ae58738
added copyright header
hrajwade96 2bfbe1c
setup changes implemented
hrajwade96 857cfbe
put class name in square brackets
hrajwade96 7150436
added isNullOrEmpty check
hrajwade96 da11a67
added exit offline cta, hiding other controls, removed json string
hrajwade96 941ec88
fix for request selection
hrajwade96 852b144
comments resolved
hrajwade96 433af38
removing getFullRequestData section
hrajwade96 4644b00
using updateOrAddAll for setting data
hrajwade96 6fd98d0
Wrapped with OfflineAwareControls, removed if-else.
hrajwade96 665ea0d
removed addAutoDisposeListener as we are using OfflineAwareControls
hrajwade96 b6df572
removed try-catch
hrajwade96 be63db4
code refactoring
hrajwade96 91505b5
added enum
hrajwade96 0715379
code refactoring, reverting changes on network service
hrajwade96 ade8b5f
using values from enum
hrajwade96 f21a4f6
delegating toJson using extension methods, code refactoring
hrajwade96 454ca92
socket data de-serialisation changes
hrajwade96 b7b9679
Merge branch 'master' into network_screen_offline_support
hrajwade96 ac726e8
used factory constructor, made enum private
hrajwade96 2ea3044
reorder params, used isEmpty
hrajwade96 e01bc3c
added tests, minor fixes
hrajwade96 e7b5c90
removed repetitive expect statements
hrajwade96 6ab0884
Merge remote-tracking branch 'upstream/master' into network_screen_of…
hrajwade96 4f31b92
minor fix
hrajwade96 04c8b41
added timelineMicrosOffset to offline data
hrajwade96 ab8c519
added timelineMicrosOffset to tests
hrajwade96 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please address this TODO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While testing I noticed 2 issues :
Attaching SS for 1st point :
Online mode
Offline mode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another point is that in offline mode data of socket and http requests is grouped when it's populated.
I think we should show them in their original order as the requests came in? Perhaps we can pack them in a combined way instead, or other way is to reorder before rendering based on timestamps (once we fix the timings) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually past 2 weeks I've been on vacation, and then unwell so got very little time to investigate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The table should already sort the data by timestamp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kenzieschmoll can you help with first 2 points? also point 3) on page refresh when in offline mode, this is how it is looking like currently, it's clearing the list, and also not showing exit cta
what is the expected behaviour here?
Do we need to persist the data in storage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think 1st point is due to value of _timelineMicrosBase being reset when entering offline mode, causing a discrepancy in timing calculation with this function :
int timelineMicrosecondsSinceEpoch(int micros) { return _timelineMicrosBase + micros; }
I fixed it by adding timelineMicrosOffset in offline data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2nd and 3rd points are remaining,
for (2) - I tried fetching full data with periodic timer
so that full data is always available before entering in offline mode. Or alternatively we can fetch each time new request data is added in the current list. Is this fine? or any better ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for (3) is there any simple way to add anther check for showing exit offline cta?
such as whenever it is disconnected, current check doesn't seem to work after page refresh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW I will be unavailable here for the next 4 days (27th - 1st Dec)