From 7a6e2b3242a7a7ed5b912c45b7e6065c186091f0 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Sat, 21 Jun 2014 16:42:58 +0200 Subject: [PATCH] Ensure validity of Qt objects --- doc/HACKING.asciidoc | 3 +- qutebrowser/browser/commands.py | 37 ++++++++++++++--------- qutebrowser/browser/downloads.py | 3 +- qutebrowser/browser/hints.py | 5 +++ qutebrowser/browser/quickmarks.py | 9 ++++-- qutebrowser/config/configdata.py | 6 ++-- qutebrowser/config/conftypes.py | 5 ++- qutebrowser/models/basecompletion.py | 5 +-- qutebrowser/models/downloadmodel.py | 6 ++-- qutebrowser/utils/misc.py | 35 ++++++++++++++++++--- qutebrowser/utils/url.py | 6 +++- qutebrowser/utils/webelem.py | 6 ++-- qutebrowser/widgets/completiondelegate.py | 6 ++-- qutebrowser/widgets/downloads.py | 7 +++-- qutebrowser/widgets/mainwindow.py | 4 ++- qutebrowser/widgets/tabbedbrowser.py | 3 ++ qutebrowser/widgets/webview.py | 5 ++- 17 files changed, 107 insertions(+), 44 deletions(-) diff --git a/doc/HACKING.asciidoc b/doc/HACKING.asciidoc index e85c16603..eb1a6744f 100644 --- a/doc/HACKING.asciidoc +++ b/doc/HACKING.asciidoc @@ -288,7 +288,8 @@ displaying it to the user. * Name a string URL something like `urlstr`, and a `QUrl` something like `url`. * Mention in the docstring whether your function needs a URL string or a `QUrl`. -* Call QUrl.isValid() and take appropriate action if not. +* Call `qt_ensure_valid` from `utils.misc` whenever getting or creating a +`QUrl` and take appropriate action if not. Style conventions diff --git a/qutebrowser/browser/commands.py b/qutebrowser/browser/commands.py index 9fe69e98f..64c6fdd1a 100644 --- a/qutebrowser/browser/commands.py +++ b/qutebrowser/browser/commands.py @@ -38,7 +38,7 @@ import qutebrowser.browser.quickmarks as quickmarks import qutebrowser.utils.log as log import qutebrowser.utils.url as urlutils from qutebrowser.utils.misc import (check_overflow, shell_escape, - check_print_compat) + check_print_compat, qt_ensure_valid) from qutebrowser.utils.editor import ExternalEditor from qutebrowser.commands.exceptions import CommandError from qutebrowser.commands.userscripts import UserscriptRunner @@ -70,6 +70,12 @@ class CommandDispatcher: self._tabs = parent self._editor = None + def _current_url(self): + """Get the URL of the current tab.""" + url = self._tabs.currentWidget().url() + qt_ensure_valid(url) + return url + def _scroll_percent(self, perc=None, count=None, orientation=None): """Inner logic for scroll_percent_(x|y). @@ -97,7 +103,8 @@ class CommandDispatcher: frame = widget.page().currentFrame() if frame is None: raise CommandError("No frame focused!") - widget.hintmanager.follow_prevnext(frame, widget.url(), prev, newtab) + widget.hintmanager.follow_prevnext(frame, self._current_url(), prev, + newtab) def _tab_move_absolute(self, idx): """Get an index for moving a tab absolutely. @@ -270,7 +277,7 @@ class CommandDispatcher: target = getattr(hints.Target, targetstr.replace('-', '_')) except AttributeError: raise CommandError("Unknown hinting target {}!".format(targetstr)) - widget.hintmanager.start(frame, widget.url(), group, target) + widget.hintmanager.start(frame, self._current_url(), group, target) @cmdutils.register(instance='mainwindow.tabs.cmd', hide=True) def follow_hint(self): @@ -356,8 +363,8 @@ class CommandDispatcher: Args: sel: True to use primary selection, False to use clipboard """ - urlstr = self._tabs.currentWidget().url().toString( - QUrl.FullyEncoded | QUrl.RemovePassword) + urlstr = self._current_url().toString(QUrl.FullyEncoded | + QUrl.RemovePassword) if sel: mode = QClipboard.Selection target = "primary selection" @@ -450,19 +457,19 @@ class CommandDispatcher: @cmdutils.register(instance='mainwindow.tabs.cmd', hide=True) def open_tab_cur(self): """Set the statusbar to :tabopen and the current URL.""" - urlstr = self._tabs.currentWidget().url().toDisplayString() + urlstr = self._current_url().toDisplayString() message.set_cmd_text(':open-tab ' + urlstr) @cmdutils.register(instance='mainwindow.tabs.cmd', hide=True) def open_cur(self): """Set the statusbar to :open and the current URL.""" - urlstr = self._tabs.currentWidget().url().toDisplayString() + urlstr = self._current_url().toDisplayString() message.set_cmd_text(':open ' + urlstr) @cmdutils.register(instance='mainwindow.tabs.cmd', hide=True) def open_tab_bg_cur(self): """Set the statusbar to :tabopen-bg and the current URL.""" - urlstr = self._tabs.currentWidget().url().toDisplayString() + urlstr = self._current_url().toDisplayString() message.set_cmd_text(':open-tab-bg ' + urlstr) @cmdutils.register(instance='mainwindow.tabs.cmd') @@ -609,8 +616,8 @@ class CommandDispatcher: Args: cmd: The command to execute. """ - urlstr = self._tabs.currentWidget().url().toString( - QUrl.FullyEncoded | QUrl.RemovePassword) + urlstr = self._current_url().toString(QUrl.FullyEncoded | + QUrl.RemovePassword) cmd = cmd.replace('{}', shell_escape(urlstr)) log.procs.debug("Executing: {}".format(cmd)) subprocess.Popen(cmd, shell=True) @@ -625,7 +632,7 @@ class CommandDispatcher: """Run an userscript given as argument.""" # We don't remove the password in the URL here, as it's probably safe # to pass via env variable. - urlstr = self._tabs.currentWidget().url().toString(QUrl.FullyEncoded) + urlstr = self._current_url().toString(QUrl.FullyEncoded) runner = UserscriptRunner(self._tabs) runner.got_cmd.connect(self._tabs.got_cmd) runner.run(cmd, *args, env={'QUTE_URL': urlstr}) @@ -634,7 +641,7 @@ class CommandDispatcher: @cmdutils.register(instance='mainwindow.tabs.cmd') def quickmark_save(self): """Save the current page as a quickmark.""" - quickmarks.prompt_save(self._tabs.currentWidget().url()) + quickmarks.prompt_save(self._current_url()) @cmdutils.register(instance='mainwindow.tabs.cmd') def quickmark_load(self, name): @@ -642,7 +649,8 @@ class CommandDispatcher: urlstr = quickmarks.get(name) url = QUrl(urlstr) if not url.isValid(): - raise CommandError("Invalid URL {}".format(urlstr)) + raise CommandError("Invalid URL {} ({})".format( + urlstr, url.errorString())) self._tabs.currentWidget().openurl(url) @cmdutils.register(instance='mainwindow.tabs.cmd') @@ -680,8 +688,7 @@ class CommandDispatcher: @cmdutils.register(instance='mainwindow.tabs.cmd') def download_page(self): """Download the current page.""" - url = self._tabs.currentWidget().url() - QApplication.instance().downloadmanager.get(url) + QApplication.instance().downloadmanager.get(self._current_url()) @cmdutils.register(instance='mainwindow.tabs.cmd', modes=['insert'], hide=True) diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py index 36a6b9039..23db7ca39 100644 --- a/qutebrowser/browser/downloads.py +++ b/qutebrowser/browser/downloads.py @@ -36,7 +36,7 @@ from qutebrowser.utils.log import downloads as logger from qutebrowser.utils.log import fix_rfc2622 from qutebrowser.utils.usertypes import PromptMode, Question, Timer from qutebrowser.utils.misc import (interpolate_color, format_seconds, - format_size) + format_size, qt_ensure_valid) from qutebrowser.commands.exceptions import CommandError @@ -366,6 +366,7 @@ class DownloadManager(QObject): Args: url: The URL to get, as QUrl """ + qt_ensure_valid(url) req = QNetworkRequest(url) reply = QCoreApplication.instance().networkmanager.get(req) self.fetch(reply) diff --git a/qutebrowser/browser/hints.py b/qutebrowser/browser/hints.py index f415ea159..6d44b4f13 100644 --- a/qutebrowser/browser/hints.py +++ b/qutebrowser/browser/hints.py @@ -33,6 +33,7 @@ import qutebrowser.utils.webelem as webelem from qutebrowser.commands.exceptions import CommandError from qutebrowser.utils.usertypes import enum from qutebrowser.utils.log import hints as logger +from qutebrowser.utils.misc import qt_ensure_valid ElemTuple = namedtuple('ElemTuple', 'elem, label') @@ -299,6 +300,7 @@ class HintManager(QObject): Args: url: The URL to open as a QURL. """ + qt_ensure_valid(url) sel = self._context.target == Target.yank_primary mode = QClipboard.Selection if sel else QClipboard.Clipboard urlstr = url.toString(QUrl.FullyEncoded | QUrl.RemovePassword) @@ -312,6 +314,7 @@ class HintManager(QObject): Args: url: The URL to open as a QUrl. """ + qt_ensure_valid(url) commands = { Target.cmd: 'open', Target.cmd_tab: 'open-tab', @@ -326,6 +329,7 @@ class HintManager(QObject): Args: url: The URL to download, as a QUrl. """ + qt_ensure_valid(url) QApplication.instance().downloadmanager.get(url) def _resolve_url(self, elem, baseurl=None): @@ -347,6 +351,7 @@ class HintManager(QObject): url = QUrl(text) if url.isRelative(): url = baseurl.resolved(url) + qt_ensure_valid(url) return url def _find_prevnext(self, frame, prev=False): diff --git a/qutebrowser/browser/quickmarks.py b/qutebrowser/browser/quickmarks.py index f99ed2494..cf2738ad9 100644 --- a/qutebrowser/browser/quickmarks.py +++ b/qutebrowser/browser/quickmarks.py @@ -33,7 +33,7 @@ import qutebrowser.utils.message as message import qutebrowser.commands.utils as cmdutils from qutebrowser.utils.usertypes import PromptMode from qutebrowser.config.lineparser import LineConfigParser -from qutebrowser.utils.misc import get_standard_dir +from qutebrowser.utils.misc import get_standard_dir, qt_ensure_valid from qutebrowser.commands.exceptions import CommandError @@ -63,6 +63,7 @@ def prompt_save(url): Args: url: The quickmark url as a QUrl. """ + qt_ensure_valid(url) urlstr = url.toString(QUrl.RemovePassword | QUrl.FullyEncoded) message.question("Add quickmark:", PromptMode.text, partial(quickmark_add, urlstr)) @@ -81,6 +82,8 @@ def quickmark_add(url, name): if not url: raise CommandError("Can't set mark with empty URL!") + qt_ensure_valid(url) + def set_mark(): """Really set the quickmark.""" marks[name] = url @@ -99,6 +102,6 @@ def get(name): urlstr = marks[name] url = QUrl(urlstr) if not url.isValid(): - raise CommandError("Invalid URL for quickmark {}: {}".format( - name, urlstr)) + raise CommandError("Invalid URL for quickmark {}: {} ({})".format( + name, urlstr, url.errorString())) return url diff --git a/qutebrowser/config/configdata.py b/qutebrowser/config/configdata.py index d0f2370bf..bfab28891 100644 --- a/qutebrowser/config/configdata.py +++ b/qutebrowser/config/configdata.py @@ -767,7 +767,7 @@ DATA = OrderedDict([ ('colors', sect.KeyValue( ('completion.fg', - SettingValue(types.Color(), 'white'), + SettingValue(types.QtColor(), 'white'), "Text color of the completion widget."), ('completion.bg', @@ -779,7 +779,7 @@ DATA = OrderedDict([ "Background color of completion widget items."), ('completion.category.fg', - SettingValue(types.Color(), 'white'), + SettingValue(types.QtColor(), 'white'), "Foreground color of completion widget category headers."), ('completion.category.bg', @@ -796,7 +796,7 @@ DATA = OrderedDict([ "Bottom border color of the completion widget category headers."), ('completion.item.selected.fg', - SettingValue(types.Color(), 'black'), + SettingValue(types.QtColor(), 'black'), "Foreground color of the selected completion item."), ('completion.item.selected.bg', diff --git a/qutebrowser/config/conftypes.py b/qutebrowser/config/conftypes.py index 7871a967b..0d8d49a11 100644 --- a/qutebrowser/config/conftypes.py +++ b/qutebrowser/config/conftypes.py @@ -751,7 +751,10 @@ class Proxy(BaseType): if value in self.valid_values: return url = QUrl(value) - if (url.isValid() and not url.isEmpty() and + if not url.isValid(): + raise ValidationError(value, "invalid url, {}".format( + url.errorString())) + elif (url.isValid() and not url.isEmpty() and url.scheme() in self.PROXY_TYPES): if url.userName() and url.password(): pass diff --git a/qutebrowser/models/basecompletion.py b/qutebrowser/models/basecompletion.py index 48a387b52..15845aa41 100644 --- a/qutebrowser/models/basecompletion.py +++ b/qutebrowser/models/basecompletion.py @@ -27,6 +27,7 @@ from PyQt5.QtCore import Qt from PyQt5.QtGui import QStandardItemModel, QStandardItem from qutebrowser.utils.usertypes import enum +from qutebrowser.utils.misc import qt_ensure_valid Role = enum('marks', 'sort', start=Qt.UserRole) @@ -73,6 +74,7 @@ class BaseCompletionModel(QStandardItemModel): index: A QModelIndex of the item to mark. needle: The string to mark. """ + qt_ensure_valid(index) haystack = self.data(index) marks = self._get_marks(needle, haystack) self.setData(index, marks, Role.marks) @@ -128,8 +130,7 @@ class BaseCompletionModel(QStandardItemModel): Return: The item flags, or Qt.NoItemFlags on error. """ - if not index.isValid(): - return Qt.NoItemFlags + qt_ensure_valid(index) if index.parent().isValid(): # item return Qt.ItemIsEnabled | Qt.ItemIsSelectable diff --git a/qutebrowser/models/downloadmodel.py b/qutebrowser/models/downloadmodel.py index 87357d0b4..8651c5956 100644 --- a/qutebrowser/models/downloadmodel.py +++ b/qutebrowser/models/downloadmodel.py @@ -26,6 +26,7 @@ from PyQt5.QtWidgets import QApplication import qutebrowser.config.config as config from qutebrowser.utils.usertypes import enum from qutebrowser.utils.log import downloads as logger +from qutebrowser.utils.misc import qt_ensure_valid Role = enum('item', start=Qt.UserRole) @@ -73,9 +74,8 @@ class DownloadModel(QAbstractListModel): def data(self, index, role): """Download data from DownloadManager.""" - if not index.isValid(): - return QVariant() - elif index.parent().isValid() or index.column() != 0: + qt_ensure_valid(index) + if index.parent().isValid() or index.column() != 0: return QVariant() try: diff --git a/qutebrowser/utils/misc.py b/qutebrowser/utils/misc.py index b042a9678..1e8455cae 100644 --- a/qutebrowser/utils/misc.py +++ b/qutebrowser/utils/misc.py @@ -331,10 +331,8 @@ def interpolate_color(start, end, percent, colorspace=QColor.Rgb): Raise: ValueError if invalid parameters are passed. """ - if not start.isValid(): - raise ValueError("Invalid start color") - if not end.isValid(): - raise ValueError("Invalid end color") + qt_ensure_valid(start) + qt_ensure_valid(end) out = QColor() if colorspace == QColor.Rgb: a_c1, a_c2, a_c3, _alpha = start.getRgb() @@ -356,7 +354,9 @@ def interpolate_color(start, end, percent, colorspace=QColor.Rgb): out.setHsl(*components) else: raise ValueError("Invalid colorspace!") - return out.convertTo(start.spec()) + out = out.convertTo(start.spec()) + qt_ensure_valid(out) + return out def format_seconds(total_seconds): @@ -395,6 +395,31 @@ def check_print_compat(): return not (os.name == 'nt' and qt_version_check('5.3.0', operator.lt)) +def qt_ensure_valid(obj): + """Ensure a Qt object with an .isValid() method is valid. + + Raise: + QtValueError if the object is invalid. + """ + if not obj.isValid(): + raise QtValueError(obj) + + +class QtValueError(ValueError): + + """Exception which gets raised by qt_ensure_valid.""" + + def __init__(self, obj): + try: + self.reason = obj.errorString() + except AttributeError: + self.reason = None + err = "{} is not valid".format(obj) + if self.reason: + err += ": {}".format(self.reason) + super().__init__(err) + + class EventLoop(QEventLoop): """A thin wrapper around QEventLoop. diff --git a/qutebrowser/utils/url.py b/qutebrowser/utils/url.py index 4e08062df..cc8825cb8 100644 --- a/qutebrowser/utils/url.py +++ b/qutebrowser/utils/url.py @@ -28,6 +28,7 @@ from PyQt5.QtNetwork import QHostInfo import qutebrowser.config.config as config from qutebrowser.utils.log import url as logger +from qutebrowser.utils.misc import qt_ensure_valid # FIXME: we probably could raise some exceptions on invalid URLs @@ -63,7 +64,9 @@ def _get_search_url(txt): logger.debug("engine: default, term '{}'".format(txt)) if not term: raise FuzzyUrlError("No search term given") - return QUrl.fromUserInput(template.format(urllib.parse.quote(term))) + url = QUrl.fromUserInput(template.format(urllib.parse.quote(term))) + qt_ensure_valid(url) + return url def _is_url_naive(urlstr): @@ -131,6 +134,7 @@ def fuzzy_url(urlstr): url = QUrl.fromUserInput(urlstr) logger.debug("Converting fuzzy term {} to URL -> {}".format( urlstr, url.toDisplayString())) + qt_ensure_valid(url) return url diff --git a/qutebrowser/utils/webelem.py b/qutebrowser/utils/webelem.py index 0894aec90..4173d0404 100644 --- a/qutebrowser/utils/webelem.py +++ b/qutebrowser/utils/webelem.py @@ -94,8 +94,10 @@ def is_visible(elem, mainframe): # Then check if it's visible in its frame if it's not in the main frame. elem_frame = elem.webFrame() elem_rect = elem.geometry() - if elem_frame.parentFrame() is not None: - framegeom = QRect(elem_frame.geometry()) + framegeom = QRect(elem_frame.geometry()) + if not framegeom.isValid(): + visible_in_frame = False + elif elem_frame.parentFrame() is not None: framegeom.moveTo(0, 0) framegeom.translate(elem_frame.scrollPosition()) if elem_rect.isValid(): diff --git a/qutebrowser/widgets/completiondelegate.py b/qutebrowser/widgets/completiondelegate.py index 2acaca8dd..70186c1e5 100644 --- a/qutebrowser/widgets/completiondelegate.py +++ b/qutebrowser/widgets/completiondelegate.py @@ -26,7 +26,7 @@ import html from PyQt5.QtWidgets import QStyle, QStyleOptionViewItem, QStyledItemDelegate from PyQt5.QtCore import QRectF, QSize, Qt from PyQt5.QtGui import (QIcon, QPalette, QTextDocument, QTextOption, - QTextCursor, QColor, QAbstractTextDocumentLayout) + QTextCursor, QAbstractTextDocumentLayout) import qutebrowser.config.config as config from qutebrowser.models.basecompletion import Role @@ -147,9 +147,9 @@ class CompletionItemDelegate(QStyledItemDelegate): else: option = 'completion.fg' try: - self._painter.setPen(QColor(config.get('colors', option))) + self._painter.setPen(config.get('colors', option)) except config.NoOptionError: - self._painter.setPen(QColor(config.get('colors', 'completion.fg'))) + self._painter.setPen(config.get('colors', 'completion.fg')) ctx = QAbstractTextDocumentLayout.PaintContext() ctx.palette.setColor(QPalette.Text, self._painter.pen().color()) if clip.isValid(): diff --git a/qutebrowser/widgets/downloads.py b/qutebrowser/widgets/downloads.py index 314863851..e07bb1f6c 100644 --- a/qutebrowser/widgets/downloads.py +++ b/qutebrowser/widgets/downloads.py @@ -24,6 +24,7 @@ from PyQt5.QtWidgets import QListView, QSizePolicy, QMenu from qutebrowser.models.downloadmodel import DownloadModel, Role from qutebrowser.config.style import set_register_stylesheet +from qutebrowser.utils.misc import qt_ensure_valid class DownloadView(QListView): @@ -87,6 +88,8 @@ class DownloadView(QListView): idx = self.model().last_index() height = self.visualRect(idx).bottom() if height != -1: - return QSize(0, height + 2) + size = QSize(0, height + 2) else: - return QSize(0, 0) + size = QSize(0, 0) + qt_ensure_valid(size) + return size diff --git a/qutebrowser/widgets/mainwindow.py b/qutebrowser/widgets/mainwindow.py index e503b8e8c..62f9a163f 100644 --- a/qutebrowser/widgets/mainwindow.py +++ b/qutebrowser/widgets/mainwindow.py @@ -129,7 +129,9 @@ class MainWindow(QWidget): topleft_y = utils.check_overflow(topleft_y, 'int', fatal=False) topleft = QPoint(0, topleft_y) bottomright = self.status.geometry().topRight() - self.completion.setGeometry(QRect(topleft, bottomright)) + rect = QRect(topleft, bottomright) + utils.qt_ensure_valid(rect) + self.completion.setGeometry(rect) @cmdutils.register(instance='mainwindow', name=['quit', 'q'], nargs=0) def close(self): diff --git a/qutebrowser/widgets/tabbedbrowser.py b/qutebrowser/widgets/tabbedbrowser.py index e48278a77..71ffce1ae 100644 --- a/qutebrowser/widgets/tabbedbrowser.py +++ b/qutebrowser/widgets/tabbedbrowser.py @@ -32,6 +32,7 @@ from qutebrowser.widgets.tabwidget import TabWidget, EmptyTabIcon from qutebrowser.widgets.webview import WebView from qutebrowser.browser.signalfilter import SignalFilter from qutebrowser.browser.commands import CommandDispatcher +from qutebrowser.utils.misc import qt_ensure_valid class TabbedBrowser(TabWidget): @@ -247,6 +248,7 @@ class TabbedBrowser(TabWidget): url: The URL to open as QUrl. newtab: True to open URL in a new tab, False otherwise. """ + qt_ensure_valid(url) if newtab: self.tabopen(url, background=False) else: @@ -277,6 +279,7 @@ class TabbedBrowser(TabWidget): Return: The opened WebView instance. """ + qt_ensure_valid(url) log.webview.debug("Creating new tab with URL {}".format(url)) tab = WebView(self) self._connect_tab_signals(tab) diff --git a/qutebrowser/widgets/webview.py b/qutebrowser/widgets/webview.py index 3aedb9052..4da493c8d 100644 --- a/qutebrowser/widgets/webview.py +++ b/qutebrowser/widgets/webview.py @@ -31,7 +31,7 @@ import qutebrowser.keyinput.modeman as modeman import qutebrowser.utils.message as message import qutebrowser.utils.webelem as webelem import qutebrowser.utils.log as log -from qutebrowser.utils.misc import elide +from qutebrowser.utils.misc import elide, qt_ensure_valid from qutebrowser.browser.webpage import BrowserPage from qutebrowser.browser.hints import HintManager from qutebrowser.utils.usertypes import NeighborList, enum @@ -293,6 +293,7 @@ class WebView(QWebView): Emit: titleChanged """ + qt_ensure_valid(url) urlstr = url.toDisplayString() log.webview.debug("New title: {}".format(urlstr)) self.titleChanged.emit(urlstr) @@ -379,6 +380,7 @@ class WebView(QWebView): @pyqtSlot('QUrl') def on_url_changed(self, url): """Update url_text when URL has changed.""" + qt_ensure_valid(url) self.url_text = url.toString() @pyqtSlot(str) @@ -394,6 +396,7 @@ class WebView(QWebView): url = QUrl(urlstr) if not url.isValid(): message.error("Invalid link {} clicked!".format(urlstr)) + log.webview.debug(url.errorString()) return if self._open_target == Target.tab: self.tabbedbrowser.tabopen(url, False)