From 1a759f81df5f418206c81e5c7c8d590aae241b90 Mon Sep 17 00:00:00 2001 From: Kostas Karmas Date: Thu, 5 Nov 2020 16:58:47 +0100 Subject: [PATCH 01/11] Change print sequence when the number of enabled extruders changes Fixes https://github.com/Ultimaker/Cura/issues/8682 while still maintaining the correct behavior for CURA-6914. CURA-7827 --- cura/Settings/MachineManager.py | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/cura/Settings/MachineManager.py b/cura/Settings/MachineManager.py index 58fd8171b5..034a0ac6cd 100755 --- a/cura/Settings/MachineManager.py +++ b/cura/Settings/MachineManager.py @@ -128,6 +128,7 @@ class MachineManager(QObject): self.activeQualityChangesGroupChanged.connect(self.activeQualityDisplayNameChanged) self.activeStackValueChanged.connect(self._reCalculateNumUserSettings) + self.numberExtrudersEnabledChanged.connect(self._correctPrintSequence) activeQualityDisplayNameChanged = pyqtSignal() @@ -826,11 +827,6 @@ class MachineManager(QObject): result = [] # type: List[str] for setting_instance in container.findInstances(): setting_key = setting_instance.definition.key - if setting_key == "print_sequence": - old_value = container.getProperty(setting_key, "value") - Logger.log("d", "Reset setting [%s] in [%s] because its old value [%s] is no longer valid", setting_key, container, old_value) - result.append(setting_key) - continue if not self._global_container_stack.getProperty(setting_key, "type") in ("extruder", "optional_extruder"): continue @@ -862,6 +858,16 @@ class MachineManager(QObject): title = catalog.i18nc("@info:title", "Settings updated")) caution_message.show() + def _correctPrintSequence(self) -> None: + """Resets the Print Sequence setting when there are more than one enabled extruders.""" + + setting_key = "print_sequence" + new_value = "all_at_once" + user_changes_container = self._global_container_stack.userChanges + if self.numberExtrudersEnabled > 1: + user_changes_container.setProperty(setting_key, "value", new_value) + Logger.log("d", f"Setting '{setting_key}' in '{user_changes_container}' to '{new_value}' because there are more than 1 enabled extruders.") + def setActiveMachineExtruderCount(self, extruder_count: int) -> None: """Set the amount of extruders on the active machine (global stack) From 845c994cbcde85483e8d32a522d098bf8efcf178 Mon Sep 17 00:00:00 2001 From: Kostas Karmas Date: Thu, 5 Nov 2020 17:38:34 +0100 Subject: [PATCH 02/11] Replace f string since it's not available in Python3.5 That should fix the failing test which produces a SyntaxError. CURA-7827 --- cura/Settings/MachineManager.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cura/Settings/MachineManager.py b/cura/Settings/MachineManager.py index 034a0ac6cd..8a38836ac4 100755 --- a/cura/Settings/MachineManager.py +++ b/cura/Settings/MachineManager.py @@ -866,7 +866,7 @@ class MachineManager(QObject): user_changes_container = self._global_container_stack.userChanges if self.numberExtrudersEnabled > 1: user_changes_container.setProperty(setting_key, "value", new_value) - Logger.log("d", f"Setting '{setting_key}' in '{user_changes_container}' to '{new_value}' because there are more than 1 enabled extruders.") + Logger.log("d", "Setting '{}' in '{}' to '{}' because there are more than 1 enabled extruders.".format(setting_key, user_changes_container, new_value)) def setActiveMachineExtruderCount(self, extruder_count: int) -> None: """Set the amount of extruders on the active machine (global stack) From a1c7ebe1c37c8dbabe4719209f63235119917d5c Mon Sep 17 00:00:00 2001 From: Kostas Karmas Date: Thu, 5 Nov 2020 19:04:29 +0100 Subject: [PATCH 03/11] Fix mypy complaints CURA-7827 --- cura/Settings/MachineManager.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/cura/Settings/MachineManager.py b/cura/Settings/MachineManager.py index 8a38836ac4..e363895e08 100755 --- a/cura/Settings/MachineManager.py +++ b/cura/Settings/MachineManager.py @@ -861,6 +861,9 @@ class MachineManager(QObject): def _correctPrintSequence(self) -> None: """Resets the Print Sequence setting when there are more than one enabled extruders.""" + if self._global_container_stack is None: + return + setting_key = "print_sequence" new_value = "all_at_once" user_changes_container = self._global_container_stack.userChanges From c1becbe43cb878295d50f7b17e9fdf944b0e9b5d Mon Sep 17 00:00:00 2001 From: Kostas Karmas Date: Fri, 6 Nov 2020 09:19:21 +0100 Subject: [PATCH 04/11] Fix failing tests in TestMachineManager When the machine manager calls the _onGlobalContainerChanged(), it calls updateNumberExtruders Enabled, which triggers the signal numberExtrudersEnabledChanged. This, in turn, triggers the need to check the MachineManager's pyqtProperty numberExtrudersEnabled. Now, since this property has no setter, it cannot be patched. Instead, to work properly, patch the updateNumberExtrudersEnabled. CURA-7827 --- tests/TestMachineManager.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/TestMachineManager.py b/tests/TestMachineManager.py index 4f15a0670c..ae681f927d 100644 --- a/tests/TestMachineManager.py +++ b/tests/TestMachineManager.py @@ -21,6 +21,7 @@ def machine_manager(application, extruder_manager, container_registry, global_st application.getGlobalContainerStack = MagicMock(return_value = global_stack) with patch("cura.Settings.CuraContainerRegistry.CuraContainerRegistry.getInstance", MagicMock(return_value=container_registry)): manager = MachineManager(application) + manager.updateNumberExtrudersEnabled = MagicMock() manager._onGlobalContainerChanged() return manager @@ -253,4 +254,4 @@ def test_isActiveQualityNotSupported(machine_manager): def test_isActiveQualityNotSupported_noQualityGroup(machine_manager): machine_manager.activeQualityGroup = MagicMock(return_value=None) - assert not machine_manager.isActiveQualitySupported \ No newline at end of file + assert not machine_manager.isActiveQualitySupported From bd5ed7a3ac411851a9f269772b8c0aaf16165076 Mon Sep 17 00:00:00 2001 From: Kostas Karmas Date: Fri, 6 Nov 2020 09:21:55 +0100 Subject: [PATCH 05/11] Add tests for _correctPrintSequence CURA-7827 --- tests/TestMachineManager.py | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/tests/TestMachineManager.py b/tests/TestMachineManager.py index ae681f927d..3d9c61dab6 100644 --- a/tests/TestMachineManager.py +++ b/tests/TestMachineManager.py @@ -255,3 +255,40 @@ def test_isActiveQualityNotSupported(machine_manager): def test_isActiveQualityNotSupported_noQualityGroup(machine_manager): machine_manager.activeQualityGroup = MagicMock(return_value=None) assert not machine_manager.isActiveQualitySupported + + +def test_correctPrintSequence_with2ExtrudersEnabled(machine_manager, application): + mocked_stack = application.getGlobalContainerStack() + + # The definition changes container reports 2 enabled extruders so the correctPrintSequence should attempt to + # change the print_sequence setting to all-at-once + mocked_definition_changes_container = MagicMock(name="DefinitionChangesContainer") + mocked_definition_changes_container.getProperty = lambda key, prop: 2 if (key == "extruders_enabled_count" and prop == "value") else -1 + + mocked_user_changes_container = MagicMock(name="UserChangesContainer") + + mocked_stack.userChanges = mocked_user_changes_container + mocked_stack.definitionChanges = mocked_definition_changes_container + + machine_manager._correctPrintSequence() + + # After the function is called, the user changes container should attempt to correct its print sequence to all-at-once + mocked_user_changes_container.setProperty.assert_called_once_with("print_sequence", "value", "all_at_once") + + +def test_correctPrintSequence_with1ExtruderEnabled(machine_manager, application): + mocked_stack = application.getGlobalContainerStack() + + # The definition changes container reports 1 enabled extruder so the correctPrintSequence should not change anything + mocked_definition_changes_container = MagicMock(name="DefinitionChangesContainer") + mocked_definition_changes_container.getProperty = lambda key, prop: 1 if (key == "extruders_enabled_count" and prop == "value") else -1 + + mocked_user_changes_container = MagicMock(name="UserChangesContainer") + + mocked_stack.userChanges = mocked_user_changes_container + mocked_stack.definitionChanges = mocked_definition_changes_container + + machine_manager._correctPrintSequence() + + # After the function is called, the user changes container should not have tried to change any properties + assert not mocked_user_changes_container.setProperty.called, "Method should not have been called" From 171939404904135834dcd579be44ebdb456f0118 Mon Sep 17 00:00:00 2001 From: Kostas Karmas Date: Fri, 6 Nov 2020 09:38:15 +0100 Subject: [PATCH 06/11] Patch the updateNumberExtrudersEnabled temporarily CURA-7827 --- tests/TestMachineManager.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/TestMachineManager.py b/tests/TestMachineManager.py index 3d9c61dab6..b415bad59c 100644 --- a/tests/TestMachineManager.py +++ b/tests/TestMachineManager.py @@ -21,8 +21,8 @@ def machine_manager(application, extruder_manager, container_registry, global_st application.getGlobalContainerStack = MagicMock(return_value = global_stack) with patch("cura.Settings.CuraContainerRegistry.CuraContainerRegistry.getInstance", MagicMock(return_value=container_registry)): manager = MachineManager(application) - manager.updateNumberExtrudersEnabled = MagicMock() - manager._onGlobalContainerChanged() + with patch.object(MachineManager, "updateNumberExtrudersEnabled", return_value = None): + manager._onGlobalContainerChanged() return manager From 2ccdd11098c230a30578b1c65c5263e6b7b11675 Mon Sep 17 00:00:00 2001 From: Kostas Karmas Date: Mon, 9 Nov 2020 16:20:13 +0100 Subject: [PATCH 07/11] Properly set the print sequence in the correct container Previously, the print sequence would be always set to all-at-once in the user container whenever the second extruder would be enabled. This was causing an interface issue, where the circular reset-setting arrow would appear next to the setting, even if the quality changes already had the correct value ("all-at-once"). This is now fixed by checking the following: - If print_sequence == "one_at_a_time" in the quality (so it is already saved in a quality profile) then set the print_sequence to "all_at_once" in the user changes so that it will be displayed as an unsaved change - If print_sequence == "one_at_a_time" in the user changes container only, meaning that it is not saved in any quality profiles, then just reset the print_sequence in the user changes, so that it will have the correct value ("all-at-once") without the reset-setting arrow appearing next to it. CURA-7827 --- cura/Settings/MachineManager.py | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/cura/Settings/MachineManager.py b/cura/Settings/MachineManager.py index e363895e08..401c4152e6 100755 --- a/cura/Settings/MachineManager.py +++ b/cura/Settings/MachineManager.py @@ -861,15 +861,29 @@ class MachineManager(QObject): def _correctPrintSequence(self) -> None: """Resets the Print Sequence setting when there are more than one enabled extruders.""" - if self._global_container_stack is None: - return - setting_key = "print_sequence" new_value = "all_at_once" + + if self._global_container_stack is None \ + or self._global_container_stack.getProperty(setting_key, "value") == new_value \ + or self.numberExtrudersEnabled < 2: + return + user_changes_container = self._global_container_stack.userChanges - if self.numberExtrudersEnabled > 1: + quality_changes_container = self._global_container_stack.qualityChanges + print_sequence_in_quality_changes = quality_changes_container.getProperty(setting_key, "value") + print_sequence_in_user_changes = user_changes_container.getProperty(setting_key, "value") + + # If the quality changes has the wrong value, then set the correct value in the user changes + if print_sequence_in_quality_changes and print_sequence_in_quality_changes != new_value: user_changes_container.setProperty(setting_key, "value", new_value) Logger.log("d", "Setting '{}' in '{}' to '{}' because there are more than 1 enabled extruders.".format(setting_key, user_changes_container, new_value)) + # If the quality changes has no value or the correct value and the user changes container has the wrong value, + # then reset the setting in the user changes (so that the circular revert-changes arrow will now show up in the + # interface) + elif print_sequence_in_user_changes and print_sequence_in_user_changes != new_value: + user_changes_container.removeInstance(setting_key) + Logger.log("d", "Resetting '{}' in container '{}' because there are more than 1 enabled extruders.".format(setting_key, user_changes_container)) def setActiveMachineExtruderCount(self, extruder_count: int) -> None: """Set the amount of extruders on the active machine (global stack) From 07453e101e2dbd6f79ea2e257b3c9e72c7cdc27b Mon Sep 17 00:00:00 2001 From: Kostas Karmas Date: Mon, 9 Nov 2020 16:32:05 +0100 Subject: [PATCH 08/11] Add detailed documentation for _correctPrintSequence function CURA-7827 --- cura/Settings/MachineManager.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/cura/Settings/MachineManager.py b/cura/Settings/MachineManager.py index 401c4152e6..a9ee9e5096 100755 --- a/cura/Settings/MachineManager.py +++ b/cura/Settings/MachineManager.py @@ -859,7 +859,13 @@ class MachineManager(QObject): caution_message.show() def _correctPrintSequence(self) -> None: - """Resets the Print Sequence setting when there are more than one enabled extruders.""" + """ + Sets the Print Sequence setting to "all-at-once" when there are more than one enabled extruders. + + This setting has to be explicitly changed whenever we have more than one enabled extruders to make sure that the + Cura UI is properly updated to reset all the UI elements changes that occur due to the one-at-a-time mode (such + as the reduced build volume, the different convex hulls of the objects etc.). + """ setting_key = "print_sequence" new_value = "all_at_once" From fab2c31b4f55d99ed07e9b1b5867458fc410e2c2 Mon Sep 17 00:00:00 2001 From: Kostas Karmas Date: Mon, 9 Nov 2020 16:55:43 +0100 Subject: [PATCH 09/11] Cover the last remaining case when setting the print sequence If for some reason the print sequence is set to one-at-a-time be default in the printer definition and the number of extruders of the printer is set to >=2, then the print_sequence won't show up in neither the quality changes nor the user changes, yet it will still have "one-at-a-time" as a value. In a such case, it will still need to be set to "all-at-once" in the user changes. This is a theoretical case, as it is very unlikely for a printer to have "one-at-a-time" set by default in its definition (.def.json) file and still be able to support more than one extruders. But it is a world full of possibilities out there, so you never know... CURA-7827 --- cura/Settings/MachineManager.py | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/cura/Settings/MachineManager.py b/cura/Settings/MachineManager.py index a9ee9e5096..79bd784f76 100755 --- a/cura/Settings/MachineManager.py +++ b/cura/Settings/MachineManager.py @@ -877,19 +877,21 @@ class MachineManager(QObject): user_changes_container = self._global_container_stack.userChanges quality_changes_container = self._global_container_stack.qualityChanges - print_sequence_in_quality_changes = quality_changes_container.getProperty(setting_key, "value") - print_sequence_in_user_changes = user_changes_container.getProperty(setting_key, "value") + print_sequence_quality_changes = quality_changes_container.getProperty(setting_key, "value") + print_sequence_user_changes = user_changes_container.getProperty(setting_key, "value") - # If the quality changes has the wrong value, then set the correct value in the user changes - if print_sequence_in_quality_changes and print_sequence_in_quality_changes != new_value: - user_changes_container.setProperty(setting_key, "value", new_value) - Logger.log("d", "Setting '{}' in '{}' to '{}' because there are more than 1 enabled extruders.".format(setting_key, user_changes_container, new_value)) - # If the quality changes has no value or the correct value and the user changes container has the wrong value, - # then reset the setting in the user changes (so that the circular revert-changes arrow will now show up in the - # interface) - elif print_sequence_in_user_changes and print_sequence_in_user_changes != new_value: + # If the user changes container has a value and its the incorrect value, then reset the setting in the user + # changes (so that the circular revert-changes arrow will now show up in the interface) + if print_sequence_user_changes and print_sequence_user_changes != new_value: user_changes_container.removeInstance(setting_key) Logger.log("d", "Resetting '{}' in container '{}' because there are more than 1 enabled extruders.".format(setting_key, user_changes_container)) + # If the print sequence doesn't exist in either the user changes or the quality changes (yet it still has the + # wrong value in the global stack), or it exists in the quality changes and it has the wrong value, then set it + # in the user changes + elif (not print_sequence_quality_changes and not print_sequence_user_changes) \ + or (print_sequence_quality_changes and print_sequence_quality_changes != new_value): + user_changes_container.setProperty(setting_key, "value", new_value) + Logger.log("d", "Setting '{}' in '{}' to '{}' because there are more than 1 enabled extruders.".format(setting_key, user_changes_container, new_value)) def setActiveMachineExtruderCount(self, extruder_count: int) -> None: """Set the amount of extruders on the active machine (global stack) From 69e0e1c4e59f1052dbd9be3ac4cffd4776c5c11a Mon Sep 17 00:00:00 2001 From: Kostas Karmas Date: Mon, 9 Nov 2020 17:50:45 +0100 Subject: [PATCH 10/11] Remove outdated tests CURA-7827 --- tests/TestMachineManager.py | 37 ------------------------------------- 1 file changed, 37 deletions(-) diff --git a/tests/TestMachineManager.py b/tests/TestMachineManager.py index b415bad59c..788b8eee41 100644 --- a/tests/TestMachineManager.py +++ b/tests/TestMachineManager.py @@ -255,40 +255,3 @@ def test_isActiveQualityNotSupported(machine_manager): def test_isActiveQualityNotSupported_noQualityGroup(machine_manager): machine_manager.activeQualityGroup = MagicMock(return_value=None) assert not machine_manager.isActiveQualitySupported - - -def test_correctPrintSequence_with2ExtrudersEnabled(machine_manager, application): - mocked_stack = application.getGlobalContainerStack() - - # The definition changes container reports 2 enabled extruders so the correctPrintSequence should attempt to - # change the print_sequence setting to all-at-once - mocked_definition_changes_container = MagicMock(name="DefinitionChangesContainer") - mocked_definition_changes_container.getProperty = lambda key, prop: 2 if (key == "extruders_enabled_count" and prop == "value") else -1 - - mocked_user_changes_container = MagicMock(name="UserChangesContainer") - - mocked_stack.userChanges = mocked_user_changes_container - mocked_stack.definitionChanges = mocked_definition_changes_container - - machine_manager._correctPrintSequence() - - # After the function is called, the user changes container should attempt to correct its print sequence to all-at-once - mocked_user_changes_container.setProperty.assert_called_once_with("print_sequence", "value", "all_at_once") - - -def test_correctPrintSequence_with1ExtruderEnabled(machine_manager, application): - mocked_stack = application.getGlobalContainerStack() - - # The definition changes container reports 1 enabled extruder so the correctPrintSequence should not change anything - mocked_definition_changes_container = MagicMock(name="DefinitionChangesContainer") - mocked_definition_changes_container.getProperty = lambda key, prop: 1 if (key == "extruders_enabled_count" and prop == "value") else -1 - - mocked_user_changes_container = MagicMock(name="UserChangesContainer") - - mocked_stack.userChanges = mocked_user_changes_container - mocked_stack.definitionChanges = mocked_definition_changes_container - - machine_manager._correctPrintSequence() - - # After the function is called, the user changes container should not have tried to change any properties - assert not mocked_user_changes_container.setProperty.called, "Method should not have been called" From 4b35bd1724cd0c0c8da5b85f6627a9678918bd0d Mon Sep 17 00:00:00 2001 From: Kostas Karmas Date: Mon, 9 Nov 2020 18:10:50 +0100 Subject: [PATCH 11/11] Correct the print sequence when discarding the current changes CURA-7827 --- cura/Settings/ContainerManager.py | 3 +++ cura/Settings/MachineManager.py | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/cura/Settings/ContainerManager.py b/cura/Settings/ContainerManager.py index 80a0d64474..08fdf707cf 100644 --- a/cura/Settings/ContainerManager.py +++ b/cura/Settings/ContainerManager.py @@ -345,6 +345,9 @@ class ContainerManager(QObject): # user changes are possibly added to make the current setup match the current enabled extruders machine_manager.correctExtruderSettings() + # The Print Sequence should be changed to match the current setup + machine_manager.correctPrintSequence() + for container in send_emits_containers: container.sendPostponedEmits() diff --git a/cura/Settings/MachineManager.py b/cura/Settings/MachineManager.py index 79bd784f76..1a2ab72a33 100755 --- a/cura/Settings/MachineManager.py +++ b/cura/Settings/MachineManager.py @@ -128,7 +128,7 @@ class MachineManager(QObject): self.activeQualityChangesGroupChanged.connect(self.activeQualityDisplayNameChanged) self.activeStackValueChanged.connect(self._reCalculateNumUserSettings) - self.numberExtrudersEnabledChanged.connect(self._correctPrintSequence) + self.numberExtrudersEnabledChanged.connect(self.correctPrintSequence) activeQualityDisplayNameChanged = pyqtSignal() @@ -858,7 +858,7 @@ class MachineManager(QObject): title = catalog.i18nc("@info:title", "Settings updated")) caution_message.show() - def _correctPrintSequence(self) -> None: + def correctPrintSequence(self) -> None: """ Sets the Print Sequence setting to "all-at-once" when there are more than one enabled extruders.