From 8a9b98c2dc2b9c55808bcd2274b4039ede87b3a8 Mon Sep 17 00:00:00 2001 From: Ryan Roden-Corrent Date: Mon, 18 Dec 2017 14:42:05 -0500 Subject: [PATCH 01/15] Editor triggers update on every save. For any command that spawns an editor, tirgger an update on save, not just on exit. - :open-editor writes the text field on save - :edit-url navigates on save - :edit-url -t opens a new tab on each save - :edit-command updates the statusbar text on save - :edit-command --run runs a command on each save - :config-edit reloads the config on save Resolves #2307. Helps mitigate #1596 by allowing users to 'save' partial work, and notice if there was an error without closing the editor. --- qutebrowser/misc/editor.py | 39 +++++++++++++---------- tests/end2end/features/editor.feature | 14 +++++++- tests/end2end/features/test_editor_bdd.py | 27 +++++++++++++--- 3 files changed, 58 insertions(+), 22 deletions(-) diff --git a/qutebrowser/misc/editor.py b/qutebrowser/misc/editor.py index 553c700e7..62d4a2d6f 100644 --- a/qutebrowser/misc/editor.py +++ b/qutebrowser/misc/editor.py @@ -22,7 +22,8 @@ import os import tempfile -from PyQt5.QtCore import pyqtSignal, pyqtSlot, QObject, QProcess +from PyQt5.QtCore import (pyqtSignal, pyqtSlot, QObject, QProcess, + QFileSystemWatcher) from qutebrowser.config import config from qutebrowser.utils import message, log @@ -39,6 +40,7 @@ class ExternalEditor(QObject): _remove_file: Whether the file should be removed when the editor is closed. _proc: The GUIProcess of the editor. + _watcher: A QFileSystemWatcher to watch the edited file for changes. """ editing_finished = pyqtSignal(str) @@ -48,10 +50,12 @@ class ExternalEditor(QObject): self._filename = None self._proc = None self._remove_file = None + self._watcher = QFileSystemWatcher(parent=self) def _cleanup(self): """Clean up temporary files after the editor closed.""" assert self._remove_file is not None + self._watcher.fileChanged.disconnect() if self._filename is None or not self._remove_file: # Could not create initial file. return @@ -75,22 +79,7 @@ class ExternalEditor(QObject): # No error/cleanup here, since we already handle this in # on_proc_error. return - try: - if exitcode != 0: - return - encoding = config.val.editor.encoding - try: - with open(self._filename, 'r', encoding=encoding) as f: - text = f.read() - except OSError as e: - # NOTE: Do not replace this with "raise CommandError" as it's - # executed async. - message.error("Failed to read back edited file: {}".format(e)) - return - log.procs.debug("Read back: {}".format(text)) - self.editing_finished.emit(text) - finally: - self._cleanup() + self._cleanup() @pyqtSlot(QProcess.ProcessError) def on_proc_error(self, _err): @@ -128,6 +117,19 @@ class ExternalEditor(QObject): line, column = self._calc_line_and_column(text, caret_position) self._start_editor(line=line, column=column) + def _on_file_changed(self, path): + encoding = config.val.editor.encoding + try: + with open(path, 'r', encoding=encoding) as f: + text = f.read() + except OSError as e: + # NOTE: Do not replace this with "raise CommandError" as it's + # executed async. + message.error("Failed to read back edited file: {}".format(e)) + return + log.procs.debug("Read back: {}".format(text)) + self.editing_finished.emit(text) + def edit_file(self, filename): """Edit the file with the given filename.""" self._filename = filename @@ -147,6 +149,9 @@ class ExternalEditor(QObject): editor = config.val.editor.command executable = editor[0] + self._watcher.addPath(self._filename) + self._watcher.fileChanged.connect(self._on_file_changed) + args = [self._sub_placeholder(arg, line, column) for arg in editor[1:]] log.procs.debug("Calling \"{}\" with args {}".format(executable, args)) self._proc.start(executable, args) diff --git a/tests/end2end/features/editor.feature b/tests/end2end/features/editor.feature index d01e3d229..489fc7200 100644 --- a/tests/end2end/features/editor.feature +++ b/tests/end2end/features/editor.feature @@ -119,7 +119,7 @@ Feature: Opening external editors # There's no guarantee that the tab gets deleted... @posix @flaky Scenario: Spawning an editor and closing the tab - When I set up a fake editor that waits + When I set up a fake editor that writes "foobar" on save And I open data/editor.html And I run :click-element id qute-textarea And I wait for "Entering mode KeyMode.insert (reason: clicking input)" in the log @@ -129,6 +129,18 @@ Feature: Opening external editors And I kill the waiting editor Then the error "Edited element vanished" should be shown + # Could not get signals working on Windows + @posix + Scenario: Spawning an editor and saving + When I set up a fake editor that writes "foobar" on save + And I open data/editor.html + And I run :click-element id qute-textarea + And I wait for "Entering mode KeyMode.insert (reason: clicking input)" in the log + And I run :open-editor + And I save without exiting the editor + And I run :click-element id qute-button + Then the javascript message "text: foobar" should be logged + Scenario: Spawning an editor in caret mode When I set up a fake editor returning "foobar" And I open data/editor.html diff --git a/tests/end2end/features/test_editor_bdd.py b/tests/end2end/features/test_editor_bdd.py index 260197a46..18551271b 100644 --- a/tests/end2end/features/test_editor_bdd.py +++ b/tests/end2end/features/test_editor_bdd.py @@ -70,8 +70,9 @@ def set_up_editor_empty(quteproc, tmpdir): set_up_editor(quteproc, tmpdir, "") -@bdd.when(bdd.parsers.parse('I set up a fake editor that waits')) -def set_up_editor_wait(quteproc, tmpdir): +@bdd.when(bdd.parsers.parse('I set up a fake editor that writes "{text}" on ' + 'save')) +def set_up_editor_wait(quteproc, tmpdir, text): """Set up editor.command to a small python script inserting a text.""" assert not utils.is_windows pidfile = tmpdir / 'editor_pid' @@ -82,12 +83,19 @@ def set_up_editor_wait(quteproc, tmpdir): import time import signal + def handle(sig, frame): + with open(sys.argv[1], 'w', encoding='utf-8') as f: + f.write({text!r}) + if sig == signal.SIGUSR1: + sys.exit(0) + with open(r'{pidfile}', 'w') as f: f.write(str(os.getpid())) - signal.signal(signal.SIGUSR1, lambda s, f: sys.exit(0)) + signal.signal(signal.SIGUSR1, handle) + signal.signal(signal.SIGUSR2, handle) time.sleep(100) - """.format(pidfile=pidfile))) + """.format(pidfile=pidfile, text=text))) editor = json.dumps([sys.executable, str(script), '{}']) quteproc.set_setting('editor.command', editor) @@ -101,3 +109,14 @@ def kill_editor_wait(tmpdir): # for posix, there IS a member so we need to ignore useless-suppression # pylint: disable=no-member,useless-suppression os.kill(pid, signal.SIGUSR1) + + +@bdd.when(bdd.parsers.parse('I save without exiting the editor')) +def save_editor_wait(tmpdir): + """Trigger the waiting editor to write without exiting.""" + pidfile = tmpdir / 'editor_pid' + pid = int(pidfile.read()) + # windows has no SIGUSR2, but we don't run this on windows anyways + # for posix, there IS a member so we need to ignore useless-suppression + # pylint: disable=no-member,useless-suppression + os.kill(pid, signal.SIGUSR2) From 23eb6a6c534f397885e89f91fd48f641b807e10a Mon Sep 17 00:00:00 2001 From: Ryan Roden-Corrent Date: Fri, 29 Dec 2017 16:08:59 -0500 Subject: [PATCH 02/15] Fix test_editor for edit-on-write behavior. Now that the editor fires editing_finished on every write, the unit tests had to be updated. - Add qtbot to the editor fixture to resolve `QtWarningMsg: QSocketNotifier: Can only be used with threads started with QThread` - Use removePaths instead of disconnect to stop the watcher from signalling. This avoids an error when the editor is forcibly cleaned up by the tests without the signal ever being connected, but otherwise has the same behavior as disconnecting the singal. - wait for a signal on write instead of proc closed - wait for _watcher.fileChanged in test_unreadable to ensure the write event is fired before the test exits. --- qutebrowser/misc/editor.py | 2 +- tests/unit/misc/test_editor.py | 19 +++++++++++-------- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/qutebrowser/misc/editor.py b/qutebrowser/misc/editor.py index 62d4a2d6f..53067ca2c 100644 --- a/qutebrowser/misc/editor.py +++ b/qutebrowser/misc/editor.py @@ -55,7 +55,7 @@ class ExternalEditor(QObject): def _cleanup(self): """Clean up temporary files after the editor closed.""" assert self._remove_file is not None - self._watcher.fileChanged.disconnect() + self._watcher.removePaths(self._watcher.files()) if self._filename is None or not self._remove_file: # Could not create initial file. return diff --git a/tests/unit/misc/test_editor.py b/tests/unit/misc/test_editor.py index 7dea6e069..55425a1c4 100644 --- a/tests/unit/misc/test_editor.py +++ b/tests/unit/misc/test_editor.py @@ -37,7 +37,7 @@ def patch_things(config_stub, monkeypatch, stubs): @pytest.fixture -def editor(caplog): +def editor(caplog, qtbot): ed = editormod.ExternalEditor() yield ed with caplog.at_level(logging.ERROR): @@ -118,18 +118,21 @@ class TestFileHandling: os.remove(filename) - def test_unreadable(self, message_mock, editor, caplog): + def test_unreadable(self, message_mock, editor, caplog, qtbot): """Test file handling when closing with an unreadable file.""" editor.edit("") filename = editor._filename assert os.path.exists(filename) - os.chmod(filename, 0o077) + os.chmod(filename, 0o277) if os.access(filename, os.R_OK): # Docker container or similar pytest.skip("File was still readable") with caplog.at_level(logging.ERROR): - editor._proc.finished.emit(0, QProcess.NormalExit) + with qtbot.wait_signal(editor._watcher.fileChanged): + with open(filename, 'w', encoding='utf-8') as f: + f.write('unreadable') + editor._proc.finished.emit(0, QProcess.NormalExit) assert not os.path.exists(filename) msg = message_mock.getmsg(usertypes.MessageLevel.error) assert msg.text.startswith("Failed to read back edited file: ") @@ -170,11 +173,11 @@ def test_modify(qtbot, editor, initial_text, edited_text): with open(editor._filename, 'r', encoding='utf-8') as f: assert f.read() == initial_text - with open(editor._filename, 'w', encoding='utf-8') as f: - f.write(edited_text) - with qtbot.wait_signal(editor.editing_finished) as blocker: - editor._proc.finished.emit(0, QProcess.NormalExit) + with open(editor._filename, 'w', encoding='utf-8') as f: + f.write(edited_text) + + editor._proc.finished.emit(0, QProcess.NormalExit) assert blocker.args == [edited_text] From a940de3717d167cd7c13ce5429fd58d6b23d74d7 Mon Sep 17 00:00:00 2001 From: Ryan Roden-Corrent Date: Fri, 29 Dec 2017 22:05:04 -0500 Subject: [PATCH 03/15] Rename editing_finished to file_updated. ExternalEditor now fires an event on save rather than on exit, so the signal name should be updated to match the behavior. --- qutebrowser/browser/commands.py | 8 ++++---- qutebrowser/config/configcommands.py | 4 ++-- qutebrowser/mainwindow/statusbar/command.py | 2 +- qutebrowser/misc/editor.py | 4 ++-- tests/unit/misc/test_editor.py | 2 +- 5 files changed, 10 insertions(+), 10 deletions(-) diff --git a/qutebrowser/browser/commands.py b/qutebrowser/browser/commands.py index fdd552b32..55af2c50a 100644 --- a/qutebrowser/browser/commands.py +++ b/qutebrowser/browser/commands.py @@ -1622,8 +1622,8 @@ class CommandDispatcher: caret_position = elem.caret_position() ed = editor.ExternalEditor(self._tabbed_browser) - ed.editing_finished.connect(functools.partial( - self.on_editing_finished, elem)) + ed.file_updated.connect(functools.partial( + self.on_file_updated, elem)) ed.edit(text, caret_position) @cmdutils.register(instance='command-dispatcher', scope='window') @@ -1636,7 +1636,7 @@ class CommandDispatcher: tab = self._current_widget() tab.elements.find_focused(self._open_editor_cb) - def on_editing_finished(self, elem, text): + def on_file_updated(self, elem, text): """Write the editor text into the form field and clean up tempfile. Callback for GUIProcess when the editor was closed. @@ -2146,7 +2146,7 @@ class CommandDispatcher: ed = editor.ExternalEditor(self._tabbed_browser) # Passthrough for openurl args (e.g. -t, -b, -w) - ed.editing_finished.connect(functools.partial( + ed.file_updated.connect(functools.partial( self._open_if_changed, old_url=old_url, bg=bg, tab=tab, window=window, private=private, related=related)) diff --git a/qutebrowser/config/configcommands.py b/qutebrowser/config/configcommands.py index 93a1f7ce0..faf31d718 100644 --- a/qutebrowser/config/configcommands.py +++ b/qutebrowser/config/configcommands.py @@ -253,7 +253,7 @@ class ConfigCommands: Args: no_source: Don't re-source the config file after editing. """ - def on_editing_finished(): + def on_file_updated(): """Source the new config when editing finished. This can't use cmdexc.CommandError as it's run async. @@ -265,7 +265,7 @@ class ConfigCommands: ed = editor.ExternalEditor(self._config) if not no_source: - ed.editing_finished.connect(on_editing_finished) + ed.file_updated.connect(on_file_updated) filename = os.path.join(standarddir.config(), 'config.py') ed.edit_file(filename) diff --git a/qutebrowser/mainwindow/statusbar/command.py b/qutebrowser/mainwindow/statusbar/command.py index 26ba802c0..6689d3bf5 100644 --- a/qutebrowser/mainwindow/statusbar/command.py +++ b/qutebrowser/mainwindow/statusbar/command.py @@ -193,7 +193,7 @@ class Command(misc.MinimalLineEditMixin, misc.CommandLineEdit): if run: self.command_accept() - ed.editing_finished.connect(callback) + ed.file_updated.connect(callback) ed.edit(self.text()) @pyqtSlot(usertypes.KeyMode) diff --git a/qutebrowser/misc/editor.py b/qutebrowser/misc/editor.py index 53067ca2c..f300bc2e1 100644 --- a/qutebrowser/misc/editor.py +++ b/qutebrowser/misc/editor.py @@ -43,7 +43,7 @@ class ExternalEditor(QObject): _watcher: A QFileSystemWatcher to watch the edited file for changes. """ - editing_finished = pyqtSignal(str) + file_updated = pyqtSignal(str) def __init__(self, parent=None): super().__init__(parent) @@ -128,7 +128,7 @@ class ExternalEditor(QObject): message.error("Failed to read back edited file: {}".format(e)) return log.procs.debug("Read back: {}".format(text)) - self.editing_finished.emit(text) + self.file_updated.emit(text) def edit_file(self, filename): """Edit the file with the given filename.""" diff --git a/tests/unit/misc/test_editor.py b/tests/unit/misc/test_editor.py index 55425a1c4..b0ddbff25 100644 --- a/tests/unit/misc/test_editor.py +++ b/tests/unit/misc/test_editor.py @@ -173,7 +173,7 @@ def test_modify(qtbot, editor, initial_text, edited_text): with open(editor._filename, 'r', encoding='utf-8') as f: assert f.read() == initial_text - with qtbot.wait_signal(editor.editing_finished) as blocker: + with qtbot.wait_signal(editor.file_updated) as blocker: with open(editor._filename, 'w', encoding='utf-8') as f: f.write(edited_text) From 2e5595b5c6868f22e820eea12900821601aa9061 Mon Sep 17 00:00:00 2001 From: Ryan Roden-Corrent Date: Fri, 29 Dec 2017 22:07:37 -0500 Subject: [PATCH 04/15] Update test_configcommands for new editor behavior. Now that the editor signals on save, the configcommands editing unittests need to emit the signal in the patch rather than relying on on_proc_closed to emit the signal. --- tests/unit/config/test_configcommands.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/config/test_configcommands.py b/tests/unit/config/test_configcommands.py index 92796e8e5..a376e19b9 100644 --- a/tests/unit/config/test_configcommands.py +++ b/tests/unit/config/test_configcommands.py @@ -330,7 +330,7 @@ class TestEdit: def _write_file(editor_self): with open(editor_self._filename, 'w', encoding='utf-8') as f: f.write(text) - editor_self.on_proc_closed(0, QProcess.NormalExit) + editor_self.file_updated.emit(text) return mocker.patch('qutebrowser.config.configcommands.editor.' 'ExternalEditor._start_editor', autospec=True, From 7c33ff4046c6566038bb61d7007291b2d1538d3b Mon Sep 17 00:00:00 2001 From: Ryan Roden-Corrent Date: Wed, 24 Jan 2018 07:40:34 -0500 Subject: [PATCH 05/15] Fix flaky editor test. Give the process time to write its PID before trying to interrupt it. --- tests/end2end/features/test_editor_bdd.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/end2end/features/test_editor_bdd.py b/tests/end2end/features/test_editor_bdd.py index 18551271b..e2415da46 100644 --- a/tests/end2end/features/test_editor_bdd.py +++ b/tests/end2end/features/test_editor_bdd.py @@ -22,6 +22,7 @@ import json import textwrap import os import signal +import time import pytest_bdd as bdd bdd.scenarios('editor.feature') @@ -115,6 +116,11 @@ def kill_editor_wait(tmpdir): def save_editor_wait(tmpdir): """Trigger the waiting editor to write without exiting.""" pidfile = tmpdir / 'editor_pid' + # give the "editor" process time to write its pid + for _ in range(10): + if pidfile.check(): + break + time.sleep(1) pid = int(pidfile.read()) # windows has no SIGUSR2, but we don't run this on windows anyways # for posix, there IS a member so we need to ignore useless-suppression From 530a1859a331d49d67fa71509063b8cb384a5e4c Mon Sep 17 00:00:00 2001 From: Ryan Roden-Corrent Date: Fri, 26 Jan 2018 11:12:07 -0500 Subject: [PATCH 06/15] Trigger editor signal on exit if content changed. With the previous code, the editor could miss the final signal on a save-and-exit. This is avoided by always running the file changed handler on a successful exit, but only firing the signal if the content actually changed (to avoid double-signalling). --- qutebrowser/misc/editor.py | 8 +++++++- tests/unit/misc/test_editor.py | 26 +++++++++++++++++++++----- 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/qutebrowser/misc/editor.py b/qutebrowser/misc/editor.py index f300bc2e1..6752a37e3 100644 --- a/qutebrowser/misc/editor.py +++ b/qutebrowser/misc/editor.py @@ -51,6 +51,7 @@ class ExternalEditor(QObject): self._proc = None self._remove_file = None self._watcher = QFileSystemWatcher(parent=self) + self._content = None def _cleanup(self): """Clean up temporary files after the editor closed.""" @@ -79,6 +80,8 @@ class ExternalEditor(QObject): # No error/cleanup here, since we already handle this in # on_proc_error. return + # do a final read to make sure we don't miss the last signal + self._on_file_changed(self._filename) self._cleanup() @pyqtSlot(QProcess.ProcessError) @@ -128,7 +131,10 @@ class ExternalEditor(QObject): message.error("Failed to read back edited file: {}".format(e)) return log.procs.debug("Read back: {}".format(text)) - self.file_updated.emit(text) + if self._content != text: + self._content = text + self.file_updated.emit(text) + def edit_file(self, filename): """Edit the file with the given filename.""" diff --git a/tests/unit/misc/test_editor.py b/tests/unit/misc/test_editor.py index b0ddbff25..3f7d8acaf 100644 --- a/tests/unit/misc/test_editor.py +++ b/tests/unit/misc/test_editor.py @@ -129,10 +129,7 @@ class TestFileHandling: pytest.skip("File was still readable") with caplog.at_level(logging.ERROR): - with qtbot.wait_signal(editor._watcher.fileChanged): - with open(filename, 'w', encoding='utf-8') as f: - f.write('unreadable') - editor._proc.finished.emit(0, QProcess.NormalExit) + editor._proc.finished.emit(0, QProcess.NormalExit) assert not os.path.exists(filename) msg = message_mock.getmsg(usertypes.MessageLevel.error) assert msg.text.startswith("Failed to read back edited file: ") @@ -177,11 +174,30 @@ def test_modify(qtbot, editor, initial_text, edited_text): with open(editor._filename, 'w', encoding='utf-8') as f: f.write(edited_text) - editor._proc.finished.emit(0, QProcess.NormalExit) + with qtbot.assert_not_emitted(editor.file_updated): + editor._proc.finished.emit(0, QProcess.NormalExit) assert blocker.args == [edited_text] +def test_modify_multiple(qtbot, editor): + """Test that multiple saves all trigger file_updated.""" + editor.edit('foo') + + with qtbot.wait_signal(editor.file_updated) as blocker: + with open(editor._filename, 'w', encoding='utf-8') as f: + f.write('bar') + assert blocker.args == ['bar'] + + with qtbot.wait_signal(editor.file_updated) as blocker: + with open(editor._filename, 'w', encoding='utf-8') as f: + f.write('baz') + assert blocker.args == ['baz'] + + with qtbot.assert_not_emitted(editor.file_updated): + editor._proc.finished.emit(0, QProcess.NormalExit) + + @pytest.mark.parametrize('text, caret_position, result', [ ('', 0, (1, 1)), ('a', 0, (1, 1)), From e9023ce2336cc68cce084893534ff2a27f995322 Mon Sep 17 00:00:00 2001 From: Ryan Roden-Corrent Date: Mon, 29 Jan 2018 07:50:32 -0500 Subject: [PATCH 07/15] Remove newline in editor.py --- qutebrowser/misc/editor.py | 1 - 1 file changed, 1 deletion(-) diff --git a/qutebrowser/misc/editor.py b/qutebrowser/misc/editor.py index 6752a37e3..70bd4e68a 100644 --- a/qutebrowser/misc/editor.py +++ b/qutebrowser/misc/editor.py @@ -135,7 +135,6 @@ class ExternalEditor(QObject): self._content = text self.file_updated.emit(text) - def edit_file(self, filename): """Edit the file with the given filename.""" self._filename = filename From eab9b70f2826abd8f0b7668196d0807b3988852f Mon Sep 17 00:00:00 2001 From: Ryan Roden-Corrent Date: Thu, 1 Feb 2018 20:43:35 -0500 Subject: [PATCH 08/15] Fix pylint for editor.py. Notate unused parameter. --- qutebrowser/misc/editor.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qutebrowser/misc/editor.py b/qutebrowser/misc/editor.py index 70bd4e68a..b6707f886 100644 --- a/qutebrowser/misc/editor.py +++ b/qutebrowser/misc/editor.py @@ -70,7 +70,7 @@ class ExternalEditor(QObject): message.error("Failed to delete tempfile... ({})".format(e)) @pyqtSlot(int, QProcess.ExitStatus) - def on_proc_closed(self, exitcode, exitstatus): + def on_proc_closed(self, _exitcode, exitstatus): """Write the editor text into the form field and clean up tempfile. Callback for QProcess when the editor was closed. From 0aefffce4dab9afd99c278558af605d626441ca8 Mon Sep 17 00:00:00 2001 From: Ryan Roden-Corrent Date: Thu, 1 Feb 2018 20:55:18 -0500 Subject: [PATCH 09/15] Attempt to solve flaky editor tests. These are passing locally but failing in travis. This fixes two possible timing issues: - Ensure the signals are set up befor the pidfile is written. The function that sends the signal waits for the pidfile to exist, so this ensures we don't miss a signal. - Wait for the log message indicating that the editor file was read back, so the test doesn't run through before we get a chance to read from the editor. --- tests/end2end/features/editor.feature | 1 + tests/end2end/features/test_editor_bdd.py | 5 +++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/end2end/features/editor.feature b/tests/end2end/features/editor.feature index 489fc7200..391e88749 100644 --- a/tests/end2end/features/editor.feature +++ b/tests/end2end/features/editor.feature @@ -138,6 +138,7 @@ Feature: Opening external editors And I wait for "Entering mode KeyMode.insert (reason: clicking input)" in the log And I run :open-editor And I save without exiting the editor + And I wait for "Read back: foobar" in the log And I run :click-element id qute-button Then the javascript message "text: foobar" should be logged diff --git a/tests/end2end/features/test_editor_bdd.py b/tests/end2end/features/test_editor_bdd.py index e2415da46..4e480b85d 100644 --- a/tests/end2end/features/test_editor_bdd.py +++ b/tests/end2end/features/test_editor_bdd.py @@ -90,11 +90,12 @@ def set_up_editor_wait(quteproc, tmpdir, text): if sig == signal.SIGUSR1: sys.exit(0) + signal.signal(signal.SIGUSR1, handle) + signal.signal(signal.SIGUSR2, handle) + with open(r'{pidfile}', 'w') as f: f.write(str(os.getpid())) - signal.signal(signal.SIGUSR1, handle) - signal.signal(signal.SIGUSR2, handle) time.sleep(100) """.format(pidfile=pidfile, text=text))) editor = json.dumps([sys.executable, str(script), '{}']) From ceab4a4c1f6f650089d6d4267d67fb6094d506ef Mon Sep 17 00:00:00 2001 From: Ryan Roden-Corrent Date: Sat, 3 Feb 2018 08:12:45 -0500 Subject: [PATCH 10/15] Fix pylint warnings --- qutebrowser/misc/editor.py | 2 +- tests/unit/config/test_configcommands.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/qutebrowser/misc/editor.py b/qutebrowser/misc/editor.py index b6707f886..b9a3ed97f 100644 --- a/qutebrowser/misc/editor.py +++ b/qutebrowser/misc/editor.py @@ -23,7 +23,7 @@ import os import tempfile from PyQt5.QtCore import (pyqtSignal, pyqtSlot, QObject, QProcess, - QFileSystemWatcher) + QFileSystemWatcher) from qutebrowser.config import config from qutebrowser.utils import message, log diff --git a/tests/unit/config/test_configcommands.py b/tests/unit/config/test_configcommands.py index a376e19b9..1f1637ed3 100644 --- a/tests/unit/config/test_configcommands.py +++ b/tests/unit/config/test_configcommands.py @@ -22,7 +22,7 @@ import logging import unittest.mock import pytest -from PyQt5.QtCore import QUrl, QProcess +from PyQt5.QtCore import QUrl from qutebrowser.config import configcommands from qutebrowser.commands import cmdexc From 833df95485e9cf9516d7869315808e55bcc3a16d Mon Sep 17 00:00:00 2001 From: Ryan Roden-Corrent Date: Sat, 3 Feb 2018 08:27:36 -0500 Subject: [PATCH 11/15] Only detect save for open-editor and config-edit. Scope down the new trigger-on-save behavior to only open-editor and config-edit. Other uses of the editor such as edit-url and edit-command will behave as before. --- qutebrowser/browser/commands.py | 2 +- qutebrowser/config/configcommands.py | 2 +- qutebrowser/misc/editor.py | 16 +++++++++++----- tests/unit/misc/test_editor.py | 12 ++++++------ 4 files changed, 19 insertions(+), 13 deletions(-) diff --git a/qutebrowser/browser/commands.py b/qutebrowser/browser/commands.py index 55af2c50a..094563bcc 100644 --- a/qutebrowser/browser/commands.py +++ b/qutebrowser/browser/commands.py @@ -1621,7 +1621,7 @@ class CommandDispatcher: caret_position = elem.caret_position() - ed = editor.ExternalEditor(self._tabbed_browser) + ed = editor.ExternalEditor(watch=True, parent=self._tabbed_browser) ed.file_updated.connect(functools.partial( self.on_file_updated, elem)) ed.edit(text, caret_position) diff --git a/qutebrowser/config/configcommands.py b/qutebrowser/config/configcommands.py index faf31d718..1a07b2020 100644 --- a/qutebrowser/config/configcommands.py +++ b/qutebrowser/config/configcommands.py @@ -263,7 +263,7 @@ class ConfigCommands: except configexc.ConfigFileErrors as e: message.error(str(e)) - ed = editor.ExternalEditor(self._config) + ed = editor.ExternalEditor(watch=True, parent=self._config) if not no_source: ed.file_updated.connect(on_file_updated) diff --git a/qutebrowser/misc/editor.py b/qutebrowser/misc/editor.py index b9a3ed97f..9565ce28b 100644 --- a/qutebrowser/misc/editor.py +++ b/qutebrowser/misc/editor.py @@ -41,22 +41,27 @@ class ExternalEditor(QObject): closed. _proc: The GUIProcess of the editor. _watcher: A QFileSystemWatcher to watch the edited file for changes. + Only set if watch=True. """ file_updated = pyqtSignal(str) - def __init__(self, parent=None): + def __init__(self, parent=None, watch=False): super().__init__(parent) self._filename = None self._proc = None self._remove_file = None - self._watcher = QFileSystemWatcher(parent=self) + if watch: + self._watcher = QFileSystemWatcher(parent=self) + else: + self._watcher = None self._content = None def _cleanup(self): """Clean up temporary files after the editor closed.""" assert self._remove_file is not None - self._watcher.removePaths(self._watcher.files()) + if self._watcher: + self._watcher.removePaths(self._watcher.files()) if self._filename is None or not self._remove_file: # Could not create initial file. return @@ -154,8 +159,9 @@ class ExternalEditor(QObject): editor = config.val.editor.command executable = editor[0] - self._watcher.addPath(self._filename) - self._watcher.fileChanged.connect(self._on_file_changed) + if self._watcher: + self._watcher.addPath(self._filename) + self._watcher.fileChanged.connect(self._on_file_changed) args = [self._sub_placeholder(arg, line, column) for arg in editor[1:]] log.procs.debug("Calling \"{}\" with args {}".format(executable, args)) diff --git a/tests/unit/misc/test_editor.py b/tests/unit/misc/test_editor.py index 3f7d8acaf..37506d923 100644 --- a/tests/unit/misc/test_editor.py +++ b/tests/unit/misc/test_editor.py @@ -170,18 +170,18 @@ def test_modify(qtbot, editor, initial_text, edited_text): with open(editor._filename, 'r', encoding='utf-8') as f: assert f.read() == initial_text - with qtbot.wait_signal(editor.file_updated) as blocker: - with open(editor._filename, 'w', encoding='utf-8') as f: - f.write(edited_text) + with open(editor._filename, 'w', encoding='utf-8') as f: + f.write(edited_text) - with qtbot.assert_not_emitted(editor.file_updated): + with qtbot.wait_signal(editor.file_updated) as blocker: editor._proc.finished.emit(0, QProcess.NormalExit) assert blocker.args == [edited_text] -def test_modify_multiple(qtbot, editor): - """Test that multiple saves all trigger file_updated.""" +def test_modify_watch(qtbot): + """Test that saving triggers file_updated when watch=True.""" + editor = editormod.ExternalEditor(watch=True) editor.edit('foo') with qtbot.wait_signal(editor.file_updated) as blocker: From a8733d7228f47ee7f24982deb68934902992f2f4 Mon Sep 17 00:00:00 2001 From: Ryan Roden-Corrent Date: Sun, 4 Feb 2018 07:02:25 -0500 Subject: [PATCH 12/15] Increase timeout in test_editor. The test with watch=True was failing on the Travis OSX environment becausee it was timing out before the file_updated signal was fired. --- tests/unit/misc/test_editor.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/misc/test_editor.py b/tests/unit/misc/test_editor.py index 37506d923..58f6edae7 100644 --- a/tests/unit/misc/test_editor.py +++ b/tests/unit/misc/test_editor.py @@ -184,7 +184,7 @@ def test_modify_watch(qtbot): editor = editormod.ExternalEditor(watch=True) editor.edit('foo') - with qtbot.wait_signal(editor.file_updated) as blocker: + with qtbot.wait_signal(editor.file_updated, timeout=3000) as blocker: with open(editor._filename, 'w', encoding='utf-8') as f: f.write('bar') assert blocker.args == ['bar'] From abd56a5abd21613697b1933624d2d23abe027bb4 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Wed, 7 Feb 2018 21:40:03 +0100 Subject: [PATCH 13/15] Wait until the mtime changed --- tests/unit/misc/test_editor.py | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/tests/unit/misc/test_editor.py b/tests/unit/misc/test_editor.py index 58f6edae7..3a60aaff7 100644 --- a/tests/unit/misc/test_editor.py +++ b/tests/unit/misc/test_editor.py @@ -19,6 +19,7 @@ """Tests for qutebrowser.misc.editor.""" +import time import os import os.path import logging @@ -179,19 +180,31 @@ def test_modify(qtbot, editor, initial_text, edited_text): assert blocker.args == [edited_text] +def _update_file(filename, contents): + """Update the given file and make sure its mtime changed. + + This might write the file multiple times, but different systems have + different mtime's, so we can't be sure how long to wait otherwise. + """ + old_mtime = new_mtime = os.stat(filename).st_mtime + while old_mtime == new_mtime: + time.sleep(0.1) + with open(filename, 'w', encoding='utf-8') as f: + f.write(contents) + new_mtime = os.stat(filename).st_mtime + + def test_modify_watch(qtbot): """Test that saving triggers file_updated when watch=True.""" editor = editormod.ExternalEditor(watch=True) editor.edit('foo') with qtbot.wait_signal(editor.file_updated, timeout=3000) as blocker: - with open(editor._filename, 'w', encoding='utf-8') as f: - f.write('bar') + _update_file(editor._filename, 'bar') assert blocker.args == ['bar'] with qtbot.wait_signal(editor.file_updated) as blocker: - with open(editor._filename, 'w', encoding='utf-8') as f: - f.write('baz') + _update_file(editor._filename, 'baz') assert blocker.args == ['baz'] with qtbot.assert_not_emitted(editor.file_updated): From 01ccbc679d05adf9fd193f1688d76c1170865a17 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Wed, 7 Feb 2018 22:26:32 +0100 Subject: [PATCH 14/15] Fix lint --- qutebrowser/browser/commands.py | 2 +- qutebrowser/misc/editor.py | 9 +++------ tests/end2end/features/test_editor_bdd.py | 2 +- 3 files changed, 5 insertions(+), 8 deletions(-) diff --git a/qutebrowser/browser/commands.py b/qutebrowser/browser/commands.py index 094563bcc..da5ccbc89 100644 --- a/qutebrowser/browser/commands.py +++ b/qutebrowser/browser/commands.py @@ -1639,7 +1639,7 @@ class CommandDispatcher: def on_file_updated(self, elem, text): """Write the editor text into the form field and clean up tempfile. - Callback for GUIProcess when the editor was closed. + Callback for GUIProcess when the edited text was updated. Args: elem: The WebElementWrapper which was modified. diff --git a/qutebrowser/misc/editor.py b/qutebrowser/misc/editor.py index 9565ce28b..db7d3e3e2 100644 --- a/qutebrowser/misc/editor.py +++ b/qutebrowser/misc/editor.py @@ -51,10 +51,7 @@ class ExternalEditor(QObject): self._filename = None self._proc = None self._remove_file = None - if watch: - self._watcher = QFileSystemWatcher(parent=self) - else: - self._watcher = None + self._watcher = QFileSystemWatcher(parent=self) if watch else None self._content = None def _cleanup(self): @@ -125,10 +122,10 @@ class ExternalEditor(QObject): line, column = self._calc_line_and_column(text, caret_position) self._start_editor(line=line, column=column) + @pyqtSlot(str) def _on_file_changed(self, path): - encoding = config.val.editor.encoding try: - with open(path, 'r', encoding=encoding) as f: + with open(path, 'r', encoding=config.val.editor.encoding) as f: text = f.read() except OSError as e: # NOTE: Do not replace this with "raise CommandError" as it's diff --git a/tests/end2end/features/test_editor_bdd.py b/tests/end2end/features/test_editor_bdd.py index 4e480b85d..12de7c92c 100644 --- a/tests/end2end/features/test_editor_bdd.py +++ b/tests/end2end/features/test_editor_bdd.py @@ -84,7 +84,7 @@ def set_up_editor_wait(quteproc, tmpdir, text): import time import signal - def handle(sig, frame): + def handle(sig, _frame): with open(sys.argv[1], 'w', encoding='utf-8') as f: f.write({text!r}) if sig == signal.SIGUSR1: From cd97d5e6e2b78c570f19eda91720b9d95b7eed81 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Wed, 7 Feb 2018 22:31:28 +0100 Subject: [PATCH 15/15] Update changelog --- doc/changelog.asciidoc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/doc/changelog.asciidoc b/doc/changelog.asciidoc index c9cb327b3..56f3a4b30 100644 --- a/doc/changelog.asciidoc +++ b/doc/changelog.asciidoc @@ -41,6 +41,8 @@ Changed a performance and stability improvement. - The `url.incdec_segments` option now also can take `port` as possible segment. - QtWebEngine: `:view-source` now uses Chromium's `view-source:` scheme. +- When an editor is spawned with `:open-editor` and `:config-edit`, the changes + are now applied as soon as the file is saved in the editor. Fixed ~~~~~