Repository: qutebrowser/qutebrowser
Pull request: Add search.wrap configuration option
Closes #5136.

With QtWebEngine, the API does not support the necessary flags to stop
wrapping, so the implementation via checking which match the search is
currently on is required. (The new functionality is disabled on Qt < 5.14)

It is not possible to display a message with QtWebKit since there is no
feedback from the search function.

This pull request is still WIP, as I have not yet created tests and am not completely certain of the stability of the solution.

### To do

- [x] Make tests for the introduced behavior

And probably more.

### Comments

I had to change the `findText` call from the `QWebEngineView` object to its `page()`, since only `QWebEnginePage` supports the new API for some reason. (Another good question would be why QWebEngine is lacking QWebKit functionality even though it's supposed to be its successor). The WebEngine solution is obviously stateful and thus potentially prone to breaking in all kinds of fascinating ways; I am currently still unsure whether connecting `_store_match_data` to the `QWebEnginePage` once will work consistently - more testing required.

Problem statement:
Make it possible to enable/disable wrapping-around for :search-next and :search-prev
Maybe I just missed it, but it seems that `:search-next` and `:search-prev` always wrap around at the moment.
Would it be possible to add a boolean argument to these commands to control whether search wraps around ?

Hints from issue discussion:
Currently, this isn't possible with the API we get. However, it should be possible to implement with Qt 5.14 with the [new QWebEngineFindTextResult class](https://codereview.qt-project.org/c/qt/qtwebengine/+/267764) (see #4616). The Qt 5.14 release is planned for next week.
Maybe I'm just not creative enough, but I've had a look at the relevant code and the new FindTextResult class (I thought I might try this as my first contribution) and don't see how this could be consistently implemented, as the FindTextResult is only available when the first search is finished (through the signal `findTextFinished`). So the result could be used in `:search-next` or `:search-prev` to prevent wrapping, but the first search would still wrap around at the end (or beginning) of the page. Maybe someone else knows how to 'fake' the first search so that we know whether it would wrap? Or maybe there's something else I haven't considered...
I don't really follow - how is it possible for the first match to wrap around? Searches always start at the top of the page - so either there is a result somewhere on the page (and there's no need to wrap around), or there was no result (and thus no match). Am I missing something?

Let me also note that a proper fix for this should also:

- Set the `QWebPage.FindWrapsAroundDocument` flag on QtWebKit (which is hardcoded to be set at the moment)
- Make sure that nothing breaks on Qt versions older than Qt 5.14
From my experiments with the behaviour, it seemed like the search started from the current scroll position - so `--reverse` would scroll up, while the default would scroll down (unless of course there was wrapping). For example, when I scrolled past the first instance of the word and then searched with `--reverse`, the search snapped to the first instance, even though there were instances further down on the page. Given that, if searches always start at the top, how does `--reverse` actually work on the first search?
What `--reverse` does is essentially setting QtWebEngine's `FindBackward` flag, which in turn passes it to Chromium. All the internals are handled by Chromium, so I can only observe what it does: It seems to start at the first match at the bottom of the page then. Is this also what you're seeing? If not, which Qt version, qutebrowser version and backend are you on? (see `:version`)
Hmm. Now it's working in the way you describe - maybe my search history was messing with me in testing. Anyway, that's good news - it means that the implementation could work properly. I'll try some things in the next few days. (Btw, thank you for your quick response! I was looking for a good first project to contribute to and was very impressed with the contributing info and the availability of "easy" issues - very welcoming.)
I'm glad to hear that! Let me note that reviewing/merging the PR might take a longer while because of the rather big PR backlog (and me losing track of that for a rather long time). I'm now trying to get things ramped up again and this one hopefully should be rather small - but it might still take a couple of days or weeks until I get back to you.

I really appreciate you reaching out before starting things! Always helps to get everyone on the same page first. Let me know if you get stuck somewhere!
Hmm. I've run into an unfortunate obstacle: I think it would be good to have a similar message when wrapping as when a wrap has been prevented; this isn't a problem with the WebEngine implementation, since I can just show a "Search hit TOP/BOTTOM" message when I prevent a wrap. But in WebKit, there's no information on whether a wrap was prevented or not, so I can't show a message. Should I also remove the message from WebEngine for the sake of consistency?
Seems fine to me to show it with QtWebEngine only - it's likely only a very small minority using QtWebKit anyways.

Commit message: Fix small mistake in QtWebKit output in tests

