From f0da508c218ad57289bdb9268faeba7b7741a233 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 12 Jul 2016 16:10:35 +0200 Subject: [PATCH] Move searching out of WebView subclass This also makes it work for QtWebEngine. It also seems to fix #1050 though I'm not 100% sure why. --- qutebrowser/browser/browsertab.py | 23 +++-- qutebrowser/browser/commands.py | 83 +++++++++++++++++-- qutebrowser/browser/webengine/webenginetab.py | 45 ++++++++-- qutebrowser/browser/webkit/webkittab.py | 46 ++++++---- qutebrowser/browser/webkit/webview.py | 42 ---------- tests/end2end/features/search.feature | 4 +- 6 files changed, 166 insertions(+), 77 deletions(-) diff --git a/qutebrowser/browser/browsertab.py b/qutebrowser/browser/browsertab.py index f6579c87e..8e883d9f5 100644 --- a/qutebrowser/browser/browsertab.py +++ b/qutebrowser/browser/browsertab.py @@ -133,7 +133,7 @@ class AbstractSearch(QObject): Attributes: text: The last thing this view was searched for. - _flags: The flags of the last search. + _flags: The flags of the last search (needs to be set by subclasses). _widget: The underlying WebView widget. """ @@ -141,9 +141,9 @@ class AbstractSearch(QObject): super().__init__(parent) self._widget = None self.text = None - self._flags = 0 - def search(self, text, *, ignore_case=False, wrap=False, reverse=False): + def search(self, text, *, ignore_case=False, wrap=False, reverse=False, + result_cb=None): """Find the given text on the page. Args: @@ -151,6 +151,7 @@ class AbstractSearch(QObject): ignore_case: Search case-insensitively. (True/False/'smart') wrap: Wrap around to the top when arriving at the bottom. reverse: Reverse search direction. + result_cb: Called with a bool indicating whether a match was found. """ raise NotImplementedError @@ -158,12 +159,20 @@ class AbstractSearch(QObject): """Clear the current search.""" raise NotImplementedError - def prev_result(self): - """Go to the previous result of the current search.""" + def prev_result(self, *, result_cb=None): + """Go to the previous result of the current search. + + Args: + result_cb: Called with a bool indicating whether a match was found. + """ raise NotImplementedError - def next_result(self): - """Go to the next result of the current search.""" + def next_result(self, *, result_cb=None): + """Go to the next result of the current search. + + Args: + 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 761c58280..785215eca 100644 --- a/qutebrowser/browser/commands.py +++ b/qutebrowser/browser/commands.py @@ -1440,6 +1440,47 @@ class CommandDispatcher: this.dispatchEvent(event); """.format(webelem.javascript_escape(sel))) + def _search_cb(self, found, *, tab, old_scroll_pos, options, text, prev): + """Callback called from search/search_next/search_prev. + + Args: + found: Whether the text was found. + tab: The AbstractTab in which the search was made. + old_scroll_pos: The scroll position (QPoint) before the search. + 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: + # Check if the scroll position got smaller and show info. + if not going_up and tab.scroll.pos_px().y() < old_scroll_pos.y(): + message.info(self._win_id, "Search hit BOTTOM, continuing " + "at TOP", immediately=True) + elif going_up and tab.scroll.pos_px().y() > old_scroll_pos.y(): + message.info(self._win_id, "Search hit TOP, continuing at " + "BOTTOM", immediately=True) + else: + # User disabled wrapping; but findText() just returns False. If we + # have a selection, we know there's a match *somewhere* on the page + if not options['wrap'] and tab.caret.has_selection(): + if going_up: + message.warning(self._win_id, "Search hit TOP without " + "match for: {}".format(text), + immediately=True) + else: + message.warning(self._win_id, "Search hit BOTTOM without " + "match for: {}".format(text), + immediately=True) + else: + message.warning(self._win_id, "Text '{}' not found on " + "page!".format(text), immediately=True) + @cmdutils.register(instance='command-dispatcher', scope='window', maxsplit=0) def search(self, text="", reverse=False): @@ -1458,10 +1499,18 @@ class CommandDispatcher: 'wrap': config.get('general', 'wrap-search'), 'reverse': reverse, } - tab.search.search(text, **options) - self._tabbed_browser.search_text = text - self._tabbed_browser.search_options = options + self._tabbed_browser.search_options = dict(options) + + if text: + cb = functools.partial(self._search_cb, tab=tab, + old_scroll_pos=tab.scroll.pos_px(), + options=options, text=text, prev=False) + else: + cb = None + + options['result_cb'] = cb + tab.search.search(text, **options) @cmdutils.register(instance='command-dispatcher', hide=True, scope='window') @@ -1476,6 +1525,9 @@ class CommandDispatcher: window_text = self._tabbed_browser.search_text window_options = self._tabbed_browser.search_options + if window_text is None: + raise cmdexc.CommandError("No search done yet.") + self.set_mark("'") if window_text is not None and window_text != tab.search.text: @@ -1483,8 +1535,17 @@ class CommandDispatcher: tab.search.search(window_text, **window_options) count -= 1 - for _ in range(count): + if count == 0: + return + + cb = functools.partial(self._search_cb, tab=tab, + old_scroll_pos=tab.scroll.pos_px(), + options=window_options, text=window_text, + prev=False) + + for _ in range(count - 1): tab.search.next_result() + tab.search.next_result(result_cb=cb) @cmdutils.register(instance='command-dispatcher', hide=True, scope='window') @@ -1499,6 +1560,9 @@ class CommandDispatcher: window_text = self._tabbed_browser.search_text window_options = self._tabbed_browser.search_options + if window_text is None: + raise cmdexc.CommandError("No search done yet.") + self.set_mark("'") if window_text is not None and window_text != tab.search.text: @@ -1506,8 +1570,17 @@ class CommandDispatcher: tab.search.search(window_text, **window_options) count -= 1 - for _ in range(count): + if count == 0: + return + + cb = functools.partial(self._search_cb, tab=tab, + old_scroll_pos=tab.scroll.pos_px(), + options=window_options, text=window_text, + prev=True) + + for _ in range(count - 1): tab.search.prev_result() + tab.search.prev_result(result_cb=cb) @cmdutils.register(instance='command-dispatcher', hide=True, modes=[KeyMode.caret], scope='window') diff --git a/qutebrowser/browser/webengine/webenginetab.py b/qutebrowser/browser/webengine/webenginetab.py index 04c60c02d..438206a71 100644 --- a/qutebrowser/browser/webengine/webenginetab.py +++ b/qutebrowser/browser/webengine/webenginetab.py @@ -61,17 +61,48 @@ class WebEngineSearch(browsertab.AbstractSearch): """QtWebEngine implementations related to searching on the page.""" - def search(self, text, *, ignore_case=False, wrap=False, reverse=False): - log.stub() + def __init__(self, parent=None): + super().__init__(parent) + self._flags = QWebEnginePage.FindFlags(0) + + def _find(self, text, flags, cb=None): + """Call findText on the widget with optional callback.""" + if cb is None: + self._widget.findText(text, flags) + else: + self._widget.findText(text, flags, cb) + + def search(self, text, *, ignore_case=False, wrap=False, reverse=False, + result_cb=None): + flags = QWebEnginePage.FindFlags(0) + if ignore_case == 'smart': + if not text.islower(): + flags |= QWebEnginePage.FindCaseSensitively + elif not ignore_case: + flags |= QWebEnginePage.FindCaseSensitively + if not wrap: + log.stub('With wrap=False (ignoring)') + if reverse: + flags |= QWebEnginePage.FindBackward + + self.text = text + self._flags = flags + self._find(text, flags, result_cb) def clear(self): - log.stub() + self._widget.findText('') - def prev_result(self): - log.stub() + 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)) + if flags & QWebEnginePage.FindBackward: + flags &= ~QWebEnginePage.FindBackward + else: + flags |= QWebEnginePage.FindBackward + self._find(self.text, self._flags, result_cb) - def next_result(self): - log.stub() + def next_result(self, *, result_cb=None): + self._find(self.text, self._flags, result_cb) class WebEngineCaret(browsertab.AbstractCaret): diff --git a/qutebrowser/browser/webkit/webkittab.py b/qutebrowser/browser/webkit/webkittab.py index 48016d627..4784e9d00 100644 --- a/qutebrowser/browser/webkit/webkittab.py +++ b/qutebrowser/browser/webkit/webkittab.py @@ -64,13 +64,30 @@ class WebKitSearch(browsertab.AbstractSearch): """QtWebKit implementations related to searching on the page.""" + def __init__(self, parent=None): + super().__init__(parent) + self._flags = QWebPage.FindFlags(0) + + def _call_cb(self, callback, found): + """Call the given callback if it's non-None. + + Delays the call via a QTimer so the website is re-rendered in between. + + Args: + callback: What to call + found: If the text was found + """ + if callback is not None: + QTimer.singleShot(0, functools.partial(callback, found)) + def clear(self): # We first clear the marked text, then the highlights - self._widget.search('', 0) - self._widget.search('', QWebPage.HighlightAllOccurrences) + self._widget.findText('') + self._widget.findText('', QWebPage.HighlightAllOccurrences) - def search(self, text, *, ignore_case=False, wrap=False, reverse=False): - flags = 0 + def search(self, text, *, ignore_case=False, wrap=False, reverse=False, + result_cb=None): + flags = QWebPage.FindFlags(0) if ignore_case == 'smart': if not text.islower(): flags |= QWebPage.FindCaseSensitively @@ -82,24 +99,25 @@ class WebKitSearch(browsertab.AbstractSearch): flags |= QWebPage.FindBackward # We actually search *twice* - once to highlight everything, then again # to get a mark so we can navigate. - self._widget.search(text, flags) - self._widget.search(text, flags | QWebPage.HighlightAllOccurrences) + found = self._widget.findText(text, flags) + self._widget.findText(text, flags | QWebPage.HighlightAllOccurrences) self.text = text self._flags = flags + self._call_cb(result_cb, found) - def next_result(self): - self._widget.search(self.text, self._flags) + def next_result(self, *, result_cb=None): + found = self._widget.findText(self.text, self._flags) + self._call_cb(result_cb, found) - def prev_result(self): - # The int() here serves as a QFlags constructor to create a copy of the - # QFlags instance rather as a reference. I don't know why it works this - # way, but it does. - flags = int(self._flags) + def prev_result(self, *, result_cb=None): + # The int() here makes sure we get a copy of the flags. + flags = QWebPage.FindFlags(int(self._flags)) if flags & QWebPage.FindBackward: flags &= ~QWebPage.FindBackward else: flags |= QWebPage.FindBackward - self._widget.search(self.text, flags) + found = self._widget.findText(self.text, flags) + self._call_cb(result_cb, found) class WebKitCaret(browsertab.AbstractCaret): diff --git a/qutebrowser/browser/webkit/webview.py b/qutebrowser/browser/webkit/webview.py index 70dc28528..79379c5cf 100644 --- a/qutebrowser/browser/webkit/webview.py +++ b/qutebrowser/browser/webkit/webview.py @@ -352,48 +352,6 @@ class WebView(QWebView): "left.".format(mode)) self.setFocusPolicy(Qt.WheelFocus) - def search(self, text, flags): - """Search for text in the current page. - - Args: - text: The text to search for. - flags: The QWebPage::FindFlags. - """ - log.webview.debug("Searching with text '{}' and flags " - "0x{:04x}.".format(text, int(flags))) - old_scroll_pos = self.scroll_pos - flags = QWebPage.FindFlags(flags) - found = self.findText(text, flags) - backward = flags & QWebPage.FindBackward - - if not found and not flags & QWebPage.HighlightAllOccurrences and text: - # User disabled wrapping; but findText() just returns False. If we - # have a selection, we know there's a match *somewhere* on the page - if (not flags & QWebPage.FindWrapsAroundDocument and - self.hasSelection()): - if not backward: - message.warning(self.win_id, "Search hit BOTTOM without " - "match for: {}".format(text), - immediately=True) - else: - message.warning(self.win_id, "Search hit TOP without " - "match for: {}".format(text), - immediately=True) - else: - message.warning(self.win_id, "Text '{}' not found on " - "page!".format(text), immediately=True) - else: - def check_scroll_pos(): - """Check if the scroll position got smaller and show info.""" - if not backward and self.scroll_pos < old_scroll_pos: - message.info(self.win_id, "Search hit BOTTOM, continuing " - "at TOP", immediately=True) - elif backward and self.scroll_pos > old_scroll_pos: - message.info(self.win_id, "Search hit TOP, continuing at " - "BOTTOM", immediately=True) - # We first want QWebPage to refresh. - QTimer.singleShot(0, check_scroll_pos) - def createWindow(self, wintype): """Called by Qt when a page wants to create a new window. diff --git a/tests/end2end/features/search.feature b/tests/end2end/features/search.feature index 8ff205f89..6cf1bcc82 100644 --- a/tests/end2end/features/search.feature +++ b/tests/end2end/features/search.feature @@ -100,7 +100,7 @@ Feature: Searching on a page # Make sure there was no search in the same window before When I open data/search.html in a new window And I run :search-next - Then no crash should happen + Then the error "No search done yet." should be shown Scenario: Repeating search in a second tab (issue #940) When I open data/search.html in a new tab @@ -141,7 +141,7 @@ Feature: Searching on a page # Make sure there was no search in the same window before When I open data/search.html in a new window And I run :search-prev - Then no crash should happen + Then the error "No search done yet." should be shown ## wrapping