From 38bb3673db2215e0d88f56673b2c25893e286132 Mon Sep 17 00:00:00 2001 From: Ryan Roden-Corrent Date: Mon, 12 Mar 2018 08:34:50 -0400 Subject: [PATCH 1/3] Preserve a backup if editor callback fails. Currently the editor deletes its temp file whenever editing is finished. With this patch, the file will not be deleted if the editor callback encounters an exception. One example is if the tab containing the edited element is closed. The editor errors with "Edited element vanished", but with this patch it will also print "Backup at ..." so the user does not lose their work. Resolves #1596. Supersedes #3641, using the cleaner approach started in #1677. --- qutebrowser/browser/commands.py | 7 +++-- qutebrowser/misc/editor.py | 40 ++++++++++++++++++--------- tests/end2end/features/editor.feature | 1 + tests/unit/misc/test_editor.py | 19 +++++++++++++ 4 files changed, 51 insertions(+), 16 deletions(-) diff --git a/qutebrowser/browser/commands.py b/qutebrowser/browser/commands.py index b84bde7a8..fb447d6a9 100644 --- a/qutebrowser/browser/commands.py +++ b/qutebrowser/browser/commands.py @@ -53,7 +53,6 @@ class CommandDispatcher: cmdutils.register() decorators are run, currentWidget() will return None. Attributes: - _editor: The ExternalEditor object. _win_id: The window ID the CommandDispatcher is associated with. _tabbed_browser: The TabbedBrowser used. """ @@ -1639,7 +1638,7 @@ class CommandDispatcher: ed = editor.ExternalEditor(watch=True, parent=self._tabbed_browser) ed.file_updated.connect(functools.partial( - self.on_file_updated, elem)) + self.on_file_updated, ed, elem)) ed.editing_finished.connect(lambda: mainwindow.raise_window( objreg.last_focused_window(), alert=False)) ed.edit(text, caret_position) @@ -1654,7 +1653,7 @@ class CommandDispatcher: tab = self._current_widget() tab.elements.find_focused(self._open_editor_cb) - def on_file_updated(self, elem, text): + def on_file_updated(self, ed, elem, text): """Write the editor text into the form field and clean up tempfile. Callback for GUIProcess when the edited text was updated. @@ -1667,7 +1666,9 @@ class CommandDispatcher: elem.set_value(text) except webelem.OrphanedError as e: message.error('Edited element vanished') + ed.backup() except webelem.Error as e: + ed.backup() raise cmdexc.CommandError(str(e)) @cmdutils.register(instance='command-dispatcher', maxsplit=0, diff --git a/qutebrowser/misc/editor.py b/qutebrowser/misc/editor.py index 154660001..d9024000b 100644 --- a/qutebrowser/misc/editor.py +++ b/qutebrowser/misc/editor.py @@ -42,6 +42,7 @@ class ExternalEditor(QObject): _proc: The GUIProcess of the editor. _watcher: A QFileSystemWatcher to watch the edited file for changes. Only set if watch=True. + _content: The last-saved text of the editor. Signals: file_updated: The text in the edited file was updated. @@ -112,19 +113,7 @@ class ExternalEditor(QObject): if self._filename is not None: raise ValueError("Already editing a file!") try: - # Close while the external process is running, as otherwise systems - # with exclusive write access (e.g. Windows) may fail to update - # the file from the external editor, see - # https://github.com/qutebrowser/qutebrowser/issues/1767 - with tempfile.NamedTemporaryFile( - # pylint: disable=bad-continuation - mode='w', prefix='qutebrowser-editor-', - encoding=config.val.editor.encoding, - delete=False) as fobj: - # pylint: enable=bad-continuation - if text: - fobj.write(text) - self._filename = fobj.name + self._filename = self._create_tempfile(text) except OSError as e: message.error("Failed to create initial file: {}".format(e)) return @@ -134,6 +123,31 @@ class ExternalEditor(QObject): line, column = self._calc_line_and_column(text, caret_position) self._start_editor(line=line, column=column) + def backup(self): + """Create a backup if the content has changed from the original.""" + if not self._content: + return + try: + fname = self._create_tempfile(self._content) + message.info('Editor backup at {}'.format(fname)) + except OSError as e: + message.error('Failed to create editor backup: {}'.format(e)) + + def _create_tempfile(self, text): + # Close while the external process is running, as otherwise systems + # with exclusive write access (e.g. Windows) may fail to update + # the file from the external editor, see + # https://github.com/qutebrowser/qutebrowser/issues/1767 + with tempfile.NamedTemporaryFile( + # pylint: disable=bad-continuation + mode='w', prefix='qutebrowser-editor-', + encoding=config.val.editor.encoding, + delete=False) as fobj: + # pylint: enable=bad-continuation + if text: + fobj.write(text) + return fobj.name + @pyqtSlot(str) def _on_file_changed(self, path): try: diff --git a/tests/end2end/features/editor.feature b/tests/end2end/features/editor.feature index 391e88749..33535856c 100644 --- a/tests/end2end/features/editor.feature +++ b/tests/end2end/features/editor.feature @@ -128,6 +128,7 @@ Feature: Opening external editors And I run :tab-close And I kill the waiting editor Then the error "Edited element vanished" should be shown + And the message "Editor backup at *" should be shown # Could not get signals working on Windows @posix diff --git a/tests/unit/misc/test_editor.py b/tests/unit/misc/test_editor.py index 8cf778d89..caa0b42cf 100644 --- a/tests/unit/misc/test_editor.py +++ b/tests/unit/misc/test_editor.py @@ -157,6 +157,25 @@ class TestFileHandling: with pytest.raises(ValueError): editor.edit("") + def test_backup(self, qtbot, message_mock): + editor = editormod.ExternalEditor(watch=True) + editor.edit('foo') + with qtbot.wait_signal(editor.file_updated) as blocker: + _update_file(editor._filename, 'bar') + + editor.backup() + + msg = message_mock.getmsg(usertypes.MessageLevel.info) + prefix = 'Editor backup at ' + assert msg.text.startswith(prefix) + fname = msg.text[len(prefix):] + + with qtbot.wait_signal(editor.editing_finished) as blocker: + editor._proc.finished.emit(0, QProcess.NormalExit) + + with open(fname, 'r') as f: + assert f.read() == 'bar' + @pytest.mark.parametrize('initial_text, edited_text', [ ('', 'Hello'), From 27966c94a60b744592a6038351778589d9cc640d Mon Sep 17 00:00:00 2001 From: Ryan Roden-Corrent Date: Tue, 13 Mar 2018 07:31:48 -0400 Subject: [PATCH 2/3] Fix up editor backup patch. - Use qutebrowser-editor-backup as the backup file prefix - Consistently use message.error instead of cmdexc - Improve test coverage for the backup function - Fix lint errors in the unit test code --- qutebrowser/browser/commands.py | 2 +- qutebrowser/misc/editor.py | 9 +++++---- tests/unit/misc/test_editor.py | 25 ++++++++++++++++++++++--- 3 files changed, 28 insertions(+), 8 deletions(-) diff --git a/qutebrowser/browser/commands.py b/qutebrowser/browser/commands.py index fb447d6a9..0a30c7135 100644 --- a/qutebrowser/browser/commands.py +++ b/qutebrowser/browser/commands.py @@ -1668,8 +1668,8 @@ class CommandDispatcher: message.error('Edited element vanished') ed.backup() except webelem.Error as e: + message.error(str(e)) ed.backup() - raise cmdexc.CommandError(str(e)) @cmdutils.register(instance='command-dispatcher', maxsplit=0, scope='window') diff --git a/qutebrowser/misc/editor.py b/qutebrowser/misc/editor.py index d9024000b..473f67c3e 100644 --- a/qutebrowser/misc/editor.py +++ b/qutebrowser/misc/editor.py @@ -113,7 +113,7 @@ class ExternalEditor(QObject): if self._filename is not None: raise ValueError("Already editing a file!") try: - self._filename = self._create_tempfile(text) + self._filename = self._create_tempfile(text, 'qutebrowser-editor-') except OSError as e: message.error("Failed to create initial file: {}".format(e)) return @@ -128,19 +128,20 @@ class ExternalEditor(QObject): if not self._content: return try: - fname = self._create_tempfile(self._content) + fname = self._create_tempfile(self._content, + 'qutebrowser-editor-backup-') message.info('Editor backup at {}'.format(fname)) except OSError as e: message.error('Failed to create editor backup: {}'.format(e)) - def _create_tempfile(self, text): + def _create_tempfile(self, text, prefix): # Close while the external process is running, as otherwise systems # with exclusive write access (e.g. Windows) may fail to update # the file from the external editor, see # https://github.com/qutebrowser/qutebrowser/issues/1767 with tempfile.NamedTemporaryFile( # pylint: disable=bad-continuation - mode='w', prefix='qutebrowser-editor-', + mode='w', prefix=prefix, encoding=config.val.editor.encoding, delete=False) as fobj: # pylint: enable=bad-continuation diff --git a/tests/unit/misc/test_editor.py b/tests/unit/misc/test_editor.py index caa0b42cf..095f00ef2 100644 --- a/tests/unit/misc/test_editor.py +++ b/tests/unit/misc/test_editor.py @@ -160,7 +160,7 @@ class TestFileHandling: def test_backup(self, qtbot, message_mock): editor = editormod.ExternalEditor(watch=True) editor.edit('foo') - with qtbot.wait_signal(editor.file_updated) as blocker: + with qtbot.wait_signal(editor.file_updated): _update_file(editor._filename, 'bar') editor.backup() @@ -170,12 +170,31 @@ class TestFileHandling: assert msg.text.startswith(prefix) fname = msg.text[len(prefix):] - with qtbot.wait_signal(editor.editing_finished) as blocker: + with qtbot.wait_signal(editor.editing_finished): editor._proc.finished.emit(0, QProcess.NormalExit) - with open(fname, 'r') as f: + with open(fname, 'r', encoding='utf-8') as f: assert f.read() == 'bar' + def test_backup_no_content(self, qtbot, message_mock): + editor = editormod.ExternalEditor(watch=True) + editor.edit('foo') + editor.backup() + # content has not changed, so no backup should be created + assert not message_mock.messages + + def test_backup(self, qtbot, message_mock, mocker): + editor = editormod.ExternalEditor(watch=True) + editor.edit('foo') + with qtbot.wait_signal(editor.file_updated): + _update_file(editor._filename, 'bar') + + mocker.patch('tempfile.NamedTemporaryFile', side_effect=OSError) + editor.backup() + + msg = message_mock.getmsg(usertypes.MessageLevel.error) + assert msg.text.startswith('Failed to create editor backup:') + @pytest.mark.parametrize('initial_text, edited_text', [ ('', 'Hello'), From 73517f0a51f6f64bc9c2cf7c8508b1e7f12d7185 Mon Sep 17 00:00:00 2001 From: Ryan Roden-Corrent Date: Tue, 13 Mar 2018 08:50:34 -0400 Subject: [PATCH 3/3] Fix test_backup_error. - Need caplog at level error - Rename test to be unique --- tests/unit/misc/test_editor.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/unit/misc/test_editor.py b/tests/unit/misc/test_editor.py index 095f00ef2..94021484a 100644 --- a/tests/unit/misc/test_editor.py +++ b/tests/unit/misc/test_editor.py @@ -183,14 +183,15 @@ class TestFileHandling: # content has not changed, so no backup should be created assert not message_mock.messages - def test_backup(self, qtbot, message_mock, mocker): + def test_backup_error(self, qtbot, message_mock, mocker, caplog): editor = editormod.ExternalEditor(watch=True) editor.edit('foo') with qtbot.wait_signal(editor.file_updated): _update_file(editor._filename, 'bar') mocker.patch('tempfile.NamedTemporaryFile', side_effect=OSError) - editor.backup() + with caplog.at_level(logging.ERROR): + editor.backup() msg = message_mock.getmsg(usertypes.MessageLevel.error) assert msg.text.startswith('Failed to create editor backup:')