QtWebKit actually outputs False when wrapping is stopped in the search.

Patch to review:
diff --git a/qutebrowser/browser/browsertab.py b/qutebrowser/browser/browsertab.py
index 5a0f8cc279f..7e8ec478f74 100644
--- a/qutebrowser/browser/browsertab.py
+++ b/qutebrowser/browser/browsertab.py
@@ -317,6 +317,7 @@ def _is_case_sensitive(self, ignore_case: usertypes.IgnoreCase) -> bool:
     def search(self, text: str, *,
                ignore_case: usertypes.IgnoreCase = usertypes.IgnoreCase.never,
                reverse: bool = False,
+               wrap: bool = True,
                result_cb: _Callback = None) -> None:
         """Find the given text on the page.
 
@@ -324,6 +325,7 @@ def search(self, text: str, *,
             text: The text to search for.
             ignore_case: Search case-insensitively.
             reverse: Reverse search direction.
+            wrap: Allow wrapping at the top or bottom of the page.
             result_cb: Called with a bool indicating whether a match was found.
         """
         raise NotImplementedError
diff --git a/qutebrowser/browser/commands.py b/qutebrowser/browser/commands.py
index cf79672ca04..cb897366208 100644
--- a/qutebrowser/browser/commands.py
+++ b/qutebrowser/browser/commands.py
@@ -1503,6 +1503,7 @@ def search(self, text="", reverse=False):
         options = {
             'ignore_case': config.val.search.ignore_case,
             'reverse': reverse,
+            'wrap': config.val.search.wrap,
         }
 
         self._tabbed_browser.search_text = text
diff --git a/qutebrowser/browser/webengine/webenginetab.py b/qutebrowser/browser/webengine/webenginetab.py
index c130d92ea35..c51da415481 100644
--- a/qutebrowser/browser/webengine/webenginetab.py
+++ b/qutebrowser/browser/webengine/webenginetab.py
@@ -162,17 +162,46 @@ class WebEngineSearch(browsertab.AbstractSearch):
 
     Attributes:
         _flags: The QWebEnginePage.FindFlags of the last search.
+        _flag_wrap: An additional flag indicating whether the last search
+                    used wrapping.
         _pending_searches: How many searches have been started but not called
                            back yet.
