WIXFEAT:5319 Remove IVariables since they were a leaky abstraction.

5 files changed, 171 insertions, 249 deletions

diff --git a/src/WixToolset.Mba.Core/Engine.cs b/src/WixToolset.Mba.Core/Engine.cs
index 2c544f29..98427cfa 100644
--- a/src/WixToolset.Mba.Core/Engine.cs
+++ b/src/WixToolset.Mba.Core/Engine.cs
@@ -18,10 +18,6 @@ namespace WixToolset.Mba.Core
         private static readonly string normalizeVersionFormatString = "{0} must be less than or equal to " + UInt16.MaxValue;
 
         private IBootstrapperEngine engine;
-        private Variables<long> numericVariables;
-        private Variables<SecureString> secureStringVariables;
-        private Variables<string> stringVariables;
-        private Variables<Version> versionVariables;
 
         /// <summary>
         /// Creates a new instance of the <see cref="Engine"/> container class.
@@ -30,135 +26,6 @@ namespace WixToolset.Mba.Core
         internal Engine(IBootstrapperEngine engine)
         {
             this.engine = engine;
-
-            // Wrap the calls to get and set numeric variables.
-            this.numericVariables = new Variables<long>(
-                delegate(string name)
-                {
-                    long value;
-                    int ret = this.engine.GetVariableNumeric(name, out value);
-                    if (NativeMethods.S_OK != ret)
-                    {
-                        throw new Win32Exception(ret);
-                    }
-
-                    return value;
-                },
-                delegate(string name, long value)
-                {
-                    this.engine.SetVariableNumeric(name, value);
-                },
-                delegate(string name)
-                {
-                    long value;
-                    int ret = this.engine.GetVariableNumeric(name, out value);
-
-                    return NativeMethods.E_NOTFOUND != ret;
-                }
-            );
-
-            // Wrap the calls to get and set string variables using SecureStrings.
-            this.secureStringVariables = new Variables<SecureString>(
-                delegate(string name)
-                {
-                    var pUniString = this.getStringVariable(name, out var length);
-                    try
-                    {
-                        return this.convertToSecureString(pUniString, length);
-                    }
-                    finally
-                    {
-                        if (IntPtr.Zero != pUniString)
-                        {
-                            Marshal.FreeCoTaskMem(pUniString);
-                        }
-                    }
-                },
-                delegate(string name, SecureString value)
-                {
-                    IntPtr pValue = Marshal.SecureStringToCoTaskMemUnicode(value);
-                    try
-                    {
-                        this.engine.SetVariableString(name, pValue, true);
-                    }
-                    finally
-                    {
-                        Marshal.FreeCoTaskMem(pValue);
-                    }
-                },
-                delegate(string name)
-                {
-                    return this.containsVariable(name);
-                }
-            );
-
-            // Wrap the calls to get and set string variables.
-            this.stringVariables = new Variables<string>(
-                delegate(string name)
-                {
-                    int length;
-                    IntPtr pUniString = this.getStringVariable(name, out length);
-                    try
-                    {
-                        return Marshal.PtrToStringUni(pUniString, length);
-                    }
-                    finally
-                    {
-                        if (IntPtr.Zero != pUniString)
-                        {
-                            Marshal.FreeCoTaskMem(pUniString);
-                        }
-                    }
-                },
-                delegate(string name, string value)
-                {
-                    IntPtr pValue = Marshal.StringToCoTaskMemUni(value);
-                    try
-                    {
-                        this.engine.SetVariableString(name, pValue, true);
-                    }
-                    finally
-                    {
-                        Marshal.FreeCoTaskMem(pValue);
-                    }
-                },
-                delegate(string name)
-                {
-                    return this.containsVariable(name);
-                }
-            );
-
-            // Wrap the calls to get and set version variables.
-            this.versionVariables = new Variables<Version>(
-                delegate(string name)
-                {
-                    long value;
-                    int ret = this.engine.GetVariableVersion(name, out value);
-                    if (NativeMethods.S_OK != ret)
-                    {
-                        throw new Win32Exception(ret);
-                    }
-
-                    return LongToVersion(value);
-                },
-                delegate(string name, Version value)
-                {
-                    long version = VersionToLong(value);
-                    this.engine.SetVariableVersion(name, version);
-                },
-                delegate(string name)
-                {
-                    long value;
-                    int ret = this.engine.GetVariableVersion(name, out value);
-
-                    return NativeMethods.E_NOTFOUND != ret;
-                }
-            );
-        }
-
-        public IVariables<long> NumericVariables
-        {
-            get { return this.numericVariables; }
         }
 
         public int PackageCount
