CadQuery / CQ-editor

CadQuery GUI editor based on PyQT
Apache License 2.0
757 stars 116 forks source link

fix #338 Current document is not saved #433

Closed aavogt closed 3 months ago

aavogt commented 5 months ago

This makes _file_changed() consistent with load_from_file(). With this change, C-s C-o goes straight to the file picker after doing something in the editor widget.

I noticed that save ends up calling triggerRerender.emit(True) both directly and through _file_changed(), but maybe that doesn't matter because I don't notice any slowness or flickering in the UI.

codecov[bot] commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 88.69%. Comparing base (03df6fa) to head (2f8c551).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #433 +/- ## ======================================= Coverage 88.68% 88.69% ======================================= Files 19 19 Lines 1564 1565 +1 Branches 190 190 ======================================= + Hits 1387 1388 +1 Misses 143 143 Partials 34 34 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Wren6991 commented 3 months ago

I was about to raise a PR for the exact same thing! Good thing I checked PRs first.

The patch I was going to raise was similar to this one, except for consistency I replaced the raw set_text_from_file() call with the load_from_file wrapper:

diff --git a/cq_editor/widgets/editor.py b/cq_editor/widgets/editor.py
index eb70537..d2f1701 100644
--- a/cq_editor/widgets/editor.py
+++ b/cq_editor/widgets/editor.py
@@ -236,7 +236,8 @@ class Editor(CodeEditor,ComponentMixin):
     def _file_changed(self):
         # neovim writes a file by removing it first so must re-add each time
         self._watch_paths()
-        self.set_text_from_file(self._filename)
+        # Do not call set_text_from_file() directly as it sets isModified()
+        self.load_from_file(self._filename)
         self.triggerRerender.emit(True)

     # Turn autoreload on/off.

I might go as far as saying set_text_from_file() should never be called directly and should be derived to an Exception or to just call load_from_file() to avoid similar mistakes in future, but that's not the immediate issue.

I was actually doing this to fix a different bug: the editor asks for save confirmation on quit if it has done an auto-reload. These were my steps to reproduce:

  1. Open CQ Editor
  2. Close the editor tab (view->editor uncheck)
  3. Create an empty file: touch empty.py and open it in CQ Editor
  4. Enable "automatic load and preview" using button on top bar
  5. Update the file externally: touch empty.py
  6. Close CQ Editor

A dialog closes up saying "Close without saving?" and I have to click "yes" to exit.

I tested that my patch above fixes the issue, and also tested OP's patch and confirmed it does the same thing.

adam-urbanczyk commented 3 months ago

Thanks @aavogt and @Wren6991, LGTM.