Removal of TimerHook and update Kernel32/User32/HotKey #3663
Conversation
…rom Win32 API which will not necessarily throw exceptions.
RetailCoder.VBE/Common/TimerHook.cs
Outdated
| @@ -31,8 +33,27 @@ public void Attach() | |||
| try | |||
| { | |||
| var timerId = (IntPtr)Kernel32.GlobalAddAtom(Guid.NewGuid().ToString()); | |||
There was a problem hiding this comment.
So I'm reading the documentation on the SetTimer and there is no mention of needing a global atom. I'm not sure what purpose the global atom is supposed to serve here, given that we are generating a new GUID every time we invoke the Attach which will never match any existing entries in the global atom table. Can anyone explain why we need this? Otherwise, this strikes me as a cargo-cult programming...
There was a problem hiding this comment.
+1, although you need to supply a timer ID of some sort. The relevant part of the documentation says "If the hWnd parameter is not NULL and the window specified by hWnd already has a timer with the value nIDEvent, then the existing timer is replaced by the new timer". The rest of it (e.g. saying that a new timer ID will be allocated for you) refers to where hWnd is being passed a NULL, so doesn't seem to apply.
Using a global atom is clearly not a requirement though. You just need to be prepared for your specified ID to be already assigned to a timer (e.g. by another addin, or the VBE itself), so make it something as unique as you can to avoid conflicts. It's rubbish actually, because if it just replaces the existing timer hook, then it gives you no chance to try another ID without affecting the old one that we might not have created.
RetailCoder.VBE/Common/TimerHook.cs
Outdated
| using Rubberduck.Common.WinAPI; | ||
| using NLog; | ||
|
|
||
| namespace Rubberduck.Common | ||
| { | ||
| public class TimerHook : IAttachable, IDisposable |
There was a problem hiding this comment.
Is this class even in use? It is referenced in the RubberduckIocInstaller but by time RD loads, the class is never instantiated nor its methods called....
There was a problem hiding this comment.
... indeed, that does appear to be the case! I love wasting my time ;-)
There was a problem hiding this comment.
I think it is a remenant of the old timed logger interceptor, which has been removed because there has been the feeling that the inability to proply use reflection on the intercepted inspections (to get their type) was too much of a problem.
There was a problem hiding this comment.
Ok, if there's no objection, I will revise the PR to delete the file altogether and remove it from the IoC installer.
There was a problem hiding this comment.
sounds good. If it's never used, it should be deleted 👍
Remove TimerHook's entry from the IoC. Clean up of Kernel32 and User32, removing unused declares. Make calls to Kernel32 in HotKey compliant with documentation and explicitly checking the error status for all Kernel32 calls.
bclothier
changed the title
Closes #3662; add explicit logging for errors arising from Win32 API which will not necessarily throw exceptions.