Issue #1178863: Separate initialisation from setting when initializing

Tkinter.Variables; harmonize exceptions to ValueError; only delete variables
that have not been deleted; assert that variable names are strings
Patch by Andrew Svetlov.

3 files changed, 193 insertions, 12 deletions

diff --git a/Lib/tkinter/__init__.py b/Lib/tkinter/__init__.py
index f4f9583..35f724f 100644
--- a/Lib/tkinter/__init__.py
+++ b/Lib/tkinter/__init__.py
@@ -155,6 +155,7 @@ class Variable:
     Subclasses StringVar, IntVar, DoubleVar, BooleanVar are specializations
     that constrain the type of the value returned from get()."""
     _default = ""
+    _tk = None
     def __init__(self, master=None, value=None, name=None):
         """Construct a variable
 
@@ -165,6 +166,11 @@ class Variable:
         If NAME matches an existing variable and VALUE is omitted
         then the existing value is retained.
         """
+        # check for type of NAME parameter to override weird error message
+        # raised from Modules/_tkinter.c:SetVar like:
+        # TypeError: setvar() takes exactly 3 arguments (2 given)
+        if name is not None and not isinstance(name, str):
+            raise TypeError("name must be a string")
         global _varnum
         if not master:
             master = _default_root
@@ -176,18 +182,21 @@ class Variable:
             self._name = 'PY_VAR' + repr(_varnum)
             _varnum += 1
         if value is not None:
-            self.set(value)
+            self.initialize(value)
         elif not self._tk.call("info", "exists", self._name):
-            self.set(self._default)
+            self.initialize(self._default)
     def __del__(self):
         """Unset the variable in Tcl."""
-        self._tk.globalunsetvar(self._name)
+        if (self._tk is not None and self._tk.call("info", "exists",
+                                                   self._name)):
+            self._tk.globalunsetvar(self._name)
     def __str__(self):
         """Return the name of the variable in Tcl."""
         return self._name
     def set(self, value):
         """Set the variable to VALUE."""
         return self._tk.globalsetvar(self._name, value)
+    initialize = set
     def get(self):
         """Return value of variable."""
         return self._tk.globalgetvar(self._name)
@@ -262,12 +271,6 @@ class IntVar(Variable):
         """
         Variable.__init__(self, master, value, name)
 
-    def set(self, value):
-        """Set the variable to value, converting booleans to integers."""
-        if isinstance(value, bool):
-            value = int(value)
-        return Variable.set(self, value)
-
     def get(self):
         """Return the value of the variable as an integer."""
         return getint(self._tk.globalgetvar(self._name))
@@ -308,7 +311,10 @@ class BooleanVar(Variable):
 
     def get(self):
         """Return the value of the variable as a bool."""
-        return self._tk.getboolean(self._tk.globalgetvar(self._name))
+        try:
+            return self._tk.getboolean(self._tk.globalgetvar(self._name))
+        except TclError:
+            raise ValueError("invalid literal for getboolean()")
 
 def mainloop(n=0):
     """Run the main loop of Tcl."""
@@ -320,7 +326,10 @@ getdouble = float
 
 def getboolean(s):
     """Convert true and false to integer values 1 and 0."""
-    return _default_root.tk.getboolean(s)
+    try:
+        return _default_root.tk.getboolean(s)
+    except TclError:
+        raise ValueError("invalid literal for getboolean()")
 
 # Methods defined on both toplevel and interior widgets
 class Misc:
@@ -410,7 +419,10 @@ class Misc:
     getdouble = float
     def getboolean(self, s):
         """Return a boolean value for Tcl boolean values true and false given as parameter."""
