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
|
|
||
| [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
|
|
||
| // 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.
| 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. |
3 participants
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.