+        _nowrap_available: Whether the nowrap functionality is available.
+        _active_match: The 1-based index of the currently active matc
+                       on the page.
+        _total_matches: The total number of search matches on the page.
     """
 
     def __init__(self, tab, parent=None):
         super().__init__(tab, parent)
         self._flags = QWebEnginePage.FindFlags(0)  # type: ignore
+        self._flag_wrap = True
         self._pending_searches = 0
+        # The API necessary to stop wrapping was added in this version
+        if qtutils.version_check("5.14"):
+            self._active_match = 0
+            self._total_matches = 0
+            self._nowrap_available = True
+            self._store_match_data_connected = False
+        else:
+            self._nowrap_available = False
+
+    def _store_match_data(self, result):
+        self._active_match = result.activeMatch()
+        self._total_matches = result.numberOfMatches()
+        log.webview.debug("active match: number {0} of {1}"
+                          .format(self._active_match, self._total_matches))
+
+    def _reset_match_data(self):
+        self._active_match = 0
+        self._total_matches = 0
 
     def _find(self, text, flags, callback, caller):
         """Call findText on the widget."""
+        if self._nowrap_available and not self._store_match_data_connected:
+            self._widget.page().findTextFinished.connect(
+                self._store_match_data)
+            self._store_match_data_connected = True
         self.search_displayed = True
         self._pending_searches += 1
 
@@ -207,10 +236,10 @@ def wrapped_callback(found):
                 callback(found)
             self.finished.emit(found)
 
-        self._widget.findText(text, flags, wrapped_callback)
+        self._widget.page().findText(text, flags, wrapped_callback)
 
     def search(self, text, *, ignore_case=usertypes.IgnoreCase.never,
-               reverse=False, result_cb=None):
+               reverse=False, wrap=True, result_cb=None):
         # Don't go to next entry on duplicate search
         if self.text == text and self.search_displayed:
             log.webview.debug("Ignoring duplicate search request"
@@ -219,6 +248,9 @@ def search(self, text, *, ignore_case=usertypes.IgnoreCase.never,
 
         self.text = text
         self._flags = QWebEnginePage.FindFlags(0)  # type: ignore
+        if self._nowrap_available:
+            self._reset_match_data()
+            self._flag_wrap = wrap
         if self._is_case_sensitive(ignore_case):
             self._flags |= QWebEnginePage.FindCaseSensitively
         if reverse:
@@ -230,18 +262,42 @@ def clear(self):
         if self.search_displayed:
             self.cleared.emit()
         self.search_displayed = False
+        if self._nowrap_available:
+            self._reset_match_data()
         self._widget.findText('')
 
+    def _message_wrap_prevented(self, top=True):
+        if top:
+            message.info("Search hit TOP")
+        else:
+            message.info("Search hit BOTTOM")
+
     def prev_result(self, *, result_cb=None):
         # The int() here makes sure we get a copy of the flags.
         flags = QWebEnginePage.FindFlags(int(self._flags))  # type: ignore
+        nowrap = self._nowrap_available and not self._flag_wrap
         if flags & QWebEnginePage.FindBackward:
+            if nowrap and self._active_match == self._total_matches:
+                self._message_wrap_prevented(top=False)
+                return
             flags &= ~QWebEnginePage.FindBackward
         else:
+            if nowrap and self._active_match == 1:
+                self._message_wrap_prevented(top=True)
+                return
             flags |= QWebEnginePage.FindBackward
         self._find(self.text, flags, result_cb, 'prev_result')
 
     def next_result(self, *, result_cb=None):
+        if self._nowrap_available and not self._flag_wrap:
+            going_up = self._flags & QWebEnginePage.FindBackward
+
+            if going_up and self._active_match == 1:
+                self._message_wrap_prevented(top=True)
+                return
+            elif not going_up and self._active_match == self._total_matches:
+                self._message_wrap_prevented(top=False)
+                return
         self._find(self.text, self._flags, result_cb, 'next_result')
 
 
diff --git a/qutebrowser/browser/webkit/webkittab.py b/qutebrowser/browser/webkit/webkittab.py
index 4aa0abcf502..a8091463779 100644
--- a/qutebrowser/browser/webkit/webkittab.py
+++ b/qutebrowser/browser/webkit/webkittab.py
@@ -125,7 +125,7 @@ def clear(self):
         self._widget.findText('', QWebPage.HighlightAllOccurrences)
 
     def search(self, text, *, ignore_case=usertypes.IgnoreCase.never,
-               reverse=False, result_cb=None):
+               reverse=False, wrap=True, result_cb=None):
         # Don't go to next entry on duplicate search
         if self.text == text and self.search_displayed:
             log.webview.debug("Ignoring duplicate search request"
@@ -137,11 +137,13 @@ def search(self, text, *, ignore_case=usertypes.IgnoreCase.never,
 
         self.text = text
         self.search_displayed = True
-        self._flags = QWebPage.FindWrapsAroundDocument
+        self._flags = QWebPage.FindFlags(0)
         if self._is_case_sensitive(ignore_case):
             self._flags |= QWebPage.FindCaseSensitively
         if reverse:
             self._flags |= QWebPage.FindBackward
+        if wrap:
+            self._flags |= QWebPage.FindWrapsAroundDocument
         # We actually search *twice* - once to highlight everything, then again
         # to get a mark so we can navigate.
         found = self._widget.findText(text, self._flags)
diff --git a/qutebrowser/config/configdata.yml b/qutebrowser/config/configdata.yml
index 895ef4ae65f..60e6c0a8a62 100644
--- a/qutebrowser/config/configdata.yml
+++ b/qutebrowser/config/configdata.yml
@@ -48,6 +48,15 @@ search.incremental:
   default: True
   desc: Find text on a page incrementally, renewing the search for each typed character.
 
+search.wrap:
+  type: Bool
+  default: True
+  desc: >-
+    Wrap around at the top and bottom of the page when advancing through text matches
+    using `:search-next` and `:search-prev`.
+
+    When using QtWebEngine, Qt>=5.14 is required to disable wrapping.
+
 new_instance_open_target:
   type:
     name: String
diff --git a/tests/end2end/features/search.feature b/tests/end2end/features/search.feature
index 2c0aa06a510..de815a5b7bf 100644
--- a/tests/end2end/features/search.feature
+++ b/tests/end2end/features/search.feature
@@ -213,6 +213,56 @@ Feature: Searching on a page
     # TODO: wrapping message with scrolling
     # TODO: wrapping message without scrolling
 
+    ## wrapping prevented
+
+    @qtwebkit_skip @qt>=5.14
+    Scenario: Preventing wrapping at the top of the page with QtWebEngine
+        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 "Search hit TOP" in the log
+        Then "foo" should be found
+
+    @qtwebkit_skip @qt>=5.14
+    Scenario: Preventing wrapping at the bottom of the page with QtWebEngine
+        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 "Search hit BOTTOM" in the log
+        Then "Foo" should be found
+
+    @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
+
     ## follow searched links
     @skip  # Too flaky
     Scenario: Follow a searched link


Code review comments:
diff_hunk: @@ -48,6 +48,15 @@ search.incremental:
   default: True
   desc: Find text on a page incrementally, renewing the search for each typed character.
 
+search.wrap:
+  type: Bool
+  default: True
+  desc: >-
+    Wrap around at the top and bottom of the page when advancing through text matches
+    using `:search-next` and `:search-prev`.
+
+    When using QtWebEngine, Qt>=5.14 is required to disable wrapping.
path: qutebrowser/config/configdata.yml
text: @user1:
Instead of mentioning it in the documentation, you can use the `backend` key:

```yaml
  backend:
    QtWebEngine: Qt 5.14
    QtWebKit: true
```

diff_hunk: @@ -162,17 +162,46 @@ class WebEngineSearch(browsertab.AbstractSearch):
 
     Attributes:
         _flags: The QWebEnginePage.FindFlags of the last search.
+        _flag_wrap: An additional flag indicating whether the last search
+                    used wrapping.
         _pending_searches: How many searches have been started but not called
                            back yet.
+        _nowrap_available: Whether the nowrap functionality is available.
+        _active_match: The 1-based index of the currently active matc
+                       on the page.
+        _total_matches: The total number of search matches on the page.
     """
 
     def __init__(self, tab, parent=None):
         super().__init__(tab, parent)
         self._flags = QWebEnginePage.FindFlags(0)  # type: ignore
