From 04034d86e62a481e4b1e1711d318759cf6cb220c Mon Sep 17 00:00:00 2001 From: rozyczko Date: Tue, 8 Sep 2026 11:53:09 +0200 Subject: [PATCH 1/4] Improved project tracking, #401 --- .../Backends/Mock/Project.qml | 10 ++ .../Backends/Py/logic/project.py | 5 + EasyReflectometryApp/Backends/Py/project.py | 73 +++++++++++- .../Gui/ApplicationWindow.qml | 55 ++++++++- .../Gui/Globals/BackendWrapper.qml | 21 ++++ EasyReflectometryApp/Gui/StatusBar.qml | 17 +++ pyproject.toml | 2 +- tests/factories.py | 1 + tests/test_logic_project.py | 18 +++ tests/test_py_project.py | 109 +++++++++++++++++- tests/test_qml_project_save_ui.py | 58 ++++++++++ 11 files changed, 362 insertions(+), 7 deletions(-) create mode 100644 tests/test_qml_project_save_ui.py diff --git a/EasyReflectometryApp/Backends/Mock/Project.qml b/EasyReflectometryApp/Backends/Mock/Project.qml index 74825414..d7d2db17 100644 --- a/EasyReflectometryApp/Backends/Mock/Project.qml +++ b/EasyReflectometryApp/Backends/Mock/Project.qml @@ -6,6 +6,10 @@ QtObject { property bool created: false property string creationDate: '' + property string lastSaved: '' + + signal projectSaved(string path) + signal projectSaveError(string message) property string name: 'Super duper project' function setName(value) { name = value } @@ -18,21 +22,27 @@ QtObject { console.debug(`Creating project ${name}`) creationDate = `${new Date().toLocaleDateString()} ${new Date().toLocaleTimeString()}` created = true + lastSaved = new Date().toISOString() + projectSaved(location) } function save() { console.debug(`Saving project ${name}`) + lastSaved = new Date().toISOString() + projectSaved(location) } function reset() { console.debug(`Reset project ${name}`) created = false + lastSaved = '' } function load(path) { console.debug(`Loading project from ${path}`) creationDate = `${new Date().toLocaleDateString()} ${new Date().toLocaleTimeString()}` created = true + lastSaved = '' } } diff --git a/EasyReflectometryApp/Backends/Py/logic/project.py b/EasyReflectometryApp/Backends/Py/logic/project.py index 32b6a883..bec5df7c 100644 --- a/EasyReflectometryApp/Backends/Py/logic/project.py +++ b/EasyReflectometryApp/Backends/Py/logic/project.py @@ -20,6 +20,11 @@ def created(self) -> bool: def path(self) -> str: return str(self._project_lib.path) + @property + def path_json(self) -> str: + """Path of the project file itself, as named in save feedback and error messages.""" + return str(self._project_lib.path_json) + @property def root_path(self) -> str: return str(self._project_lib.path.parent) diff --git a/EasyReflectometryApp/Backends/Py/project.py b/EasyReflectometryApp/Backends/Py/project.py index 50337442..1f8ce832 100644 --- a/EasyReflectometryApp/Backends/Py/project.py +++ b/EasyReflectometryApp/Backends/Py/project.py @@ -1,4 +1,5 @@ import warnings +from datetime import datetime from easyreflectometry import Project as ProjectLib from easyreflectometry.orso_utils import load_orso_model @@ -17,6 +18,7 @@ class Project(QObject): nameChanged = Signal() descriptionChanged = Signal() locationChanged = Signal() + lastSavedChanged = Signal() externalCreatedChanged = Signal() externalNameChanged = Signal() @@ -24,10 +26,13 @@ class Project(QObject): externalProjectReset = Signal() sampleLoadWarning = Signal(str) projectLoadError = Signal(str) + projectSaved = Signal(str) + projectSaveError = Signal(str) def __init__(self, project_lib: ProjectLib, parent=None): super().__init__(parent) self._logic = ProjectLogic(project_lib) + self._last_saved = '' # Properties @@ -43,6 +48,16 @@ def creationDate(self) -> str: def currentProjectPath(self) -> str: return self._logic.path + @Property(str, notify=lastSavedChanged) + def lastSaved(self) -> str: + """ISO-8601 wall-clock time of the last successful save, or '' if never saved. + + This is the time of the last save made from this session, which is deliberately not + the same as `creationDate` (the project's stored modification stamp). The value is + left unformatted so that QML can render it with a locale-aware `Qt.formatTime`. + """ + return self._last_saved + # Properties with setters @Property(str, notify=nameChanged) @@ -78,11 +93,56 @@ def setLocation(self, new_value: str) -> None: # Methods + def _clear_last_saved(self) -> None: + """Drop the save stamp when the project it referred to is gone (reset or load).""" + if self._last_saved: + self._last_saved = '' + self.lastSavedChanged.emit() + + def _mark_saved(self) -> None: + self._last_saved = datetime.now().isoformat(timespec='seconds') + self.lastSavedChanged.emit() + self.projectSaved.emit(self._logic.path_json) + + def _save_error_message(self, exception: Exception) -> str: + """Turn a save failure into a sentence a user can act on, keeping the raw text as detail. + + The library raises rather than prints since the atomic-save change, so these are the + failures that actually reach the GUI. The previously saved file is always intact. + """ + path = self._logic.path_json + if isinstance(exception, FileExistsError): + explanation = f'A project already exists at "{path}".\nChoose a different name or location.' + elif isinstance(exception, PermissionError): + explanation = f'No permission to write "{path}".\nThe file may be read-only or open in another program.' + elif isinstance(exception, ValueError): + # Raised while serializing, e.g. a constraint that depends on a parameter which is + # not reachable from the models. + explanation = f'The project could not be saved to "{path}" because it cannot be serialized.' + elif isinstance(exception, OSError): + explanation = f'The project could not be written to "{path}".' + else: + return f'Failed to save the project to "{path}".\n\n{exception}' + return f'{explanation}\n\nDetails: {exception}' + @Slot() def create(self) -> None: - self._logic.create() + # create() writes the project file, so it is a first save and reports through the same + # signals. It can fail on a colliding path, which the library now raises instead of + # printing. + error = None + try: + self._logic.create() + except Exception as ex: + error = self._save_error_message(ex) + # Emitted either way, so that the UI reflects the real `created` state even when the + # directories were made but the file was not written. self.createdChanged.emit() self.externalCreatedChanged.emit() + if error is not None: + self.projectSaveError.emit(error) + else: + self._mark_saved() @Slot(str) def load(self, path: str) -> None: @@ -102,6 +162,7 @@ def load(self, path: str) -> None: message = str(ex) self.projectLoadError.emit(message) return + self._clear_last_saved() self.createdChanged.emit() self.nameChanged.emit() self.descriptionChanged.emit() @@ -110,11 +171,19 @@ def load(self, path: str) -> None: @Slot() def save(self) -> None: - self._logic.save() + # The whole call is guarded: the library's unlink-free atomic save raises, and a locked + # destination raises out of os.replace, so nothing may escape into this slot uncaught. + try: + self._logic.save() + except Exception as ex: + self.projectSaveError.emit(self._save_error_message(ex)) + return + self._mark_saved() @Slot() def reset(self) -> None: self._logic.reset() + self._clear_last_saved() self.createdChanged.emit() self.nameChanged.emit() self.descriptionChanged.emit() diff --git a/EasyReflectometryApp/Gui/ApplicationWindow.qml b/EasyReflectometryApp/Gui/ApplicationWindow.qml index 13777efa..3325c97b 100644 --- a/EasyReflectometryApp/Gui/ApplicationWindow.qml +++ b/EasyReflectometryApp/Gui/ApplicationWindow.qml @@ -30,11 +30,24 @@ EaComponents.ApplicationWindow { appBarLeftButtons: [ EaElements.ToolButton { - enabled: Globals.BackendWrapper.projectCreated + id: saveButton + // Saving serializes the same model state the fitter thread is writing to, so it is + // blocked while a fit runs rather than silently storing half-updated parameters. + enabled: Globals.BackendWrapper.projectCreated && !Globals.BackendWrapper.analysisFittingRunning highlighted: true - fontIcon: "save" - ToolTip.text: qsTr("Save current state of the project") + fontIcon: saveFlashTimer.running ? "check-circle" : "save" + ToolTip.text: Globals.BackendWrapper.analysisFittingRunning + ? qsTr("Saving is disabled while a fit is running") + : qsTr("Save current state of the project") onClicked: Globals.BackendWrapper.projectSave() + + // Success feedback in place, where the user just clicked. A save during the flash + // restarts it rather than cutting it short. + Timer { + id: saveFlashTimer + interval: 2000 + repeat: false + } }, EaElements.ToolButton { @@ -166,6 +179,42 @@ EaComponents.ApplicationWindow { onClosing: Qt.quit() + Shortcut { + sequences: [StandardKey.Save] + enabled: saveButton.enabled + onActivated: Globals.BackendWrapper.projectSave() + } + + // Save feedback is asymmetric: a failure must not be missable, so it is modal, while a + // successful save flashes the tool button and updates the status bar instead of interrupting. + Connections { + target: Globals.BackendWrapper + + function onProjectSaved(path) { + saveFlashTimer.restart() + } + + function onProjectSaveError(message) { + projectSaveErrorDialog.errorMessage = message + projectSaveErrorDialog.open() + } + } + + EaElements.Dialog { + id: projectSaveErrorDialog + title: qsTr('Project Save Error') + standardButtons: Dialog.Ok + closePolicy: Popup.CloseOnEscape | Popup.CloseOnPressOutside + + property string errorMessage: '' + + EaElements.Label { + text: projectSaveErrorDialog.errorMessage + wrapMode: Text.WordWrap + width: EaStyle.Sizes.sideBarContentWidth + } + } + EaElements.Dialog { id: resetStateDialog diff --git a/EasyReflectometryApp/Gui/Globals/BackendWrapper.qml b/EasyReflectometryApp/Gui/Globals/BackendWrapper.qml index 426ceaae..519be193 100644 --- a/EasyReflectometryApp/Gui/Globals/BackendWrapper.qml +++ b/EasyReflectometryApp/Gui/Globals/BackendWrapper.qml @@ -100,6 +100,27 @@ QtObject { return null } + // Project save signals - forwarded from backend + readonly property string projectLastSaved: activeBackend.project.lastSaved ?? '' + + signal projectSaved(string path) + + property var _projectSavedConnection: { + if (activeBackend && activeBackend.project && activeBackend.project.projectSaved) { + activeBackend.project.projectSaved.connect(projectSaved) + } + return null + } + + signal projectSaveError(string message) + + property var _projectSaveErrorConnection: { + if (activeBackend && activeBackend.project && activeBackend.project.projectSaveError) { + activeBackend.project.projectSaveError.connect(projectSaveError) + } + return null + } + /////////////// // Sample page diff --git a/EasyReflectometryApp/Gui/StatusBar.qml b/EasyReflectometryApp/Gui/StatusBar.qml index 6b14a296..c1590ef5 100644 --- a/EasyReflectometryApp/Gui/StatusBar.qml +++ b/EasyReflectometryApp/Gui/StatusBar.qml @@ -54,6 +54,23 @@ EaElements.StatusBar { ToolTip.text: qsTr('Number of parameters: total, free and fixed') } + EaElements.StatusBarItem { + visible: Globals.BackendWrapper.projectLastSaved !== '' + keyIcon: 'save' + keyText: qsTr('Saved') + // The backend reports an ISO-8601 stamp so that the time is rendered in the user's locale. + // Minute resolution: some locales' short time format carries seconds, which is more + // precision than a save stamp needs. + valueText: { + if (Globals.BackendWrapper.projectLastSaved === '') { + return '' + } + const format = Qt.locale().timeFormat(Locale.ShortFormat).replace(/[:.]?s+/g, '') + return Qt.formatTime(new Date(Globals.BackendWrapper.projectLastSaved), format) + } + ToolTip.text: qsTr('Time of the last successful save') + } + EaElements.StatusBarItem { visible: Globals.BackendWrapper.analysisFittingRunning keyIcon: 'play-circle' diff --git a/pyproject.toml b/pyproject.toml index fc5d1417..3e5e7984 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -31,7 +31,7 @@ classifiers = [ requires-python = '>=3.12' dependencies = [ 'easyapplication', - 'easyreflectometry @ git+https://github.com/easyscience/reflectometry-lib.git@develop', + 'easyreflectometry @ git+https://github.com/easyscience/reflectometry-lib.git@401-project-save', #'easyreflectometry', 'asteval', 'PySide6', diff --git a/tests/factories.py b/tests/factories.py index 3863d1ff..dd022373 100644 --- a/tests/factories.py +++ b/tests/factories.py @@ -378,6 +378,7 @@ def __init__( self._info = {'name': 'Demo Project', 'short_description': 'Demo Description', 'modified': '2026-03-19'} self.created = False self.path = Path('C:/tmp/demo-project') + self.path_json = self.path / 'project.json' self.q_min = 0.01 self.q_max = 0.5 self.q_resolution = 200 diff --git a/tests/test_logic_project.py b/tests/test_logic_project.py index 971e01e0..f9050ec0 100644 --- a/tests/test_logic_project.py +++ b/tests/test_logic_project.py @@ -1,5 +1,7 @@ from pathlib import Path +import pytest + from EasyReflectometryApp.Backends.Py.logic.project import Project from tests.factories import make_assembly from tests.factories import make_layer @@ -111,3 +113,19 @@ def test_project_reset_calls_reset_and_default_model(): assert project_lib.calls == [('reset',), ('default_model',)] + + +def test_save_and_create_propagate_library_failures(): + """The logic layer must not swallow save failures; the backend turns them into a dialog.""" + project_lib = make_project_with_sample() + logic = Project(project_lib) + + def _raise(overwrite=False): + raise OSError('disk full') + + project_lib.save_as_json = _raise + + with pytest.raises(OSError): + logic.save() + with pytest.raises(OSError): + logic.create() diff --git a/tests/test_py_project.py b/tests/test_py_project.py index 99201344..f5ac9bab 100644 --- a/tests/test_py_project.py +++ b/tests/test_py_project.py @@ -7,7 +7,8 @@ class StubProjectLogic: def __init__(self, _project_lib): self.created = False self.creation_date = '2026-03-22' - self.path = 'project.json' + self.path = 'C:/tmp/demo-project' + self.path_json = 'project.json' self.name = 'Demo' self.description = 'Desc' self.root_path = 'C:/work' @@ -143,3 +144,109 @@ def _raise_format_error(_path): 'Please re-create the project from its underlying data and save it again.' ] assert loaded['count'] == 0 + + +def _spy_save_signals(project): + saved = [] + errors = [] + stamps = [] + project.projectSaved.connect(lambda path: saved.append(path)) + project.projectSaveError.connect(lambda msg: errors.append(msg)) + project.lastSavedChanged.connect(lambda: stamps.append(project.lastSaved)) + return saved, errors, stamps + + +def test_save_emits_projectSaved_and_stamps_last_saved(monkeypatch, qcore_application): + project = _build_project(monkeypatch) + saved, errors, stamps = _spy_save_signals(project) + + assert project.lastSaved == '' + + project.save() + + assert saved == ['project.json'] + assert errors == [] + assert len(stamps) == 1 + assert project.lastSaved != '' + + +def test_save_emits_error_and_leaves_last_saved_untouched(monkeypatch, qcore_application): + project = _build_project(monkeypatch) + + def _raise_permission_error(): + raise PermissionError('project.json is open in another program') + + monkeypatch.setattr(project._logic, 'save', _raise_permission_error) + saved, errors, stamps = _spy_save_signals(project) + + project.save() + + assert saved == [] + assert stamps == [] + assert project.lastSaved == '' + assert len(errors) == 1 + assert 'No permission to write "project.json"' in errors[0] + assert 'open in another program' in errors[0] + + +def test_save_reports_serialization_failure(monkeypatch, qcore_application): + project = _build_project(monkeypatch) + + def _raise_value_error(): + raise ValueError('constraint depends on an unreachable parameter') + + monkeypatch.setattr(project._logic, 'save', _raise_value_error) + _saved, errors, _stamps = _spy_save_signals(project) + + project.save() + + assert len(errors) == 1 + assert 'cannot be serialized' in errors[0] + assert 'unreachable parameter' in errors[0] + + +def test_create_reports_save_through_the_same_signals(monkeypatch, qcore_application): + project = _build_project(monkeypatch) + saved, errors, _stamps = _spy_save_signals(project) + + project.create() + + assert saved == ['project.json'] + assert errors == [] + assert project.lastSaved != '' + + +def test_create_emits_error_when_the_project_file_already_exists(monkeypatch, qcore_application): + project = _build_project(monkeypatch) + + def _raise_file_exists(): + raise FileExistsError('File already exists project.json') + + monkeypatch.setattr(project._logic, 'create', _raise_file_exists) + saved, errors, _stamps = _spy_save_signals(project) + created_counts = {'created': 0} + project.createdChanged.connect(lambda: created_counts.__setitem__('created', created_counts['created'] + 1)) + + project.create() + + assert saved == [] + assert project.lastSaved == '' + assert len(errors) == 1 + assert 'A project already exists at "project.json"' in errors[0] + # The UI is still told to re-read `created`, so it reflects the real state after a failure. + assert created_counts['created'] == 1 + + +def test_reset_and_load_clear_the_last_saved_stamp(monkeypatch, qcore_application): + project = _build_project(monkeypatch) + monkeypatch.setattr(project_module.IO, 'generalizePath', lambda path: path) + + project.save() + assert project.lastSaved != '' + project.reset() + assert project.lastSaved == '' + + project.save() + assert project.lastSaved != '' + project.load('other.json') + assert project.lastSaved == '' diff --git a/tests/test_qml_project_save_ui.py b/tests/test_qml_project_save_ui.py new file mode 100644 index 00000000..1d1828ea --- /dev/null +++ b/tests/test_qml_project_save_ui.py @@ -0,0 +1,58 @@ +from pathlib import Path + +ROOT = Path(__file__).resolve().parents[1] +GUI = ROOT / 'EasyReflectometryApp' / 'Gui' + + +def test_save_button_flashes_on_success_and_is_blocked_during_a_fit(): + application_window_qml = (GUI / 'ApplicationWindow.qml').read_text(encoding='utf-8') + + assert 'id: saveButton' in application_window_qml + assert ( + 'enabled: Globals.BackendWrapper.projectCreated && !Globals.BackendWrapper.analysisFittingRunning' + in application_window_qml + ) + assert 'fontIcon: saveFlashTimer.running ? "check-circle" : "save"' in application_window_qml + assert 'id: saveFlashTimer' in application_window_qml + assert 'saveFlashTimer.restart()' in application_window_qml + assert 'sequences: [StandardKey.Save]' in application_window_qml + + +def test_save_failure_opens_a_modal_with_the_error_message(): + application_window_qml = (GUI / 'ApplicationWindow.qml').read_text(encoding='utf-8') + + assert 'id: projectSaveErrorDialog' in application_window_qml + assert 'function onProjectSaveError(message)' in application_window_qml + assert 'projectSaveErrorDialog.errorMessage = message' in application_window_qml + assert 'projectSaveErrorDialog.open()' in application_window_qml + # A successful save must not raise a modal. + assert 'function onProjectSaved(path)' in application_window_qml + + +def test_status_bar_shows_the_last_save_time_in_the_user_locale(): + status_bar_qml = (GUI / 'StatusBar.qml').read_text(encoding='utf-8') + + assert "keyText: qsTr('Saved')" in status_bar_qml + assert "visible: Globals.BackendWrapper.projectLastSaved !== ''" in status_bar_qml + # Locale-aware, but at minute resolution: the locale's short format may carry seconds. + assert 'Qt.locale().timeFormat(Locale.ShortFormat)' in status_bar_qml + assert "replace(/[:.]?s+/g, '')" in status_bar_qml + assert 'Qt.formatTime(new Date(Globals.BackendWrapper.projectLastSaved), format)' in status_bar_qml + + +def test_backend_wrapper_forwards_the_save_signals(): + wrapper_qml = (GUI / 'Globals' / 'BackendWrapper.qml').read_text(encoding='utf-8') + + assert 'signal projectSaved(string path)' in wrapper_qml + assert 'signal projectSaveError(string message)' in wrapper_qml + assert 'activeBackend.project.projectSaved.connect(projectSaved)' in wrapper_qml + assert 'activeBackend.project.projectSaveError.connect(projectSaveError)' in wrapper_qml + assert 'readonly property string projectLastSaved' in wrapper_qml + + +def test_mock_backend_matches_the_save_api(): + mock_qml = (ROOT / 'EasyReflectometryApp' / 'Backends' / 'Mock' / 'Project.qml').read_text(encoding='utf-8') + + assert 'signal projectSaved(string path)' in mock_qml + assert 'signal projectSaveError(string message)' in mock_qml + assert 'property string lastSaved' in mock_qml From 5417612e23c81a93c8456ddfa323592bfb0f3d26 Mon Sep 17 00:00:00 2001 From: rozyczko Date: Tue, 8 Sep 2026 12:14:47 +0200 Subject: [PATCH 2/4] backend state added --- .../Backends/Mock/Project.qml | 11 +- EasyReflectometryApp/Backends/Py/project.py | 100 ++++++++++++--- .../Backends/Py/py_backend.py | 58 +++++++++ .../Gui/ApplicationWindow.qml | 90 ++++++++++++-- .../Gui/Globals/BackendWrapper.qml | 1 + tests/test_py_backend.py | 4 + tests/test_py_dirty_tracking.py | 115 ++++++++++++++++++ tests/test_py_project.py | 71 +++++++++++ tests/test_qml_project_save_ui.py | 52 +++++++- 9 files changed, 467 insertions(+), 35 deletions(-) create mode 100644 tests/test_py_dirty_tracking.py diff --git a/EasyReflectometryApp/Backends/Mock/Project.qml b/EasyReflectometryApp/Backends/Mock/Project.qml index d7d2db17..379fde00 100644 --- a/EasyReflectometryApp/Backends/Mock/Project.qml +++ b/EasyReflectometryApp/Backends/Mock/Project.qml @@ -7,28 +7,31 @@ QtObject { property bool created: false property string creationDate: '' property string lastSaved: '' + property bool hasUnsavedChanges: false signal projectSaved(string path) signal projectSaveError(string message) property string name: 'Super duper project' - function setName(value) { name = value } + function setName(value) { name = value; hasUnsavedChanges = true } property string description: 'Default project description from Mock proxy' - function setDescription(value) { description = value } + function setDescription(value) { description = value; hasUnsavedChanges = true } property string location: '/path to the project' - function setLocation(value) { location = value } + function setLocation(value) { location = value; hasUnsavedChanges = true } function create() { console.debug(`Creating project ${name}`) creationDate = `${new Date().toLocaleDateString()} ${new Date().toLocaleTimeString()}` created = true lastSaved = new Date().toISOString() + hasUnsavedChanges = false projectSaved(location) } function save() { console.debug(`Saving project ${name}`) lastSaved = new Date().toISOString() + hasUnsavedChanges = false projectSaved(location) } @@ -36,6 +39,7 @@ QtObject { console.debug(`Reset project ${name}`) created = false lastSaved = '' + hasUnsavedChanges = false } function load(path) { @@ -43,6 +47,7 @@ QtObject { creationDate = `${new Date().toLocaleDateString()} ${new Date().toLocaleTimeString()}` created = true lastSaved = '' + hasUnsavedChanges = false } } diff --git a/EasyReflectometryApp/Backends/Py/project.py b/EasyReflectometryApp/Backends/Py/project.py index 1f8ce832..83e92ad4 100644 --- a/EasyReflectometryApp/Backends/Py/project.py +++ b/EasyReflectometryApp/Backends/Py/project.py @@ -1,4 +1,5 @@ import warnings +from contextlib import contextmanager from datetime import datetime from easyreflectometry import Project as ProjectLib @@ -19,6 +20,7 @@ class Project(QObject): descriptionChanged = Signal() locationChanged = Signal() lastSavedChanged = Signal() + hasUnsavedChangesChanged = Signal() externalCreatedChanged = Signal() externalNameChanged = Signal() @@ -33,6 +35,8 @@ def __init__(self, project_lib: ProjectLib, parent=None): super().__init__(parent) self._logic = ProjectLogic(project_lib) self._last_saved = '' + self._has_unsaved_changes = False + self._dirty_suspended = 0 # Properties @@ -58,6 +62,16 @@ def lastSaved(self) -> str: """ return self._last_saved + @Property(bool, notify=hasUnsavedChangesChanged) + def hasUnsavedChanges(self) -> bool: + """Whether the project holds edits that `save()` would write to disk. + + Set from every backend signal that changes what the project file would contain (the + inventory lives in `py_backend.DIRTYING_SIGNALS`) and from this object's own setters; + cleared by create, save, load and reset. + """ + return self._has_unsaved_changes + # Properties with setters @Property(str, notify=nameChanged) @@ -68,6 +82,7 @@ def name(self) -> str: def setName(self, new_value: str) -> None: if self._logic.name != new_value: self._logic.name = new_value + self.markDirty() self.nameChanged.emit() self.externalNameChanged.emit() @@ -79,6 +94,7 @@ def description(self) -> str: def setDescription(self, new_value: str) -> None: if self._logic.description != new_value: self._logic.description = new_value + self.markDirty() self.descriptionChanged.emit() @Property(str, notify=locationChanged) @@ -89,10 +105,48 @@ def location(self) -> str: def setLocation(self, new_value: str) -> None: if self._logic.root_path != new_value: self._logic.root_path = new_value + self.markDirty() self.locationChanged.emit() # Methods + @Slot() + def markDirty(self) -> None: + """Record that the project differs from the file on disk. + + Connected to every dirtying backend signal by `py_backend._connect_dirty_tracking`, and + called directly by this object's setters. Ignored while a create/load/reset is fanning + out its own signals, since those end in a clean project. + """ + if self._dirty_suspended or self._has_unsaved_changes: + return + self._has_unsaved_changes = True + self.hasUnsavedChangesChanged.emit() + + def _clear_dirty(self) -> None: + if not self._has_unsaved_changes: + return + self._has_unsaved_changes = False + self.hasUnsavedChangesChanged.emit() + + @contextmanager + def _suspended_dirty_tracking(self): + """Run a create/load/reset without its own relays marking the project dirty. + + Those relays run through the sample, experiment and analysis parts, which emit the very + signals dirty tracking listens to. Suppressing them during the fan-out is what keeps a + freshly created or loaded project clean, without depending on the order in which the + slots happen to emit. + + Suspending is all this does; clearing the flag is left to the callers, because only they + know whether the change actually reached disk. A failed create must stay dirty. + """ + self._dirty_suspended += 1 + try: + yield + finally: + self._dirty_suspended -= 1 + def _clear_last_saved(self) -> None: """Drop the save stamp when the project it referred to is gone (reset or load).""" if self._last_saved: @@ -100,6 +154,7 @@ def _clear_last_saved(self) -> None: self.lastSavedChanged.emit() def _mark_saved(self) -> None: + self._clear_dirty() self._last_saved = datetime.now().isoformat(timespec='seconds') self.lastSavedChanged.emit() self.projectSaved.emit(self._logic.path_json) @@ -131,14 +186,15 @@ def create(self) -> None: # signals. It can fail on a colliding path, which the library now raises instead of # printing. error = None - try: - self._logic.create() - except Exception as ex: - error = self._save_error_message(ex) - # Emitted either way, so that the UI reflects the real `created` state even when the - # directories were made but the file was not written. - self.createdChanged.emit() - self.externalCreatedChanged.emit() + with self._suspended_dirty_tracking(): + try: + self._logic.create() + except Exception as ex: + error = self._save_error_message(ex) + # Emitted either way, so that the UI reflects the real `created` state even when the + # directories were made but the file was not written. + self.createdChanged.emit() + self.externalCreatedChanged.emit() if error is not None: self.projectSaveError.emit(error) else: @@ -163,11 +219,13 @@ def load(self, path: str) -> None: self.projectLoadError.emit(message) return self._clear_last_saved() - self.createdChanged.emit() - self.nameChanged.emit() - self.descriptionChanged.emit() - self.locationChanged.emit() - self.externalProjectLoaded.emit() + with self._suspended_dirty_tracking(): + self.createdChanged.emit() + self.nameChanged.emit() + self.descriptionChanged.emit() + self.locationChanged.emit() + self.externalProjectLoaded.emit() + self._clear_dirty() @Slot() def save(self) -> None: @@ -184,13 +242,15 @@ def save(self) -> None: def reset(self) -> None: self._logic.reset() self._clear_last_saved() - self.createdChanged.emit() - self.nameChanged.emit() - self.descriptionChanged.emit() - self.locationChanged.emit() - self.externalCreatedChanged.emit() - self.externalNameChanged.emit() - self.externalProjectReset.emit() + with self._suspended_dirty_tracking(): + self.createdChanged.emit() + self.nameChanged.emit() + self.descriptionChanged.emit() + self.locationChanged.emit() + self.externalCreatedChanged.emit() + self.externalNameChanged.emit() + self.externalProjectReset.emit() + self._clear_dirty() @Slot(str, bool) def sampleLoad(self, url: str, append: bool = True) -> None: diff --git a/EasyReflectometryApp/Backends/Py/py_backend.py b/EasyReflectometryApp/Backends/Py/py_backend.py index 59a6f8a2..731133a8 100644 --- a/EasyReflectometryApp/Backends/Py/py_backend.py +++ b/EasyReflectometryApp/Backends/Py/py_backend.py @@ -15,6 +15,56 @@ from .status import Status from .summary import Summary +# Signals whose emission changes what `Project.save()` would write to disk. Every one of them +# marks the project dirty (see `_connect_dirty_tracking`). The inventory is explicit rather than +# derived from a naming convention because the `external*` relays do not cover every mutation: +# a material rename emits only `materialsTableChanged`, reordering layers or editing a repeated +# assembly's repetitions emits only `externalRefreshPlot`, and the free/fixed checkbox emits only +# `parametersChanged`. Missing one of those means the close prompt does not fire and the user +# loses work, so anything doubtful belongs in this list rather than out of it. +# +# `Project`'s own signals are absent on purpose: its setters call `markDirty` directly, because +# create, load and reset fan out through those same signal names and must end up clean. +DIRTYING_SIGNALS = { + '_sample': ( + 'externalSampleChanged', + 'externalRefreshPlot', + 'materialsTableChanged', + 'modelsTableChanged', + 'constraintsChanged', + 'calculationEngineChanged', # stored as 'calculator' in the project file + 'qRangeChanged', + ), + '_experiment': ( + 'externalExperimentChanged', + 'experimentLoaded', + 'qRangeUpdated', + ), + '_analysis': ( + 'externalCalculatorChanged', + 'externalExperimentChanged', + 'externalFittingChanged', # a finished fit rewrites parameter values + 'externalMinimizerChanged', # stored as 'fitter_minimizer' in the project file + 'externalParametersChanged', + 'parametersChanged', + ), +} + +# `external*` signals deliberately left out of dirty tracking, with the reason. Together with +# DIRTYING_SIGNALS this must account for every `external*` signal on every backend part; +# tests/test_py_backend.py fails if a new one appears in neither, so it cannot silently skip +# dirty tracking. +NON_DIRTYING_EXTERNAL_SIGNALS = { + '_project': ( + # Project lifecycle, not project content: create/load/reset end in a clean project and + # the setters mark dirty themselves. + 'externalCreatedChanged', + 'externalNameChanged', + 'externalProjectLoaded', + 'externalProjectReset', + ), +} + class PyBackend(QObject): # Signal for multi-experiment selection changes @@ -220,6 +270,14 @@ def _connect_backend_parts(self) -> None: self._connect_sample_page() self._connect_experiment_page() self._connect_analysis_page() + self._connect_dirty_tracking() + + def _connect_dirty_tracking(self) -> None: + """Route every content-changing signal to the project's unsaved-changes flag.""" + for part_name, signal_names in DIRTYING_SIGNALS.items(): + part = getattr(self, part_name) + for signal_name in signal_names: + getattr(part, signal_name).connect(self._project.markDirty) ######### Forming connections between the backend parts def _connect_project_page(self) -> None: diff --git a/EasyReflectometryApp/Gui/ApplicationWindow.qml b/EasyReflectometryApp/Gui/ApplicationWindow.qml index 3325c97b..76ef1756 100644 --- a/EasyReflectometryApp/Gui/ApplicationWindow.qml +++ b/EasyReflectometryApp/Gui/ApplicationWindow.qml @@ -31,14 +31,24 @@ EaComponents.ApplicationWindow { EaElements.ToolButton { id: saveButton - // Saving serializes the same model state the fitter thread is writing to, so it is - // blocked while a fit runs rather than silently storing half-updated parameters. - enabled: Globals.BackendWrapper.projectCreated && !Globals.BackendWrapper.analysisFittingRunning + // Disabled when the project is already on disk unchanged, so that an enabled button + // is itself the signal that there is something to save. Saving also serializes the + // same model state the fitter thread is writing to, so it is blocked during a fit + // rather than silently storing half-updated parameters. + enabled: Globals.BackendWrapper.projectCreated + && Globals.BackendWrapper.projectHasUnsavedChanges + && !Globals.BackendWrapper.analysisFittingRunning highlighted: true fontIcon: saveFlashTimer.running ? "check-circle" : "save" - ToolTip.text: Globals.BackendWrapper.analysisFittingRunning - ? qsTr("Saving is disabled while a fit is running") - : qsTr("Save current state of the project") + ToolTip.text: { + if (Globals.BackendWrapper.analysisFittingRunning) { + return qsTr("Saving is disabled while a fit is running") + } + if (Globals.BackendWrapper.projectCreated && !Globals.BackendWrapper.projectHasUnsavedChanges) { + return qsTr("No changes to save") + } + return qsTr("Save current state of the project") + } onClicked: Globals.BackendWrapper.projectSave() // Success feedback in place, where the user just clicked. A save during the flash @@ -177,7 +187,57 @@ EaComponents.ApplicationWindow { // MISC /////// - onClosing: Qt.quit() + // Closing with unsaved work asks first. Qt5 had this (Components/CloseDialog.qml) and the + // Qt6 migration left `onClosing: Qt.quit()` as a no-op, so this restores the behaviour — + // with the Cancel button the Qt5 dialog was missing. + onClosing: function(close) { + if (Globals.BackendWrapper.projectHasUnsavedChanges) { + close.accepted = false + closeDialog.open() + } + } + + // Set while the close dialog's "Save and exit" is in flight. Unlike Qt5, which quit + // unconditionally, the app only exits once the save has actually reported success — a failed + // save leaves the window open with its error dialog rather than discarding the work. + property bool quitAfterSave: false + + EaElements.Dialog { + id: closeDialog + title: qsTr('Unsaved Changes') + closePolicy: Popup.CloseOnEscape + + EaElements.Label { + text: qsTr('The project has unsaved changes.\nDo you want to save them before exiting?') + wrapMode: Text.WordWrap + width: EaStyle.Sizes.sideBarContentWidth + } + + footer: EaElements.DialogButtonBox { + EaElements.Button { + text: qsTr('Cancel') + onClicked: closeDialog.close() + } + + EaElements.Button { + text: qsTr('Exit without saving') + onClicked: { + closeDialog.close() + Qt.quit() + } + } + + EaElements.Button { + text: qsTr('Save and exit') + enabled: !Globals.BackendWrapper.analysisFittingRunning + onClicked: { + closeDialog.close() + applicationWindow.quitAfterSave = true + Globals.BackendWrapper.projectSave() + } + } + } + } Shortcut { sequences: [StandardKey.Save] @@ -192,9 +252,15 @@ EaComponents.ApplicationWindow { function onProjectSaved(path) { saveFlashTimer.restart() + if (applicationWindow.quitAfterSave) { + applicationWindow.quitAfterSave = false + Qt.quit() + } } function onProjectSaveError(message) { + // A failed "Save and exit" must not exit: the work is still only in memory. + applicationWindow.quitAfterSave = false projectSaveErrorDialog.errorMessage = message projectSaveErrorDialog.open() } @@ -222,7 +288,15 @@ EaComponents.ApplicationWindow { EaElements.Label { horizontalAlignment: Text.AlignHCenter - text: qsTr("Are you sure you want to reset the application to its\noriginal state without project, sample and data?\n\nThis operation cannot be undone.") + // Now that unsaved changes are tracked, the dialog can say what is actually at risk + // instead of warning about loss that may not exist. + text: { + const question = qsTr("Are you sure you want to reset the application to its\noriginal state without project, sample and data?") + if (Globals.BackendWrapper.projectHasUnsavedChanges) { + return question + '\n\n' + qsTr("The project has unsaved changes that will be lost.") + } + return question + '\n\n' + qsTr("This operation cannot be undone.") + } } footer: EaElements.DialogButtonBox { diff --git a/EasyReflectometryApp/Gui/Globals/BackendWrapper.qml b/EasyReflectometryApp/Gui/Globals/BackendWrapper.qml index 519be193..68c08b2c 100644 --- a/EasyReflectometryApp/Gui/Globals/BackendWrapper.qml +++ b/EasyReflectometryApp/Gui/Globals/BackendWrapper.qml @@ -102,6 +102,7 @@ QtObject { // Project save signals - forwarded from backend readonly property string projectLastSaved: activeBackend.project.lastSaved ?? '' + readonly property bool projectHasUnsavedChanges: activeBackend.project.hasUnsavedChanges ?? false signal projectSaved(string path) diff --git a/tests/test_py_backend.py b/tests/test_py_backend.py index baf962bf..1b1e8310 100644 --- a/tests/test_py_backend.py +++ b/tests/test_py_backend.py @@ -24,6 +24,10 @@ class StubProject(QObject): def __init__(self, _project_lib, parent=None): super().__init__(parent) + self.dirty_calls = 0 + + def markDirty(self): + self.dirty_calls += 1 class StubSample(QObject): diff --git a/tests/test_py_dirty_tracking.py b/tests/test_py_dirty_tracking.py new file mode 100644 index 00000000..3cee9403 --- /dev/null +++ b/tests/test_py_dirty_tracking.py @@ -0,0 +1,115 @@ +"""Dirty tracking: the inventory of signals that mark the project unsaved. + +The point of these tests is the inventory, not the flag. A mutation path whose signal is missing +from `DIRTYING_SIGNALS` leaves `hasUnsavedChanges` False, so the close prompt never fires and the +user's work is discarded silently. `test_every_external_signal_is_classified` is what makes that +hard to do by accident. +""" + +import pytest +from PySide6.QtCore import QMetaMethod +from PySide6.QtCore import Signal + +from EasyReflectometryApp.Backends.Py import py_backend as backend_module +from EasyReflectometryApp.Backends.Py.analysis import Analysis +from EasyReflectometryApp.Backends.Py.experiment import Experiment +from EasyReflectometryApp.Backends.Py.project import Project +from EasyReflectometryApp.Backends.Py.sample import Sample + +PART_CLASSES = { + '_project': Project, + '_sample': Sample, + '_experiment': Experiment, + '_analysis': Analysis, +} + + +def _signal_names(cls) -> set: + return {name for name, value in vars(cls).items() if isinstance(value, Signal)} + + +_ARGUMENT_DEFAULTS = {'int': 0, 'double': 0.0, 'float': 0.0, 'QString': '', 'bool': False} + + +def _emit(owner, signal_name: str) -> None: + """Emit a signal without caring what it carries; only the connection is under test.""" + meta_object = owner.metaObject() + for index in range(meta_object.methodCount()): + method = meta_object.method(index) + if method.methodType() != QMetaMethod.MethodType.Signal: + continue + if bytes(method.name()).decode() != signal_name: + continue + argument_types = [bytes(parameter).decode() for parameter in method.parameterTypes()] + getattr(owner, signal_name).emit(*(_ARGUMENT_DEFAULTS[each] for each in argument_types)) + return + raise AssertionError(f'{type(owner).__name__} exposes no signal {signal_name} to Qt') + + +@pytest.fixture(scope='module') +def py_backend(qcore_application): + return backend_module.PyBackend() + + +def test_every_external_signal_is_classified(): + """A new external* signal must be declared as dirtying or explicitly excluded. + + This fails when someone adds one and wires it nowhere, which is the failure mode that costs + a user their unsaved fit. + """ + for part_name, cls in PART_CLASSES.items(): + external = {name for name in _signal_names(cls) if name.startswith('external')} + classified = set(backend_module.DIRTYING_SIGNALS.get(part_name, ())) | set( + backend_module.NON_DIRTYING_EXTERNAL_SIGNALS.get(part_name, ()) + ) + unclassified = external - classified + assert not unclassified, ( + f'{cls.__name__} signal(s) {sorted(unclassified)} are neither in DIRTYING_SIGNALS nor ' + f'in NON_DIRTYING_EXTERNAL_SIGNALS. Decide whether they change what save() writes.' + ) + + +def test_declared_signals_exist_on_their_backend_part(): + """Guards against a rename silently emptying the inventory.""" + for mapping in (backend_module.DIRTYING_SIGNALS, backend_module.NON_DIRTYING_EXTERNAL_SIGNALS): + for part_name, signal_names in mapping.items(): + cls = PART_CLASSES[part_name] + missing = set(signal_names) - _signal_names(cls) + assert not missing, f'{cls.__name__} has no signal(s) {sorted(missing)}' + + +@pytest.mark.parametrize( + ('part_name', 'signal_name'), + [(part, signal) for part, signals in backend_module.DIRTYING_SIGNALS.items() for signal in signals], +) +def test_each_dirtying_signal_is_connected(py_backend, part_name, signal_name): + """Emitting the signal on a real backend must set the flag — proves the connection exists.""" + project = py_backend._project + project._clear_dirty() + assert project.hasUnsavedChanges is False + + _emit(getattr(py_backend, part_name), signal_name) + + assert project.hasUnsavedChanges is True, f'{part_name}.{signal_name} does not mark the project dirty' + + +def test_project_starts_clean_and_setters_mark_it_dirty(py_backend): + project = py_backend._project + project._clear_dirty() + + project.setName('A new name') + assert project.hasUnsavedChanges is True + + project._clear_dirty() + project.setDescription('A new description') + assert project.hasUnsavedChanges is True + + +def test_setter_that_changes_nothing_does_not_dirty(py_backend): + project = py_backend._project + project._clear_dirty() + + project.setName(project.name) + project.setDescription(project.description) + + assert project.hasUnsavedChanges is False diff --git a/tests/test_py_project.py b/tests/test_py_project.py index f5ac9bab..cb4eea4c 100644 --- a/tests/test_py_project.py +++ b/tests/test_py_project.py @@ -250,3 +250,74 @@ def test_reset_and_load_clear_the_last_saved_stamp(monkeypatch, qcore_applicatio assert project.lastSaved != '' project.load('other.json') assert project.lastSaved == '' + + +def test_lifecycle_leaves_the_project_clean(monkeypatch, qcore_application): + project = _build_project(monkeypatch) + monkeypatch.setattr(project_module.IO, 'generalizePath', lambda path: path) + # Stand in for the relays that run while a project is created, loaded or reset: they emit the + # very signals dirty tracking listens to, and must not leave the project looking edited. + project.externalCreatedChanged.connect(project.markDirty) + project.externalProjectLoaded.connect(project.markDirty) + project.externalProjectReset.connect(project.markDirty) + + project.create() + assert project.hasUnsavedChanges is False + + project.setName('Edited') + assert project.hasUnsavedChanges is True + project.save() + assert project.hasUnsavedChanges is False + + project.setName('Edited again') + project.load('other.json') + assert project.hasUnsavedChanges is False + + project.setName('Edited once more') + project.reset() + assert project.hasUnsavedChanges is False + + +def test_failed_save_keeps_the_project_dirty(monkeypatch, qcore_application): + """The edits are still only in memory, so the close prompt must keep firing.""" + project = _build_project(monkeypatch) + + def _raise_permission_error(): + raise PermissionError('locked') + + monkeypatch.setattr(project._logic, 'save', _raise_permission_error) + project.setName('Edited') + assert project.hasUnsavedChanges is True + + project.save() + + assert project.hasUnsavedChanges is True + + +def test_failed_create_does_not_report_a_clean_project(monkeypatch, qcore_application): + project = _build_project(monkeypatch) + project.setName('Edited') + + def _raise_file_exists(): + raise FileExistsError('collision') + + monkeypatch.setattr(project._logic, 'create', _raise_file_exists) + + project.create() + + # create() failed, so nothing reached disk; the name edit is still unsaved. + assert project.hasUnsavedChanges is True + + +def test_unsaved_changes_notifies_only_on_transitions(monkeypatch, qcore_application): + project = _build_project(monkeypatch) + changes = [] + project.hasUnsavedChangesChanged.connect(lambda: changes.append(project.hasUnsavedChanges)) + + project.setName('One') + project.setDescription('Two') + project.setLocation('D:/three') + assert changes == [True] + + project.save() + assert changes == [True, False] diff --git a/tests/test_qml_project_save_ui.py b/tests/test_qml_project_save_ui.py index 1d1828ea..cc9888f8 100644 --- a/tests/test_qml_project_save_ui.py +++ b/tests/test_qml_project_save_ui.py @@ -1,3 +1,4 @@ +import re from pathlib import Path ROOT = Path(__file__).resolve().parents[1] @@ -8,10 +9,7 @@ def test_save_button_flashes_on_success_and_is_blocked_during_a_fit(): application_window_qml = (GUI / 'ApplicationWindow.qml').read_text(encoding='utf-8') assert 'id: saveButton' in application_window_qml - assert ( - 'enabled: Globals.BackendWrapper.projectCreated && !Globals.BackendWrapper.analysisFittingRunning' - in application_window_qml - ) + assert '!Globals.BackendWrapper.analysisFittingRunning' in application_window_qml assert 'fontIcon: saveFlashTimer.running ? "check-circle" : "save"' in application_window_qml assert 'id: saveFlashTimer' in application_window_qml assert 'saveFlashTimer.restart()' in application_window_qml @@ -56,3 +54,49 @@ def test_mock_backend_matches_the_save_api(): assert 'signal projectSaved(string path)' in mock_qml assert 'signal projectSaveError(string message)' in mock_qml assert 'property string lastSaved' in mock_qml + + +def test_save_button_is_disabled_when_there_is_nothing_to_save(): + application_window_qml = (GUI / 'ApplicationWindow.qml').read_text(encoding='utf-8') + + assert 'Globals.BackendWrapper.projectHasUnsavedChanges' in application_window_qml + assert 'enabled: Globals.BackendWrapper.projectCreated' in application_window_qml + assert "qsTr(\"No changes to save\")" in application_window_qml + + +def test_closing_with_unsaved_changes_asks_first(): + application_window_qml = (GUI / 'ApplicationWindow.qml').read_text(encoding='utf-8') + + assert 'onClosing: function(close)' in application_window_qml + # The migration's bare no-op handler must be gone (a mention in a comment does not count). + assert re.search(r'^\s*onClosing:\s*Qt\.quit\(\)\s*$', application_window_qml, re.MULTILINE) is None + assert 'id: closeDialog' in application_window_qml + assert 'close.accepted = false' in application_window_qml + # Qt5's dialog offered no way back; this one does. + assert "text: qsTr('Cancel')" in application_window_qml + assert "text: qsTr('Exit without saving')" in application_window_qml + assert "text: qsTr('Save and exit')" in application_window_qml + + +def test_save_and_exit_only_exits_once_the_save_succeeded(): + application_window_qml = (GUI / 'ApplicationWindow.qml').read_text(encoding='utf-8') + + assert 'property bool quitAfterSave: false' in application_window_qml + assert 'applicationWindow.quitAfterSave = true' in application_window_qml + # A failed save must cancel the pending exit, not fall through to Qt.quit(). + save_error_handler = application_window_qml.split('function onProjectSaveError(message)')[1] + assert 'applicationWindow.quitAfterSave = false' in save_error_handler.split('}')[0] + + +def test_reset_dialog_names_the_unsaved_work_at_risk(): + application_window_qml = (GUI / 'ApplicationWindow.qml').read_text(encoding='utf-8') + + assert 'The project has unsaved changes that will be lost.' in application_window_qml + + +def test_backend_wrapper_and_mock_expose_the_dirty_flag(): + wrapper_qml = (GUI / 'Globals' / 'BackendWrapper.qml').read_text(encoding='utf-8') + mock_qml = (ROOT / 'EasyReflectometryApp' / 'Backends' / 'Mock' / 'Project.qml').read_text(encoding='utf-8') + + assert 'readonly property bool projectHasUnsavedChanges' in wrapper_qml + assert 'property bool hasUnsavedChanges' in mock_qml From f4b3c8b085a9e95cc861a0c24a12b60291721759 Mon Sep 17 00:00:00 2001 From: Piotr Rozyczko Date: Tue, 8 Sep 2026 15:00:58 +0200 Subject: [PATCH 3/4] CR review fixes --- .../Backends/Mock/Project.qml | 6 +- .../Backends/Py/logic/models.py | 12 + .../Backends/Py/logic/project.py | 16 ++ EasyReflectometryApp/Backends/Py/project.py | 93 ++++++-- EasyReflectometryApp/Backends/Py/sample.py | 5 + .../Gui/ApplicationWindow.qml | 59 ++++- tests/test_py_dirty_tracking.py | 209 +++++++++++++++++- tests/test_py_project.py | 163 ++++++++++++-- tests/test_qml_close_behaviour.py | 111 ++++++++++ tests/test_qml_project_save_ui.py | 130 ++++++----- 10 files changed, 689 insertions(+), 115 deletions(-) create mode 100644 tests/test_qml_close_behaviour.py diff --git a/EasyReflectometryApp/Backends/Mock/Project.qml b/EasyReflectometryApp/Backends/Mock/Project.qml index 379fde00..680568d2 100644 --- a/EasyReflectometryApp/Backends/Mock/Project.qml +++ b/EasyReflectometryApp/Backends/Mock/Project.qml @@ -9,8 +9,10 @@ QtObject { property string lastSaved: '' property bool hasUnsavedChanges: false + // Like the Python backend, carries the path of the project file that was written. signal projectSaved(string path) signal projectSaveError(string message) + function projectFilePath() { return `${location}/${name}/project.json` } property string name: 'Super duper project' function setName(value) { name = value; hasUnsavedChanges = true } @@ -25,14 +27,14 @@ QtObject { created = true lastSaved = new Date().toISOString() hasUnsavedChanges = false - projectSaved(location) + projectSaved(projectFilePath()) } function save() { console.debug(`Saving project ${name}`) lastSaved = new Date().toISOString() hasUnsavedChanges = false - projectSaved(location) + projectSaved(projectFilePath()) } function reset() { diff --git a/EasyReflectometryApp/Backends/Py/logic/models.py b/EasyReflectometryApp/Backends/Py/logic/models.py index b5e22e2d..76aaeab7 100644 --- a/EasyReflectometryApp/Backends/Py/logic/models.py +++ b/EasyReflectometryApp/Backends/Py/logic/models.py @@ -117,14 +117,26 @@ def default_model_content(self, model: Model) -> None: def add_new(self) -> None: self._models.add_model() self.default_model_content(self._models[-1]) + self._attach_calculator(self._models[-1]) # Update index to point to the new model self.index = len(self._models) - 1 def duplicate_selected_model(self) -> None: self._models.duplicate_model(self.index) + self._attach_calculator(self._models[-1]) # Update index to point to the duplicated model self.index = len(self._models) - 1 + def _attach_calculator(self, model: Model) -> None: + """Bind a model added through the collection to the project's calculator. + + The collection's `add_model`/`duplicate_model` do not know the project's calculator, so + a model added through them has no interface. The project's fitter is built lazily for + the current model, and `Project.as_dict` (hence every save) touches it, so saving with + such a model selected failed with an internal error instead of writing the file. + """ + model.interface = self._project_lib._calculator + def move_selected_up(self) -> None: if self.index > 0: self._models.move_up(self.index) diff --git a/EasyReflectometryApp/Backends/Py/logic/project.py b/EasyReflectometryApp/Backends/Py/logic/project.py index bec5df7c..57fe9769 100644 --- a/EasyReflectometryApp/Backends/Py/logic/project.py +++ b/EasyReflectometryApp/Backends/Py/logic/project.py @@ -1,3 +1,5 @@ +import hashlib +import json from copy import copy from pathlib import Path @@ -123,6 +125,20 @@ def info(self) -> dict: info['location'] = self._project_lib.path return info + def content_fingerprint(self) -> str: + """A digest of exactly what `save()` would write. + + Used to tell a real edit from a signal that merely looks like one: selecting another + model or assembly emits the same signals as editing them, but does not change this. + Costs one serialization (under 10 ms with an experiment loaded), so callers keep it to + the moment a project might turn from clean to edited, not to every signal. + + :raises Exception: whatever `as_dict` raises; a caller that cannot fingerprint the + project must treat it as changed. + """ + content = self._project_lib.as_dict(include_materials_not_in_model=True) + return hashlib.sha256(json.dumps(content, sort_keys=True).encode('utf-8')).hexdigest() + def create(self) -> None: self._project_lib.create() self._project_lib.save_as_json() diff --git a/EasyReflectometryApp/Backends/Py/project.py b/EasyReflectometryApp/Backends/Py/project.py index 83e92ad4..2ae1a317 100644 --- a/EasyReflectometryApp/Backends/Py/project.py +++ b/EasyReflectometryApp/Backends/Py/project.py @@ -1,3 +1,4 @@ +import logging import warnings from contextlib import contextmanager from datetime import datetime @@ -13,6 +14,8 @@ from .helpers import IO from .logic.project import Project as ProjectLogic +logger = logging.getLogger(__name__) + class Project(QObject): createdChanged = Signal() @@ -37,6 +40,10 @@ def __init__(self, project_lib: ProjectLib, parent=None): self._last_saved = '' self._has_unsaved_changes = False self._dirty_suspended = 0 + # Fingerprint of the project content as it was last known to match the disk (taken on + # create, save, load and reset). None means "unknown", in which case a dirtying signal + # is trusted as is. + self._clean_fingerprint = None # Properties @@ -54,7 +61,7 @@ def currentProjectPath(self) -> str: @Property(str, notify=lastSavedChanged) def lastSaved(self) -> str: - """ISO-8601 wall-clock time of the last successful save, or '' if never saved. + """ISO-8601 time of the last successful save, with UTC offset, or '' if never saved. This is the time of the last save made from this session, which is deliberately not the same as `creationDate` (the project's stored modification stamp). The value is @@ -68,7 +75,8 @@ def hasUnsavedChanges(self) -> bool: Set from every backend signal that changes what the project file would contain (the inventory lives in `py_backend.DIRTYING_SIGNALS`) and from this object's own setters; - cleared by create, save, load and reset. + cleared by create, save, load and reset. Always False while no project has been + created, since there is nothing on disk for the edits to differ from. """ return self._has_unsaved_changes @@ -115,14 +123,45 @@ def markDirty(self) -> None: """Record that the project differs from the file on disk. Connected to every dirtying backend signal by `py_backend._connect_dirty_tracking`, and - called directly by this object's setters. Ignored while a create/load/reset is fanning - out its own signals, since those end in a clean project. + called directly by this object's setters. + + Ignored while no project has been created (nothing on disk to differ from, and `save()` + refuses anyway) and while a create/load/reset is fanning out its own signals. The + signals are a fast, over-approximate trigger: some fire on a mere selection change, and + the sample's coalesced `constraintsChanged` fires one event-loop turn after the load + that caused it. So the clean-to-edited transition is confirmed against the content + fingerprint recorded at the last clean point, which costs one serialization per + transition rather than one per signal. """ if self._dirty_suspended or self._has_unsaved_changes: return + if not self._logic.created: + return + if self._content_unchanged_since_clean(): + return self._has_unsaved_changes = True self.hasUnsavedChangesChanged.emit() + def _content_unchanged_since_clean(self) -> bool: + if self._clean_fingerprint is None: + return False + try: + return self._logic.content_fingerprint() == self._clean_fingerprint + except Exception: + # A project that cannot be serialized right now cannot be proven unchanged; the + # save path will report the actual problem. + logger.debug('Could not fingerprint the project; treating it as changed', exc_info=True) + return False + + def _record_clean_state(self) -> None: + """Remember the current content as matching the disk and clear the flag.""" + try: + self._clean_fingerprint = self._logic.content_fingerprint() + except Exception: + logger.debug('Could not fingerprint the project after a clean point', exc_info=True) + self._clean_fingerprint = None + self._clear_dirty() + def _clear_dirty(self) -> None: if not self._has_unsaved_changes: return @@ -134,12 +173,12 @@ def _suspended_dirty_tracking(self): """Run a create/load/reset without its own relays marking the project dirty. Those relays run through the sample, experiment and analysis parts, which emit the very - signals dirty tracking listens to. Suppressing them during the fan-out is what keeps a - freshly created or loaded project clean, without depending on the order in which the - slots happen to emit. + signals dirty tracking listens to. Suppressing them during the fan-out keeps the + fan-out from fingerprinting the project once per signal. (Deferred emissions land after + the block and are caught by the fingerprint check instead.) Suspending is all this does; clearing the flag is left to the callers, because only they - know whether the change actually reached disk. A failed create must stay dirty. + know whether the change actually reached disk. """ self._dirty_suspended += 1 try: @@ -154,23 +193,25 @@ def _clear_last_saved(self) -> None: self.lastSavedChanged.emit() def _mark_saved(self) -> None: - self._clear_dirty() - self._last_saved = datetime.now().isoformat(timespec='seconds') + self._record_clean_state() + # Aware stamp: unambiguous if it is ever logged or shown outside the local session. + self._last_saved = datetime.now().astimezone().isoformat(timespec='seconds') self.lastSavedChanged.emit() self.projectSaved.emit(self._logic.path_json) def _save_error_message(self, exception: Exception) -> str: """Turn a save failure into a sentence a user can act on, keeping the raw text as detail. - The library raises rather than prints since the atomic-save change, so these are the - failures that actually reach the GUI. The previously saved file is always intact. + The library raises rather than prints, so these are the failures that actually reach the + GUI. The previously saved file is always intact. """ path = self._logic.path_json if isinstance(exception, FileExistsError): - explanation = f'A project already exists at "{path}".\nChoose a different name or location.' + # Only create() can raise this (save() overwrites): the project directory is taken. + explanation = f'A project already exists at "{self._logic.path}".\nChoose a different name or location.' elif isinstance(exception, PermissionError): explanation = f'No permission to write "{path}".\nThe file may be read-only or open in another program.' - elif isinstance(exception, ValueError): + elif isinstance(exception, (TypeError, ValueError)): # Raised while serializing, e.g. a constraint that depends on a parameter which is # not reachable from the models. explanation = f'The project could not be saved to "{path}" because it cannot be serialized.' @@ -183,8 +224,7 @@ def _save_error_message(self, exception: Exception) -> str: @Slot() def create(self) -> None: # create() writes the project file, so it is a first save and reports through the same - # signals. It can fail on a colliding path, which the library now raises instead of - # printing. + # signals. It can fail on a colliding path, which the library raises instead of printing. error = None with self._suspended_dirty_tracking(): try: @@ -202,8 +242,12 @@ def create(self) -> None: @Slot(str) def load(self, path: str) -> None: + path = IO.generalizePath(path) try: - self._logic.load(IO.generalizePath(path)) + self._logic.load(path) + except FileNotFoundError: + self.projectLoadError.emit(f'The project file "{path}" does not exist.') + return except ValueError as ex: # easyreflectometry rejects project files whose file_format predates # the current schema. Show a user-facing message for that case and @@ -218,7 +262,13 @@ def load(self, path: str) -> None: message = str(ex) self.projectLoadError.emit(message) return + except OSError as ex: + self.projectLoadError.emit(f'The project file "{path}" could not be read.\n\nDetails: {ex}') + return self._clear_last_saved() + # The fingerprint is taken before the fan-out: whatever the relays emit, now or on a + # later event-loop turn, is compared against the state that was just loaded. + self._record_clean_state() with self._suspended_dirty_tracking(): self.createdChanged.emit() self.nameChanged.emit() @@ -229,6 +279,11 @@ def load(self, path: str) -> None: @Slot() def save(self) -> None: + if not self._logic.created: + # Nothing has been created, so there is no project file of our own to update; saving + # would write the in-memory defaults over whatever sits at the current path. + self.projectSaveError.emit('No project has been created yet.\nCreate or open a project before saving.') + return # The whole call is guarded: the library's unlink-free atomic save raises, and a locked # destination raises out of os.replace, so nothing may escape into this slot uncaught. try: @@ -242,6 +297,7 @@ def save(self) -> None: def reset(self) -> None: self._logic.reset() self._clear_last_saved() + self._record_clean_state() with self._suspended_dirty_tracking(): self.createdChanged.emit() self.nameChanged.emit() @@ -275,5 +331,8 @@ def sampleLoad(self, url: str, append: bool = True) -> None: else: # Replace all existing models with the loaded sample self._logic.replace_models_from_orso(sample) + # An imported sample is project content, unlike the project loads this signal otherwise + # announces; marked here rather than left to the relay's incidental table signals. + self.markDirty() # notify listeners self.externalProjectLoaded.emit() diff --git a/EasyReflectometryApp/Backends/Py/sample.py b/EasyReflectometryApp/Backends/Py/sample.py index 1281bb30..4f432219 100644 --- a/EasyReflectometryApp/Backends/Py/sample.py +++ b/EasyReflectometryApp/Backends/Py/sample.py @@ -533,15 +533,20 @@ def setCurrentLayerIndex(self, new_value: int) -> None: self._project_lib.current_layer_index = new_value self.layersIndexChanged.emit() + # A rename is project content and shows up in the Analysis parameter names, so it emits + # externalSampleChanged like the assembly rename does; layersChange alone would only reach + # dirty tracking through the deferred constraintsChanged relay, by accident. @Slot(str) def setCurrentLayerName(self, new_value: str) -> None: if self._layers_logic.set_name_at_current_index(new_value): self._clearCacheAndEmitLayersChanged() + self.externalSampleChanged.emit() @Slot(int, str) def setLayerNameAtIndex(self, index: int, new_value: str) -> None: if self._layers_logic.set_name_at_index(index, new_value): self._clearCacheAndEmitLayersChanged() + self.externalSampleChanged.emit() @Slot(int) def setCurrentLayerMaterial(self, new_value: int) -> None: diff --git a/EasyReflectometryApp/Gui/ApplicationWindow.qml b/EasyReflectometryApp/Gui/ApplicationWindow.qml index 76ef1756..8987f0cb 100644 --- a/EasyReflectometryApp/Gui/ApplicationWindow.qml +++ b/EasyReflectometryApp/Gui/ApplicationWindow.qml @@ -34,13 +34,16 @@ EaComponents.ApplicationWindow { // Disabled when the project is already on disk unchanged, so that an enabled button // is itself the signal that there is something to save. Saving also serializes the // same model state the fitter thread is writing to, so it is blocked during a fit - // rather than silently storing half-updated parameters. - enabled: Globals.BackendWrapper.projectCreated - && Globals.BackendWrapper.projectHasUnsavedChanges - && !Globals.BackendWrapper.analysisFittingRunning + // rather than silently storing half-updated parameters. The success flash keeps the + // enabled style, otherwise the check mark is drawn greyed out because the save that + // triggered it has just disabled the button. + enabled: applicationWindow.canSaveProject || saveFlashTimer.running highlighted: true fontIcon: saveFlashTimer.running ? "check-circle" : "save" ToolTip.text: { + if (saveFlashTimer.running) { + return qsTr("Project saved") + } if (Globals.BackendWrapper.analysisFittingRunning) { return qsTr("Saving is disabled while a fit is running") } @@ -49,7 +52,11 @@ EaComponents.ApplicationWindow { } return qsTr("Save current state of the project") } - onClicked: Globals.BackendWrapper.projectSave() + onClicked: { + if (applicationWindow.canSaveProject) { + Globals.BackendWrapper.projectSave() + } + } // Success feedback in place, where the user just clicked. A save during the flash // restarts it rather than cutting it short. @@ -187,16 +194,35 @@ EaComponents.ApplicationWindow { // MISC /////// - // Closing with unsaved work asks first. Qt5 had this (Components/CloseDialog.qml) and the - // Qt6 migration left `onClosing: Qt.quit()` as a no-op, so this restores the behaviour — - // with the Cancel button the Qt5 dialog was missing. + // Whether there is a project on disk that differs from what is in memory and can be written + // right now. Shared by the Save button, the Ctrl+S shortcut and the close prompt. + readonly property bool canSaveProject: Globals.BackendWrapper.projectCreated + && Globals.BackendWrapper.projectHasUnsavedChanges + && !Globals.BackendWrapper.analysisFittingRunning + + // Closing with unsaved work asks first. Qt5 had this (Components/CloseDialog.qml); the Qt6 + // migration replaced it with `onClosing: Qt.quit()`, which exited unconditionally without + // asking. This restores the prompt, with the Cancel button the Qt5 dialog was missing. + // + // The prompt is gated on a created project as well as on the flag: before a create there is + // nothing on disk that the edits could be "unsaved" relative to, and "Save and exit" would + // otherwise write the defaults over whatever project sits at the chosen path. onClosing: function(close) { - if (Globals.BackendWrapper.projectHasUnsavedChanges) { + if (Globals.BackendWrapper.projectCreated + && Globals.BackendWrapper.projectHasUnsavedChanges + && !applicationWindow.discardChangesOnClose) { close.accepted = false closeDialog.open() } } + // Set by "Exit without saving" (and test mode) before the window is closed. In Qt 6, + // Qt.quit() does not stop the event loop directly: it asks every top-level window to close + // first, which runs onClosing again, so a handler that rejects the close while the project + // is dirty would swallow the quit and reopen the prompt forever. Recording the decision here + // lets onClosing honour it. + property bool discardChangesOnClose: false + // Set while the close dialog's "Save and exit" is in flight. Unlike Qt5, which quit // unconditionally, the app only exits once the save has actually reported success — a failed // save leaves the window open with its error dialog rather than discarding the work. @@ -205,6 +231,9 @@ EaComponents.ApplicationWindow { EaElements.Dialog { id: closeDialog title: qsTr('Unsaved Changes') + // Modal, so the user cannot keep editing, start a fit or hit the window's close button + // again behind the question. (EaElements.Dialog is modeless by default.) + modal: true closePolicy: Popup.CloseOnEscape EaElements.Label { @@ -223,13 +252,14 @@ EaComponents.ApplicationWindow { text: qsTr('Exit without saving') onClicked: { closeDialog.close() - Qt.quit() + applicationWindow.discardChangesOnClose = true + applicationWindow.close() } } EaElements.Button { text: qsTr('Save and exit') - enabled: !Globals.BackendWrapper.analysisFittingRunning + enabled: applicationWindow.canSaveProject onClicked: { closeDialog.close() applicationWindow.quitAfterSave = true @@ -241,7 +271,7 @@ EaComponents.ApplicationWindow { Shortcut { sequences: [StandardKey.Save] - enabled: saveButton.enabled + enabled: applicationWindow.canSaveProject onActivated: Globals.BackendWrapper.projectSave() } @@ -249,6 +279,7 @@ EaComponents.ApplicationWindow { // successful save flashes the tool button and updates the status bar instead of interrupting. Connections { target: Globals.BackendWrapper + ignoreUnknownSignals: true function onProjectSaved(path) { saveFlashTimer.restart() @@ -270,6 +301,7 @@ EaComponents.ApplicationWindow { id: projectSaveErrorDialog title: qsTr('Project Save Error') standardButtons: Dialog.Ok + modal: true closePolicy: Popup.CloseOnEscape | Popup.CloseOnPressOutside property string errorMessage: '' @@ -285,6 +317,7 @@ EaComponents.ApplicationWindow { id: resetStateDialog title: qsTr("Reset state") + modal: true EaElements.Label { horizontalAlignment: Text.AlignHCenter @@ -372,6 +405,8 @@ EaComponents.ApplicationWindow { console.debug('*** TEST MODE START ***') delay(30000, function() { console.debug('*** TEST MODE 30 s DELAYED END ***') + // The harness must never hang on the unsaved-changes prompt. + applicationWindow.discardChangesOnClose = true Qt.quit() }) } diff --git a/tests/test_py_dirty_tracking.py b/tests/test_py_dirty_tracking.py index 3cee9403..49a04984 100644 --- a/tests/test_py_dirty_tracking.py +++ b/tests/test_py_dirty_tracking.py @@ -1,9 +1,15 @@ -"""Dirty tracking: the inventory of signals that mark the project unsaved. +"""Dirty tracking against the real backend. -The point of these tests is the inventory, not the flag. A mutation path whose signal is missing -from `DIRTYING_SIGNALS` leaves `hasUnsavedChanges` False, so the close prompt never fires and the -user's work is discarded silently. `test_every_external_signal_is_classified` is what makes that -hard to do by accident. +Two layers are under test. The inventory (`DIRTYING_SIGNALS`): a mutation path whose signal is +missing from it leaves `hasUnsavedChanges` False, so the close prompt never fires and the user's +work is discarded silently; `test_every_external_signal_is_classified` makes that hard to do by +accident. And the content check behind the flag: the signals over-approximate (a selection change +emits the same ones as an edit, and the sample relays `layersChange` into `constraintsChanged` +through a 0 ms timer, so a load's own fan-out lands after the load has finished), so the flag +only flips when the project content actually differs from the last clean state. + +The event-loop tests here are the ones that matter: a synchronous check passes while a freshly +loaded project turns dirty one `processEvents()` later. """ import pytest @@ -47,10 +53,31 @@ def _emit(owner, signal_name: str) -> None: @pytest.fixture(scope='module') -def py_backend(qcore_application): +def _backend(qcore_application): return backend_module.PyBackend() +@pytest.fixture +def py_backend(_backend, qcore_application, tmp_path): + """The shared backend with a freshly created, clean project in a temporary directory.""" + project = _backend._project + project.reset() + # setLocation takes the parent of what it is given; the project ends up at tmp_path/. + project.setLocation(str(tmp_path / 'anything')) + project.setName('DirtyTrackingProject') + project.create() + qcore_application.processEvents() + assert project.created is True + assert project.hasUnsavedChanges is False + return _backend + + +def _forget_clean_state(project) -> None: + """Make the next dirtying signal count regardless of content: the connection is under test.""" + project._clear_dirty() + project._clean_fingerprint = None + + def test_every_external_signal_is_classified(): """A new external* signal must be declared as dirtying or explicitly excluded. @@ -85,7 +112,7 @@ def test_declared_signals_exist_on_their_backend_part(): def test_each_dirtying_signal_is_connected(py_backend, part_name, signal_name): """Emitting the signal on a real backend must set the flag — proves the connection exists.""" project = py_backend._project - project._clear_dirty() + _forget_clean_state(project) assert project.hasUnsavedChanges is False _emit(getattr(py_backend, part_name), signal_name) @@ -93,23 +120,183 @@ def test_each_dirtying_signal_is_connected(py_backend, part_name, signal_name): assert project.hasUnsavedChanges is True, f'{part_name}.{signal_name} does not mark the project dirty' -def test_project_starts_clean_and_setters_mark_it_dirty(py_backend): +def test_setters_mark_a_created_project_dirty(py_backend): project = py_backend._project - project._clear_dirty() project.setName('A new name') assert project.hasUnsavedChanges is True - project._clear_dirty() + project._record_clean_state() project.setDescription('A new description') assert project.hasUnsavedChanges is True def test_setter_that_changes_nothing_does_not_dirty(py_backend): project = py_backend._project - project._clear_dirty() project.setName(project.name) project.setDescription(project.description) assert project.hasUnsavedChanges is False + + +# --- Lifecycle: clean now, and still clean once the event loop has turned ----------------------- + + +def test_create_stays_clean_after_the_event_loop_turns(py_backend, qcore_application): + project = py_backend._project + # The fixture created the project and turned the loop once already; turn it again to be sure + # nothing is still pending. + qcore_application.processEvents() + assert project.hasUnsavedChanges is False + + +def test_reset_stays_clean_after_the_event_loop_turns(py_backend, qcore_application): + project = py_backend._project + py_backend._sample.setCurrentMaterialSld(1.234) + assert project.hasUnsavedChanges is True + + project.reset() + assert project.hasUnsavedChanges is False + qcore_application.processEvents() + + assert project.hasUnsavedChanges is False + + +def test_load_stays_clean_after_the_event_loop_turns(py_backend, qcore_application): + project = py_backend._project + path_json = project._logic.path_json + py_backend._sample.setCurrentMaterialSld(1.234) + project.save() + assert project.hasUnsavedChanges is False + + py_backend._sample.setCurrentMaterialSld(5.678) + assert project.hasUnsavedChanges is True + project.load(path_json) + assert project.hasUnsavedChanges is False + qcore_application.processEvents() + + assert project.hasUnsavedChanges is False + assert project.created is True + + +def test_load_of_a_missing_file_reports_and_keeps_the_project(py_backend, qcore_application, tmp_path): + project = py_backend._project + errors = [] + project.projectLoadError.connect(lambda message: errors.append(message)) + + project.load(str(tmp_path / 'nowhere' / 'project.json')) + + assert len(errors) == 1 + assert 'does not exist' in errors[0] + assert project.created is True + assert project.name == 'DirtyTrackingProject' + + +# --- Selection is not content ------------------------------------------------------------------ + + +def test_selecting_a_model_assembly_or_layer_does_not_dirty(py_backend, qcore_application): + project = py_backend._project + sample = py_backend._sample + sample.addNewModel() + assert project.hasUnsavedChanges is True + project.save() + assert project.hasUnsavedChanges is False + + sample.setCurrentModelIndex(1) + sample.setCurrentModelIndex(0) + sample.setCurrentAssemblyIndex(1) + sample.setCurrentAssemblyIndex(0) + sample.setCurrentLayerIndex(0) + qcore_application.processEvents() + + assert project.hasUnsavedChanges is False + + +def test_selecting_an_experiment_does_not_dirty(py_backend, qcore_application): + project = py_backend._project + analysis = py_backend._analysis + + analysis.setExperimentCurrentIndex(0) + analysis.setSelectedExperimentIndices([]) + py_backend.analysisSetSelectedExperimentIndices([0]) + qcore_application.processEvents() + + assert project.hasUnsavedChanges is False + + +# --- Edits are content --------------------------------------------------------------------------- + + +def test_renaming_a_layer_dirties_immediately(py_backend): + """Persisted in the project file, and tracked on its own rather than through the deferred + constraints relay.""" + project = py_backend._project + + py_backend._sample.setCurrentLayerName('Renamed layer') + + assert project.hasUnsavedChanges is True + + +def test_renaming_a_layer_by_index_dirties_immediately(py_backend): + project = py_backend._project + + py_backend._sample.setLayerNameAtIndex(0, 'Renamed layer') + + assert project.hasUnsavedChanges is True + + +@pytest.mark.parametrize( + 'edit', + [ + lambda backend: backend._sample.setCurrentAssemblyName('Renamed assembly'), + lambda backend: backend._sample.setCurrentMaterialSld(1.234), + lambda backend: backend._sample.addNewModel(), + lambda backend: backend._project.setDescription('A new description'), + ], + ids=['assembly rename', 'material sld', 'add model', 'description'], +) +def test_content_edits_dirty(py_backend, edit): + project = py_backend._project + + edit(py_backend) + + assert project.hasUnsavedChanges is True + + +def test_save_records_the_new_clean_state(py_backend, qcore_application): + project = py_backend._project + py_backend._sample.setCurrentMaterialSld(1.234) + project.save() + assert project.hasUnsavedChanges is False + + # The same signals again, without an edit: still clean. + py_backend._sample.setCurrentAssemblyIndex(0) + qcore_application.processEvents() + assert project.hasUnsavedChanges is False + + # A further edit: dirty again. + py_backend._sample.setCurrentMaterialSld(2.345) + assert project.hasUnsavedChanges is True + + +# --- The save path itself ------------------------------------------------------------------------ + + +def test_save_succeeds_with_a_newly_added_or_duplicated_model_selected(py_backend): + """A model added through the collection had no calculator interface; the project's lazily + built fitter then crashed inside `as_dict`, so every save with that model selected failed.""" + project = py_backend._project + errors = [] + project.projectSaveError.connect(lambda message: errors.append(message)) + + py_backend._sample.addNewModel() + project.save() + assert errors == [] + assert project.hasUnsavedChanges is False + + py_backend._sample.duplicateSelectedModel() + project.save() + assert errors == [] + assert project.hasUnsavedChanges is False diff --git a/tests/test_py_project.py b/tests/test_py_project.py index cb4eea4c..b13c7b61 100644 --- a/tests/test_py_project.py +++ b/tests/test_py_project.py @@ -1,4 +1,7 @@ import warnings +from datetime import datetime + +import pytest from EasyReflectometryApp.Backends.Py import project as project_module @@ -18,6 +21,11 @@ def __init__(self, _project_lib): self.reset_calls = 0 self.added_samples = [] self.replaced_samples = [] + # Stands in for the model/experiment content the real fingerprint covers. + self.content_version = 0 + + def content_fingerprint(self) -> str: + return f'{self.name}|{self.description}|{self.root_path}|{self.content_version}' def create(self): self.created_calls += 1 @@ -25,23 +33,30 @@ def create(self): def load(self, path): self.loaded_paths.append(path) + self.created = True def save(self): self.saved_calls += 1 def reset(self): self.reset_calls += 1 + self.created = False def add_sample_from_orso(self, sample): self.added_samples.append(sample) + self.content_version += 1 def replace_models_from_orso(self, sample): self.replaced_samples.append(sample) + self.content_version += 1 -def _build_project(monkeypatch): +def _build_project(monkeypatch, created=False): monkeypatch.setattr(project_module, 'ProjectLogic', StubProjectLogic) - return project_module.Project(project_lib=object()) + project = project_module.Project(project_lib=object()) + if created: + project.create() + return project def test_setters_emit_only_on_change(monkeypatch, qcore_application): @@ -102,6 +117,20 @@ def test_sample_load_append_and_replace(monkeypatch, qcore_application): assert loaded['count'] == 2 +def test_sample_load_marks_the_project_dirty(monkeypatch, qcore_application): + """An imported sample is content to save, even though the relay signal it emits is not + classified as dirtying (it is the same one a project load uses).""" + project = _build_project(monkeypatch, created=True) + monkeypatch.setattr(project_module.IO, 'generalizePath', lambda path: path) + monkeypatch.setattr(project_module.orso, 'load_orso', lambda path: 'orso-data') + monkeypatch.setattr(project_module, 'load_orso_model', lambda _orso_data: 'sample-model') + assert project.hasUnsavedChanges is False + + project.sampleLoad('sample.orso', append=True) + + assert project.hasUnsavedChanges is True + + def test_sample_load_emits_warning_when_model_missing(monkeypatch, qcore_application): project = _build_project(monkeypatch) monkeypatch.setattr(project_module.IO, 'generalizePath', lambda path: path) @@ -146,6 +175,35 @@ def _raise_format_error(_path): assert loaded['count'] == 0 +@pytest.mark.parametrize( + ('exception', 'expected_fragment'), + [ + (FileNotFoundError('nowhere/project.json'), 'does not exist'), + (PermissionError('denied'), 'could not be read'), + ], +) +def test_load_reports_file_system_failures(monkeypatch, qcore_application, exception, expected_fragment): + """The library raises for a missing or unreadable file; the slot must report, not propagate.""" + project = _build_project(monkeypatch) + monkeypatch.setattr(project_module.IO, 'generalizePath', lambda path: path) + + def _raise(_path): + raise exception + + monkeypatch.setattr(project._logic, 'load', _raise) + errors = [] + project.projectLoadError.connect(lambda msg: errors.append(msg)) + loaded = {'count': 0} + project.externalProjectLoaded.connect(lambda: loaded.__setitem__('count', loaded['count'] + 1)) + + project.load('nowhere/project.json') + + assert len(errors) == 1 + assert expected_fragment in errors[0] + assert 'nowhere/project.json' in errors[0] + assert loaded['count'] == 0 + + def _spy_save_signals(project): saved = [] errors = [] @@ -158,6 +216,7 @@ def _spy_save_signals(project): def test_save_emits_projectSaved_and_stamps_last_saved(monkeypatch, qcore_application): project = _build_project(monkeypatch) + project._logic.created = True saved, errors, stamps = _spy_save_signals(project) assert project.lastSaved == '' @@ -168,10 +227,27 @@ def test_save_emits_projectSaved_and_stamps_last_saved(monkeypatch, qcore_applic assert errors == [] assert len(stamps) == 1 assert project.lastSaved != '' + # An aware stamp is unambiguous wherever it ends up; QML parses the offset correctly. + assert datetime.fromisoformat(project.lastSaved).tzinfo is not None + + +def test_save_refuses_when_no_project_has_been_created(monkeypatch, qcore_application): + """Saving before a create would write the defaults over whatever sits at the current path.""" + project = _build_project(monkeypatch) + saved, errors, stamps = _spy_save_signals(project) + + project.save() + + assert project._logic.saved_calls == 0 + assert saved == [] + assert stamps == [] + assert len(errors) == 1 + assert 'No project has been created' in errors[0] def test_save_emits_error_and_leaves_last_saved_untouched(monkeypatch, qcore_application): project = _build_project(monkeypatch) + project._logic.created = True def _raise_permission_error(): raise PermissionError('project.json is open in another program') @@ -189,20 +265,28 @@ def _raise_permission_error(): assert 'open in another program' in errors[0] -def test_save_reports_serialization_failure(monkeypatch, qcore_application): +@pytest.mark.parametrize( + 'exception', + [ + ValueError('constraint depends on an unreachable parameter'), + TypeError('Object of type float32 is not JSON serializable'), + ], +) +def test_save_reports_serialization_failure(monkeypatch, qcore_application, exception): project = _build_project(monkeypatch) + project._logic.created = True - def _raise_value_error(): - raise ValueError('constraint depends on an unreachable parameter') + def _raise(): + raise exception - monkeypatch.setattr(project._logic, 'save', _raise_value_error) + monkeypatch.setattr(project._logic, 'save', _raise) _saved, errors, _stamps = _spy_save_signals(project) project.save() assert len(errors) == 1 assert 'cannot be serialized' in errors[0] - assert 'unreachable parameter' in errors[0] + assert str(exception) in errors[0] def test_create_reports_save_through_the_same_signals(monkeypatch, qcore_application): @@ -216,11 +300,11 @@ def test_create_reports_save_through_the_same_signals(monkeypatch, qcore_applica assert project.lastSaved != '' -def test_create_emits_error_when_the_project_file_already_exists(monkeypatch, qcore_application): +def test_create_emits_error_when_the_project_directory_already_exists(monkeypatch, qcore_application): project = _build_project(monkeypatch) def _raise_file_exists(): - raise FileExistsError('File already exists project.json') + raise FileExistsError('Directory C:/tmp/demo-project already exists') monkeypatch.setattr(project._logic, 'create', _raise_file_exists) saved, errors, _stamps = _spy_save_signals(project) @@ -232,13 +316,15 @@ def _raise_file_exists(): assert saved == [] assert project.lastSaved == '' assert len(errors) == 1 - assert 'A project already exists at "project.json"' in errors[0] + # Names the directory, which is what collided, and tells the user what to change. + assert 'A project already exists at "C:/tmp/demo-project"' in errors[0] + assert 'Choose a different name or location' in errors[0] # The UI is still told to re-read `created`, so it reflects the real state after a failure. assert created_counts['created'] == 1 def test_reset_and_load_clear_the_last_saved_stamp(monkeypatch, qcore_application): - project = _build_project(monkeypatch) + project = _build_project(monkeypatch, created=True) monkeypatch.setattr(project_module.IO, 'generalizePath', lambda path: path) project.save() @@ -246,6 +332,7 @@ def test_reset_and_load_clear_the_last_saved_stamp(monkeypatch, qcore_applicatio project.reset() assert project.lastSaved == '' + project.load('other.json') project.save() assert project.lastSaved != '' project.load('other.json') @@ -278,9 +365,48 @@ def test_lifecycle_leaves_the_project_clean(monkeypatch, qcore_application): assert project.hasUnsavedChanges is False +def test_deferred_relay_after_a_load_does_not_dirty(monkeypatch, qcore_application): + """The sample relays part of a load through a 0 ms timer, so a dirtying signal can land after + the load has finished and cleared the flag. It carries no edit, so it must not count.""" + project = _build_project(monkeypatch) + monkeypatch.setattr(project_module.IO, 'generalizePath', lambda path: path) + + project.load('other.json') + assert project.hasUnsavedChanges is False + + project.markDirty() # what the timer's constraintsChanged does once the event loop turns + + assert project.hasUnsavedChanges is False + + +def test_signal_that_changes_nothing_does_not_dirty_but_a_real_edit_does(monkeypatch, qcore_application): + """Selection changes emit the same signals as edits; only content decides.""" + project = _build_project(monkeypatch, created=True) + + project.markDirty() + assert project.hasUnsavedChanges is False + + project._logic.content_version += 1 + project.markDirty() + assert project.hasUnsavedChanges is True + + +def test_project_that_cannot_be_fingerprinted_is_treated_as_changed(monkeypatch, qcore_application): + project = _build_project(monkeypatch, created=True) + + def _raise(): + raise ValueError('constraint depends on an unreachable parameter') + + monkeypatch.setattr(project._logic, 'content_fingerprint', _raise) + + project.markDirty() + + assert project.hasUnsavedChanges is True + + def test_failed_save_keeps_the_project_dirty(monkeypatch, qcore_application): """The edits are still only in memory, so the close prompt must keep firing.""" - project = _build_project(monkeypatch) + project = _build_project(monkeypatch, created=True) def _raise_permission_error(): raise PermissionError('locked') @@ -294,23 +420,26 @@ def _raise_permission_error(): assert project.hasUnsavedChanges is True -def test_failed_create_does_not_report_a_clean_project(monkeypatch, qcore_application): +def test_edits_before_a_create_are_not_unsaved_changes(monkeypatch, qcore_application): + """Before a create there is nothing on disk for the edits to differ from, so neither the + Save button nor the close prompt has anything to offer. In particular a failed create must + not leave a "dirty" project whose "Save and exit" would overwrite the colliding one.""" project = _build_project(monkeypatch) project.setName('Edited') + assert project.hasUnsavedChanges is False def _raise_file_exists(): raise FileExistsError('collision') monkeypatch.setattr(project._logic, 'create', _raise_file_exists) - project.create() - # create() failed, so nothing reached disk; the name edit is still unsaved. - assert project.hasUnsavedChanges is True + assert project.created is False + assert project.hasUnsavedChanges is False def test_unsaved_changes_notifies_only_on_transitions(monkeypatch, qcore_application): - project = _build_project(monkeypatch) + project = _build_project(monkeypatch, created=True) changes = [] project.hasUnsavedChangesChanged.connect(lambda: changes.append(project.hasUnsavedChanges)) diff --git a/tests/test_qml_close_behaviour.py b/tests/test_qml_close_behaviour.py new file mode 100644 index 00000000..932f8b58 --- /dev/null +++ b/tests/test_qml_close_behaviour.py @@ -0,0 +1,111 @@ +"""Runtime check of the close path ApplicationWindow.qml relies on. + +In Qt 6, `Qt.quit()` does not stop the event loop directly: it asks every top-level window to +close, which runs the window's `onClosing` handler. A handler that rejects the close while the +project is dirty therefore swallows the quit, and a dialog button that calls `Qt.quit()` after +such a handler can never exit. ApplicationWindow.qml handles this with a `discardChangesOnClose` +flag that `onClosing` consults. This test drives a window with the same handler shape through +a real `QGuiApplication` and asserts both halves: the quit is swallowed while dirty, and the +window closes once the flag is set. + +The real ApplicationWindow needs the whole application (EasyApplication components, the Globals +singletons, a backend), so the handler shape is reproduced here. The source-level test in +`test_qml_project_save_ui.py` pins that the real handler has that shape. + +A `QGuiApplication` cannot coexist with the `QCoreApplication` the other tests share, so the +harness runs in a subprocess. +""" + +import os +import subprocess +import sys +import textwrap + +import pytest + +pytest.importorskip('PySide6.QtQml') + +HARNESS = textwrap.dedent( + ''' + import os + import sys + + from PySide6.QtCore import QTimer + from PySide6.QtGui import QGuiApplication + from PySide6.QtQml import QQmlApplicationEngine + + QML = b""" + import QtQuick + + Window { + id: applicationWindow + visible: true + width: 100 + height: 100 + + property bool projectHasUnsavedChanges: true + property bool discardChangesOnClose: false + property int closingCount: 0 + + // Same shape as ApplicationWindow.qml's handler (the prompt itself is not needed here). + onClosing: function(close) { + closingCount += 1 + if (projectHasUnsavedChanges && !discardChangesOnClose) { + close.accepted = false + } + } + } + """ + + app = QGuiApplication(sys.argv) + engine = QQmlApplicationEngine() + engine.loadData(QML) + if not engine.rootObjects(): + print('HARNESS: QML failed to load') + sys.exit(2) + window = engine.rootObjects()[0] + + def quit_while_dirty(): + # What "Exit without saving" did before the fix, and what the test-mode timer does. + app.quit() + QTimer.singleShot(300, exit_with_discard_flag) + + def exit_with_discard_flag(): + # Still running: the rejected close swallowed the quit. + print(f'ALIVE after quit while dirty, closingCount={window.property("closingCount")}') + window.setProperty('discardChangesOnClose', True) + window.close() # what "Exit without saving" does now + + def report_exit(): + print(f'EXITING closingCount={window.property("closingCount")}') + + app.aboutToQuit.connect(report_exit) + QTimer.singleShot(0, quit_while_dirty) + QTimer.singleShot(10000, lambda: (print('HARNESS: timed out'), os._exit(3))) + sys.exit(app.exec()) + ''' +) + + +def test_quit_is_swallowed_while_dirty_and_the_discard_flag_lets_the_window_close(tmp_path): + script = tmp_path / 'close_harness.py' + script.write_text(HARNESS, encoding='utf-8') + environment = dict(os.environ) + environment.setdefault('QT_QPA_PLATFORM', 'offscreen') + environment.setdefault('QT_QUICK_BACKEND', 'software') + environment.setdefault('QT_LOGGING_RULES', '*.debug=false') + + completed = subprocess.run( # noqa: S603 - our own interpreter running our own script + [sys.executable, str(script)], + capture_output=True, + text=True, + timeout=60, + env=environment, + ) + + output = completed.stdout + completed.stderr + assert completed.returncode == 0, output + # First quit: onClosing ran once, rejected the close, and the application kept running. + assert 'ALIVE after quit while dirty, closingCount=1' in output, output + # With the flag set, the close is accepted and the application exits. + assert 'EXITING closingCount=2' in output, output diff --git a/tests/test_qml_project_save_ui.py b/tests/test_qml_project_save_ui.py index cc9888f8..ef3baf1f 100644 --- a/tests/test_qml_project_save_ui.py +++ b/tests/test_qml_project_save_ui.py @@ -1,3 +1,13 @@ +"""Source-level contract checks on the project-save UI. + +These pin the API between QML and the backends (property and signal names the wrapper and the +mock must expose) and the few decisions in ApplicationWindow.qml that are easy to lose in a +refactor and invisible to the Python tests: the close prompt's gating, the discard flag the +Qt 6 quit path depends on, and the dialogs being modal. They deliberately do not restate the +implementation line by line. The runtime behaviour of the close path is covered by +`test_qml_close_behaviour.py`. +""" + import re from pathlib import Path @@ -5,40 +15,17 @@ GUI = ROOT / 'EasyReflectometryApp' / 'Gui' -def test_save_button_flashes_on_success_and_is_blocked_during_a_fit(): - application_window_qml = (GUI / 'ApplicationWindow.qml').read_text(encoding='utf-8') - - assert 'id: saveButton' in application_window_qml - assert '!Globals.BackendWrapper.analysisFittingRunning' in application_window_qml - assert 'fontIcon: saveFlashTimer.running ? "check-circle" : "save"' in application_window_qml - assert 'id: saveFlashTimer' in application_window_qml - assert 'saveFlashTimer.restart()' in application_window_qml - assert 'sequences: [StandardKey.Save]' in application_window_qml - - -def test_save_failure_opens_a_modal_with_the_error_message(): - application_window_qml = (GUI / 'ApplicationWindow.qml').read_text(encoding='utf-8') - - assert 'id: projectSaveErrorDialog' in application_window_qml - assert 'function onProjectSaveError(message)' in application_window_qml - assert 'projectSaveErrorDialog.errorMessage = message' in application_window_qml - assert 'projectSaveErrorDialog.open()' in application_window_qml - # A successful save must not raise a modal. - assert 'function onProjectSaved(path)' in application_window_qml - +def _application_window() -> str: + return (GUI / 'ApplicationWindow.qml').read_text(encoding='utf-8') -def test_status_bar_shows_the_last_save_time_in_the_user_locale(): - status_bar_qml = (GUI / 'StatusBar.qml').read_text(encoding='utf-8') - assert "keyText: qsTr('Saved')" in status_bar_qml - assert "visible: Globals.BackendWrapper.projectLastSaved !== ''" in status_bar_qml - # Locale-aware, but at minute resolution: the locale's short format may carry seconds. - assert 'Qt.locale().timeFormat(Locale.ShortFormat)' in status_bar_qml - assert "replace(/[:.]?s+/g, '')" in status_bar_qml - assert 'Qt.formatTime(new Date(Globals.BackendWrapper.projectLastSaved), format)' in status_bar_qml +def _block(text: str, start_marker: str, lines: int = 8) -> str: + """The `lines` lines following the first occurrence of `start_marker`.""" + index = text.index(start_marker) + return '\n'.join(text[index:].splitlines()[:lines]) -def test_backend_wrapper_forwards_the_save_signals(): +def test_backend_wrapper_forwards_the_save_api(): wrapper_qml = (GUI / 'Globals' / 'BackendWrapper.qml').read_text(encoding='utf-8') assert 'signal projectSaved(string path)' in wrapper_qml @@ -46,6 +33,8 @@ def test_backend_wrapper_forwards_the_save_signals(): assert 'activeBackend.project.projectSaved.connect(projectSaved)' in wrapper_qml assert 'activeBackend.project.projectSaveError.connect(projectSaveError)' in wrapper_qml assert 'readonly property string projectLastSaved' in wrapper_qml + assert 'readonly property bool projectHasUnsavedChanges' in wrapper_qml + assert 'readonly property bool projectCreated' in wrapper_qml def test_mock_backend_matches_the_save_api(): @@ -54,49 +43,78 @@ def test_mock_backend_matches_the_save_api(): assert 'signal projectSaved(string path)' in mock_qml assert 'signal projectSaveError(string message)' in mock_qml assert 'property string lastSaved' in mock_qml + assert 'property bool hasUnsavedChanges' in mock_qml + assert 'property bool created' in mock_qml -def test_save_button_is_disabled_when_there_is_nothing_to_save(): - application_window_qml = (GUI / 'ApplicationWindow.qml').read_text(encoding='utf-8') +def test_status_bar_renders_the_save_stamp_in_the_user_locale(): + status_bar_qml = (GUI / 'StatusBar.qml').read_text(encoding='utf-8') - assert 'Globals.BackendWrapper.projectHasUnsavedChanges' in application_window_qml - assert 'enabled: Globals.BackendWrapper.projectCreated' in application_window_qml - assert "qsTr(\"No changes to save\")" in application_window_qml + assert "visible: Globals.BackendWrapper.projectLastSaved !== ''" in status_bar_qml + assert 'Qt.locale()' in status_bar_qml + assert 'Qt.formatTime(' in status_bar_qml -def test_closing_with_unsaved_changes_asks_first(): - application_window_qml = (GUI / 'ApplicationWindow.qml').read_text(encoding='utf-8') +def test_save_is_gated_on_a_created_dirty_project_outside_a_fit(): + application_window_qml = _application_window() - assert 'onClosing: function(close)' in application_window_qml - # The migration's bare no-op handler must be gone (a mention in a comment does not count). + gate = _block(application_window_qml, 'readonly property bool canSaveProject', lines=3) + assert 'Globals.BackendWrapper.projectCreated' in gate + assert 'Globals.BackendWrapper.projectHasUnsavedChanges' in gate + assert '!Globals.BackendWrapper.analysisFittingRunning' in gate + # The button, the shortcut and "Save and exit" all defer to the same condition. + assert application_window_qml.count('applicationWindow.canSaveProject') >= 4 + assert 'sequences: [StandardKey.Save]' in application_window_qml + + +def test_closing_with_unsaved_changes_asks_first_and_honours_the_discard_decision(): + application_window_qml = _application_window() + + # The migration's unconditional quit must be gone (a mention in a comment does not count). assert re.search(r'^\s*onClosing:\s*Qt\.quit\(\)\s*$', application_window_qml, re.MULTILINE) is None - assert 'id: closeDialog' in application_window_qml - assert 'close.accepted = false' in application_window_qml - # Qt5's dialog offered no way back; this one does. + handler = _block(application_window_qml, 'onClosing: function(close)', lines=8) + # Gated on a created project: before a create nothing on disk can be "unsaved", and saving + # from the prompt would overwrite whatever project sits at the chosen path. + assert 'Globals.BackendWrapper.projectCreated' in handler + assert 'Globals.BackendWrapper.projectHasUnsavedChanges' in handler + # Qt 6 runs onClosing again from Qt.quit(); without this flag "Exit without saving" loops. + assert '!applicationWindow.discardChangesOnClose' in handler + assert 'close.accepted = false' in handler + assert 'property bool discardChangesOnClose: false' in application_window_qml + + exit_button = _block(application_window_qml, "text: qsTr('Exit without saving')", lines=7) + assert 'applicationWindow.discardChangesOnClose = true' in exit_button + assert 'Qt.quit()' not in exit_button + assert "text: qsTr('Cancel')" in application_window_qml - assert "text: qsTr('Exit without saving')" in application_window_qml assert "text: qsTr('Save and exit')" in application_window_qml def test_save_and_exit_only_exits_once_the_save_succeeded(): - application_window_qml = (GUI / 'ApplicationWindow.qml').read_text(encoding='utf-8') + application_window_qml = _application_window() assert 'property bool quitAfterSave: false' in application_window_qml - assert 'applicationWindow.quitAfterSave = true' in application_window_qml - # A failed save must cancel the pending exit, not fall through to Qt.quit(). - save_error_handler = application_window_qml.split('function onProjectSaveError(message)')[1] - assert 'applicationWindow.quitAfterSave = false' in save_error_handler.split('}')[0] + assert 'function onProjectSaved(path)' in application_window_qml + assert 'function onProjectSaveError(message)' in application_window_qml + error_handler = _block(application_window_qml, 'function onProjectSaveError(message)', lines=6) + assert 'applicationWindow.quitAfterSave = false' in error_handler -def test_reset_dialog_names_the_unsaved_work_at_risk(): - application_window_qml = (GUI / 'ApplicationWindow.qml').read_text(encoding='utf-8') +def test_test_mode_quit_cannot_be_swallowed_by_the_close_prompt(): + application_window_qml = _application_window() - assert 'The project has unsaved changes that will be lost.' in application_window_qml + test_mode_end = _block(application_window_qml, "'*** TEST MODE 30 s DELAYED END ***'", lines=5) + assert 'applicationWindow.discardChangesOnClose = true' in test_mode_end + assert 'Qt.quit()' in test_mode_end -def test_backend_wrapper_and_mock_expose_the_dirty_flag(): - wrapper_qml = (GUI / 'Globals' / 'BackendWrapper.qml').read_text(encoding='utf-8') - mock_qml = (ROOT / 'EasyReflectometryApp' / 'Backends' / 'Mock' / 'Project.qml').read_text(encoding='utf-8') +def test_the_prompt_and_error_dialogs_are_modal(): + """EaElements.Dialog is modeless by default; these must block the window behind them.""" + application_window_qml = _application_window() - assert 'readonly property bool projectHasUnsavedChanges' in wrapper_qml - assert 'property bool hasUnsavedChanges' in mock_qml + for dialog_id in ('closeDialog', 'projectSaveErrorDialog', 'resetStateDialog'): + assert 'modal: true' in _block(application_window_qml, f'id: {dialog_id}', lines=8), dialog_id + + +def test_reset_dialog_names_the_unsaved_work_at_risk(): + assert 'The project has unsaved changes that will be lost.' in _application_window() From e9bbc72a12091063548bf568e7f97d7eefc8dbfa Mon Sep 17 00:00:00 2001 From: rozyczko Date: Wed, 9 Sep 2026 14:16:39 +0200 Subject: [PATCH 4/4] fixed density subdialog enablement. Reparented lib to develop --- .../Backends/Py/logic/material.py | 10 +++++- EasyReflectometryApp/Backends/Py/project.py | 18 +++++++++-- pyproject.toml | 2 +- tests/test_logic_material.py | 23 ++++++++++++++ tests/test_py_project.py | 31 +++++++++++++++++++ 5 files changed, 80 insertions(+), 4 deletions(-) diff --git a/EasyReflectometryApp/Backends/Py/logic/material.py b/EasyReflectometryApp/Backends/Py/logic/material.py index 474c1672..0412253e 100644 --- a/EasyReflectometryApp/Backends/Py/logic/material.py +++ b/EasyReflectometryApp/Backends/Py/logic/material.py @@ -3,6 +3,7 @@ from easyreflectometry import Project as ProjectLib from easyreflectometry.sample import MaterialCollection +from easyreflectometry.sample import MaterialDensity logger = logging.getLogger(__name__) @@ -61,7 +62,14 @@ def remove_at_index(self, value: str) -> None: self._materials.pop(int(value)) def add_new(self) -> None: - self._materials.add_material() + # A material added from the GUI is a density material, so that the Material editor's + # detail panel (formula, density, SLD coupling) applies to it: that panel is shown only + # for `kind == 'density'`, and before this the GUI could not produce such a material at + # all — they arrived only through an ORSO sample load. Nothing is lost by the default: + # unchecking the coupling hands the SLD back for direct entry and fitting, which is what + # a plain `Material` offers. The library's defaults (Si at 2.33 g/cm3) also start the + # material at a physical SLD rather than the zero a plain `Material` would carry. + self._materials.add_material(MaterialDensity(name='Material added')) def duplicate_selected(self) -> None: self._materials.duplicate_material(self.index) diff --git a/EasyReflectometryApp/Backends/Py/project.py b/EasyReflectometryApp/Backends/Py/project.py index 2ae1a317..bdc6b036 100644 --- a/EasyReflectometryApp/Backends/Py/project.py +++ b/EasyReflectometryApp/Backends/Py/project.py @@ -139,8 +139,7 @@ def markDirty(self) -> None: return if self._content_unchanged_since_clean(): return - self._has_unsaved_changes = True - self.hasUnsavedChangesChanged.emit() + self._set_dirty() def _content_unchanged_since_clean(self) -> bool: if self._clean_fingerprint is None: @@ -162,6 +161,12 @@ def _record_clean_state(self) -> None: self._clean_fingerprint = None self._clear_dirty() + def _set_dirty(self) -> None: + if self._has_unsaved_changes: + return + self._has_unsaved_changes = True + self.hasUnsavedChangesChanged.emit() + def _clear_dirty(self) -> None: if not self._has_unsaved_changes: return @@ -236,6 +241,15 @@ def create(self) -> None: self.createdChanged.emit() self.externalCreatedChanged.emit() if error is not None: + if self._logic.created: + # The library makes the directories before writing the file, so a failure of the + # write alone leaves a project the UI considers created with nothing of it on + # disk. That state has to be dirty: the Save button, Ctrl+S and the close prompt + # are all gated on the flag, and without it the only way to retry the write is to + # make an unrelated edit first — while closing the window discards the work + # without asking. + self._clean_fingerprint = None + self._set_dirty() self.projectSaveError.emit(error) else: self._mark_saved() diff --git a/pyproject.toml b/pyproject.toml index 3e5e7984..fc5d1417 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -31,7 +31,7 @@ classifiers = [ requires-python = '>=3.12' dependencies = [ 'easyapplication', - 'easyreflectometry @ git+https://github.com/easyscience/reflectometry-lib.git@401-project-save', + 'easyreflectometry @ git+https://github.com/easyscience/reflectometry-lib.git@develop', #'easyreflectometry', 'asteval', 'PySide6', diff --git a/tests/test_logic_material.py b/tests/test_logic_material.py index cf83ba44..936f7408 100644 --- a/tests/test_logic_material.py +++ b/tests/test_logic_material.py @@ -113,6 +113,29 @@ def test_set_density_at_index_clamps_below_the_min_bound(): assert materials[0].density.value == pytest.approx(0.0) +def test_material_logic_add_new_creates_a_density_material(): + """A material added from the GUI must be a density material. + + The Material editor shows its formula/density/SLD-coupling panel only for + `kind == 'density'`, so a plain `Material` (what the collection appends by default) + gives the user a row with no way to reach any of that. + """ + materials = make_material_collection(make_material('Air', sld=0.0)) + project = make_project(materials=materials) + logic = Material(project) + + logic.add_new() + + added = logic.materials[-1] + assert added['label'] == 'Material added' + assert added['kind'] == 'density' + # The library's defaults, so the material starts at a physical SLD rather than zero. + assert added['formula'] == 'Si' + assert float(added['density']) == pytest.approx(2.33) + assert float(added['sld']) == pytest.approx(2.0737, abs=1e-4) + assert added['sld_coupled'] is True + + def test_material_logic_add_duplicate_move_and_remove(): materials = make_material_collection( make_material('Air', sld=0.0), diff --git a/tests/test_py_project.py b/tests/test_py_project.py index b13c7b61..f5534b4e 100644 --- a/tests/test_py_project.py +++ b/tests/test_py_project.py @@ -321,6 +321,37 @@ def _raise_file_exists(): assert 'Choose a different name or location' in errors[0] # The UI is still told to re-read `created`, so it reflects the real state after a failure. assert created_counts['created'] == 1 + # Nothing was created, so there is still nothing on disk for edits to differ from. + assert project.hasUnsavedChanges is False + + +def test_create_that_fails_after_the_directories_are_made_stays_saveable(monkeypatch, qcore_application): + """`ProjectLogic.create()` makes the directories and then writes the file. When only the + write fails, the library has already flipped `created`, so the UI shows a project whose file + was never written. That state must be dirty: the Save button, Ctrl+S and the close prompt are + all gated on the flag, so without it the write cannot be retried without making an unrelated + edit first, and closing the window discards the work without asking.""" + project = _build_project(monkeypatch) + saved, errors, _stamps = _spy_save_signals(project) + + def _create_then_fail_to_write(): + project._logic.created = True + raise PermissionError('project.json is read-only') + + monkeypatch.setattr(project._logic, 'create', _create_then_fail_to_write) + + project.create() + + assert project.created is True + assert saved == [] + assert len(errors) == 1 + assert project.hasUnsavedChanges is True + + # And the retry is what clears it, not an unrelated edit. + project.save() + + assert saved == ['project.json'] + assert project.hasUnsavedChanges is False def test_reset_and_load_clear_the_last_saved_stamp(monkeypatch, qcore_application):