search

hardcodet / wpf-notifyicon

NotifyIcon (aka system tray icon or taskbar icon) for the WPF platform
Other
810 stars 125 forks source link

Implement WM_ContextMenu #53

Closed AliveDevil closed 3 years ago

AliveDevil commented 3 years ago

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.

Lakritzator commented 3 years ago

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.

Lakritzator commented 3 years ago

Is there any chance you can also check the other TODOs in the WindowMessageSink? They are NIN_SELECT & NIN_KEYSELECT as described here: https://docs.microsoft.com/en-us/windows/win32/api/shellapi/nf-shellapi-shell_notifyiconw

AliveDevil commented 3 years ago

I think it would be beneficial to add those aswell. Already found that Space and Enter don‘t activate the context menu. Eventually I can drop that casting and shifting around by adapting GET_X_LPARAM and GET_Y_LPARAM. According to documentation those macros should enable multi monitor scenarios. I‘ll convert the PR to a draft until I‘m finished and verified correct function.

AliveDevil commented 3 years ago

Checked and implemented NIN_SELECT and NIN_KEYSELECT - implementation is equal to WM_CONTEXTMENU. Removed the event handler for contextmenu, and used the ShowContextMenu-implementation.

Lakritzator commented 3 years ago

Looks great from a code perspective, I will have a quick try later.

Lakritzator commented 3 years ago

@punker76 @hardcodet this PR solves some of the small TODOs that were still open for keyboard usage. I've approved the changes, I also had a quick test, looks good. But as we forgot to discuss how we move forward with the branches, I don't like to merge this right away. Also we might need to talk about how to move forward with next versions.

punker76 commented 3 years ago

@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.

Lakritzator commented 3 years ago

@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.

punker76 commented 3 years ago

@Lakritzator The missing branch was a mistake by me

Lakritzator commented 3 years ago

@Lakritzator The missing branch was a mistake by me

Okay, push it

punker76 commented 3 years ago

@Lakritzator done

Lakritzator commented 3 years ago

@AliveDevil thank you for your work.

I currently can't tell when this is released, keep an eye out on the project.