Merge with conflicts

Author: Hosch250

RetailCoder.VBE/AppMenu.cs

@@ -1,6 +1,8 @@
 using System;
 using System.Collections.Generic;
 using System.Linq;
+using System.Threading;
+using NLog;
 using Rubberduck.Parsing;
 using Rubberduck.Parsing.VBA;
 using Rubberduck.UI;
@@ -17,6 +19,8 @@ public class AppMenu : IAppMenu, IDisposable
         private readonly ISelectionChangeService _selectionService;
         private readonly RubberduckCommandBar _stateBar;
 
+        private static readonly Logger _logger = LogManager.GetCurrentClassLogger();
+
         public AppMenu(IEnumerable<IParentMenuItem> menus, IParseCoordinator parser, ISelectionChangeService selectionService, RubberduckCommandBar stateBar)
         {
             _menus = menus.ToList();
@@ -77,7 +81,13 @@ private void RemoveMenus()
         {
             foreach (var menu in _menus.Where(menu => menu.Item != null))
             {
+                _logger.Debug($"Starting removal of top-level menu {menu.GetType()}.");
                 menu.RemoveMenu();
+                //We do this here and not in the menu items because we only want to dispose of/release the parents of the top level parent menus.
+                //The parents further down get disposed of/released as part of the remove chain.
+                _logger.Trace($"Removing parent menu of top-level menu {menu.GetType()}.");
+                menu.Parent.Dispose();
+                menu.Parent = null;
             }
         }
     }

RetailCoder.VBE/Common/Hotkeys/Hotkey.cs

@@ -1,5 +1,6 @@
 using System;
 using System.Collections.Generic;
+using System.Runtime.InteropServices;
 using System.Windows.Forms;
 using Rubberduck.Common.WinAPI;
 using NLog;
@@ -32,7 +33,7 @@ public Hotkey(IntPtr hWndVbe, string key, CommandBase command, Keys secondKey =
         public Keys Combo { get; }
         public Keys SecondKey { get; }
         public bool IsTwoStepHotkey { get; }
-        public bool IsAttached { get; private set; }
+        public bool IsAttached => HotkeyInfo.HookId != IntPtr.Zero;
 
         public event EventHandler<HookEventArgs> MessageReceived;
 
@@ -68,10 +69,19 @@ public void Detach()
                 return;
             }
 
-            User32.UnregisterHotKey(_hWndVbe, HotkeyInfo.HookId);
+            if (!User32.UnregisterHotKey(_hWndVbe, HotkeyInfo.HookId))
+            {
+                Logger.Warn($"Error calling UnregisterHotKey on hokey with id {HotkeyInfo.HookId} for command of type {Command.GetType()}; the error was {Marshal.GetLastWin32Error()}; going to delete the atom anyway... The memory may leak.");
+            }
+            Kernel32.SetLastError(Kernel32.ERROR_SUCCESS);
             Kernel32.GlobalDeleteAtom(HotkeyInfo.HookId);
+            var lastError = Marshal.GetLastWin32Error();
+            if (lastError != Kernel32.ERROR_SUCCESS)
+            {
+                Logger.Warn($"Error calling DeleteGlobalAtom; the error was {lastError}, the id {HotkeyInfo.HookId} and the type of the associated command {Command.GetType()}.");
+            }
 
-            IsAttached = false;
+            HotkeyInfo = new HotkeyInfo(IntPtr.Zero, Combo);
             ClearCommandShortcutText();
         }
 
@@ -83,14 +93,21 @@ private void HookKey(Keys key, uint shift)
             }
 
             var hookId = (IntPtr)Kernel32.GlobalAddAtom(Guid.NewGuid().ToString());
+            if (hookId == IntPtr.Zero)
+            {
+                Logger.Warn($"Error calling GlobalAddAtom; the error was {Marshal.GetLastWin32Error()}; aborting the HookKey operation...");    
+                return;
+            }
+
             var success = User32.RegisterHotKey(_hWndVbe, hookId, shift, (uint)key);
             if (!success)
             {
                 Logger.Debug(RubberduckUI.CommonHotkey_KeyNotRegistered, key);
+                return;
             }
 
             HotkeyInfo = new HotkeyInfo(hookId, Combo);
-            IsAttached = true;
+            Logger.Trace($"Hotkey for the associated command {Command.GetType()} was registered with id {HotkeyInfo.HookId}");
         }
 
         private void SetCommandShortcutText()
@@ -108,8 +125,7 @@ private void ClearCommandShortcutText()
                 command.ShortcutText = string.Empty;
             }
         }
