-
Notifications
You must be signed in to change notification settings - Fork 336
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
Add filtering to the Network profiler #2340
Conversation
High level: it could make sense to support a query field where users can type a query like |
return textFieldSuffixButton(Icons.clear, onPressed); | ||
} | ||
|
||
Widget textFieldSuffixButton(IconData icon, VoidCallback onPressed) { |
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.
why is this a "suffix" button?
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.
It's for the suffix
parameter of an InputDecoration. Maybe I should rename the method to inputDecorationSuffixButton
padding: const EdgeInsets.symmetric(horizontal: densePadding), | ||
width: 24.0, | ||
child: IconButton( | ||
padding: const EdgeInsets.all(0.0), |
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.
nit: would placing the padding in the icon button look different than in the container?
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 explicit 0 padding is to override the default padding of 4.0 in the IconButton, otherwise the size constraint of 24.0 is not respected.
this.showWebSocket = true, | ||
}); | ||
|
||
static NetworkFilter from(NetworkFilter filter) { |
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.
nit: make a factory constructor even though that won't change anything.
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.
done
); | ||
} | ||
|
||
String method; |
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.
can these all be final?
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.
no because they need to be modified when adjusting the filter in the dialog. We could make a copyWith
method. Leaving as is for now since I'll likely tweak all this code in a follow up to use a query.
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.
a1bef00
to
13034a5
Compare
Updated to use query-based filter as discussed. PTAL |
@@ -351,11 +351,11 @@ class NetworkFilter { | |||
if (querySeparatorIndex != -1) { | |||
final value = part.substring(querySeparatorIndex + 1); | |||
if (value != '') { |
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.
it could be nice to have an enum like
QueryFliter { final Iterable keys; }
class so that you can reuse this parsing logic for all pages rather than writing it again on each page.
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.
If a filter is applied, the filter button is toggled:
At the bottom of the screen, the filter count "x of y" is shown:
Fixes #2316