From e15bda307e42c288b926f578e7bf8c610e4767af Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Mon, 13 Jun 2022 17:41:43 +0200 Subject: search: Split navigation/search callbacks This way, we can move more logic (checking wrapping, etc.) into the API, thus making the commands much more simple and stateless. --- qutebrowser/browser/browsertab.py | 23 ++++++-- qutebrowser/browser/commands.py | 83 +++++++++++---------------- qutebrowser/browser/webengine/webenginetab.py | 60 ++++++++++++++----- qutebrowser/browser/webkit/webkittab.py | 24 ++++++-- tests/end2end/features/search.feature | 32 ++--------- tests/unit/browser/test_caret.py | 4 +- 6 files changed, 121 insertions(+), 105 deletions(-) diff --git a/qutebrowser/browser/browsertab.py b/qutebrowser/browser/browsertab.py index f2ed3f084..ca6a9c483 100644 --- a/qutebrowser/browser/browsertab.py +++ b/qutebrowser/browser/browsertab.py @@ -324,6 +324,20 @@ class SearchMatch: return f"{self.current}/{self.total}" +class SearchNavigationResult(enum.Enum): + + """The outcome of calling prev_/next_result.""" + + found = enum.auto() + not_found = enum.auto() + + wrapped_bottom = enum.auto() + wrap_prevented_bottom = enum.auto() + + wrapped_top = enum.auto() + wrap_prevented_top = enum.auto() + + class AbstractSearch(QObject): """Attribute ``search`` of AbstractTab for doing searches. @@ -349,6 +363,7 @@ class AbstractSearch(QObject): cleared = pyqtSignal() _Callback = Callable[[bool], None] + _NavCallback = Callable[[SearchNavigationResult], None] def __init__(self, tab: 'AbstractTab', parent: QWidget = None): super().__init__(parent) @@ -392,21 +407,21 @@ class AbstractSearch(QObject): """Clear the current search.""" raise NotImplementedError - def prev_result(self, *, wrap: bool = False, result_cb: _Callback = None) -> None: + def prev_result(self, *, wrap: bool = False, callback: _NavCallback = None) -> None: """Go to the previous result of the current search. Args: wrap: Allow wrapping at the top or bottom of the page. - result_cb: Called with a bool indicating whether a match was found. + callback: Called with a SearchNavigationResult. """ raise NotImplementedError - def next_result(self, *, wrap: bool = False, result_cb: _Callback = None) -> None: + def next_result(self, *, wrap: bool = False, callback: _NavCallback = None) -> None: """Go to the next result of the current search. Args: wrap: Allow wrapping at the top or bottom of the page. - result_cb: Called with a bool indicating whether a match was found. + callback: Called with a SearchNavigationResult. """ raise NotImplementedError diff --git a/qutebrowser/browser/commands.py b/qutebrowser/browser/commands.py index 073fe1c49..f534b91b3 100644 --- a/qutebrowser/browser/commands.py +++ b/qutebrowser/browser/commands.py @@ -1538,46 +1538,39 @@ class CommandDispatcher: message.error(str(e)) ed.backup() - def _search_cb(self, found, *, tab, old_match, options, text, prev): - """Callback called from search/search_next/search_prev. + def _search_cb(self, found, *, text): + """Callback called from :search. Args: found: Whether the text was found. - tab: The AbstractTab in which the search was made. - old_match: The previously active search match before the search was - performed. - options: The options (dict) the search was made with. - text: The text searched for. - prev: Whether we're searching backwards (i.e. :search-prev) """ - # :search/:search-next without reverse -> down - # :search/:search-next with reverse -> up - # :search-prev without reverse -> up - # :search-prev with reverse -> down - going_up = options['reverse'] ^ prev - - if found: - if not config.val.search.wrap_messages: - return - - # Check if the match count change is opposite to the search direction - if old_match.current > 0: - if not going_up: - if old_match.current > tab.search.match.current: - message.info("Search hit BOTTOM, continuing at TOP", - replace="search-hit-msg") - elif old_match.current == tab.search.match.current: - message.info("Search hit BOTTOM", replace="search-hit-msg") - elif going_up: - if old_match.current < tab.search.match.current: - message.info("Search hit TOP, continuing at BOTTOM", - replace="search-hit-msg") - elif old_match.current == tab.search.match.current: - message.info("Search hit TOP", replace="search-hit-msg") - else: + if not found: message.warning(f"Text '{text}' not found on page!", replace='find-in-page') + def _search_navigation_cb(self, result): + """Callback called from :search-prev/next.""" + if result == browsertab.SearchNavigationResult.not_found: + # FIXME check if this actually can happen... + message.warning("Search result vanished...") + return + elif result == browsertab.SearchNavigationResult.found: + return + elif not config.val.search.wrap_messages: + return + + messages = { + browsertab.SearchNavigationResult.wrap_prevented_bottom: + "Search hit BOTTOM", + browsertab.SearchNavigationResult.wrap_prevented_top: + "Search hit TOP", + browsertab.SearchNavigationResult.wrapped_bottom: + "Search hit BOTTOM, continuing at TOP", + browsertab.SearchNavigationResult.wrapped_top: + "Search hit TOP, continuing at BOTTOM", + } + message.info(messages[result], replace="search-hit-msg") + @cmdutils.register(instance='command-dispatcher', scope='window', maxsplit=0) def search(self, text="", reverse=False): @@ -1600,16 +1593,12 @@ class CommandDispatcher: } self._tabbed_browser.search_text = text - self._tabbed_browser.search_options = dict(options) - - cb = functools.partial(self._search_cb, tab=tab, - old_match=browsertab.SearchMatch(), - options=options, - text=text, prev=False) - options['result_cb'] = cb + self._tabbed_browser.search_options = options tab.scroller.before_jump_requested.emit() - tab.search.search(text, **options) + + cb = functools.partial(self._search_cb, text=text) + tab.search.search(text, **options, result_cb=cb) @cmdutils.register(instance='command-dispatcher', scope='window') @cmdutils.argument('count', value=cmdutils.Value.count) @@ -1636,15 +1625,11 @@ class CommandDispatcher: if count == 0: return - cb = functools.partial(self._search_cb, tab=tab, - old_match=tab.search.match, - options=window_options, text=window_text, - prev=False) wrap = config.val.search.wrap for _ in range(count - 1): tab.search.next_result(wrap=wrap) - tab.search.next_result(result_cb=cb, wrap=wrap) + tab.search.next_result(callback=self._search_navigation_cb, wrap=wrap) @cmdutils.register(instance='command-dispatcher', scope='window') @cmdutils.argument('count', value=cmdutils.Value.count) @@ -1671,15 +1656,11 @@ class CommandDispatcher: if count == 0: return - cb = functools.partial(self._search_cb, tab=tab, - old_match=tab.search.match, - options=window_options, text=window_text, - prev=True) wrap = config.val.search.wrap for _ in range(count - 1): tab.search.prev_result(wrap=wrap) - tab.search.prev_result(result_cb=cb, wrap=wrap) + tab.search.prev_result(callback=self._search_navigation_cb, wrap=wrap) def _jseval_cb(self, out): """Show the data returned from JS.""" diff --git a/qutebrowser/browser/webengine/webenginetab.py b/qutebrowser/browser/webengine/webenginetab.py index f1d1a0cbb..7a7c5a8d4 100644 --- a/qutebrowser/browser/webengine/webenginetab.py +++ b/qutebrowser/browser/webengine/webenginetab.py @@ -114,6 +114,8 @@ class WebEngineSearch(browsertab.AbstractSearch): super().__init__(tab, parent) self._flags = self._empty_flags() self._pending_searches = 0 + self.match = browsertab.SearchMatch() + self._old_match = browsertab.SearchMatch() def _empty_flags(self): return QWebEnginePage.FindFlags(0) @@ -180,14 +182,18 @@ class WebEngineSearch(browsertab.AbstractSearch): if callback is not None: callback(found) + self.finished.emit(found) self._widget.page().findText(text, flags, wrapped_callback) def _on_find_finished(self, find_text_result): """Unwrap the result, store it, and pass it along.""" - self.match.current = find_text_result.activeMatch() - self.match.total = find_text_result.numberOfMatches() + self._old_match = self.match + self.match = browsertab.SearchMatch( + current=find_text_result.activeMatch(), + total=find_text_result.numberOfMatches(), + ) log.webview.debug(f"Active search match: {self.match}") self.match_changed.emit(self.match) @@ -214,29 +220,55 @@ class WebEngineSearch(browsertab.AbstractSearch): self.match.reset() self._widget.page().findText('') - def prev_result(self, *, wrap=False, result_cb=None): + def _prev_next_cb(self, found, *, going_up, callback): + """Call the prev/next callback based on the search result.""" + if found: + result = browsertab.SearchNavigationResult.found + # Check if the match count change is opposite to the search direction + if self._old_match.current > 0: + if not going_up and self._old_match.current > self.match.current: + result = browsertab.SearchNavigationResult.wrapped_bottom + elif going_up and self._old_match.current < self.match.current: + result = browsertab.SearchNavigationResult.wrapped_top + else: + result = browsertab.SearchNavigationResult.not_found + + callback(result) + + def prev_result(self, *, wrap=False, callback=None): # The int() here makes sure we get a copy of the flags. flags = QWebEnginePage.FindFlags(int(self._flags)) if flags & QWebEnginePage.FindBackward: - if self.match.at_limit(going_up=False) and not wrap: - result_cb(True) - return + going_up = False flags &= ~QWebEnginePage.FindBackward else: - if self.match.at_limit(going_up=True) and not wrap: - result_cb(True) - return + going_up = True flags |= QWebEnginePage.FindBackward - self._find(self.text, flags, result_cb, 'prev_result') + if self.match.at_limit(going_up=going_up) and not wrap: + res = ( + browsertab.SearchNavigationResult.wrap_prevented_top if going_up else + browsertab.SearchNavigationResult.wrap_prevented_bottom + ) + callback(res) + return - def next_result(self, *, wrap=False, result_cb=None): - going_up = self._flags & QWebEnginePage.FindBackward + cb = functools.partial(self._prev_next_cb, going_up=going_up, callback=callback) + self._find(self.text, flags, cb, 'prev_result') + + def next_result(self, *, wrap=False, callback=None): + going_up = bool(self._flags & QWebEnginePage.FindBackward) if self.match.at_limit(going_up=going_up) and not wrap: - result_cb(True) + res = ( + browsertab.SearchNavigationResult.wrap_prevented_top if going_up else + browsertab.SearchNavigationResult.wrap_prevented_bottom + ) + callback(res) return - self._find(self.text, self._flags, result_cb, 'next_result') + + cb = functools.partial(self._prev_next_cb, going_up=going_up, callback=callback) + self._find(self.text, self._flags, cb, 'next_result') class WebEngineCaret(browsertab.AbstractCaret): diff --git a/qutebrowser/browser/webkit/webkittab.py b/qutebrowser/browser/webkit/webkittab.py index 05e48b2f6..2faf93a32 100644 --- a/qutebrowser/browser/webkit/webkittab.py +++ b/qutebrowser/browser/webkit/webkittab.py @@ -148,7 +148,19 @@ class WebKitSearch(browsertab.AbstractSearch): log.webview.debug(' '.join([caller, found_text, text, flag_text]) .strip()) if callback is not None: - QTimer.singleShot(0, functools.partial(callback, found)) + if caller in ["prev_result", "next_result"]: + if found: + # no wrapping detection + cb_value = browsertab.SearchNavigationResult.found + elif flags & QWebPage.FindBackward: + cb_value = browsertab.SearchNavigationResult.wrap_prevented_top + else: + cb_value = browsertab.SearchNavigationResult.wrap_prevented_bottom + elif caller == "search": + cb_value = found + else: + raise utils.Unreachable(caller) + QTimer.singleShot(0, functools.partial(callback, cb_value)) self.finished.emit(found) @@ -183,7 +195,7 @@ class WebKitSearch(browsertab.AbstractSearch): self._flags | QWebPage.HighlightAllOccurrences) self._call_cb(result_cb, found, text, self._flags, 'search') - def next_result(self, *, wrap=False, result_cb=None): + def next_result(self, *, wrap=False, callback=None): self.search_displayed = True # The int() here makes sure we get a copy of the flags. flags = QWebPage.FindFlags( @@ -192,10 +204,10 @@ class WebKitSearch(browsertab.AbstractSearch): if wrap: flags |= QWebPage.FindWrapsAroundDocument - found = self._widget.findText(self.text, self._flags) # type: ignore[arg-type] - self._call_cb(result_cb, found, self.text, self._flags, 'next_result') + found = self._widget.findText(self.text, flags) # type: ignore[arg-type] + self._call_cb(callback, found, self.text, flags, 'next_result') - def prev_result(self, *, wrap=False, result_cb=None): + def prev_result(self, *, wrap=False, callback=None): self.search_displayed = True # The int() here makes sure we get a copy of the flags. flags = QWebPage.FindFlags( @@ -210,7 +222,7 @@ class WebKitSearch(browsertab.AbstractSearch): flags |= QWebPage.FindWrapsAroundDocument found = self._widget.findText(self.text, flags) # type: ignore[arg-type] - self._call_cb(result_cb, found, self.text, flags, 'prev_result') + self._call_cb(callback, found, self.text, flags, 'prev_result') class WebKitCaret(browsertab.AbstractCaret): diff --git a/tests/end2end/features/search.feature b/tests/end2end/features/search.feature index 86ceab70e..f2d793907 100644 --- a/tests/end2end/features/search.feature +++ b/tests/end2end/features/search.feature @@ -233,8 +233,8 @@ Feature: Searching on a page ## wrapping prevented - @qtwebkit_skip @qt>=5.14 - Scenario: Preventing wrapping at the top of the page with QtWebEngine + @qt>=5.14 + Scenario: Preventing wrapping at the top of the page When I set search.ignore_case to always And I set search.wrap to false And I set search.wrap_messages to true @@ -245,8 +245,8 @@ Feature: Searching on a page And I run :search-next Then the message "Search hit TOP" should be shown - @qtwebkit_skip @qt>=5.14 - Scenario: Preventing wrapping at the bottom of the page with QtWebEngine + @qt>=5.14 + Scenario: Preventing wrapping at the bottom of the page When I set search.ignore_case to always And I set search.wrap to false And I run :search foo @@ -256,30 +256,6 @@ Feature: Searching on a page And I run :search-next Then the message "Search hit BOTTOM" should be shown - @qtwebengine_skip - Scenario: Preventing wrapping at the top of the page with QtWebKit - When I set search.ignore_case to always - And I set search.wrap to false - And I run :search --reverse foo - And I wait for "search found foo with flags FindBackward" in the log - And I run :search-next - And I wait for "next_result found foo with flags FindBackward" in the log - And I run :search-next - And I wait for "next_result didn't find foo with flags FindBackward" in the log - Then the warning "Text 'foo' not found on page!" should be shown - - @qtwebengine_skip - Scenario: Preventing wrapping at the bottom of the page with QtWebKit - When I set search.ignore_case to always - And I set search.wrap to false - And I run :search foo - And I wait for "search found foo" in the log - And I run :search-next - And I wait for "next_result found foo" in the log - And I run :search-next - And I wait for "next_result didn't find foo" in the log - Then the warning "Text 'foo' not found on page!" should be shown - ## search match counter @qtwebkit_skip @qt>=5.14 diff --git a/tests/unit/browser/test_caret.py b/tests/unit/browser/test_caret.py index 86014040d..85301e358 100644 --- a/tests/unit/browser/test_caret.py +++ b/tests/unit/browser/test_caret.py @@ -345,8 +345,8 @@ class TestSearch: callback.assert_called_with(True) with qtbot.wait_callback() as callback: - web_tab.search.next_result(result_cb=callback) - callback.assert_called_with(True) + web_tab.search.next_result(callback=callback) + callback.assert_called_with(browsertab.SearchNavigationResult.found) mode_manager.enter(usertypes.KeyMode.caret) -- cgit v1.2.3-54-g00ecf