From a77e0859529944ef96474f193fb6c8175c34c154 Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Wed, 23 Mar 2016 23:31:38 +0100 Subject: [PATCH 01/16] dirbrowser: fix navigation on windows Issue #1334 The problem was that there were too few slashes. On Linux, absolute paths start with /, so file:// + /home gives file:///home, which is a valid path. On windows however, absolute paths start with a drive letter, so file:// + C:/Users gives file://C:/Users, which is parsed as "host C, path Users", which is why it could be written as file://c/Users (strip out the empty "port"), giving us an invalid path. The solution is to add the third slash in the template, and strip the redundant slash on unix systems. Additionally, this fixes a bug where navigating from '/home/' to the parent directory would give '/home' instead of '/' --- qutebrowser/browser/network/filescheme.py | 16 +++++++++++++--- qutebrowser/html/dirbrowser.html | 8 ++++---- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/qutebrowser/browser/network/filescheme.py b/qutebrowser/browser/network/filescheme.py index 3f5226ee4..4a3bfdcf6 100644 --- a/qutebrowser/browser/network/filescheme.py +++ b/qutebrowser/browser/network/filescheme.py @@ -44,7 +44,9 @@ def get_file_list(basedir, all_files, filterfunc): for filename in all_files: absname = os.path.join(basedir, filename) if filterfunc(absname): - items.append({'name': filename, 'absname': absname}) + # Absolute paths in Unix start with a slash ('/'), but we already + # have enough slashes in the template, so we don't need it here + items.append({'name': filename, 'absname': absname.lstrip('/')}) return sorted(items, key=lambda v: v['name'].lower()) @@ -57,6 +59,14 @@ def is_root(directory): Return: Whether the directory is a root directory or not. """ + # If you're curious as why this works: + # dirname('/') = '/' + # dirname('/home') = '/' + # dirname('/home/') = '/home' + # dirname('/home/foo') = '/home' + # basically, for files (no trailing slash) it removes the file part, and for + # directories, it removes the trailing slash, so the only way for this to be + # equal is if the directory is the root directory. return os.path.dirname(directory) == directory @@ -74,14 +84,14 @@ def dirbrowser_html(path): if is_root(path): parent = None else: - parent = os.path.dirname(path) + parent = os.path.normpath(os.path.join(path, '..')).lstrip('/') try: all_files = os.listdir(path) except OSError as e: html = jinja.render('error.html', title="Error while reading directory", - url='file://{}'.format(path), error=str(e), + url='file:///{}'.format(path), error=str(e), icon='') return html.encode('UTF-8', errors='xmlcharrefreplace') diff --git a/qutebrowser/html/dirbrowser.html b/qutebrowser/html/dirbrowser.html index ab0000c36..aee4cb874 100644 --- a/qutebrowser/html/dirbrowser.html +++ b/qutebrowser/html/dirbrowser.html @@ -46,21 +46,21 @@ ul.files > li {

Browse directory: {{url}}