-
-
+        
         private static readonly IDictionary<char,uint> Modifiers = new Dictionary<char, uint>
         {
             { '+', (uint)KeyModifier.SHIFT },

RetailCoder.VBE/Common/Hotkeys/HotkeyFactory.cs

@@ -0,0 +1,30 @@
+using Rubberduck.Settings;
+using Rubberduck.UI.Command;
+using System;
+using System.Collections.Generic;
+using System.Linq;
+
+namespace Rubberduck.Common.Hotkeys
+{
+    public class HotkeyFactory
+    {
+        private readonly IEnumerable<CommandBase> _commands;
+
+        public HotkeyFactory(IEnumerable<CommandBase> commands)
+        {
+            _commands = commands;
+        }
+
+        public Hotkey Create(HotkeySetting setting, IntPtr hWndVbe)
+        {
+            if (setting == null)
+            {
+                return null;
+            }
+
+            var commandToBind = _commands.FirstOrDefault(command => command.GetType().Name == setting.CommandTypeName);
+
+            return commandToBind == null ? null : new Hotkey(hWndVbe, setting.ToString(), commandToBind);
+        }
+    }
+}

RetailCoder.VBE/Common/RubberduckHooks.cs

@@ -1,11 +1,9 @@
 using System;
 using System.Collections.Generic;
 using System.ComponentModel;
-using System.Diagnostics;
 using System.Linq;
 using Rubberduck.Common.Hotkeys;
 using Rubberduck.Settings;
-using Rubberduck.UI.Command;
 using NLog;
 using Rubberduck.VBEditor.SafeComWrappers.Abstract;
 using Rubberduck.VBEditor.WindowsApi;
@@ -15,20 +13,15 @@ namespace Rubberduck.Common
     public class RubberduckHooks : SubclassingWindow, IRubberduckHooks
     {
         private readonly IGeneralConfigService _config;
-        private readonly IEnumerable<CommandBase> _commands;
+        private readonly HotkeyFactory _hotkeyFactory;
         private readonly IList<IAttachable> _hooks = new List<IAttachable>();
         private static readonly Logger Logger = LogManager.GetCurrentClassLogger();
 
-        public RubberduckHooks(IVBE vbe, IGeneralConfigService config, IEnumerable<CommandBase> commands)
+        public RubberduckHooks(IVBE vbe, IGeneralConfigService config, HotkeyFactory hotkeyFactory)
             : base((IntPtr)vbe.MainWindow.HWnd, (IntPtr)vbe.MainWindow.HWnd)
         {
-            _commands = commands;
             _config = config;
-        }
-
-        private CommandBase Command(RubberduckHotkey hotkey)
-        {
-            return _commands.FirstOrDefault(s => s.Hotkey == hotkey);
+            _hotkeyFactory = hotkeyFactory;
         }
 
         public void HookHotkeys()
@@ -39,16 +32,15 @@ public void HookHotkeys()
             var config = _config.LoadConfiguration();
             var settings = config.UserSettings.HotkeySettings;
 
-            foreach (var hotkey in settings.Settings.Where(hotkey => hotkey.IsEnabled))
+            foreach (var hotkeySetting in settings.Settings.Where(hotkeySetting => hotkeySetting.IsEnabled))
             {
-                if (Enum.TryParse(hotkey.Name, out RubberduckHotkey assigned))
+                var hotkey = _hotkeyFactory.Create(hotkeySetting, Hwnd);
+                if (hotkey != null)
                 {
-                    var command = Command(assigned);
-                    Debug.Assert(command != null);
-
-                    AddHook(new Hotkey(Hwnd, hotkey.ToString(), command));
+                    AddHook(hotkey);
                 }
             }
+
             Attach();
         }
 
@@ -155,7 +147,6 @@ public override int SubClassProc(IntPtr hWnd, IntPtr msg, IntPtr wParam, IntPtr
                     break;
                 case WM.CLOSE:
                 case WM.DESTROY:
-                case WM.RUBBERDUCK_SINKING:
                     Detach();
                     break;
             }
@@ -180,5 +171,14 @@ private bool HandleHotkeyMessage(IntPtr wParam)
             }
             return processed;
         }
+
+        protected override void Dispose(bool disposing)
+        {
+            if (disposing)
+            {
+                Detach();
+            }
+            base.Dispose(disposing);
+        }
     }
 }

RetailCoder.VBE/Common/TimerHook.cs

@@ -29,27 +29,14 @@ public static class Kernel32
         /// <returns>The function always returns (ATOM) 0.</returns>
         [DllImport("kernel32.dll", SetLastError=true, ExactSpelling=true)]
         public static extern ushort GlobalDeleteAtom(IntPtr nAtom);
-
+        
         /// <summary>
-        /// Retrieves a module handle for the specified module. 
-        /// The module must have been loaded by the calling process.
+        /// Sets the last-error code for the calling thread.
         /// </summary>
-        /// <param name="lpModuleName">The name of the loaded module (either a .dll or .exe file). 
-        /// If the file name extension is omitted, the default library extension .dll is appended. 
-        /// The file name string can include a trailing point character (.) to indicate that the module name has no extension. 
-        /// The string does not have to specify a path. When specifying a path, be sure to use backslashes (\), not forward slashes (/). 
-        /// The name is compared (case independently) to the names of modules currently mapped into the address space of the calling process.</param>
-        /// <returns>If the function succeeds, the return value is a handle to the specified module. 
-        /// If the function fails, the return value is NULL. To get extended error information, call GetLastError.</returns>
-        /// <remarks>The returned handle is not global or inheritable. It cannot be duplicated or used by another process.
-        /// This function must be used carefully in a multithreaded application. There is no guarantee that the module handle remains valid between the time this function returns the handle and the time it is used. 
-        /// For example, suppose that a thread retrieves a module handle, but before it uses the handle, a second thread frees the module. 
-        /// If the system loads another module, it could reuse the module handle that was recently freed. 
-        /// Therefore, the first thread would have a handle to a different module than the one intended.
-        /// </remarks>
-        [DllImport("kernel32.dll", CharSet = CharSet.Auto)]
-        public static extern IntPtr GetModuleHandle(string lpModuleName);
-
+        /// <param name="dwErrorCode">The last-error code for the thread.</param>
+        [DllImport("kernel32.dll", SetLastError = true)]
+        public static extern void SetLastError(uint dwErrorCode);
 
+        public static uint ERROR_SUCCESS = 0;
     }
 }