diff --git a/packages/pdfrx/lib/src/pdf_document_ref.dart b/packages/pdfrx/lib/src/pdf_document_ref.dart index 150b406c..c3097859 100644 --- a/packages/pdfrx/lib/src/pdf_document_ref.dart +++ b/packages/pdfrx/lib/src/pdf_document_ref.dart @@ -440,6 +440,12 @@ class PdfDocumentListenable extends Listenable { setError(err, stackTrace); return report?.copyWith(elapsedTime: stopwatch.elapsed); } + // Every listener left while the document was loading: this listenable was evicted, and the next resolve builds a + // new one. Nothing can reach the document anymore, so keeping it would leak it. + if (!identical(PdfDocumentRef._listenables[ref], this)) { + if (ref.autoDispose) await document.dispose(); + return report?.copyWith(elapsedTime: stopwatch.elapsed); + } setDocument(document); return report?.copyWith(elapsedTime: stopwatch.elapsed); }); diff --git a/packages/pdfrx/lib/src/widgets/pdf_text_searcher.dart b/packages/pdfrx/lib/src/widgets/pdf_text_searcher.dart index d486dc5c..bb7319ef 100644 --- a/packages/pdfrx/lib/src/widgets/pdf_text_searcher.dart +++ b/packages/pdfrx/lib/src/widgets/pdf_text_searcher.dart @@ -83,6 +83,13 @@ class PdfTextSearcher extends Listenable { bool goToFirstMatch = true, bool searchImmediately = false, }) { + // Re-issuing the running pattern must not cancel it: search() below would return early for an identical pattern, + // leaving the matches half collected and isSearching stuck. Only drop a pending search for another pattern. + final last = _lastSearchCondition; + if (last != null && last.caseInsensitive == caseInsensitive && _isIdenticalPattern(last.pattern, pattern)) { + _searchTextTimer?.cancel(); + return; + } _cancelTextSearch(); final searchSession = ++_searchSession; @@ -118,7 +125,7 @@ class PdfTextSearcher extends Listenable { _resetTextSearch(notify: false); } - void _resetTextSearch({bool notify = true, bool clearSearchCondition = true}) { + void _resetTextSearch({bool notify = true}) { _cancelTextSearch(); _matches = const []; _matchesPageStartIndices = const []; @@ -126,9 +133,7 @@ class PdfTextSearcher extends Listenable { _currentIndex = null; _currentMatch = null; _isSearching = false; - if (clearSearchCondition) { - _lastSearchCondition = null; - } + _lastSearchCondition = null; if (notify) { notifyListeners(); } @@ -145,6 +150,9 @@ class PdfTextSearcher extends Listenable { final textMatchesPageStartIndex = []; var first = true; _isSearching = true; + // The previous pattern's position does not index into the new matches. + _currentIndex = null; + _currentMatch = null; _totalPageCount = document.pages.length; for (final page in document.pages) { _searchingPageNumber = page.pageNumber; @@ -188,13 +196,15 @@ class PdfTextSearcher extends Listenable { } void _restartSearch() { - _resetTextSearch(clearSearchCondition: false); + final condition = _lastSearchCondition; + // Clearing the condition lets startTextSearch run the same pattern again. + _resetTextSearch(); _cachedText.clear(); - if (_lastSearchCondition != null) { + if (condition != null) { startTextSearch( - _lastSearchCondition!.pattern, - caseInsensitive: _lastSearchCondition!.caseInsensitive, - goToFirstMatch: _lastSearchCondition!.goToFirstMatch, + condition.pattern, + caseInsensitive: condition.caseInsensitive, + goToFirstMatch: condition.goToFirstMatch, ); } } @@ -244,6 +254,7 @@ class PdfTextSearcher extends Listenable { ); controller?.setCurrentPageNumber(match.pageNumber); controller?.invalidate(); + notifyListeners(); } /// Get the matches range for the given page number. diff --git a/packages/pdfrx/lib/src/widgets/pdf_viewer.dart b/packages/pdfrx/lib/src/widgets/pdf_viewer.dart index f2acca5f..25b0cecc 100644 --- a/packages/pdfrx/lib/src/widgets/pdf_viewer.dart +++ b/packages/pdfrx/lib/src/widgets/pdf_viewer.dart @@ -1661,6 +1661,7 @@ class _PdfViewerState extends State FilterQuality filterQuality = FilterQuality.high, }) { final unusedPageList = []; + final previewRequests = <({PdfPage page, double scale, Rect rect})>[]; final unmeasuredPageList = []; // Pages inside the extent that are painting as a blank white rectangle // because no preview image has landed for them yet. This is the white-page @@ -1749,7 +1750,7 @@ class _PdfViewerState extends State if (enableLowResolutionPagePreview && (previewImage == null || previewImage.isDirty || previewImage.scale != previewScaleLimit)) { - _requestPagePreviewImageCached(cache, page, previewScaleLimit); + previewRequests.add((page: page, scale: previewScaleLimit, rect: rect)); } final pageScale = page.width > 0 && page.height > 0 @@ -1797,20 +1798,28 @@ class _PdfViewerState extends State callback(canvas, rect, page); } } + } - if (unusedPageList.isNotEmpty) { - final currentPageNumber = _pageNumber; - if (currentPageNumber != null && currentPageNumber > 0) { - final currentPage = _document!.pages[currentPageNumber - 1]; - cache.removeCacheImagesIfCacheBytesExceedsLimit( - unusedPageList, - maxImageCacheBytes, - currentPage, - dist: (pageNumber) => - (_layout!.pageLayouts[pageNumber - 1].center - _layout!.pageLayouts[currentPage.pageNumber - 1].center) - .distanceSquared, - ); - } + // The preview lock serves requests in call order, and the loop above walks pages by index -- so the page above + // the viewport used to render before the one on screen. Ask for the visible pages first. + previewRequests.sort((a, b) => _distanceToRect(a.rect, targetRect).compareTo(_distanceToRect(b.rect, targetRect))); + for (final request in previewRequests) { + _requestPagePreviewImageCached(cache, request.page, request.scale); + } + + // Once per paint, after the loop: inside it the list was still growing and the cache was trimmed once per page. + if (unusedPageList.isNotEmpty) { + final currentPageNumber = _pageNumber; + if (currentPageNumber != null && currentPageNumber > 0) { + final currentPage = _document!.pages[currentPageNumber - 1]; + cache.removeCacheImagesIfCacheBytesExceedsLimit( + unusedPageList, + maxImageCacheBytes, + currentPage, + dist: (pageNumber) => + (_layout!.pageLayouts[pageNumber - 1].center - _layout!.pageLayouts[currentPage.pageNumber - 1].center) + .distanceSquared, + ); } } @@ -2024,6 +2033,13 @@ class _PdfViewerState extends State void _invalidate() => _updateStream.add(_txController.value); + /// 0 for a rect that overlaps [target], otherwise the gap between them. + static double _distanceToRect(Rect rect, Rect target) { + final dx = max(0.0, max(target.left - rect.right, rect.left - target.right)); + final dy = max(0.0, max(target.top - rect.bottom, rect.top - target.bottom)); + return dx * dx + dy * dy; + } + Future _requestPagePreviewImageCached(_PdfPageImageCache cache, PdfPage page, double scale) async { final width = page.width * scale; final height = page.height * scale; @@ -4232,6 +4248,9 @@ class _PdfPageImageCache { pageImages.values.fold(0, (sum, e) => sum + getBytesConsumed(e.image)) + pageImagesPartial.values.fold(0, (sum, e) => sum + getBytesConsumed(e.image)); for (final key in pageNumbers) { + if (bytesConsumed <= acceptableBytes) { + break; + } final removed = pageImages.remove(key); if (removed != null) { bytesConsumed -= getBytesConsumed(removed.image); @@ -4242,9 +4261,6 @@ class _PdfPageImageCache { bytesConsumed -= getBytesConsumed(removedPartial.image); removedPartial.dispose(); } - if (bytesConsumed <= acceptableBytes) { - break; - } } } } diff --git a/packages/pdfrx/test/pdf_document_ref_abandoned_load_test.dart b/packages/pdfrx/test/pdf_document_ref_abandoned_load_test.dart new file mode 100644 index 00000000..bf839604 --- /dev/null +++ b/packages/pdfrx/test/pdf_document_ref_abandoned_load_test.dart @@ -0,0 +1,49 @@ +import 'dart:async'; + +import 'package:flutter_test/flutter_test.dart'; +import 'package:pdfrx/pdfrx.dart'; + +class _FakeDocument extends Fake implements PdfDocument { + bool disposed = false; + + @override + Future dispose() async => disposed = true; +} + +void main() { + test('a load that finishes after every listener left disposes its document', () async { + final loading = Completer(); + final ref = PdfDocumentRefByLoader((_) => loading.future, key: PdfDocumentRefKey('abandoned-load-test')); + final listenable = ref.resolveListenable(); + void listener() {} + listenable.addListener(listener); + final load = listenable.load(); + + // e.g. the viewer is torn down to retry a slow load. + listenable.removeListener(listener); + final document = _FakeDocument(); + loading.complete(document); + await load; + + expect(document.disposed, isTrue); + expect(listenable.document, isNull); + }); + + test('a load with a listener still attached keeps its document', () async { + final loading = Completer(); + final ref = PdfDocumentRefByLoader((_) => loading.future, key: PdfDocumentRefKey('kept-load-test')); + final listenable = ref.resolveListenable(); + void listener() {} + listenable.addListener(listener); + final load = listenable.load(); + + final document = _FakeDocument(); + loading.complete(document); + await load; + + expect(document.disposed, isFalse); + expect(listenable.document, same(document)); + listenable.removeListener(listener); + expect(document.disposed, isTrue); + }); +} diff --git a/packages/pdfrx/test/pdf_text_searcher_test.dart b/packages/pdfrx/test/pdf_text_searcher_test.dart new file mode 100644 index 00000000..130f9ad7 --- /dev/null +++ b/packages/pdfrx/test/pdf_text_searcher_test.dart @@ -0,0 +1,143 @@ +import 'dart:async'; + +import 'package:flutter/widgets.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:pdfrx/pdfrx.dart'; + +class _FakePage extends Fake implements PdfPage { + _FakePage(this.pageNumber); + + @override + final int pageNumber; +} + +class _FakeDocument extends Fake implements PdfDocument { + _FakeDocument(int pageCount) : pages = [for (var i = 1; i <= pageCount; i++) _FakePage(i)]; + + @override + final List pages; + + @override + Stream get events => const Stream.empty(); +} + +class _FakeController extends PdfViewerController { + _FakeController(this.fakeDocument); + + final _FakeDocument fakeDocument; + int currentPageSet = 0; + + @override + bool get isReady => true; + + @override + PdfDocument get document => fakeDocument; + + @override + void invalidate() {} + + @override + Rect calcRectForRectInsidePage({required int pageNumber, required PdfRect rect}) => Rect.zero; + + @override + Future ensureVisible( + Rect rect, { + Duration duration = const Duration(milliseconds: 200), + double margin = 0, + }) async {} + + @override + void setCurrentPageNumber(int pageNumber) => currentPageSet = pageNumber; + + @override + FutureOr useDocument( + FutureOr Function(PdfDocument document) task, { + bool ensureLoaded = true, + Completer? cancelLoading, + }) => task(fakeDocument); +} + +/// Serves page text from [texts]; a page listed in [gates] waits for its completer. +class _GatedSearcher extends PdfTextSearcher { + _GatedSearcher(super.controller, this.texts); + + final Map texts; + final Map> gates = {}; + + @override + Future loadText({required int pageNumber}) async { + await gates[pageNumber]?.future; + final text = texts[pageNumber] ?? ''; + return PdfPageText( + pageNumber: pageNumber, + fullText: text, + charRects: [for (var i = 0; i < text.length; i++) PdfRect(i * 10.0, 10, i * 10.0 + 10, 0)], + fragments: const [], + ); + } +} + +void main() { + test('re-issuing the running pattern does not cut the search short', () async { + final searcher = _GatedSearcher(_FakeController(_FakeDocument(3)), {1: 'word', 2: 'word word', 3: 'word'}); + addTearDown(searcher.dispose); + final gate = searcher.gates[2] = Completer(); + + searcher.startTextSearch('word', goToFirstMatch: false, searchImmediately: true); + await pumpEventQueue(); + expect(searcher.isSearching, isTrue); + expect(searcher.matches, hasLength(1)); + + // e.g. a search field that fires again with the text unchanged. + searcher.startTextSearch('word', goToFirstMatch: false, searchImmediately: true); + gate.complete(); + await pumpEventQueue(); + + expect(searcher.matches.map((m) => m.pageNumber), [1, 2, 2, 3]); + expect(searcher.isSearching, isFalse); + }); + + test('a different pattern still replaces the running search', () async { + final searcher = _GatedSearcher(_FakeController(_FakeDocument(2)), {1: 'alpha beta', 2: 'beta'}); + addTearDown(searcher.dispose); + final gate = searcher.gates[2] = Completer(); + + searcher.startTextSearch('alpha', goToFirstMatch: false, searchImmediately: true); + await pumpEventQueue(); + searcher.startTextSearch('beta', goToFirstMatch: false, searchImmediately: true); + gate.complete(); + await pumpEventQueue(); + + expect(searcher.matches.map((m) => m.pageNumber), [1, 2]); + expect(searcher.isSearching, isFalse); + }); + + test('a new pattern clears the position left by the previous one', () async { + final searcher = _GatedSearcher(_FakeController(_FakeDocument(1)), {1: 'alpha alpha beta'}); + addTearDown(searcher.dispose); + searcher.startTextSearch('alpha', goToFirstMatch: false, searchImmediately: true); + await pumpEventQueue(); + await searcher.goToMatchOfIndex(1); + + searcher.startTextSearch('beta', goToFirstMatch: false, searchImmediately: true); + await pumpEventQueue(); + + expect(searcher.matches, hasLength(1)); + expect(searcher.currentIndex, isNull); + }); + + test('moving to a match notifies listeners, so a "3/120" counter can follow', () async { + final searcher = _GatedSearcher(_FakeController(_FakeDocument(1)), {1: 'word word'}); + addTearDown(searcher.dispose); + searcher.startTextSearch('word', goToFirstMatch: false, searchImmediately: true); + await pumpEventQueue(); + + var notified = 0; + searcher.addListener(() => notified++); + await searcher.goToMatchOfIndex(1); + + expect(searcher.currentIndex, 1); + expect(notified, greaterThan(0)); + expect((searcher.controller! as _FakeController).currentPageSet, 1); + }); +}