diff --git a/doc/changelog.asciidoc b/doc/changelog.asciidoc index 2262927de..ea9025280 100644 --- a/doc/changelog.asciidoc +++ b/doc/changelog.asciidoc @@ -45,6 +45,8 @@ Changed - The `url.incdec_segments` option now also can take `port` as possible segment. - QtWebEngine: `:view-source` now uses Chromium's `view-source:` scheme. - Tabs now show their full title as tooltip. +- 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 ~~~~~ diff --git a/qutebrowser/browser/commands.py b/qutebrowser/browser/commands.py index 7be7937a0..97f631df9 100644 --- a/qutebrowser/browser/commands.py +++ b/qutebrowser/browser/commands.py @@ -1616,9 +1616,9 @@ 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 = editor.ExternalEditor(watch=True, parent=self._tabbed_browser) + ed.file_updated.connect(functools.partial( + self.on_file_updated, elem)) ed.edit(text, caret_position) @cmdutils.register(instance='command-dispatcher', scope='window') @@ -1631,10 +1631,10 @@ 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. + Callback for GUIProcess when the edited text was updated. Args: elem: The WebElementWrapper which was modified. @@ -2141,7 +2141,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 a3a7d298a..8bc2a9ed8 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. @@ -263,9 +263,9 @@ 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.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 5f1451af3..681e20ac5 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 6b464724f..c562a1d67 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,19 +40,25 @@ 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. + Only set if watch=True. """ - editing_finished = pyqtSignal(str) + 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 else None + self._content = None def _cleanup(self): """Clean up temporary files after the editor closed.""" assert self._remove_file is not None + 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 @@ -65,7 +72,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. @@ -75,22 +82,9 @@ 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() + # 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) def on_proc_error(self, _err): @@ -128,6 +122,21 @@ 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): + try: + 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 + # executed async. + message.error("Failed to read back edited file: {}".format(e)) + return + log.procs.debug("Read back: {}".format(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.""" self._filename = filename @@ -147,6 +156,10 @@ class ExternalEditor(QObject): editor = config.val.editor.command executable = editor[0] + 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)) self._proc.start(executable, args) diff --git a/tests/end2end/features/editor.feature b/tests/end2end/features/editor.feature index d01e3d229..391e88749 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,19 @@ 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 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 + 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 24309d070..2f425c41b 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') @@ -70,8 +71,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 +84,20 @@ 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) + + 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, lambda s, f: sys.exit(0)) 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 +111,19 @@ 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' + # 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 + # pylint: disable=no-member,useless-suppression + os.kill(pid, signal.SIGUSR2) diff --git a/tests/unit/config/test_configcommands.py b/tests/unit/config/test_configcommands.py index f98f09ca8..c82195906 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 @@ -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, diff --git a/tests/unit/misc/test_editor.py b/tests/unit/misc/test_editor.py index e334b2645..c9842cc7a 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 @@ -37,7 +38,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,12 +119,12 @@ 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") @@ -173,12 +174,43 @@ def test_modify(qtbot, editor, initial_text, edited_text): with open(editor._filename, 'w', encoding='utf-8') as f: f.write(edited_text) - with qtbot.wait_signal(editor.editing_finished) as blocker: + with qtbot.wait_signal(editor.file_updated) as blocker: editor._proc.finished.emit(0, QProcess.NormalExit) 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: + _update_file(editor._filename, 'bar') + assert blocker.args == ['bar'] + + with qtbot.wait_signal(editor.file_updated) as blocker: + _update_file(editor._filename, '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)),