From 0f1b6472d08d91625996017fa46019a41ec899f2 Mon Sep 17 00:00:00 2001
From: Mark Dickinson <mdickinson@enthought.com>
Date: Tue, 27 Aug 2024 07:50:27 +0100
Subject: [PATCH] Fix premature writing of preferences (#583)

This PR will eventually fix #582. The fix itself is straightforward, but
I first want to check that my regression test fails reliably in the
various CI workflows.

The new test also required apptools >= 5.3.0 to pass, so the EDM-based
tests won't pass until apptools 5.3.0 is available in EDS.
---
 envisage/ui/tasks/preferences_pane.py         | 10 ++-
 .../ui/tasks/tests/test_preferences_pane.py   | 66 +++++++++++++++++++
 2 files changed, 75 insertions(+), 1 deletion(-)
 create mode 100644 envisage/ui/tasks/tests/test_preferences_pane.py

diff --git a/envisage/ui/tasks/preferences_pane.py b/envisage/ui/tasks/preferences_pane.py
index f39d78b8..8422eb28 100644
--- a/envisage/ui/tasks/preferences_pane.py
+++ b/envisage/ui/tasks/preferences_pane.py
@@ -64,7 +64,15 @@ def trait_context(self):
             else:
                 raise ValueError("A preferences pane must have a model!")
 
-        self._model = self.model.clone_traits()
+        # Make sure that we don't clone the preferences trait, since that
+        # can lead to the preferences node being updated prematurely.
+        # xref: enthought/envisage#582
+        traits_to_clone = [
+            trait_name
+            for trait_name in self.model.copyable_trait_names()
+            if trait_name != "preferences"
+        ]
+        self._model = self.model.clone_traits(traits_to_clone)
         self._model.preferences = None
         return {"object": self._model, "controller": self, "handler": self}
 
diff --git a/envisage/ui/tasks/tests/test_preferences_pane.py b/envisage/ui/tasks/tests/test_preferences_pane.py
new file mode 100644
index 00000000..0111370b
--- /dev/null
+++ b/envisage/ui/tasks/tests/test_preferences_pane.py
@@ -0,0 +1,66 @@
+# (C) Copyright 2007-2024 Enthought, Inc., Austin, TX
+# All rights reserved.
+#
+# This software is provided without warranty under the terms of the BSD
+# license included in LICENSE.txt and may be redistributed only under
+# the conditions described in the aforementioned license. The license
+# is also available online at http://www.enthought.com/licenses/BSD.txt
+#
+# Thanks for using Enthought open source!
+
+"""Tests for the PreferencesPane."""
+
+import unittest
+
+from apptools.preferences.api import (
+    IPreferences,
+    Preferences,
+    PreferencesHelper,
+    ScopedPreferences,
+)
+from traits.api import Instance, Str
+from traitsui.api import Item, View
+
+from envisage.ui.tasks.api import PreferencesPane
+
+
+class MyPreferencesHelper(PreferencesHelper):
+    #: Redeclare preferences to force trait copying order.
+    preferences = Instance(IPreferences)
+
+    #: The node that contains the preferences.
+    preferences_path = Str("app")
+
+    #: The user's favourite colour
+    color = Str()
+
+
+class MyPreferencesPane(PreferencesPane):
+    model_factory = MyPreferencesHelper
+
+    view = View(Item("color"))
+
+
+class TestPreferencesPane(unittest.TestCase):
+    def test_no_preference_changes_without_apply(self):
+        # Regression test for https://github.com/enthought/envisage/issues/582
+
+        # Given scoped preferences where the default preferences layer has
+        # a value for app.color ...
+        default_preferences = Preferences(name="default")
+        application_preferences = Preferences(name="application")
+        preferences = ScopedPreferences(
+            scopes=[application_preferences, default_preferences]
+        )
+        default_preferences.set("app.color", "red")
+
+        # When we create the preferences helper and pane, and trigger the
+        # trait_context method (which will usually be called as part of
+        # creating the TraitsUI UI).
+        helper = MyPreferencesHelper(preferences=preferences)
+        self.assertIsNone(application_preferences.get("app.color"))
+        pane = MyPreferencesPane(model=helper)
+        pane.trait_context()["object"]
+
+        # Then the application preferences should not have been changed.
+        self.assertIsNone(application_preferences.get("app.color"))