Implement WM_ContextMenu #53
Conversation
|
Definitively a useful addition, I planed this initially (this is why there was a placeholder) but somehow it never got in. I will make a small code review, because there is something I don't see as very useful. |
src/NotifyIconWpf/Interop/WinApi.cs
Outdated
| @@ -87,5 +87,15 @@ internal static class WinApi | |||
|
|
|||
| [DllImport(User32, SetLastError = true)] | |||
| public static extern bool GetCursorPos(ref Point lpPoint); | |||
|
|
|||
There was a problem hiding this comment.
I'm all for clean code and reuse, but somethings we should not over engineer code.
I actually don't see any re-use for this, unless there is no need for this elsewhere I would suggest to just have this inside the switch statement with the WindowsMessages.WM_CONTEXTMENU
src/NotifyIconWpf/TaskbarIcon.cs
Outdated
| @@ -132,6 +132,7 @@ public TaskbarIcon() | |||
|
|
|||
| // register event listeners | |||
| messageSink.MouseEventReceived += OnMouseEvent; | |||
| messageSink.ContextMenuReceived += OnContextMenuEvent; | |||
There was a problem hiding this comment.
I'm not sure if the added redirect via the OnContextMenuEvent adds anything to clear up the code, maybe we just do the following?
messageSink.ContextMenuReceived += ShowContextMenu;
There was a problem hiding this comment.
I don't know whether DPI has any effect on the wParam-Position, so I just added another method here.
| @@ -242,8 +248,11 @@ private void ProcessWindowMessage(uint msg, IntPtr wParam, IntPtr lParam) | |||
| switch (message) | |||
| { | |||
| case WindowsMessages.WM_CONTEXTMENU: | |||
| // TODO: Handle WM_CONTEXTMENU, see https://docs.microsoft.com/en-us/windows/win32/api/shellapi/nf-shellapi-shell_notifyiconw | |||
| Debug.WriteLine("Unhandled WM_CONTEXTMENU"); | |||
| ContextMenuReceived?.Invoke(new Point() | |||
There was a problem hiding this comment.
As mentioned in the other comment, why not place the code to do the low / high bit logic directly here, as it's probably not used elsewhere.
|
Is there any chance you can also check the other TODOs in the WindowMessageSink? |
|
I think it would be beneficial to add those aswell. |
|
Checked and implemented NIN_SELECT and NIN_KEYSELECT - implementation is equal to WM_CONTEXTMENU. |
|
Looks great from a code perspective, I will have a quick try later. |
|
@punker76 @hardcodet this PR solves some of the small TODOs that were still open for keyboard usage. |
|
@Lakritzator IMO we should work on the develop branch until we want to release a new version which will be merged to the master and a new branch like release/x.x.x. |
|
@punker76 I should have added my suggestion... yes, work in develop, and branch out. The 1.1 hasn't been branched out, I will do so, so I can merge this. |
|
@Lakritzator The missing branch was a mistake by me |
Okay, push it |
|
@Lakritzator done |
|
@AliveDevil thank you for your work. I currently can't tell when this is released, keep an eye out on the project. |
Needed this functionality as a user requested it once. Would appreciate that this is taken upstream, instead of relying on our private fork, from 2016.