@@ -172,21 +39,6 @@ namespace WixToolset.Mba.Core
             }
         }
 
-        public IVariables<SecureString> SecureStringVariables
-        {
-            get { return this.secureStringVariables; }
-        }
-
-        public IVariables<string> StringVariables
-        {
-            get { return this.stringVariables; }
-        }
-
-        public IVariables<Version> VersionVariables
-        {
-            get { return this.versionVariables; }
-        }
-
         public void Apply(IntPtr hwndParent)
         {
             this.engine.Apply(hwndParent);
@@ -197,6 +49,13 @@ namespace WixToolset.Mba.Core
             this.engine.CloseSplashScreen();
         }
 
+        public bool ContainsVariable(string name)
+        {
+            int capacity = 0;
+            int ret = this.engine.GetVariableString(name, IntPtr.Zero, ref capacity);
+            return NativeMethods.E_NOTFOUND != ret;
+        }
+
         public void Detect()
         {
             this.Detect(IntPtr.Zero);
@@ -275,6 +134,61 @@ namespace WixToolset.Mba.Core
             return sb.ToString();
         }
 
+        public long GetVariableNumeric(string name)
+        {
+            int ret = this.engine.GetVariableNumeric(name, out long value);
+            if (NativeMethods.S_OK != ret)
+            {
+                throw new Win32Exception(ret);
+            }
+
+            return value;
+        }
+
+        public SecureString GetVariableSecureString(string name)
+        {
+            var pUniString = this.getStringVariable(name, out var length);
+            try
+            {
+                return this.convertToSecureString(pUniString, length);
+            }
+            finally
+            {
+                if (IntPtr.Zero != pUniString)
+                {
+                    Marshal.FreeCoTaskMem(pUniString);
+                }
+            }
+        }
+
+        public string GetVariableString(string name)
+        {
+            int length;
+            IntPtr pUniString = this.getStringVariable(name, out length);
+            try
+            {
+                return Marshal.PtrToStringUni(pUniString, length);
+            }
+            finally
+            {
+                if (IntPtr.Zero != pUniString)
+                {
+                    Marshal.FreeCoTaskMem(pUniString);
+                }
+            }
+        }
+
+        public Version GetVariableVersion(string name)
+        {
+            int ret = this.engine.GetVariableVersion(name, out long value);
+            if (NativeMethods.S_OK != ret)
+            {
+                throw new Win32Exception(ret);
+            }
+
+            return LongToVersion(value);
+        }
+
         public void LaunchApprovedExe(IntPtr hwndParent, string approvedExeForElevationId, string arguments)
         {
             this.LaunchApprovedExe(hwndParent, approvedExeForElevationId, arguments, 0);
@@ -310,6 +224,43 @@ namespace WixToolset.Mba.Core
             this.engine.SetDownloadSource(packageOrContainerId, payloadId, url, user, password);
         }
 
+        public void SetVariable(string name, long value)
+        {
+            this.engine.SetVariableNumeric(name, value);
+        }
+
+        public void SetVariable(string name, SecureString value, bool formatted)
+        {
+            IntPtr pValue = Marshal.SecureStringToCoTaskMemUnicode(value);
+            try
+            {
+                this.engine.SetVariableString(name, pValue, formatted);
+            }
+            finally
+            {
+                Marshal.FreeCoTaskMem(pValue);
+            }
+        }
+
+        public void SetVariable(string name, string value, bool formatted)
+        {
+            IntPtr pValue = Marshal.StringToCoTaskMemUni(value);
+            try
+            {
+                this.engine.SetVariableString(name, pValue, formatted);
+            }
+            finally
+            {
+                Marshal.FreeCoTaskMem(pValue);
+            }
+        }
+
+        public void SetVariable(string name, Version value)
+        {
+            long version = VersionToLong(value);
+            this.engine.SetVariableVersion(name, version);
+        }
+
         public int SendEmbeddedError(int errorCode, string message, int uiHint)
         {
             int result = 0;
@@ -329,49 +280,6 @@ namespace WixToolset.Mba.Core
             this.engine.Quit(exitCode);
         }
 
