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.

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)