Skip to content
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.

Sign up for GitHub

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 search to the Network profiler #2333

Merged
merged 4 commits into from
Sep 14, 2020
Merged

Add search to the Network profiler #2333

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fd4af52
Add search to the Network profiler
kenzieschmoll Sep 11, 2020
45f2a54
Merge branch 'master' of github.com:flutter/devtools into search
kenzieschmoll Sep 14, 2020
43478b6
Review comments
kenzieschmoll Sep 14, 2020
cf9ba41
review comments
kenzieschmoll Sep 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions packages/devtools_app/lib/src/http/http_request_data.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,8 +136,9 @@ class HttpRequestData extends NetworkRequest {
/// around the issue by displaying them as "in-progress". It would be
/// reasonable to display them as "unknown start time" but that seems like
/// more complexity than it is worth.
// TODO(kenz): figure out how to handle HTTP body events in the network
// profiler. For now, mark them as invalid.
// TODO(kenz): https://github.com/flutter/devtools/issues/2335 - figure out
// how to handle HTTP body events in the network profiler. For now, mark them
// as invalid.
bool get isValid =>
_startEvent != null && !_startEvent.name.contains('HTTP CLIENT response');
kenzieschmoll marked this conversation as resolved.
Show resolved Hide resolved

Expand Down
100 changes: 57 additions & 43 deletions packages/devtools_app/lib/src/table.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -679,8 +679,6 @@ class _TableState<T> extends State<_Table<T>> with AutoDisposeMixin {
SortDirection sortDirection;
ScrollController scrollController;

void Function() activeSearchListener;

@override
void initState() {
super.initState();
Expand All @@ -691,13 +689,7 @@ class _TableState<T> extends State<_Table<T>> with AutoDisposeMixin {
scrollController = ScrollController();

if (widget.activeSearchMatchNotifier != null) {
widget.activeSearchMatchNotifier
.addListener(activeSearchListener = _onActiveSearchChange);

// TODO(kenz): why isn't the listener firing when added via
// `addAutoDisploseListener`?
// addAutoDisposeListener(
// widget.activeSearchMatchNotifier, _onActiveSearchChange);
_initSearchListener();
}
}

Expand All @@ -719,6 +711,13 @@ class _TableState<T> extends State<_Table<T>> with AutoDisposeMixin {
}
}

void _initSearchListener() {
addAutoDisposeListener(
kenzieschmoll marked this conversation as resolved.
Show resolved Hide resolved
widget.activeSearchMatchNotifier,
_onActiveSearchChange,
);
}

@override
void didUpdateWidget(_Table oldWidget) {
super.didUpdateWidget(oldWidget);
Expand All @@ -740,6 +739,10 @@ class _TableState<T> extends State<_Table<T>> with AutoDisposeMixin {
}
});
});

Copy link
Contributor

Choose a reason for hiding this comment

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

why isn't the selection notifier listener also tracked in the helper called from initState and didUpdateWidget?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't modify anything about the selection listener because it's not in the scope of this PR - will add a note to address

if (widget.activeSearchMatchNotifier != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

redundant check for the notifier being not null.

Copy link
Member Author

Choose a reason for hiding this comment

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

removed now that the check is inside _initSearchListener

_initSearchListener();
}
}

/// Return the number of visible rows above the selected node.
Expand Down Expand Up @@ -784,9 +787,7 @@ class _TableState<T> extends State<_Table<T>> with AutoDisposeMixin {

@override
void dispose() {
widget.activeSearchMatchNotifier.removeListener(activeSearchListener);
scrollController.dispose();

super.dispose();
}

Expand Down Expand Up @@ -1000,6 +1001,10 @@ class _TableRowState<T> extends State<TableRow<T>>

ScrollController scrollController;

bool isSearchMatch = false;

bool isActiveSearchMatch = false;

@override
void initState() {
super.initState();
Expand All @@ -1015,35 +1020,7 @@ class _TableRowState<T> extends State<TableRow<T>>
}
});

if (widget.searchMatchesNotifier != null) {
searchMatches = widget.searchMatchesNotifier.value;
addAutoDisposeListener(widget.searchMatchesNotifier, () {
final isPreviousMatch = searchMatches.contains(widget.node);
final isNewMatch =
widget.searchMatchesNotifier.value.contains(widget.node);
searchMatches = widget.searchMatchesNotifier.value;

// We only want to rebuild the row if it the match status has changed.
if (isPreviousMatch ^ isNewMatch) {
setState(() {});
}
});
}

if (widget.activeSearchMatchNotifier != null) {
activeSearchMatch = widget.activeSearchMatchNotifier.value;
addAutoDisposeListener(widget.activeSearchMatchNotifier, () {
final isPreviousActiveSearchMatch = activeSearchMatch == widget.node;
final isNewActiveSearchMatch =
widget.activeSearchMatchNotifier.value == widget.node;
activeSearchMatch = widget.activeSearchMatchNotifier.value;

// We only want to rebuild the row if it the match status has changed.
if (isPreviousActiveSearchMatch ^ isNewActiveSearchMatch) {
setState(() {});
}
});
}
_initSearchListeners();
}

@override
Expand All @@ -1055,6 +1032,9 @@ class _TableRowState<T> extends State<TableRow<T>>
scrollController?.dispose();
scrollController = widget.linkedScrollControllerGroup.addAndGet();
}

cancel();
_initSearchListeners();
}

@override
Expand Down Expand Up @@ -1106,14 +1086,48 @@ class _TableRowState<T> extends State<TableRow<T>>
);
}

void _initSearchListeners() {
if (widget.searchMatchesNotifier != null) {
searchMatches = widget.searchMatchesNotifier.value;
isSearchMatch = searchMatches.contains(widget.node);
addAutoDisposeListener(widget.searchMatchesNotifier, () {
final isPreviousMatch = searchMatches.contains(widget.node);
searchMatches = widget.searchMatchesNotifier.value;
final isNewMatch = searchMatches.contains(widget.node);

// We only want to rebuild the row if it the match status has changed.
if (isPreviousMatch != isNewMatch) {
setState(() {
isSearchMatch = isNewMatch;
});
}
});
}

if (widget.activeSearchMatchNotifier != null) {
activeSearchMatch = widget.activeSearchMatchNotifier.value;
isActiveSearchMatch = activeSearchMatch == widget.node;
addAutoDisposeListener(widget.activeSearchMatchNotifier, () {
final isPreviousActiveSearchMatch = activeSearchMatch == widget.node;
activeSearchMatch = widget.activeSearchMatchNotifier.value;
final isNewActiveSearchMatch = activeSearchMatch == widget.node;

// We only want to rebuild the row if it the match status has changed.
if (isPreviousActiveSearchMatch != isNewActiveSearchMatch) {
setState(() {
isActiveSearchMatch = isNewActiveSearchMatch;
});
}
});
}
}

Color _searchAwareBackgroundColor() {
final backgroundColor =
widget.backgroundColor ?? titleSolidBackgroundColor(Theme.of(context));
final isSearchMatch = searchMatches.contains(widget.node);
final isActiveSearch = activeSearchMatch == widget.node;
final searchAwareBackgroundColor = isSearchMatch
? Color.alphaBlend(
isActiveSearch
isActiveSearchMatch
? activeSearchMatchColorOpaque
: searchMatchColorOpaque,
backgroundColor,
Expand Down