-        internal sealed class Variables<T> : IVariables<T>
-        {
-            // .NET 2.0 does not support Func<T, TResult> or Action<T1, T2>.
-            internal delegate T Getter(string name);
-            internal delegate void Setter(string name, T value);
-
-            private Getter getter;
-            private Setter setter;
-            private Predicate<string> contains;
-
-            internal Variables(Getter getter, Setter setter, Predicate<string> contains)
-            {
-                this.getter = getter;
-                this.setter = setter;
-                this.contains = contains;
-            }
-
-            public T this[string name]
-            {
-                get { return this.getter(name); }
-                set { this.setter(name, value); }
-            }
-
-            public bool Contains(string name)
-            {
-                return this.contains(name);
-            }
-        }
-
-        /// <summary>
-        /// Gets whether the variable given by <paramref name="name"/> exists.
-        /// </summary>
-        /// <param name="name">The name of the variable to check.</param>
-        /// <returns>True if the variable given by <paramref name="name"/> exists; otherwise, false.</returns>
-        internal bool containsVariable(string name)
-        {
-            int capacity = 0;
-            IntPtr pValue = IntPtr.Zero;
-            int ret = this.engine.GetVariableString(name, pValue, ref capacity);
-
-            return NativeMethods.E_NOTFOUND != ret;
-        }
-
         /// <summary>
         /// Gets the variable given by <paramref name="name"/> as a string.
         /// </summary>