-        return self.tk.getboolean(s)
+        try:
+            return self.tk.getboolean(s)
+        except TclError:
+            raise ValueError("invalid literal for getboolean()")
     def focus_set(self):
         """Direct input focus to this widget.
 
diff --git a/Lib/tkinter/test/test_tkinter/test_variables.py b/Lib/tkinter/test/test_tkinter/test_variables.py
new file mode 100644
index 0000000..8db7aca
--- /dev/null
+++ b/Lib/tkinter/test/test_tkinter/test_variables.py
@@ -0,0 +1,165 @@
+import unittest
+
+from tkinter import Variable, StringVar, IntVar, DoubleVar, BooleanVar, Tk
+
+
+class Var(Variable):
+
+    _default = "default"
+    side_effect = False
+
+    def set(self, value):
+        self.side_effect = True
+        super().set(value)
+
+
+class TestBase(unittest.TestCase):
+
+    def setUp(self):
+        self.root = Tk()
+
+    def tearDown(self):
+        self.root.destroy()
+
+
+class TestVariable(TestBase):
+
+    def test_default(self):
+        v = Variable(self.root)
+        self.assertEqual("", v.get())
+        self.assertRegex(str(v), r"^PY_VAR(\d+)$")
+
+    def test_name_and_value(self):
+        v = Variable(self.root, "sample string", "varname")
+        self.assertEqual("sample string", v.get())
+        self.assertEqual("varname", str(v))
+
+    def test___del__(self):
+        self.assertFalse(self.root.call("info", "exists", "varname"))
+        v = Variable(self.root, "sample string", "varname")
+        self.assertTrue(self.root.call("info", "exists", "varname"))
+        del v
+        self.assertFalse(self.root.call("info", "exists", "varname"))
+
+    def test_dont_unset_not_existing(self):
+        self.assertFalse(self.root.call("info", "exists", "varname"))
+        v1 = Variable(self.root, name="name")
+        v2 = Variable(self.root, name="name")
+        del v1
+        self.assertFalse(self.root.call("info", "exists", "name"))
+        # shouldn't raise exception
+        del v2
+        self.assertFalse(self.root.call("info", "exists", "name"))
+
+    def test___eq__(self):
+        # values doesn't matter, only class and name are checked
+        v1 = Variable(self.root, name="abc")
+        v2 = Variable(self.root, name="abc")
+        self.assertEqual(v1, v2)
+
+        v3 = Variable(self.root, name="abc")
+        v4 = StringVar(self.root, name="abc")
+        self.assertNotEqual(v3, v4)
+
+    def test_invalid_name(self):
+        with self.assertRaises(TypeError):
+            Variable(self.root, name=123)
+
+    def test_initialize(self):
+        v = Var()
+        self.assertFalse(v.side_effect)
+        v.set("value")
+        self.assertTrue(v.side_effect)
+
+
+class TestStringVar(TestBase):
+
+    def test_default(self):
+        v = StringVar(self.root)
+        self.assertEqual("", v.get())
+
+    def test_get(self):
+        v = StringVar(self.root, "abc", "name")
+        self.assertEqual("abc", v.get())
+        self.root.globalsetvar("name", True)
+        self.assertEqual("1", v.get())
+
+
+class TestIntVar(TestBase):
+
+    def test_default(self):
+        v = IntVar(self.root)
+        self.assertEqual(0, v.get())
+
+    def test_get(self):
+        v = IntVar(self.root, 123, "name")
+        self.assertEqual(123, v.get())
+        self.root.globalsetvar("name", "345")
+        self.assertEqual(345, v.get())
+
+    def test_invalid_value(self):
+        v = IntVar(self.root, name="name")
+        self.root.globalsetvar("name", "value")
+        with self.assertRaises(ValueError):
+            v.get()
+        self.root.globalsetvar("name", "345.0")
+        with self.assertRaises(ValueError):
+            v.get()
+
+
+class TestDoubleVar(TestBase):
+
+    def test_default(self):
+        v = DoubleVar(self.root)
+        self.assertEqual(0.0, v.get())
+
+    def test_get(self):
+        v = DoubleVar(self.root, 1.23, "name")
+        self.assertAlmostEqual(1.23, v.get())
+        self.root.globalsetvar("name", "3.45")
+        self.assertAlmostEqual(3.45, v.get())
+
+    def test_get_from_int(self):
+        v = DoubleVar(self.root, 1.23, "name")
+        self.assertAlmostEqual(1.23, v.get())
+        self.root.globalsetvar("name", "3.45")
+        self.assertAlmostEqual(3.45, v.get())
+        self.root.globalsetvar("name", "456")
+        self.assertAlmostEqual(456, v.get())
+
+    def test_invalid_value(self):
+        v = DoubleVar(self.root, name="name")
+        self.root.globalsetvar("name", "value")
+        with self.assertRaises(ValueError):
+            v.get()
+
+
+class TestBooleanVar(TestBase):
+
+    def test_default(self):
+        v = BooleanVar(self.root)
+        self.assertEqual(False, v.get())
+
+    def test_get(self):
+        v = BooleanVar(self.root, True, "name")
+        self.assertAlmostEqual(True, v.get())
+        self.root.globalsetvar("name", "0")
+        self.assertAlmostEqual(False, v.get())
+
+    def test_invalid_value_domain(self):
+        v = BooleanVar(self.root, name="name")
+        self.root.globalsetvar("name", "value")
+        with self.assertRaises(ValueError):
+            v.get()
+        self.root.globalsetvar("name", "1.0")
+        with self.assertRaises(ValueError):
+            v.get()
+
+
+tests_gui = (TestVariable, TestStringVar, TestIntVar,
+             TestDoubleVar, TestBooleanVar)
+
+
+if __name__ == "__main__":
+    from test.support import run_unittest
+    run_unittest(*tests_gui)
diff --git a/Misc/NEWS b/Misc/NEWS
index 7bbe890..dab53d3 100644
--- a/Misc/NEWS
+++ b/Misc/NEWS
@@ -24,6 +24,10 @@ Core and Builtins
 Library
 -------
 
+- Issue #1178863: Separate initialisation from setting when initializing
+  Tkinter.Variables; harmonize exceptions to ValueError; only delete variables
+  that have not been deleted; assert that variable names are strings.
+
 - Issue #14104: Implement time.monotonic() on Mac OS X, patch written by
   Nicholas Riley.