Repository: qutebrowser/qutebrowser
Pull request: 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.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/qutebrowser/qutebrowser/3704)
<!-- Reviewable:end -->


Problem statement:
Temporary files created by `open-editor` are deleted if textfield is lost
If, for whatever reason, the initiating textfield is not available anymore when the editor closes, the temporary file is deleted which results in loosing the changes.

The files should be kept around if qutebrowser failed to insert the text into the textfield.

Hints from issue discussion:
There's an abandoned PR at #1677.

Commit message: 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.

Patch to review:
diff --git a/qutebrowser/browser/commands.py b/qutebrowser/browser/commands.py
index b84bde7a8d6..fb447d6a932 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 @@ def _open_editor_cb(self, elem):
 
         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 @@ def open_editor(self):
         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 @@ def on_file_updated(self, elem, text):
             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 15466000138..d9024000bb9 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 @@ def edit(self, text, caret_position=None):
         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 @@ def edit(self, text, caret_position=None):
         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 391e887497c..33535856c94 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 8cf778d8935..caa0b42cfad 100644
--- a/tests/unit/misc/test_editor.py
+++ b/tests/unit/misc/test_editor.py
@@ -157,6 +157,25 @@ def test_double_edit(self, editor):
         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'),


Code review comments:
diff_hunk: @@ -1667,7 +1666,9 @@ def on_file_updated(self, elem, text):
             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))
path: qutebrowser/browser/commands.py
text: @user1:
Probably would make sense to make those two consistent - i.e. do `ed.backup()` and then `raise cmdexc.CommandError` at both places, or use `message.error` and then `ed.backup()` at both places.

@author:
I'll switch both to `message.error`, as I think the 'backup at ...' message should come after the error

diff_hunk: @@ -134,6 +123,31 @@ def edit(self, text, caret_position=None):
         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)
path: qutebrowser/misc/editor.py
text: @user1:
Maybe pass a different `prefix` here, i.e. `qutebrowser-editor-backup-` or so? That'd make it easier for users to find the file.

diff_hunk: @@ -157,6 +157,25 @@ def test_double_edit(self, editor):
         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:
path: tests/unit/misc/test_editor.py
text: @user1:
You don't need the `blocker` (to e.g. read args) here, so don't do `as blocker` :wink:

diff_hunk: @@ -157,6 +157,25 @@ def test_double_edit(self, editor):
         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:
path: tests/unit/misc/test_editor.py
text: @user1:
`, encoding='utf-8'`