diff --git a/src/WixToolset.Mba.Core/IEngine.cs b/src/WixToolset.Mba.Core/IEngine.cs
index 5ddc4176..84d93bb0 100644
--- a/src/WixToolset.Mba.Core/IEngine.cs
+++ b/src/WixToolset.Mba.Core/IEngine.cs
@@ -9,38 +9,11 @@ namespace WixToolset.Mba.Core
     public interface IEngine
     {
         /// <summary>
-        /// Gets or sets numeric variables for the engine.
-        /// </summary>
-        IVariables<long> NumericVariables { get; }
-
-        /// <summary>
         /// Gets the number of packages in the bundle.
         /// </summary>
         int PackageCount { get; }
 
         /// <summary>
-        /// Gets or sets string variables for the engine using SecureStrings.
-        /// </summary>
-        IVariables<SecureString> SecureStringVariables { get; }
-
-        /// <summary>
-        /// Gets or sets string variables for the engine.
-        /// </summary>
-        IVariables<string> StringVariables { get; }
-
-        /// <summary>
-        /// Gets or sets <see cref="Version"/> variables for the engine.
-        /// 
-        /// The <see cref="Version"/> class can keep track of when the build and revision fields are undefined, but the engine can't.
-        /// Therefore, the build and revision fields must be defined when setting a <see cref="Version"/> variable.
-        /// Use the NormalizeVersion method to make sure the engine can accept the Version.
-        /// 
-        /// To keep track of versions without build or revision fields, use StringVariables instead.
-        /// </summary>
-        /// <exception cref="OverflowException">The given <see cref="Version"/> was invalid.</exception>
-        IVariables<Version> VersionVariables { get; }
-
-        /// <summary>
         /// Install the packages.
         /// </summary>
         /// <param name="hwndParent">The parent window for the installation user interface.</param>
@@ -53,6 +26,13 @@ namespace WixToolset.Mba.Core
         void CloseSplashScreen();
 
         /// <summary>
+        /// Checks if a variable exists in the engine.
+        /// </summary>
+        /// <param name="name">The name of the variable.</param>
+        /// <returns>Whether the variable exists.</returns>
+        bool ContainsVariable(string name);
+
+        /// <summary>
         /// Determine if all installation conditions are fulfilled.
         /// </summary>
         void Detect();
@@ -95,6 +75,30 @@ namespace WixToolset.Mba.Core
         string FormatString(string format);
 
         /// <summary>
+        /// Gets numeric variables for the engine.
+        /// </summary>
+        /// <param name="name">The name of the variable.</param>
+        long GetVariableNumeric(string name);
+
+        /// <summary>
+        /// Gets string variables for the engine using SecureStrings.
+        /// </summary>
+        /// <param name="name">The name of the variable.</param>
+        SecureString GetVariableSecureString(string name);
+
+        /// <summary>
+        /// Gets string variables for the engine.
+        /// </summary>
+        /// <param name="name">The name of the variable.</param>
+        string GetVariableString(string name);
+
+        /// <summary>
+        /// Gets <see cref="Version"/> variables for the engine.
+        /// </summary>
+        /// <param name="name">The name of the variable.</param>
+        Version GetVariableVersion(string name);
+
+        /// <summary>
         /// Launches a preapproved executable elevated.  As long as the engine already elevated, there will be no UAC prompt.
         /// </summary>
         /// <param name="hwndParent">The parent window of the elevation dialog (if the engine hasn't elevated yet).</param>
@@ -153,6 +157,43 @@ namespace WixToolset.Mba.Core
         void SetDownloadSource(string packageOrContainerId, string payloadId, string url, string user, string password);
 
         /// <summary>
+        /// Sets numeric variables for the engine.
+        /// </summary>
+        /// <param name="name">The name of the variable.</param>
+        /// <param name="value">The value to set.</param>
+        void SetVariable(string name, long value);
+
+        /// <summary>
+        /// Sets string variables for the engine using SecureStrings.
+        /// </summary>
+        /// <param name="name">The name of the variable.</param>
+        /// <param name="value">The value to set.</param>
+        /// <param name="formatted">False if the value is a literal string.</param>
+        void SetVariable(string name, SecureString value, bool formatted);
+
+        /// <summary>
+        /// Sets string variables for the engine.
+        /// </summary>
+        /// <param name="name">The name of the variable.</param>
+        /// <param name="value">The value to set.</param>
+        /// <param name="formatted">False if the value is a literal string.</param>
+        void SetVariable(string name, string value, bool formatted);
+
+        /// <summary>
+        /// Sets <see cref="Version"/> variables for the engine.
+        /// 
+        /// The <see cref="Version"/> class can keep track of when the build and revision fields are undefined, but the engine can't.
+        /// Therefore, the build and revision fields must be defined when setting a <see cref="Version"/> variable.
+        /// Use the NormalizeVersion method to make sure the engine can accept the Version.
+        /// 
+        /// To keep track of versions without build or revision fields, use StringVariables instead.
+        /// </summary>
+        /// <param name="name">The name of the variable.</param>
+        /// <param name="value">The value to set.</param>
+        /// <exception cref="OverflowException">The given <see cref="Version"/> was invalid.</exception>
+        void SetVariable(string name, Version value);
+
+        /// <summary>
         /// Sends error message when embedded.
         /// </summary>
         /// <param name="errorCode">Error code.</param>
diff --git a/src/WixToolset.Mba.Core/IVariables.cs b/src/WixToolset.Mba.Core/IVariables.cs
deleted file mode 100644
index c7994f0b..00000000
--- a/src/WixToolset.Mba.Core/IVariables.cs
+++ /dev/null
@@ -1,27 +0,0 @@
-// Copyright (c) .NET Foundation and contributors. All rights reserved. Licensed under the Microsoft Reciprocal License. See LICENSE.TXT file in the project root for full license information.
-
-namespace WixToolset.Mba.Core
-{
-    using System;
-
-    /// <summary>
-    /// An accessor for numeric, string, and version variables for the engine.
-    /// </summary>
-    public interface IVariables<T>
-    {
-        /// <summary>
-        /// Gets or sets the variable given by <paramref name="name"/>.
-        /// </summary>
-        /// <param name="name">The name of the variable to get/set.</param>
-        /// <returns>The value of the given variable.</returns>
-        /// <exception cref="Exception">An error occurred getting the variable.</exception>
-        T this[string name] { get; set; }
-
-        /// <summary>
-        /// Gets whether the variable given by <paramref name="name"/> exists.
-        /// </summary>
-        /// <param name="name">The name of the variable to check.</param>
-        /// <returns>True if the variable given by <paramref name="name"/> exists; otherwise, false.</returns>
-        bool Contains(string name);
-    }
-}
diff --git a/src/balutil/balutil.cpp b/src/balutil/balutil.cpp
index ebfaede4..faca70f5 100644
--- a/src/balutil/balutil.cpp
+++ b/src/balutil/balutil.cpp
@@ -167,7 +167,7 @@ LExit:
 }
 
 
-DAPI_(BOOL) BalStringVariableExists(
+DAPI_(BOOL) BalVariableExists(
     __in_z LPCWSTR wzVariable
     )
 {
@@ -183,7 +183,7 @@ DAPI_(BOOL) BalStringVariableExists(
     hr = vpEngine->GetVariableString(wzVariable, NULL, &cch);
 
 LExit:
-    return E_MOREDATA == hr; // string exists only if there are more than zero characters in the variable.
+    return E_NOTFOUND != hr;
 }
 
 
diff --git a/src/balutil/inc/balutil.h b/src/balutil/inc/balutil.h
index b718e48b..affa4925 100644
--- a/src/balutil/inc/balutil.h
+++ b/src/balutil/inc/balutil.h
@@ -108,10 +108,10 @@ DAPI_(HRESULT) BalSetNumericVariable(
     );
 
 /*******************************************************************
-BalStringVariableExists - checks if a string variable exists in the engine.
+BalVariableExists - checks if a variable exists in the engine.
 
 ********************************************************************/
-DAPI_(BOOL) BalStringVariableExists(
+DAPI_(BOOL) BalVariableExists(
     __in_z LPCWSTR wzVariable
     );