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.

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

Implement the log retention setting #7951

Merged
merged 15 commits into from
Jun 25, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,13 @@
// found in the LICENSE file.

import 'dart:async';
import 'dart:collection';

import 'package:devtools_app_shared/utils.dart';
import 'package:flutter/foundation.dart';

import '../../../shared/globals.dart';

CoderDake marked this conversation as resolved.
Show resolved Hide resolved
import '../../../shared/utils.dart';
import 'logging_controller_v2.dart';
import 'logging_table_row.dart';
Expand All @@ -17,19 +21,27 @@ import 'logging_table_v2.dart';
/// The [LoggingTableV2] table uses variable height rows. This model caches the
/// relevant heights and offsets so that the row heights only need to be calculated
/// once per parent width.
class LoggingTableModel extends ChangeNotifier {
class LoggingTableModel extends ChangeNotifier with DisposerMixin {
LoggingTableModel() {
_worker = InterruptableChunkWorker(
callback: (index) => getFilteredLogHeight(
index,
),
progressCallback: (progress) => _cacheLoadProgress.value = progress,
);

addAutoDisposeListener(
preferences.logging.retentionLimit,
_onRetentionLimitUpdate,
);

_retentionLimit = preferences.logging.retentionLimit.value;
CoderDake marked this conversation as resolved.
Show resolved Hide resolved
}

final _logs = <LogDataV2>[];
final _filteredLogs = <LogDataV2>[];
final _selectedLogs = <int>{};
final _logs = ListQueue<LogDataV2>();
final _filteredLogs = ListQueue<LogDataV2>();
final _selectedLogs = ListQueue<LogDataV2>();
late int _retentionLimit;

final cachedHeights = <int, double>{};
final cachedOffets = <int, double>{};
Expand All @@ -42,12 +54,28 @@ class LoggingTableModel extends ChangeNotifier {
ValueListenable<double?> get cacheLoadProgress => _cacheLoadProgress;
final _cacheLoadProgress = ValueNotifier<double?>(null);

void _onRetentionLimitUpdate() {
_retentionLimit = preferences.logging.retentionLimit.value;
while (_logs.length > _retentionLimit) {
if (identical(_filteredLogs.first, _logs.first)) {
// Remove a filtered log if it is about to disapear from the _logs.
_filteredLogs.removeFirst();
}
_logs.removeFirst();
}

notifyListeners();
}

@override
void dispose() {
super.dispose();
_cacheLoadProgress.dispose();
_worker.dispose();
super.dispose();
}

double get tableWidth => _tableWidth;

/// Update the width of the table.
///
/// If different from the last width, this will flush all of the calculated heights, and recalculate their heights
Expand All @@ -62,7 +90,7 @@ class LoggingTableModel extends ChangeNotifier {
}

/// Get the filtered log at [index].
LogDataV2 filteredLogAt(int index) => _filteredLogs[index];
LogDataV2 filteredLogAt(int index) => _filteredLogs.elementAt(index);

double _tableWidth = 0.0;

Expand All @@ -81,6 +109,15 @@ class LoggingTableModel extends ChangeNotifier {

_logs.add(log);
_filteredLogs.add(log);
if (_logs.length > _retentionLimit) {
if (identical(_logs.first, _filteredLogs.first)) {
// Remove a filtered log if it is about to go out of retention.
_filteredLogs.removeFirst();
}
// Remove the log that has just gone out of retention.
_logs.removeFirst();
}
CoderDake marked this conversation as resolved.
Show resolved Hide resolved

getFilteredLogHeight(
_logs.length - 1,
);
Expand All @@ -105,7 +142,7 @@ class LoggingTableModel extends ChangeNotifier {
if (cachedHeight != null) return cachedHeight;

return cachedHeights[index] = LoggingTableRow.calculateRowHeight(
_logs[index],
_logs.elementAt(index),
_tableWidth,
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'dart:math';

import 'package:devtools_app_shared/ui.dart';
import 'package:flutter/material.dart';

Expand Down Expand Up @@ -39,7 +41,7 @@ class LoggingTableRow extends StatefulWidget {
double width,
) {
final text = log.asLogDetails();
final maxWidth = width - _padding * 2;
final maxWidth = max(0.0, width - _padding * 2);

final row1Height = calculateTextSpanHeight(
TextSpan(text: text, style: detailsStyle),
Expand All @@ -49,9 +51,10 @@ class LoggingTableRow extends StatefulWidget {
// TODO(danchevalier): Improve row2 height by manually flowing metadas into another row
// if theyoverflow.
final row2Height = calculateTextSpanHeight(
TextSpan(text: text, style: metadataStyle),
TextSpan(text: 'short text for now', style: metadataStyle),
Copy link
Member

Choose a reason for hiding this comment

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

remove this hard coded string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Metadata will be handled in a future PR. I'll just leave this for now.

maxWidth: maxWidth,
);

return row1Height + row2Height + _padding * 2;
}
}
Expand All @@ -76,6 +79,7 @@ class _LoggingTableRowState extends State<LoggingTableRow> {
padding: EdgeInsets.all(LoggingTableRow._padding),
child: Column(
crossAxisAlignment: CrossAxisAlignment.start,
mainAxisSize: MainAxisSize.min,
children: [
RichText(
text: TextSpan(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import 'package:flutter/services.dart';
import '../../../service/service_extension_widgets.dart';
import '../../../shared/analytics/constants.dart' as gac;
import '../../../shared/common_widgets.dart';
import '../../../shared/globals.dart';
import 'logging_model.dart';
import 'logging_table_row.dart';

Expand Down Expand Up @@ -114,10 +115,11 @@ class _LoggingTableProgress extends StatefulWidget {
}

class _LoggingTableProgressState extends State<_LoggingTableProgress> {
final _progressStopwatch = Stopwatch();
static const _millisecondsUntilCacheProgressShows = 500;
static const _millisecondsUntilCacheProgressHelperShows = 2000;

final _progressStopwatch = Stopwatch();

@override
Widget build(BuildContext context) {
return LayoutBuilder(
Expand Down Expand Up @@ -226,9 +228,16 @@ class _LoggingTableRowsState extends State<_LoggingTableRows>
}
}

class LoggingSettingsDialogV2 extends StatelessWidget {
class LoggingSettingsDialogV2 extends StatefulWidget {
const LoggingSettingsDialogV2({super.key});

@override
State<LoggingSettingsDialogV2> createState() =>
_LoggingSettingsDialogV2State();
}

class _LoggingSettingsDialogV2State extends State<LoggingSettingsDialogV2> {
int? retentionLimit;
@override
CoderDake marked this conversation as resolved.
Show resolved Hide resolved
Widget build(BuildContext context) {
final theme = Theme.of(context);
Expand All @@ -243,75 +252,104 @@ class LoggingSettingsDialogV2 extends StatelessWidget {
'General',
),
const StructuredErrorsToggle(),
const SizedBox(height: defaultSpacing),
_RententionLimitSetting(
theme: theme,
CoderDake marked this conversation as resolved.
Show resolved Hide resolved
onRetentionLimitChange: (newRetentionLimit) =>
retentionLimit = newRetentionLimit,
),
],
),
actions: const [
DialogCloseButton(),
actions: [
DialogApplyButton(
onPressed: () {
if (retentionLimit != null) {
// Save the new retention limit to preferences.
preferences.logging.retentionLimit.value = retentionLimit!;
}
},
),
const DialogCloseButton(),
],
);
}
}

/// Shows and hides the context menu based on user gestures.
///
/// By default, shows the menu on right clicks and long presses.
class _ContextMenuRegion extends StatefulWidget {
/// Creates an instance of [_ContextMenuRegion].
const _ContextMenuRegion({
required this.child,
required this.contextMenuBuilder,
class _RententionLimitSetting extends StatefulWidget {
const _RententionLimitSetting({
required this.theme,
required this.onRetentionLimitChange,
});

/// Builds the context menu.
final ContextMenuBuilder contextMenuBuilder;

/// The child widget that will be listened to for gestures.
final Widget child;
final ThemeData theme;
final void Function(int) onRetentionLimitChange;

@override
State<_ContextMenuRegion> createState() => _ContextMenuRegionState();
State<_RententionLimitSetting> createState() =>
_RententionLimitSettingState();
}

class _ContextMenuRegionState extends State<_ContextMenuRegion> {
final _contextMenuController = ContextMenuController();

void _onSecondaryTapUp(TapUpDetails details) {
_show(details.globalPosition);
}

void _onTap() {
if (!_contextMenuController.isShown) {
return;
}
_hide();
}

void _show(Offset position) {
_contextMenuController.show(
context: context,
contextMenuBuilder: (BuildContext context) {
return widget.contextMenuBuilder(context, position);
},
);
class _RententionLimitSettingState extends State<_RententionLimitSetting>
with AutoDisposeMixin {
void updateRetentionLimit() {
newRetentionLimit = preferences.logging.retentionLimit.value;
}

void _hide() {
_contextMenuController.remove();
@override
void initState() {
super.initState();
preferences.logging.retentionLimit.addListener(updateRetentionLimit);
Copy link
Member

Choose a reason for hiding this comment

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

use addAutoDisposeListener instead

updateRetentionLimit();
}

@override
void dispose() {
_hide();
preferences.logging.retentionLimit.removeListener(updateRetentionLimit);
super.dispose();
}

late int newRetentionLimit;

@override
Widget build(BuildContext context) {
return GestureDetector(
behavior: HitTestBehavior.opaque,
onSecondaryTapUp: _onSecondaryTapUp,
onTap: _onTap,
child: widget.child,
return Row(
CoderDake marked this conversation as resolved.
Show resolved Hide resolved
children: [
Expanded(
child: Column(
crossAxisAlignment: CrossAxisAlignment.start,
children: [
Text(preferences.logging.retentionLimitTitle),
Text(
'Used to limit the number of log messages retained.',
style: widget.theme.subtleTextStyle,
),
],
),
),
const SizedBox(width: defaultSpacing),
SizedBox(
height: defaultTextFieldHeight,
width: defaultTextFieldNumberWidth,
child: TextField(
Copy link
Member

Choose a reason for hiding this comment

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

looking at the screenshot, the text is not vertically centered in the text box. Can we fix that as part of this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot 2024-06-24 at 1 35 40 PM

To get it closer to vertically centered, I've set it to TextAlignVertical.top
a

style: widget.theme.regularTextStyle,
decoration: singleLineDialogTextFieldDecoration,
controller: TextEditingController(
text: newRetentionLimit.toString(),
),
Copy link
Member

Choose a reason for hiding this comment

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

this will get created on each build and never disposed, causing memory leaks. Create the text editing controller in initState and use it here. Also dispose it in dispose.

By doing this, I think we can also drop the newRetentionLimit variable, since you can just lookup the text from the controller textEditingController.text

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated, but it doesn't look like it has a dispose function.

Copy link
Member

Choose a reason for hiding this comment

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

It should. Here is an example of another TextEditingController getting disposed: https://github.com/flutter/devtools/blob/master/packages/devtools_app/lib/src/framework/home_screen.dart/#L191

inputFormatters: <TextInputFormatter>[
// Only positive integers.
FilteringTextInputFormatter.allow(
RegExp(r'^[1-9][0-9]*'),
),
],
onChanged: (String text) {
final newValue = int.parse(text);
newRetentionLimit = newValue;
widget.onRetentionLimitChange(newValue);
},
),
),
],
);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ part 'constants/_debugger_constants.dart';
part 'constants/_deep_links_constants.dart';
part 'constants/_extension_constants.dart';
part 'constants/_memory_constants.dart';
part 'constants/_logging_constants.dart';
part 'constants/_performance_constants.dart';
part 'constants/_vs_code_sidebar_constants.dart';
part 'constants/_inspector_constants.dart';
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Copyright 2024 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

part of '../constants.dart';

// ignore: avoid_classes_with_only_static_members, requires refactor.
/// Logging event constants specific for logging screen.
class LoggingEvent {
static const changeRetentionLimit = 'changeRetentionLimit';
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// Copyright 2024 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

part of 'preferences.dart';

class LoggingPreferencesController extends DisposableController
with AutoDisposeControllerMixin {
final retentionLimitTitle = 'Limit for the number of logs retained.';

/// The number of logs to retain on the logging table.
final retentionLimit = ValueNotifier<int>(_defaultRetentionLimit);

static const _defaultRetentionLimit = 3000;

static const _retentionLimitStorageId = 'logging.retentionLimit';

Future<void> init() async {
addAutoDisposeListener(
retentionLimit,
() {
storage.setValue(
_retentionLimitStorageId,
retentionLimit.value.toString(),
);

ga.select(
gac.logging,
gac.LoggingEvent.changeRetentionLimit,
value: retentionLimit.value,
);
},
);
retentionLimit.value =
int.tryParse(await storage.getValue(_retentionLimitStorageId) ?? '') ??
_defaultRetentionLimit;
CoderDake marked this conversation as resolved.
Show resolved Hide resolved
}
}
Loading
Loading