From f23a28079cd019012ec121e2a34db2e784421e60 Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Sat, 6 Aug 2016 19:59:56 +0200 Subject: [PATCH 1/4] editor: fix external editor on windows On windows, only one process can open a file in write mode at once. We didn't close the handle we got (self._oshandle) before _cleanup, which means that we had the file open the whole time, which means that the external editor couldn't write back the changes. This patch closes the file while the external editor is running and only opens it once the editor is closed. We re-opened the file anyway, so this shouldn't be a huge change. Additionally, tempfile.NamedTemporaryFile is used instead of mkstemp, as we don't have to deal with os-level file handles that way. --- qutebrowser/misc/editor.py | 33 ++++++++++++++++++--------------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/qutebrowser/misc/editor.py b/qutebrowser/misc/editor.py index bcc07129a..fb4cd8676 100644 --- a/qutebrowser/misc/editor.py +++ b/qutebrowser/misc/editor.py @@ -35,8 +35,8 @@ class ExternalEditor(QObject): Attributes: _text: The current text before the editor is opened. - _oshandle: The OS level handle to the tmpfile. - _filehandle: The file handle to the tmpfile. + _file: The file handle as tempfile.NamedTemporaryFile. Note that this + handle will be closed after the initial file has been created. _proc: The GUIProcess of the editor. _win_id: The window ID the ExternalEditor is associated with. """ @@ -46,20 +46,18 @@ class ExternalEditor(QObject): def __init__(self, win_id, parent=None): super().__init__(parent) self._text = None - self._oshandle = None - self._filename = None + self._file = None self._proc = None self._win_id = win_id def _cleanup(self): """Clean up temporary files after the editor closed.""" - if self._oshandle is None or self._filename is None: + if self._file is None: # Could not create initial file. return try: - os.close(self._oshandle) if self._proc.exit_status() != QProcess.CrashExit: - os.remove(self._filename) + os.unlink(self._file.name) except OSError as e: # NOTE: Do not replace this with "raise CommandError" as it's # executed async. @@ -82,7 +80,7 @@ class ExternalEditor(QObject): return encoding = config.get('general', 'editor-encoding') try: - with open(self._filename, 'r', encoding=encoding) as f: + with open(self._file.name, 'r', encoding=encoding) as f: text = f.read() except OSError as e: # NOTE: Do not replace this with "raise CommandError" as it's @@ -108,13 +106,18 @@ class ExternalEditor(QObject): if self._text is not None: raise ValueError("Already editing a file!") self._text = text + encoding = config.get('general', 'editor-encoding') try: - self._oshandle, self._filename = tempfile.mkstemp( - text=True, prefix='qutebrowser-editor-') - if text: - encoding = config.get('general', 'editor-encoding') - with open(self._filename, 'w', encoding=encoding) as f: - f.write(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/The-Compiler/qutebrowser/issues/1767 + with tempfile.NamedTemporaryFile( + mode='w', prefix='qutebrowser-editor-', encoding=encoding, + delete=False) as fobj: + if text: + fobj.write(text) + self._file = fobj except OSError as e: message.error(self._win_id, "Failed to create initial file: " "{}".format(e)) @@ -125,6 +128,6 @@ class ExternalEditor(QObject): self._proc.error.connect(self.on_proc_error) editor = config.get('general', 'editor') executable = editor[0] - args = [arg.replace('{}', self._filename) for arg in editor[1:]] + args = [arg.replace('{}', self._file.name) for arg in editor[1:]] log.procs.debug("Calling \"{}\" with args {}".format(executable, args)) self._proc.start(executable, args) From 9a4655443f62f3e8041a9623f74fdb888b25e539 Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Sun, 7 Aug 2016 00:11:58 +0200 Subject: [PATCH 2/4] os.unlink -> os.remove The functions do the same, but remove sounds better. --- 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 fb4cd8676..c72841f30 100644 --- a/qutebrowser/misc/editor.py +++ b/qutebrowser/misc/editor.py @@ -57,7 +57,7 @@ class ExternalEditor(QObject): return try: if self._proc.exit_status() != QProcess.CrashExit: - os.unlink(self._file.name) + os.remove(self._file.name) except OSError as e: # NOTE: Do not replace this with "raise CommandError" as it's # executed async. From 4d00b8fce96be3411bb687c340e88cdf03e809b6 Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Sun, 7 Aug 2016 00:18:22 +0200 Subject: [PATCH 3/4] tests: fix tests for misc.editor Due to the change to NamedTemporaryFile, the _filename attribute was removed. We now have to use _file.name. --- tests/unit/misc/test_editor.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/unit/misc/test_editor.py b/tests/unit/misc/test_editor.py index f9a5d4370..26828ee12 100644 --- a/tests/unit/misc/test_editor.py +++ b/tests/unit/misc/test_editor.py @@ -69,14 +69,14 @@ class TestArg: config_stub.data['general']['editor'] = ['bin', 'foo', '{}', 'bar'] editor.edit("") editor._proc._proc.start.assert_called_with( - "bin", ["foo", editor._filename, "bar"]) + "bin", ["foo", editor._file.name, "bar"]) def test_placeholder_inline(self, config_stub, editor): """Test starting editor with placeholder arg inside of another arg.""" config_stub.data['general']['editor'] = ['bin', 'foo{}', 'bar'] editor.edit("") editor._proc._proc.start.assert_called_with( - "bin", ["foo" + editor._filename, "bar"]) + "bin", ["foo" + editor._file.name, "bar"]) class TestFileHandling: @@ -86,7 +86,7 @@ class TestFileHandling: def test_ok(self, editor): """Test file handling when closing with an exit status == 0.""" editor.edit("") - filename = editor._filename + filename = editor._file.name assert os.path.exists(filename) assert os.path.basename(filename).startswith('qutebrowser-editor-') editor._proc.finished.emit(0, QProcess.NormalExit) @@ -95,7 +95,7 @@ class TestFileHandling: def test_error(self, editor): """Test file handling when closing with an exit status != 0.""" editor.edit("") - filename = editor._filename + filename = editor._file.name assert os.path.exists(filename) editor._proc._proc.exitStatus = mock.Mock( @@ -109,7 +109,7 @@ class TestFileHandling: def test_crash(self, editor): """Test file handling when closing with a crash.""" editor.edit("") - filename = editor._filename + filename = editor._file.name assert os.path.exists(filename) editor._proc._proc.exitStatus = mock.Mock( @@ -125,7 +125,7 @@ class TestFileHandling: def test_unreadable(self, message_mock, editor): """Test file handling when closing with an unreadable file.""" editor.edit("") - filename = editor._filename + filename = editor._file.name assert os.path.exists(filename) os.chmod(filename, 0o077) editor._proc.finished.emit(0, QProcess.NormalExit) @@ -160,10 +160,10 @@ def test_modify(editor, initial_text, edited_text): """Test if inputs get modified correctly.""" editor.edit(initial_text) - with open(editor._filename, 'r', encoding='utf-8') as f: + with open(editor._file.name, 'r', encoding='utf-8') as f: assert f.read() == initial_text - with open(editor._filename, 'w', encoding='utf-8') as f: + with open(editor._file.name, 'w', encoding='utf-8') as f: f.write(edited_text) editor._proc.finished.emit(0, QProcess.NormalExit) From d1eef5d1dc0c109625dcb1b4247f7ebb0554eaa1 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Sun, 7 Aug 2016 11:19:03 +0200 Subject: [PATCH 4/4] Update changelog --- CHANGELOG.asciidoc | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index d7bda59b5..a23f283b3 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -60,6 +60,7 @@ Fixed ~~~~~ - Fixed crash when doing `:`, another corner-case introduced in v0.8.0 +- Fixed `:open-editor` (``) on Windows v0.8.2 ------