- {% if parent %} + {% if parent is not none %} {% endif %} From 700756aa16eb56624ef88b285e5ac662e6f10ea5 Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Wed, 23 Mar 2016 23:56:23 +0100 Subject: [PATCH 02/16] tests: add more cases for dirbrowser.is_root The trailing slash might have an effect on the function result, so we should have cases with/without the slash. --- tests/unit/browser/network/test_filescheme.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/unit/browser/network/test_filescheme.py b/tests/unit/browser/network/test_filescheme.py index 484bddaa7..126f81df5 100644 --- a/tests/unit/browser/network/test_filescheme.py +++ b/tests/unit/browser/network/test_filescheme.py @@ -57,6 +57,8 @@ class TestIsRoot: @pytest.mark.windows @pytest.mark.parametrize('directory, is_root', [ + ('C:\\foo\\bar', False), + ('C:\\foo\\', False), ('C:\\foo', False), ('C:\\', True) ]) @@ -65,6 +67,8 @@ class TestIsRoot: @pytest.mark.posix @pytest.mark.parametrize('directory, is_root', [ + ('/foo/bar', False), + ('/foo/', False), ('/foo', False), ('/', True) ]) From 375e60627a69d96f4743842c93ca1871a50383c7 Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Thu, 24 Mar 2016 00:14:22 +0100 Subject: [PATCH 03/16] dirbrowser: ditch .lstrip, add file_url function --- qutebrowser/browser/network/filescheme.py | 6 ++---- qutebrowser/html/dirbrowser.html | 6 +++--- qutebrowser/utils/jinja.py | 11 +++++++++++ 3 files changed, 16 insertions(+), 7 deletions(-) diff --git a/qutebrowser/browser/network/filescheme.py b/qutebrowser/browser/network/filescheme.py index 4a3bfdcf6..f8ccf5185 100644 --- a/qutebrowser/browser/network/filescheme.py +++ b/qutebrowser/browser/network/filescheme.py @@ -44,9 +44,7 @@ def get_file_list(basedir, all_files, filterfunc): for filename in all_files: absname = os.path.join(basedir, filename) if filterfunc(absname): - # Absolute paths in Unix start with a slash ('/'), but we already - # have enough slashes in the template, so we don't need it here - items.append({'name': filename, 'absname': absname.lstrip('/')}) + items.append({'name': filename, 'absname': absname}) return sorted(items, key=lambda v: v['name'].lower()) @@ -84,7 +82,7 @@ def dirbrowser_html(path): if is_root(path): parent = None else: - parent = os.path.normpath(os.path.join(path, '..')).lstrip('/') + parent = os.path.normpath(os.path.join(path, '..')) try: all_files = os.listdir(path) diff --git a/qutebrowser/html/dirbrowser.html b/qutebrowser/html/dirbrowser.html index aee4cb874..1a73261f6 100644 --- a/qutebrowser/html/dirbrowser.html +++ b/qutebrowser/html/dirbrowser.html @@ -48,19 +48,19 @@ ul.files > li { {% if parent is not none %} {% endif %} diff --git a/qutebrowser/utils/jinja.py b/qutebrowser/utils/jinja.py index e6ccd9c6c..e608cc3ff 100644 --- a/qutebrowser/utils/jinja.py +++ b/qutebrowser/utils/jinja.py @@ -74,6 +74,15 @@ def resource_url(path): return QUrl.fromLocalFile(image).toString(QUrl.FullyEncoded) +def file_url(path): + """Return a file:// url (as string) to the given local path. + + Arguments: + path: The absolute path to the local file + """ + return QUrl.fromLocalFile(path).toString(QUrl.FullyEncoded) + + def render(template, **kwargs): """Render the given template and pass the given arguments to it.""" try: @@ -85,5 +94,7 @@ def render(template, **kwargs): tb = traceback.format_exc() return err_template.format(pagename=template, traceback=tb) + _env = jinja2.Environment(loader=Loader('html'), autoescape=_guess_autoescape) _env.globals['resource_url'] = resource_url +_env.globals['file_url'] = file_url From e97b10517f363d50447d32e8b26b1f86f5fa7557 Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Thu, 24 Mar 2016 00:53:23 +0100 Subject: [PATCH 04/16] tests: use file_url for dirbrowser tests Otherwise the tests will fail on windows. --- tests/unit/browser/network/test_filescheme.py | 25 +++++++++++++------ 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/tests/unit/browser/network/test_filescheme.py b/tests/unit/browser/network/test_filescheme.py index 126f81df5..1507d9a00 100644 --- a/tests/unit/browser/network/test_filescheme.py +++ b/tests/unit/browser/network/test_filescheme.py @@ -28,6 +28,7 @@ from PyQt5.QtCore import QUrl from PyQt5.QtNetwork import QNetworkRequest from qutebrowser.browser.network import filescheme +from qutebrowser.utils import jinja @pytest.mark.parametrize('create_file, create_dir, filterfunc, expected', [ @@ -76,6 +77,15 @@ class TestIsRoot: assert filescheme.is_root(directory) == is_root +def _file_url(path): + """Return a file:// url (as string) for the given LocalPath. + + Arguments: + path: The filepath as LocalPath (as handled by py.path) + """ + return jinja.file_url(str(path)) + + class TestDirbrowserHtml: Parsed = collections.namedtuple('Parsed', 'parent, folders, files') @@ -149,8 +159,8 @@ class TestDirbrowserHtml: parsed = parser(str(tmpdir)) assert parsed.parent assert not parsed.folders - foo_item = self.Item('file://' + str(foo_file), foo_file.relto(tmpdir)) - bar_item = self.Item('file://' + str(bar_file), bar_file.relto(tmpdir)) + foo_item = self.Item(_file_url(foo_file), foo_file.relto(tmpdir)) + bar_item = self.Item(_file_url(bar_file), bar_file.relto(tmpdir)) assert parsed.files == [bar_item, foo_item] def test_html_special_chars(self, tmpdir, parser): @@ -158,8 +168,7 @@ class TestDirbrowserHtml: special_file.ensure() parsed = parser(str(tmpdir)) - item = self.Item('file://' + str(special_file), - special_file.relto(tmpdir)) + item = self.Item(_file_url(special_file), special_file.relto(tmpdir)) assert parsed.files == [item] def test_dirs(self, tmpdir, parser): @@ -171,8 +180,8 @@ class TestDirbrowserHtml: parsed = parser(str(tmpdir)) assert parsed.parent assert not parsed.files - foo_item = self.Item('file://' + str(foo_dir), foo_dir.relto(tmpdir)) - bar_item = self.Item('file://' + str(bar_dir), bar_dir.relto(tmpdir)) + foo_item = self.Item(_file_url(foo_dir), foo_dir.relto(tmpdir)) + bar_item = self.Item(_file_url(bar_dir), bar_dir.relto(tmpdir)) assert parsed.folders == [bar_item, foo_item] def test_mixed(self, tmpdir, parser): @@ -182,8 +191,8 @@ class TestDirbrowserHtml: bar_dir.ensure(dir=True) parsed = parser(str(tmpdir)) - foo_item = self.Item('file://' + str(foo_file), foo_file.relto(tmpdir)) - bar_item = self.Item('file://' + str(bar_dir), bar_dir.relto(tmpdir)) + foo_item = self.Item(_file_url(foo_file), foo_file.relto(tmpdir)) + bar_item = self.Item(_file_url(bar_dir), bar_dir.relto(tmpdir)) assert parsed.parent assert parsed.files == [foo_item] assert parsed.folders == [bar_item] From 7fe4c7e06d49b06a9b7c683c71c1304f6b96de45 Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Thu, 24 Mar 2016 01:15:41 +0100 Subject: [PATCH 05/16] fix lint --- qutebrowser/browser/network/filescheme.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/qutebrowser/browser/network/filescheme.py b/qutebrowser/browser/network/filescheme.py index f8ccf5185..becb67d66 100644 --- a/qutebrowser/browser/network/filescheme.py +++ b/qutebrowser/browser/network/filescheme.py @@ -62,9 +62,9 @@ def is_root(directory): # dirname('/home') = '/' # dirname('/home/') = '/home' # dirname('/home/foo') = '/home' - # basically, for files (no trailing slash) it removes the file part, and for - # directories, it removes the trailing slash, so the only way for this to be - # equal is if the directory is the root directory. + # basically, for files (no trailing slash) it removes the file part, and + # for directories, it removes the trailing slash, so the only way for this + # to be equal is if the directory is the root directory. return os.path.dirname(directory) == directory From 5e73a2ea37339303eb115b57d42872e9134f2a52 Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Thu, 24 Mar 2016 18:52:49 +0100 Subject: [PATCH 06/16] dirbrowser: move parent dir logic to own function --- qutebrowser/browser/network/filescheme.py | 14 ++++++++++- tests/unit/browser/network/test_filescheme.py | 23 +++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/qutebrowser/browser/network/filescheme.py b/qutebrowser/browser/network/filescheme.py index becb67d66..d4c20fd4d 100644 --- a/qutebrowser/browser/network/filescheme.py +++ b/qutebrowser/browser/network/filescheme.py @@ -68,6 +68,18 @@ def is_root(directory): return os.path.dirname(directory) == directory +def parent_dir(directory): + """Return the parent directory for the given directory. + + Args: + directory: The path to the directory. + + Return: + The path to the parent directory. + """ + return os.path.normpath(os.path.join(directory, os.pardir)) + + def dirbrowser_html(path): """Get the directory browser web page. @@ -82,7 +94,7 @@ def dirbrowser_html(path): if is_root(path): parent = None else: - parent = os.path.normpath(os.path.join(path, '..')) + parent = parent_dir(path) try: all_files = os.listdir(path) diff --git a/tests/unit/browser/network/test_filescheme.py b/tests/unit/browser/network/test_filescheme.py index 1507d9a00..e57b5ee55 100644 --- a/tests/unit/browser/network/test_filescheme.py +++ b/tests/unit/browser/network/test_filescheme.py @@ -77,6 +77,29 @@ class TestIsRoot: assert filescheme.is_root(directory) == is_root +class TestParentDir: + + @pytest.mark.windows + @pytest.mark.parametrize('directory, parent', [ + ('C:\\foo\\bar', 'C:\\foo'), + ('C:\\foo', 'C:\\'), + ('C:\\foo\\', 'C:\\'), + ('C:\\', 'C:\\'), + ]) + def test_windows(self, directory, parent): + assert filescheme.parent_dir(directory) == parent + + @pytest.mark.posix + @pytest.mark.parametrize('directory, parent', [ + ('/home/foo', '/home'), + ('/home', '/'), + ('/home/', '/'), + ('/', '/'), + ]) + def test_posix(self, directory, parent): + assert filescheme.parent_dir(directory) == parent + + def _file_url(path): """Return a file:// url (as string) for the given LocalPath. From f6e8815871b50f16d1d4a3265ef421d93d8c9bc2 Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Thu, 24 Mar 2016 21:26:55 +0100 Subject: [PATCH 07/16] tests: add integration tests for dirbrowser --- tests/integration/quteprocess.py | 12 ++ tests/integration/test_dirbrowser.py | 176 +++++++++++++++++++++++++++ 2 files changed, 188 insertions(+) create mode 100644 tests/integration/test_dirbrowser.py diff --git a/tests/integration/quteprocess.py b/tests/integration/quteprocess.py index cad527ac4..f02db102d 100644 --- a/tests/integration/quteprocess.py +++ b/tests/integration/quteprocess.py @@ -359,6 +359,18 @@ class QuteProc(testprocess.Process): else: self.send_cmd(':open ' + url) + def open_url(self, url, *, new_tab=False, new_window=False): + """Open the given url in qutebrowser.""" + if new_tab and new_window: + raise ValueError("new_tab and new_window given!") + + if new_tab: + self.send_cmd(':open -t ' + url) + elif new_window: + self.send_cmd(':open -w ' + url) + else: + self.send_cmd(':open ' + url) + def mark_expected(self, category=None, loglevel=None, message=None): """Mark a given logging message as expected.""" line = self.wait_for(category=category, loglevel=loglevel, diff --git a/tests/integration/test_dirbrowser.py b/tests/integration/test_dirbrowser.py new file mode 100644 index 000000000..c29301b61 --- /dev/null +++ b/tests/integration/test_dirbrowser.py @@ -0,0 +1,176 @@ +# vim: ft=python fileencoding=utf-8 sts=4 sw=4 et: + +# Copyright 2015-2016 Daniel Schadt +# +# This file is part of qutebrowser. +# +# qutebrowser is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# qutebrowser is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with qutebrowser. If not, see . + +"""Test the built-in directory browser.""" + +import bs4 +import collections + +import pytest + +from PyQt5.QtCore import QUrl +from qutebrowser.utils import jinja + + +class DirLayout: + + """Provide a fake directory layout to test dirbrowser.""" + + Parsed = collections.namedtuple('Parsed', 'path, parent, folders, files') + Item = collections.namedtuple('Item', 'path, link, text') + + LAYOUT = [ + 'folder0/file00', + 'folder0/file01', + 'folder1/folder10/file100', + 'folder1/file10', + 'folder1/file11', + 'file0', + 'file1', + ] + + @classmethod + def layout_folders(cls): + """Return all folders in the root directory of the layout.""" + folders = set() + for path in cls.LAYOUT: + parts = path.split('/') + if len(parts) > 1: + folders.add(parts[0]) + folders = list(folders) + folders.sort() + return folders + + @classmethod + def get_folder_content(cls, name): + """Return (folders, files) for the given folder in the root dir.""" + folders = set() + files = set() + for path in cls.LAYOUT: + if not path.startswith(name + '/'): + continue + parts = path.split('/') + if len(parts) == 2: + files.add(parts[1]) + else: + folders.add(parts[1]) + folders = list(folders) + folders.sort() + files = list(files) + files.sort() + return (folders, files) + + def __init__(self, factory): + self._factory = factory + self.base = factory.getbasetemp() + self.layout = factory.mktemp('layout') + self._mklayout() + + def _mklayout(self): + for filename in self.LAYOUT: + self.layout.ensure(filename) + + def file_url(self): + """Return a file:// link to the directory.""" + return jinja.file_url(str(self.layout)) + + def path(self, *parts): + """Return the path to the given file inside the layout folder.""" + return str(self.layout.join(*parts)) + + def parse(self, quteproc): + """Parse the dirbrowser content from the given quteproc. + + Args: + quteproc: The quteproc fixture. + """ + html = quteproc.get_content(plain=False) + soup = bs4.BeautifulSoup(html, 'html.parser') + print(soup.prettify()) + title_prefix = 'Browse directory: ' + # Strip off the title prefix to obtain the path of the folder that + # we're browsing + path = soup.title.string[len(title_prefix):] + + container = soup('div', id='dirbrowserContainer')[0] + + parent_elem = container('ul', class_='parent') + if len(parent_elem) == 0: + parent = None + else: + parent = QUrl(parent_elem[0].li.a['href']).toLocalFile() + + folders = [] + files = [] + + for css_class, list_ in [('folders', folders), ('files', files)]: + for li in container('ul', class_=css_class)[0]('li'): + item_path = QUrl(li.a['href']).toLocalFile() + list_.append(self.Item(path=item_path, link=li.a['href'], + text=str(li.a.string))) + + return self.Parsed(path=path, parent=parent, folders=folders, + files=files) + + +@pytest.fixture(scope='module') +def dir_layout(tmpdir_factory): + return DirLayout(tmpdir_factory) + + +def test_parent_folder(dir_layout, quteproc): + quteproc.open_url(dir_layout.file_url()) + page = dir_layout.parse(quteproc) + assert page.parent == str(dir_layout.base) + + +def test_parent_with_slash(dir_layout, quteproc): + """Test the parent link with an URL that has a trailing slash.""" + quteproc.open_url(dir_layout.file_url() + '/') + page = dir_layout.parse(quteproc) + assert page.parent == str(dir_layout.base) + + +def test_enter_folder_smoke(dir_layout, quteproc): + quteproc.open_url(dir_layout.file_url()) + quteproc.send_cmd(':hint all normal') + # a is the parent link, s is the first listed folder/file + quteproc.send_cmd(':follow-hint s') + page = dir_layout.parse(quteproc) + assert page.path == dir_layout.path('folder0') + + +@pytest.mark.parametrize('folder', DirLayout.layout_folders()) +def test_enter_folder(dir_layout, quteproc, folder): + quteproc.open_url(dir_layout.file_url()) + # Use Javascript and XPath to click the link that has the folder name as + # text. + quteproc.send_cmd( + ':jseval document.evaluate(\'//a[text()="{}"]\', document, null, ' + 'XPathResult.ANY_TYPE, null).iterateNext().click()' + .format(folder) + ) + page = dir_layout.parse(quteproc) + assert page.path == dir_layout.path(folder) + assert page.parent == dir_layout.path() + folders, files = DirLayout.get_folder_content(folder) + foldernames = [item.text for item in page.folders] + assert foldernames == folders + filenames = [item.text for item in page.files] + assert filenames == files From c0b40aefdd9f79cdc9787243273626bc3d931c0e Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Thu, 24 Mar 2016 22:02:57 +0100 Subject: [PATCH 08/16] tests/dirbrowser: normalize paths before comparing This avoids errors because some libraries use '/' even on windows, while others use '\' on windows. --- tests/integration/test_dirbrowser.py | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/tests/integration/test_dirbrowser.py b/tests/integration/test_dirbrowser.py index c29301b61..d95f59cea 100644 --- a/tests/integration/test_dirbrowser.py +++ b/tests/integration/test_dirbrowser.py @@ -19,6 +19,7 @@ """Test the built-in directory browser.""" +import os import bs4 import collections @@ -92,7 +93,11 @@ class DirLayout: def path(self, *parts): """Return the path to the given file inside the layout folder.""" - return str(self.layout.join(*parts)) + return os.path.normpath(str(self.layout.join(*parts))) + + def base_path(self): + """Return the path of the base temporary folder.""" + return os.path.normpath(str(self.base)) def parse(self, quteproc): """Parse the dirbrowser content from the given quteproc. @@ -107,6 +112,7 @@ class DirLayout: # Strip off the title prefix to obtain the path of the folder that # we're browsing path = soup.title.string[len(title_prefix):] + path = os.path.normpath(path) container = soup('div', id='dirbrowserContainer')[0] @@ -115,6 +121,7 @@ class DirLayout: parent = None else: parent = QUrl(parent_elem[0].li.a['href']).toLocalFile() + parent = os.path.normpath(parent) folders = [] files = [] @@ -122,6 +129,7 @@ class DirLayout: for css_class, list_ in [('folders', folders), ('files', files)]: for li in container('ul', class_=css_class)[0]('li'): item_path = QUrl(li.a['href']).toLocalFile() + item_path = os.path.normpath(item_path) list_.append(self.Item(path=item_path, link=li.a['href'], text=str(li.a.string))) @@ -137,14 +145,14 @@ def dir_layout(tmpdir_factory): def test_parent_folder(dir_layout, quteproc): quteproc.open_url(dir_layout.file_url()) page = dir_layout.parse(quteproc) - assert page.parent == str(dir_layout.base) + assert page.parent == dir_layout.base_path() def test_parent_with_slash(dir_layout, quteproc): """Test the parent link with an URL that has a trailing slash.""" quteproc.open_url(dir_layout.file_url() + '/') page = dir_layout.parse(quteproc) - assert page.parent == str(dir_layout.base) + assert page.parent == dir_layout.base_path() def test_enter_folder_smoke(dir_layout, quteproc): From 6a96e1d6d80ba6f00ba2421894f55e8df9ad12e8 Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Mon, 28 Mar 2016 23:06:05 +0200 Subject: [PATCH 09/16] quteprocess: remove duplicate code --- tests/integration/quteprocess.py | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/tests/integration/quteprocess.py b/tests/integration/quteprocess.py index f02db102d..d41356f3e 100644 --- a/tests/integration/quteprocess.py +++ b/tests/integration/quteprocess.py @@ -348,16 +348,8 @@ class QuteProc(testprocess.Process): def open_path(self, path, *, new_tab=False, new_window=False, port=None, https=False): """Open the given path on the local webserver in qutebrowser.""" - if new_tab and new_window: - raise ValueError("new_tab and new_window given!") - url = self.path_to_url(path, port=port, https=https) - if new_tab: - self.send_cmd(':open -t ' + url) - elif new_window: - self.send_cmd(':open -w ' + url) - else: - self.send_cmd(':open ' + url) + self.open_url(url, new_tab=new_tab, new_window=new_window) def open_url(self, url, *, new_tab=False, new_window=False): """Open the given url in qutebrowser.""" From 2db5b955520b94d628fafd1492b157aa599095b0 Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Tue, 29 Mar 2016 12:36:43 +0200 Subject: [PATCH 10/16] tests: use "if not parent_elem" Also add a new test for browsing the root directory --- tests/integration/test_dirbrowser.py | 11 ++++++++++- tests/unit/browser/network/test_filescheme.py | 2 +- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/tests/integration/test_dirbrowser.py b/tests/integration/test_dirbrowser.py index d95f59cea..7f93326d3 100644 --- a/tests/integration/test_dirbrowser.py +++ b/tests/integration/test_dirbrowser.py @@ -117,7 +117,7 @@ class DirLayout: container = soup('div', id='dirbrowserContainer')[0] parent_elem = container('ul', class_='parent') - if len(parent_elem) == 0: + if not parent_elem: parent = None else: parent = QUrl(parent_elem[0].li.a['href']).toLocalFile() @@ -155,6 +155,15 @@ def test_parent_with_slash(dir_layout, quteproc): assert page.parent == dir_layout.base_path() +def test_parent_in_root_dir(dir_layout, quteproc): + # This actually works on windows + root_path = os.path.realpath('/') + urlstr = QUrl.fromLocalFile(root_path).toString(QUrl.FullyEncoded) + quteproc.open_url(urlstr) + page = dir_layout.parse(quteproc) + assert page.parent is None + + def test_enter_folder_smoke(dir_layout, quteproc): quteproc.open_url(dir_layout.file_url()) quteproc.send_cmd(':hint all normal') diff --git a/tests/unit/browser/network/test_filescheme.py b/tests/unit/browser/network/test_filescheme.py index e57b5ee55..1321a649c 100644 --- a/tests/unit/browser/network/test_filescheme.py +++ b/tests/unit/browser/network/test_filescheme.py @@ -124,7 +124,7 @@ class TestDirbrowserHtml: container = soup('div', id='dirbrowserContainer')[0] parent_elem = container('ul', class_='parent') - if len(parent_elem) == 0: + if not parent_elem: parent = None else: parent = parent_elem[0].li.a.string From f085eb6eca3dd1deef8f86968866841833b2107c Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Tue, 29 Mar 2016 12:43:50 +0200 Subject: [PATCH 11/16] tests/dirbrowser: move parse out of DirLayout --- tests/integration/test_dirbrowser.py | 78 ++++++++++++++-------------- 1 file changed, 40 insertions(+), 38 deletions(-) diff --git a/tests/integration/test_dirbrowser.py b/tests/integration/test_dirbrowser.py index 7f93326d3..6265c2998 100644 --- a/tests/integration/test_dirbrowser.py +++ b/tests/integration/test_dirbrowser.py @@ -33,9 +33,6 @@ class DirLayout: """Provide a fake directory layout to test dirbrowser.""" - Parsed = collections.namedtuple('Parsed', 'path, parent, folders, files') - Item = collections.namedtuple('Item', 'path, link, text') - LAYOUT = [ 'folder0/file00', 'folder0/file01', @@ -99,42 +96,47 @@ class DirLayout: """Return the path of the base temporary folder.""" return os.path.normpath(str(self.base)) - def parse(self, quteproc): - """Parse the dirbrowser content from the given quteproc. - Args: - quteproc: The quteproc fixture. - """ - html = quteproc.get_content(plain=False) - soup = bs4.BeautifulSoup(html, 'html.parser') - print(soup.prettify()) - title_prefix = 'Browse directory: ' - # Strip off the title prefix to obtain the path of the folder that - # we're browsing - path = soup.title.string[len(title_prefix):] - path = os.path.normpath(path) +Parsed = collections.namedtuple('Parsed', 'path, parent, folders, files') +Item = collections.namedtuple('Item', 'path, link, text') - container = soup('div', id='dirbrowserContainer')[0] - parent_elem = container('ul', class_='parent') - if not parent_elem: - parent = None - else: - parent = QUrl(parent_elem[0].li.a['href']).toLocalFile() - parent = os.path.normpath(parent) +def parse(quteproc): + """Parse the dirbrowser content from the given quteproc. - folders = [] - files = [] + Args: + quteproc: The quteproc fixture. + """ + html = quteproc.get_content(plain=False) + soup = bs4.BeautifulSoup(html, 'html.parser') + print(soup.prettify()) + title_prefix = 'Browse directory: ' + # Strip off the title prefix to obtain the path of the folder that + # we're browsing + path = soup.title.string[len(title_prefix):] + path = os.path.normpath(path) - for css_class, list_ in [('folders', folders), ('files', files)]: - for li in container('ul', class_=css_class)[0]('li'): - item_path = QUrl(li.a['href']).toLocalFile() - item_path = os.path.normpath(item_path) - list_.append(self.Item(path=item_path, link=li.a['href'], - text=str(li.a.string))) + container = soup('div', id='dirbrowserContainer')[0] - return self.Parsed(path=path, parent=parent, folders=folders, - files=files) + parent_elem = container('ul', class_='parent') + if not parent_elem: + parent = None + else: + parent = QUrl(parent_elem[0].li.a['href']).toLocalFile() + parent = os.path.normpath(parent) + + folders = [] + files = [] + + for css_class, list_ in [('folders', folders), ('files', files)]: + for li in container('ul', class_=css_class)[0]('li'): + item_path = QUrl(li.a['href']).toLocalFile() + item_path = os.path.normpath(item_path) + list_.append(Item(path=item_path, link=li.a['href'], + text=str(li.a.string))) + + return Parsed(path=path, parent=parent, folders=folders, + files=files) @pytest.fixture(scope='module') @@ -144,14 +146,14 @@ def dir_layout(tmpdir_factory): def test_parent_folder(dir_layout, quteproc): quteproc.open_url(dir_layout.file_url()) - page = dir_layout.parse(quteproc) + page = parse(quteproc) assert page.parent == dir_layout.base_path() def test_parent_with_slash(dir_layout, quteproc): """Test the parent link with an URL that has a trailing slash.""" quteproc.open_url(dir_layout.file_url() + '/') - page = dir_layout.parse(quteproc) + page = parse(quteproc) assert page.parent == dir_layout.base_path() @@ -160,7 +162,7 @@ def test_parent_in_root_dir(dir_layout, quteproc): root_path = os.path.realpath('/') urlstr = QUrl.fromLocalFile(root_path).toString(QUrl.FullyEncoded) quteproc.open_url(urlstr) - page = dir_layout.parse(quteproc) + page = parse(quteproc) assert page.parent is None @@ -169,7 +171,7 @@ def test_enter_folder_smoke(dir_layout, quteproc): quteproc.send_cmd(':hint all normal') # a is the parent link, s is the first listed folder/file quteproc.send_cmd(':follow-hint s') - page = dir_layout.parse(quteproc) + page = parse(quteproc) assert page.path == dir_layout.path('folder0') @@ -183,7 +185,7 @@ def test_enter_folder(dir_layout, quteproc, folder): 'XPathResult.ANY_TYPE, null).iterateNext().click()' .format(folder) ) - page = dir_layout.parse(quteproc) + page = parse(quteproc) assert page.path == dir_layout.path(folder) assert page.parent == dir_layout.path() folders, files = DirLayout.get_folder_content(folder) From b6c5ff25fdaa8a37ad777def442eb2e28564d326 Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Tue, 29 Mar 2016 13:18:10 +0200 Subject: [PATCH 12/16] tests: add click_element to quteprocess --- tests/integration/quteprocess.py | 33 +++++++++++++++++++++++++--- tests/integration/test_dirbrowser.py | 8 +------ 2 files changed, 31 insertions(+), 10 deletions(-) diff --git a/tests/integration/quteprocess.py b/tests/integration/quteprocess.py index d41356f3e..1cce4fb1a 100644 --- a/tests/integration/quteprocess.py +++ b/tests/integration/quteprocess.py @@ -37,6 +37,7 @@ from PyQt5.QtCore import pyqtSignal, QUrl import testprocess from qutebrowser.misc import ipc from qutebrowser.utils import log, utils +from qutebrowser.browser import webelem from helpers import utils as testutils @@ -253,9 +254,13 @@ class QuteProc(testprocess.Process): path if path != '/' else '') def wait_for_js(self, message): - """Wait for the given javascript console message.""" - self.wait_for(category='js', function='javaScriptConsoleMessage', - message='[*] {}'.format(message)) + """Wait for the given javascript console message. + + Return: + The LogLine. + """ + return self.wait_for(category='js', function='javaScriptConsoleMessage', + message='[*] {}'.format(message)) def _is_error_logline(self, msg): """Check if the given LogLine is some kind of error message.""" @@ -422,6 +427,28 @@ class QuteProc(testprocess.Process): """Press the given keys using :fake-key.""" self.send_cmd(':fake-key -g "{}"'.format(keys)) + def click_element(self, text): + """Click the element with the given text.""" + # Use Javascript and XPath to find the right element, use console.log to + # return an error (no element found, ambiguous element) + script = ( + 'var _es = document.evaluate(\'//*[text()="{text}"]\', document, ' + 'null, XPathResult.ORDERED_NODE_SNAPSHOT_TYPE, null);' + 'if (_es.snapshotLength == 0) {{ console.log("qute:no elems"); }} ' + 'else if (_es.snapshotLength > 1) {{ console.log("qute:ambiguous ' + 'elems") }} ' + 'else {{ console.log("qute:okay"); _es.snapshotItem(0).click() }}' + ).format(text=webelem.javascript_escape(text)) + self.send_cmd(':jseval ' + script) + message = self.wait_for_js('qute:*').message + if message.endswith('qute:no elems'): + raise ValueError('No element with {!r} found'.format(text)) + elif message.endswith('qute:ambiguous elems'): + raise ValueError('Element with {!r} is not unique'.format(text)) + elif not message.endswith('qute:okay'): + raise ValueError('Invalid response from qutebrowser: {}' + .format(message)) + @pytest.yield_fixture(scope='module') def quteproc_process(qapp, httpbin, request): diff --git a/tests/integration/test_dirbrowser.py b/tests/integration/test_dirbrowser.py index 6265c2998..452dfbb4b 100644 --- a/tests/integration/test_dirbrowser.py +++ b/tests/integration/test_dirbrowser.py @@ -178,13 +178,7 @@ def test_enter_folder_smoke(dir_layout, quteproc): @pytest.mark.parametrize('folder', DirLayout.layout_folders()) def test_enter_folder(dir_layout, quteproc, folder): quteproc.open_url(dir_layout.file_url()) - # Use Javascript and XPath to click the link that has the folder name as - # text. - quteproc.send_cmd( - ':jseval document.evaluate(\'//a[text()="{}"]\', document, null, ' - 'XPathResult.ANY_TYPE, null).iterateNext().click()' - .format(folder) - ) + quteproc.click_element(text=folder) page = parse(quteproc) assert page.path == dir_layout.path(folder) assert page.parent == dir_layout.path() From f82d0f0c94434c46112239fd966f204550b649c8 Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Tue, 29 Mar 2016 20:34:40 +0200 Subject: [PATCH 13/16] quteprocess: properly escape xpath expression Since XPath doesn't have a way to escape quotes (or any other character), we have to use a workaround by using concat() and switching between quoting styles. --- tests/integration/quteprocess.py | 33 ++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/tests/integration/quteprocess.py b/tests/integration/quteprocess.py index 1cce4fb1a..e3f7ec882 100644 --- a/tests/integration/quteprocess.py +++ b/tests/integration/quteprocess.py @@ -432,13 +432,13 @@ class QuteProc(testprocess.Process): # Use Javascript and XPath to find the right element, use console.log to # return an error (no element found, ambiguous element) script = ( - 'var _es = document.evaluate(\'//*[text()="{text}"]\', document, ' + 'var _es = document.evaluate(\'//*[text()={text}]\', document, ' 'null, XPathResult.ORDERED_NODE_SNAPSHOT_TYPE, null);' 'if (_es.snapshotLength == 0) {{ console.log("qute:no elems"); }} ' 'else if (_es.snapshotLength > 1) {{ console.log("qute:ambiguous ' 'elems") }} ' 'else {{ console.log("qute:okay"); _es.snapshotItem(0).click() }}' - ).format(text=webelem.javascript_escape(text)) + ).format(text=webelem.javascript_escape(_xpath_escape(text))) self.send_cmd(':jseval ' + script) message = self.wait_for_js('qute:*').message if message.endswith('qute:no elems'): @@ -450,6 +450,35 @@ class QuteProc(testprocess.Process): .format(message)) +def _xpath_escape(text): + """Escape a string to be used in an XPath expression. + + The resulting string should still be escaped with javascript_escape, to + prevent javascript from interpreting the quotes. + + This function is needed because XPath does not provide any character + escaping mechanisms, so to get the string + "I'm back", he said + you have to use concat like + concat('"I', "'m back", '", he said') + + Args: + text: Text to escape + + Return: + The string "escaped" as a concat() call. + """ + # Shortcut if at most a single quoting style is used + if not "'" in text or not '"' in text: + return repr(text) + parts = re.split('([\'"])', text) + # Python's repr() of strings will automatically choose the right quote type. + # Since each part only contains one "type" of quote, no escaping should be + # necessary. + parts = [repr(part) for part in parts if part] + return 'concat({})'.format(', '.join(parts)) + + @pytest.yield_fixture(scope='module') def quteproc_process(qapp, httpbin, request): """Fixture for qutebrowser process which is started once per file.""" From 3007fbf5c2c6e7233bcfdcf0e68aef7c6d7f4eb8 Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Tue, 29 Mar 2016 20:45:15 +0200 Subject: [PATCH 14/16] tests: add tests for quteproc.click_element --- tests/integration/data/click_element.html | 11 +++++++ tests/integration/test_quteprocess.py | 36 +++++++++++++++++++++++ 2 files changed, 47 insertions(+) create mode 100644 tests/integration/data/click_element.html diff --git a/tests/integration/data/click_element.html b/tests/integration/data/click_element.html new file mode 100644 index 000000000..9e5ce5bb5 --- /dev/null +++ b/tests/integration/data/click_element.html @@ -0,0 +1,11 @@ + + + quteprocess.click_element test + + + Test Element + "Don't", he shouted + Duplicate + Duplicate + + diff --git a/tests/integration/test_quteprocess.py b/tests/integration/test_quteprocess.py index 872ec73de..4cf537a71 100644 --- a/tests/integration/test_quteprocess.py +++ b/tests/integration/test_quteprocess.py @@ -158,3 +158,39 @@ def test_log_line_parse(data, attrs): def test_log_line_no_match(): with pytest.raises(testprocess.InvalidLine): quteprocess.LogLine("Hello World!") + + +class TestClickElement: + + @pytest.fixture(autouse=True) + def open_page(self, quteproc): + quteproc.open_path('data/click_element.html') + quteproc.wait_for_load_finished('data/click_element.html') + + def test_click_element(self, quteproc): + quteproc.click_element('Test Element') + quteproc.wait_for_js('click_element clicked') + + def test_click_special_chars(self, quteproc): + quteproc.click_element('"Don\'t", he shouted') + quteproc.wait_for_js('click_element special chars') + + def test_duplicate(self, quteproc): + with pytest.raises(ValueError) as excinfo: + quteproc.click_element('Duplicate') + assert 'not unique' in str(excinfo.value) + + def test_nonexistent(self, quteproc): + with pytest.raises(ValueError) as excinfo: + quteproc.click_element('no element exists with this text') + assert 'No element' in str(excinfo.value) + + +@pytest.mark.parametrize('string, expected', [ + ('Test', "'Test'"), + ("Don't", '"Don\'t"'), + # This is some serious string escaping madness + ('"Don\'t", he said', "concat('\"', 'Don', \"'\", 't', '\"', ', he said')"), +]) +def test_xpath_escape(string, expected): + assert quteprocess._xpath_escape(string) == expected From bd5b1f207d24043b918433aaa3f00e499a4ceca2 Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Tue, 29 Mar 2016 21:02:54 +0200 Subject: [PATCH 15/16] fix lint --- tests/integration/quteprocess.py | 15 ++++++++------- tests/integration/test_quteprocess.py | 3 ++- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/tests/integration/quteprocess.py b/tests/integration/quteprocess.py index e3f7ec882..6154cfa5f 100644 --- a/tests/integration/quteprocess.py +++ b/tests/integration/quteprocess.py @@ -259,7 +259,8 @@ class QuteProc(testprocess.Process): Return: The LogLine. """ - return self.wait_for(category='js', function='javaScriptConsoleMessage', + return self.wait_for(category='js', + function='javaScriptConsoleMessage', message='[*] {}'.format(message)) def _is_error_logline(self, msg): @@ -429,8 +430,8 @@ class QuteProc(testprocess.Process): def click_element(self, text): """Click the element with the given text.""" - # Use Javascript and XPath to find the right element, use console.log to - # return an error (no element found, ambiguous element) + # Use Javascript and XPath to find the right element, use console.log + # to return an error (no element found, ambiguous element) script = ( 'var _es = document.evaluate(\'//*[text()={text}]\', document, ' 'null, XPathResult.ORDERED_NODE_SNAPSHOT_TYPE, null);' @@ -469,12 +470,12 @@ def _xpath_escape(text): The string "escaped" as a concat() call. """ # Shortcut if at most a single quoting style is used - if not "'" in text or not '"' in text: + if "'" not in text or '"' not in text: return repr(text) parts = re.split('([\'"])', text) - # Python's repr() of strings will automatically choose the right quote type. - # Since each part only contains one "type" of quote, no escaping should be - # necessary. + # Python's repr() of strings will automatically choose the right quote + # type. Since each part only contains one "type" of quote, no escaping + # should be necessary. parts = [repr(part) for part in parts if part] return 'concat({})'.format(', '.join(parts)) diff --git a/tests/integration/test_quteprocess.py b/tests/integration/test_quteprocess.py index 4cf537a71..7698e1a07 100644 --- a/tests/integration/test_quteprocess.py +++ b/tests/integration/test_quteprocess.py @@ -190,7 +190,8 @@ class TestClickElement: ('Test', "'Test'"), ("Don't", '"Don\'t"'), # This is some serious string escaping madness - ('"Don\'t", he said', "concat('\"', 'Don', \"'\", 't', '\"', ', he said')"), + ('"Don\'t", he said', + "concat('\"', 'Don', \"'\", 't', '\"', ', he said')"), ]) def test_xpath_escape(string, expected): assert quteprocess._xpath_escape(string) == expected From 8a1c99d3ff63f57086c9f9cf2f50d486e7587ddc Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 29 Mar 2016 22:38:50 +0200 Subject: [PATCH 16/16] Update changelog --- CHANGELOG.asciidoc | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 59d28aa08..43ce49669 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -65,6 +65,7 @@ Fixed - Fixed ugly UI fonts on Windows when Liberation Mono is installed - Fixed crash when unbinding key from a section which doesn't exist in the config - Fixed report window after a segfault +- Fixed some directory browser issues on Windows v0.5.1 ------