+        self._flag_wrap = True
         self._pending_searches = 0
+        # The API necessary to stop wrapping was added in this version
+        if qtutils.version_check("5.14"):
+            self._active_match = 0
+            self._total_matches = 0
+            self._nowrap_available = True
+            self._store_match_data_connected = False
+        else:
+            self._nowrap_available = False
+
+    def _store_match_data(self, result):
+        self._active_match = result.activeMatch()
+        self._total_matches = result.numberOfMatches()
+        log.webview.debug("active match: number {0} of {1}"
path: qutebrowser/browser/webengine/webenginetab.py
text: @user1:
```suggestion
        log.webview.debug("Active search match: {}/{}"
```

(to have some more context when reading the log message, and the indices aren't required since Python3)

diff_hunk: @@ -162,17 +162,46 @@ class WebEngineSearch(browsertab.AbstractSearch):
 
     Attributes:
         _flags: The QWebEnginePage.FindFlags of the last search.
+        _flag_wrap: An additional flag indicating whether the last search
+                    used wrapping.
         _pending_searches: How many searches have been started but not called
                            back yet.
+        _nowrap_available: Whether the nowrap functionality is available.
+        _active_match: The 1-based index of the currently active matc
+                       on the page.
+        _total_matches: The total number of search matches on the page.
     """
 
     def __init__(self, tab, parent=None):
         super().__init__(tab, parent)
         self._flags = QWebEnginePage.FindFlags(0)  # type: ignore
+        self._flag_wrap = True
         self._pending_searches = 0
+        # The API necessary to stop wrapping was added in this version
+        if qtutils.version_check("5.14"):
+            self._active_match = 0
+            self._total_matches = 0
+            self._nowrap_available = True
+            self._store_match_data_connected = False
+        else:
+            self._nowrap_available = False
path: qutebrowser/browser/webengine/webenginetab.py
text: @user1:
There are a lot of new attributes here, and some of them are conditionally here sometimes, and sometimes they aren't. Would be nice to find a way to clean them up a bit.

What about writing a class (`_WebEngineSearchWrapHandler` or whatever) to hold all those attributes (`self._flag_wrap` included, probably) and saving that as `self._wrap_handler` or so? Then that could be `None` if not available.

That would also make it possible to move some methods such as `_reset_match_data` to that class instead, which keeps this class a bit simpler.

@author:
It makes some of the statements more verbose, but yeah, I agree. I was a little shy creating new classes in a codebase I'm still not 100% familiar with.