From db53af3e2289306a8b9d86c94c926e0700fbfca4 Mon Sep 17 00:00:00 2001 From: ValentinVignal Date: Fri, 3 Jan 2025 21:40:47 +0800 Subject: [PATCH 1/5] Fix some memory leaks --- .../flutter_markdown/lib/src/builder.dart | 77 ++++++++++++++----- .../test/custom_syntax_test.dart | 39 +++++++++- .../flutter_markdown/test/image_test.dart | 4 + packages/flutter_markdown/test/link_test.dart | 3 + .../flutter_markdown/test/padding_test.dart | 2 + .../test/scrollable_test.dart | 1 + 6 files changed, 104 insertions(+), 22 deletions(-) diff --git a/packages/flutter_markdown/lib/src/builder.dart b/packages/flutter_markdown/lib/src/builder.dart index 50bdd11591f8..3fdd9ff13e2c 100644 --- a/packages/flutter_markdown/lib/src/builder.dart +++ b/packages/flutter_markdown/lib/src/builder.dart @@ -346,16 +346,20 @@ class MarkdownBuilder implements md.NodeVisitor { child = builders[_blocks.last.tag!]! .visitText(text, styleSheet.styles[_blocks.last.tag!]); } else if (_blocks.last.tag == 'pre') { - final ScrollController preScrollController = ScrollController(); - child = Scrollbar( - controller: preScrollController, - child: SingleChildScrollView( - controller: preScrollController, - scrollDirection: Axis.horizontal, - padding: styleSheet.codeblockPadding, - child: _buildRichText(delegate.formatText(styleSheet, text.text)), - ), - ); + child = _ScrollControllerBuilder( + builder: (BuildContext context, ScrollController preScrollController, + Widget? child) { + return Scrollbar( + controller: preScrollController, + child: SingleChildScrollView( + controller: preScrollController, + scrollDirection: Axis.horizontal, + padding: styleSheet.codeblockPadding, + child: child, + ), + ); + }, + child: _buildRichText(delegate.formatText(styleSheet, text.text))); } else { child = _buildRichText( TextSpan( @@ -448,15 +452,20 @@ class MarkdownBuilder implements md.NodeVisitor { } } else if (tag == 'table') { if (styleSheet.tableColumnWidth is FixedColumnWidth) { - final ScrollController tableScrollController = ScrollController(); - child = Scrollbar( - controller: tableScrollController, - child: SingleChildScrollView( - controller: tableScrollController, - scrollDirection: Axis.horizontal, - padding: styleSheet.tablePadding, - child: _buildTable(), - ), + child = _ScrollControllerBuilder( + builder: (BuildContext context, + ScrollController tableScrollController, Widget? child) { + return Scrollbar( + controller: tableScrollController, + child: SingleChildScrollView( + controller: tableScrollController, + scrollDirection: Axis.horizontal, + padding: styleSheet.tablePadding, + child: child, + ), + ); + }, + child: _buildTable(), ); } else { child = _buildTable(); @@ -1017,3 +1026,33 @@ class MarkdownBuilder implements md.NodeVisitor { } } } + +class _ScrollControllerBuilder extends StatefulWidget { + const _ScrollControllerBuilder({ + required this.builder, + this.child, + }); + + final ValueWidgetBuilder builder; + + final Widget? child; + + @override + State<_ScrollControllerBuilder> createState() => + _ScrollControllerBuilderState(); +} + +class _ScrollControllerBuilderState extends State<_ScrollControllerBuilder> { + final ScrollController _controller = ScrollController(); + + @override + void dispose() { + _controller.dispose(); + super.dispose(); + } + + @override + Widget build(BuildContext context) { + return widget.builder(context, _controller, widget.child); + } +} diff --git a/packages/flutter_markdown/test/custom_syntax_test.dart b/packages/flutter_markdown/test/custom_syntax_test.dart index 8dc0c806e51f..7263492bd9bf 100644 --- a/packages/flutter_markdown/test/custom_syntax_test.dart +++ b/packages/flutter_markdown/test/custom_syntax_test.dart @@ -276,9 +276,14 @@ class WikilinkSyntax extends md.InlineSyntax { class WikilinkBuilder extends MarkdownElementBuilder { @override Widget visitElementAfter(md.Element element, _) { - return Text.rich(TextSpan( - text: element.textContent, - recognizer: TapGestureRecognizer()..onTap = () {})); + final TapGestureRecognizer recognizer = TapGestureRecognizer() + ..onTap = () {}; + return _TapGestureRecognizerDisposer( + recognizer: recognizer, + child: Text.rich( + TextSpan(text: element.textContent, recognizer: recognizer), + ), + ); } } @@ -460,3 +465,31 @@ class CustomTagBlockSyntax extends md.BlockSyntax { return element; } } + +class _TapGestureRecognizerDisposer extends StatefulWidget { + const _TapGestureRecognizerDisposer({ + required this.child, + required this.recognizer, + }); + + final Widget child; + final TapGestureRecognizer recognizer; + + @override + State<_TapGestureRecognizerDisposer> createState() => + __TapGestureRecognizerDisposerState(); +} + +class __TapGestureRecognizerDisposerState + extends State<_TapGestureRecognizerDisposer> { + @override + void dispose() { + widget.recognizer.dispose(); + super.dispose(); + } + + @override + Widget build(BuildContext context) { + return widget.child; + } +} diff --git a/packages/flutter_markdown/test/image_test.dart b/packages/flutter_markdown/test/image_test.dart index 07a4e87c9058..b78072ad61cf 100644 --- a/packages/flutter_markdown/test/image_test.dart +++ b/packages/flutter_markdown/test/image_test.dart @@ -92,6 +92,7 @@ void defineTests() { testWidgets( 'local files should be files on non-web', (WidgetTester tester) async { + addTearDown(imageCache.clear); const String data = '![alt](http.png)'; await tester.pumpWidget( boilerplate( @@ -131,6 +132,7 @@ void defineTests() { 'should work with resources', (WidgetTester tester) async { TestWidgetsFlutterBinding.ensureInitialized(); + addTearDown(imageCache.clear); const String data = '![alt](resource:assets/logo.png)'; await tester.pumpWidget( boilerplate( @@ -195,6 +197,7 @@ void defineTests() { testWidgets( 'should show properly next to text', (WidgetTester tester) async { + addTearDown(imageCache.clear); const String data = 'Hello ![alt](img#50x50)'; await tester.pumpWidget( boilerplate( @@ -412,6 +415,7 @@ void defineTests() { testWidgets( 'custom image builder', (WidgetTester tester) async { + addTearDown(imageCache.clear); const String data = '![alt](https://img.png)'; Widget builder(Uri uri, String? title, String? alt) => Image.asset('assets/logo.png'); diff --git a/packages/flutter_markdown/test/link_test.dart b/packages/flutter_markdown/test/link_test.dart index 6e4b67c3dcb5..f2769abe1fd3 100644 --- a/packages/flutter_markdown/test/link_test.dart +++ b/packages/flutter_markdown/test/link_test.dart @@ -142,6 +142,7 @@ void defineTests() { testWidgets( 'multiple inline links with same content should not throw an exception', (WidgetTester tester) async { + addTearDown(imageCache.clear); //Arange final Widget toBePumped = boilerplate( Column( @@ -1025,6 +1026,7 @@ void defineTests() { // Example 525 from GFM. 'inline image link text', (WidgetTester tester) async { + addTearDown(imageCache.clear); const String data = '[![moon](moon.jpg)](/uri)'; MarkdownLink? linkTapResults; await tester.pumpWidget( @@ -1129,6 +1131,7 @@ void defineTests() { // Example 528 from GFM. 'links cannot be nested in image linksinline image link text', (WidgetTester tester) async { + addTearDown(imageCache.clear); const String data = '![[[foo](uri1)](uri2)](uri3)'; MarkdownLink? linkTapResults; await tester.pumpWidget( diff --git a/packages/flutter_markdown/test/padding_test.dart b/packages/flutter_markdown/test/padding_test.dart index 6f27ca9d0913..56560aa4a767 100644 --- a/packages/flutter_markdown/test/padding_test.dart +++ b/packages/flutter_markdown/test/padding_test.dart @@ -17,6 +17,8 @@ void defineTests() { (WidgetTester tester) async { const double paddingX = 10.0; + addTearDown(imageCache.clear); + await tester.pumpWidget( boilerplate( Markdown( diff --git a/packages/flutter_markdown/test/scrollable_test.dart b/packages/flutter_markdown/test/scrollable_test.dart index 1042a6fa98a8..90f6f8967299 100644 --- a/packages/flutter_markdown/test/scrollable_test.dart +++ b/packages/flutter_markdown/test/scrollable_test.dart @@ -68,6 +68,7 @@ void defineTests() { final ScrollController controller = ScrollController( initialScrollOffset: 209.0, ); + addTearDown(controller.dispose); await tester.pumpWidget( boilerplate( From 2ce3695def776fdfd9dc63310030bdca5b68c418 Mon Sep 17 00:00:00 2001 From: ValentinVignal Date: Fri, 3 Jan 2025 21:43:47 +0800 Subject: [PATCH 2/5] chore: Update version --- packages/flutter_markdown/CHANGELOG.md | 3 ++- packages/flutter_markdown/pubspec.yaml | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/flutter_markdown/CHANGELOG.md b/packages/flutter_markdown/CHANGELOG.md index 30e4a3804970..a7274b56908c 100644 --- a/packages/flutter_markdown/CHANGELOG.md +++ b/packages/flutter_markdown/CHANGELOG.md @@ -1,6 +1,7 @@ -## NEXT +## 0.7.5 * Updates minimum supported SDK version to Flutter 3.22/Dart 3.4. +* Fixes some memory leaks. ## 0.7.4+3 diff --git a/packages/flutter_markdown/pubspec.yaml b/packages/flutter_markdown/pubspec.yaml index 3c8f7b6ffff7..d533f9b52817 100644 --- a/packages/flutter_markdown/pubspec.yaml +++ b/packages/flutter_markdown/pubspec.yaml @@ -4,7 +4,7 @@ description: A Markdown renderer for Flutter. Create rich text output, formatted with simple Markdown tags. repository: https://github.com/flutter/packages/tree/main/packages/flutter_markdown issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+flutter_markdown%22 -version: 0.7.4+3 +version: 0.7.5 environment: sdk: ^3.4.0 From 505d9a96fc5e2b2bc6dac47705aa05fbe0fa9e6a Mon Sep 17 00:00:00 2001 From: ValentinVignal Date: Sat, 4 Jan 2025 13:52:29 +0800 Subject: [PATCH 3/5] fix memory leaks in tests --- packages/flutter_markdown/test/image_test.dart | 5 +---- packages/flutter_markdown/test/link_test.dart | 6 ++---- packages/flutter_markdown/test/padding_test.dart | 3 +-- 3 files changed, 4 insertions(+), 10 deletions(-) diff --git a/packages/flutter_markdown/test/image_test.dart b/packages/flutter_markdown/test/image_test.dart index b78072ad61cf..77cebc10934d 100644 --- a/packages/flutter_markdown/test/image_test.dart +++ b/packages/flutter_markdown/test/image_test.dart @@ -92,7 +92,6 @@ void defineTests() { testWidgets( 'local files should be files on non-web', (WidgetTester tester) async { - addTearDown(imageCache.clear); const String data = '![alt](http.png)'; await tester.pumpWidget( boilerplate( @@ -132,7 +131,6 @@ void defineTests() { 'should work with resources', (WidgetTester tester) async { TestWidgetsFlutterBinding.ensureInitialized(); - addTearDown(imageCache.clear); const String data = '![alt](resource:assets/logo.png)'; await tester.pumpWidget( boilerplate( @@ -197,7 +195,6 @@ void defineTests() { testWidgets( 'should show properly next to text', (WidgetTester tester) async { - addTearDown(imageCache.clear); const String data = 'Hello ![alt](img#50x50)'; await tester.pumpWidget( boilerplate( @@ -415,7 +412,6 @@ void defineTests() { testWidgets( 'custom image builder', (WidgetTester tester) async { - addTearDown(imageCache.clear); const String data = '![alt](https://img.png)'; Widget builder(Uri uri, String? title, String? alt) => Image.asset('assets/logo.png'); @@ -459,6 +455,7 @@ void defineTests() { find.byType(Container), matchesGoldenFile( 'assets/images/golden/image_test/custom_builder_asset_logo.png')); + imageCache.clear(); }, skip: kIsWeb, // Goldens are platform-specific. ); diff --git a/packages/flutter_markdown/test/link_test.dart b/packages/flutter_markdown/test/link_test.dart index f2769abe1fd3..c98fd6f3a1f3 100644 --- a/packages/flutter_markdown/test/link_test.dart +++ b/packages/flutter_markdown/test/link_test.dart @@ -142,8 +142,7 @@ void defineTests() { testWidgets( 'multiple inline links with same content should not throw an exception', (WidgetTester tester) async { - addTearDown(imageCache.clear); - //Arange + //Arrange final Widget toBePumped = boilerplate( Column( children: [ @@ -1026,7 +1025,6 @@ void defineTests() { // Example 525 from GFM. 'inline image link text', (WidgetTester tester) async { - addTearDown(imageCache.clear); const String data = '[![moon](moon.jpg)](/uri)'; MarkdownLink? linkTapResults; await tester.pumpWidget( @@ -1131,7 +1129,6 @@ void defineTests() { // Example 528 from GFM. 'links cannot be nested in image linksinline image link text', (WidgetTester tester) async { - addTearDown(imageCache.clear); const String data = '![[[foo](uri1)](uri2)](uri3)'; MarkdownLink? linkTapResults; await tester.pumpWidget( @@ -1480,6 +1477,7 @@ void defineTests() { gestureWidget.onTap!(); expectLinkTap(linkTapResults, const MarkdownLink('moon', '/uri')); + imageCache.clear(); }, ); diff --git a/packages/flutter_markdown/test/padding_test.dart b/packages/flutter_markdown/test/padding_test.dart index 56560aa4a767..5a6f5d9ae30f 100644 --- a/packages/flutter_markdown/test/padding_test.dart +++ b/packages/flutter_markdown/test/padding_test.dart @@ -17,8 +17,6 @@ void defineTests() { (WidgetTester tester) async { const double paddingX = 10.0; - addTearDown(imageCache.clear); - await tester.pumpWidget( boilerplate( Markdown( @@ -52,6 +50,7 @@ void defineTests() { paddings[3].padding.along(Axis.horizontal) == paddingX * 4 * 2, true, ); + imageCache.clear(); }, ); }); From 051ee937fd1c3f8ee924a9e367638d6a24d2d133 Mon Sep 17 00:00:00 2001 From: ValentinVignal Date: Sat, 4 Jan 2025 13:52:42 +0800 Subject: [PATCH 4/5] Activate leak testing --- packages/flutter_markdown/pubspec.yaml | 1 + .../flutter_markdown/test/flutter_test_config.dart | 13 +++++++++++++ 2 files changed, 14 insertions(+) create mode 100644 packages/flutter_markdown/test/flutter_test_config.dart diff --git a/packages/flutter_markdown/pubspec.yaml b/packages/flutter_markdown/pubspec.yaml index d533f9b52817..997d092ae8bd 100644 --- a/packages/flutter_markdown/pubspec.yaml +++ b/packages/flutter_markdown/pubspec.yaml @@ -20,6 +20,7 @@ dependencies: dev_dependencies: flutter_test: sdk: flutter + leak_tracker_flutter_testing: any mockito: ^5.4.4 standard_message_codec: ^0.0.1+3 diff --git a/packages/flutter_markdown/test/flutter_test_config.dart b/packages/flutter_markdown/test/flutter_test_config.dart new file mode 100644 index 000000000000..9907e578b84b --- /dev/null +++ b/packages/flutter_markdown/test/flutter_test_config.dart @@ -0,0 +1,13 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'dart:async'; + +import 'package:leak_tracker_flutter_testing/leak_tracker_flutter_testing.dart'; + +Future testExecutable(FutureOr Function() testMain) async { + LeakTesting.enable(); + LeakTracking.warnForUnsupportedPlatforms = false; + await testMain(); +} From 4042c91fc66a1af1d351477dbd33fef1d5ad0ea4 Mon Sep 17 00:00:00 2001 From: ValentinVignal Date: Sat, 4 Jan 2025 14:17:39 +0800 Subject: [PATCH 5/5] fix failing test --- .../test/custom_syntax_test.dart | 36 ++----------------- 1 file changed, 3 insertions(+), 33 deletions(-) diff --git a/packages/flutter_markdown/test/custom_syntax_test.dart b/packages/flutter_markdown/test/custom_syntax_test.dart index 7263492bd9bf..9af6106353e2 100644 --- a/packages/flutter_markdown/test/custom_syntax_test.dart +++ b/packages/flutter_markdown/test/custom_syntax_test.dart @@ -278,11 +278,9 @@ class WikilinkBuilder extends MarkdownElementBuilder { Widget visitElementAfter(md.Element element, _) { final TapGestureRecognizer recognizer = TapGestureRecognizer() ..onTap = () {}; - return _TapGestureRecognizerDisposer( - recognizer: recognizer, - child: Text.rich( - TextSpan(text: element.textContent, recognizer: recognizer), - ), + addTearDown(recognizer.dispose); + return Text.rich( + TextSpan(text: element.textContent, recognizer: recognizer), ); } } @@ -465,31 +463,3 @@ class CustomTagBlockSyntax extends md.BlockSyntax { return element; } } - -class _TapGestureRecognizerDisposer extends StatefulWidget { - const _TapGestureRecognizerDisposer({ - required this.child, - required this.recognizer, - }); - - final Widget child; - final TapGestureRecognizer recognizer; - - @override - State<_TapGestureRecognizerDisposer> createState() => - __TapGestureRecognizerDisposerState(); -} - -class __TapGestureRecognizerDisposerState - extends State<_TapGestureRecognizerDisposer> { - @override - void dispose() { - widget.recognizer.dispose(); - super.dispose(); - } - - @override - Widget build(BuildContext context) { - return widget.child; - } -}