View Issue Details

IDProjectCategoryView StatusLast Update
0035219PatchesPatchpublic2021-01-10 00:16
ReporterBenito van der Zander Assigned To 
PrioritynormalSeverityminorReproducibilityhave not tried
Status newResolutionreopened 
Product Version2.0 
Summary0035219: OnClick on opened submenu in popupmenu
DescriptionWhen a submenu in a popupmenu is opened, it needs to call onclick of the parent menuitem
TagsNo tags attached.
Fixed in Revisionr60679
LazTarget-
WidgetsetWin32/Win64
Attached Files

Relationships

related to 0037210 resolvedJuha Manninen OnClick on opened submenu in popupmenu 

Activities

Benito van der Zander

2019-03-13 12:30

reporter  

popupmenu.patch (999 bytes)   
Index: lcl/interfaces/win32/win32callback.inc
===================================================================
--- lcl/interfaces/win32/win32callback.inc	(Revision 60475)
+++ lcl/interfaces/win32/win32callback.inc	(Arbeitskopie)
@@ -401,14 +401,18 @@
 
 function TWindowProcHelper.GetPopMenuItemObject: TObject;
 var
-  MainMenuHandle: HMENU;
+  MenuHandle: HMENU;
   MenuInfo: MENUITEMINFO;
 begin
   MenuInfo.cbSize := MMenuItemInfoSize;
   MenuInfo.fMask := MIIM_DATA;
 
-  MainMenuHandle := GetMenuParent(HMENU(WParam), GetMenu(Window));
-  if GetMenuItemInfo(MainMenuHandle, LOWORD(LParam), true, @MenuInfo) then
+  MenuHandle := 0;
+  if Assigned(WindowInfo^.PopupMenu) then
+    MenuHandle := GetMenuParent(HMENU(WParam), WindowInfo^.PopupMenu.Handle);
+  if MenuHandle = 0 then
+    MenuHandle := GetMenuParent(HMENU(WParam), GetMenu(Window));
+  if GetMenuItemInfo(MenuHandle, LOWORD(LParam), true, @MenuInfo) then
     Result := TObject(MenuInfo.dwItemData)
   else
     Result := nil;
popupmenu.patch (999 bytes)   

Juha Manninen

2019-03-15 12:45

developer   ~0114836

Applied, thanks.

Richard Woolf

2020-06-13 04:20

reporter   ~0123410

I'm afraid this bugfix is incorrect.

[u][b]Summary:[/b][/u]
If you have a TPopupMenu with a TMenuItem (parent) with a [b]sub[/b] (child) TMenuItem, when you simply [b]hover[/b] on the parent TMenuItem:
Expected behaviour:
The Submenu Item should open.
Actual behaviour:
The Submenu Item opens, [b]AND[/b] it triggers the onClick event of the parent TMenuItem (if one exists) (if AutoCheck is false) [b]or[/b] toggles the 'checked' status of the parent TMenuItem (if AutoCheck is true)

[u][b]Steps to reproduce (if AutoCheck is set to true):[/b][/u]
1. Create a TPopupMenu
2. Inside it: Create a Menu Item (parent)
    Set Checked := true;
    Set AutoCheck := true; (or see below if you set AutoCheck to false)
3. Inside that menu item: Create a **sub** Menu item (child) to the right.
4. Run the program, and **hover** over the parent menu.
Problem: It toggles the 'checked' status of the parent MenuItem.

[u][b]Alternative (if AutoCheck set to false):[/b][/u]
1. Create a TPopupMenu
2. Inside it: Create a Menu Item (parent)
    Set Checked := true;
    Set AutoCheck := false;
3. Inside that menu item: Create a **sub** Menu item (child) to the right.
4. Create an **OnClick** event handler for the parent Menu Item:
[code=pascal]procedure TForm1.MenuItem1Click(Sender: TObject);
begin
  showmessage('Clicked!');
end;[/code]
5. Run the program, and **hover** over the parent menu.
Problem: It triggers the OnClick event.

I'm running Lazarus 2.0.6 on Windows 10.
Popup menu bug.jpg (16,380 bytes)   
Popup menu bug.jpg (16,380 bytes)   

Juha Manninen

2020-06-14 10:02

developer   ~0123428

Benito, the patch created a regression. Please see the related issue.

jamie philbrook

2020-07-05 16:16

reporter   ~0123764

I hope you reverted that patch because that it total BS.

Juha Manninen

2020-07-06 11:09

developer   ~0123771

I reverted r60679. I am detaching myself. Somebody with Windows as a development OS, please continue with this.

jamie philbrook

2020-07-06 12:29

reporter   ~0123775

There is no need to change it, it works correctly as it is/was.

 And why would you want the OnClick event of the parent to fire when merely opening a submenu ?

 I am only going by what the poster left for everyone else to read. If that was me leaving that patch and it got applied I could be grinning, but I am not that kind of a person.

Benito van der Zander

2020-07-06 13:19

reporter   ~0123777

> And why would you want the OnClick event of the parent to fire when merely opening a submenu ?

That is just how menus work

You can try it with a main menu rather than a popup menu, or read about it: https://stackoverflow.com/a/14076860/1501222

jamie philbrook

2020-07-06 14:20

reporter   ~0123779

that isn't how it works here..

Please incorporate that in your own code if that is what you want.

I don't want the parent OnClick to fire in my code when I merely open a sub menu. the submenu is there to allow you to make the choice from the user's stand point not automatically fire off a parent onclick for which is has a different purpose when the sub's open.

  You can elect to have a parent menu have different action for it's own onclick over a submenu...

 all of what you need can be done in user code, its not a block on your end and what you are suggesting will then limit the flexibility of the menus if not introduce bugs for other code..

 The property request would be to have a property that would optionally trigger an event in the parent of a submenu, but not the onclick. This would have to be a specific event to this task.. For example OnChildMenuOpen.
etc




 If you notice submenu's have their own onclicks, there is a reason for that

Bart Broersma

2020-07-09 22:02

developer   ~0123854

Last edited: 2020-07-11 12:22

View 2 revisions

@Jamie:
> I don't want the parent OnClick to fire in my code when I merely open a sub menu.

This is how it works in Delphi 7 (the only version I can test).

Given this popup menu
Item1->Sub
Item 2

When the popupmenu pops up and I click/traverse Item1, it opens Sub (as expected) and it fires Item1's OnClick event.

Luis Caballero

2021-01-03 11:52

reporter   ~0128042

I know this is a relatively old issue but ...

@jamie philbrook:
> And why would you want the OnClick event of the parent to fire when merely opening a submenu ?

Not that it matters, because if you do have an OnClick handler for any menu item it should fire (or not, if documented!) but here is an example where you would want it to trigger: You have a popup menu with an "Edit" item wich has a submenu with the normal Cut, Copy, Paste, etc. and you want to enable/disable those according to some conditions.

Sure, you could do it in the menu's OnPopup event but you might not want to, for whatever reason (e.g. you're sharing that code with a TMainMenu), so your only option is to do it in the OnClick handler of the "Edit" item, but if that doesn't fire ....

jamie philbrook

2021-01-03 15:26

reporter   ~0128047

I really don't understand you, honestly..

You are trying to do something that can be done in code and you know it.. period..

 A child has no reason to force its parent menu to trigger its onclick when the onclick never happen on the parent.

 If a menuitem has a child and that child gets clicked it will process the onclick handler of the child and if you wish and I have no idea why any one would want this, you can simply call the parent handler directly from any of the childs onclick handlers.

 This makes absolutely no sense at all because now you have onclicks taking place in the parent from various child menus, does that really make sense to you or any one ?

 Unless some one here is having issues trying to explain a different case then I am lost ..

 The trunk behaves exactly the way I would expect it to . No click events on the parent if it has children because the children is what is to be expected to be clicked on not the parent.

 I suppose you could perform a hack to make the parent clickable but that really makes little sense and it does not fallow the standard behavior..

Please don't hack the LCL just to please a 0.001% of the users when they can accomplish there needs in their own code.

jamie philbrook

2021-01-04 03:02

reporter   ~0128061

Problem is solvable using a TactionList on the menuitem of interest..

I never found a use for a TactionList until now..

At least saves the rest of us from dealing with unwanted events.

Benito van der Zander

2021-01-09 16:53

reporter   ~0128202

The patch does not do any of that

It triggers an onclick when the submenu is opened. Because clicking on an item with a submenu, corresponds to opening its submenu.

Delphi's OnClick events have never been about clicking only. E.g. they also trigger when you press enter or use a keyboard shortcut. For menu items it appears to mean WM_COMMAND, which Microsoft documents as the item was selected

> A child has no reason to force its parent menu to trigger its onclick when the onclick never happen on the parent.

A (parent) menu does not have an onclick

Only menu items have one

jamie philbrook

2021-01-09 17:09

reporter   ~0128204

"A (parent) menu does not have an onclick

 Only menu items have one"

Exactly..
  Not everything Delphi does is correct..

 Please tell me the logic you find in having other Onclick Events to fire when its not the item that was clicked on?

 Windows sends a WM_SELECTITEM message to the parents because the string of menus are selected, this is not a onClick event..

You are asking everyone else to change their code to make you happy when its been proven that your thoughts about how it should work is counter productive as it is in Delphi..

 If they DEVS wants to supply an option to turn that feature on/off then so be it, at least it will save the rest of us from your desires

Why don't you supply a patch to have that feature in the Widget and Class code ?

Benito van der Zander

2021-01-09 23:31

reporter   ~0128227

> Please tell me the logic you find in having other Onclick Events to fire when its not the item that was clicked on?

With the patch, only one OnClick fires, no other OnClick Events

> You are asking everyone else to change their code to make you happy when its been proven that your thoughts about how it should work is counter productive as it is in Delphi..

It is so they do not need to change their code when porting it from Delphi or to Linux

jamie philbrook

2021-01-10 00:14

reporter   ~0128228

Last edited: 2021-01-10 00:16

View 2 revisions

My suggestions would be to rather add a OnSelect event instead of mangling the onclick event..

The widget can simple send a WM_MENUSELECT message to the LCL part of the code where it can then handle the message and check for an assigned OnSelect Event, and if so then call it with
the current TmenuItem as the sender. This event shall not do anything else, no OnClicks and no toggling of the Check mark.
 
 Basically give the LCL part a DoSelect event.

 Each TmenuItem can have an added event "OnSelection"..

 But for this to go through management, other widgets would want to implement such event, otherwise it would be a windows only solution..

 In the mean time, you can simply implement the OwnerDraw for the items. Draw the items yourself in your code and while you are there you can also process each item as you wish.
This gives the same effect as you are looking for but with greater control.
  
 As for modifying the existing code to make every one happy its has to be talked about as to which way it should be done.

 I am more then willing to make the changes but this will be a two file patch, one for the widget and the other for the LCL.

Issue History

Date Modified Username Field Change
2019-03-13 12:30 Benito van der Zander New Issue
2019-03-13 12:30 Benito van der Zander File Added: popupmenu.patch
2019-03-15 12:42 Juha Manninen Assigned To => Juha Manninen
2019-03-15 12:42 Juha Manninen Status new => assigned
2019-03-15 12:45 Juha Manninen Fixed in Revision => r60679
2019-03-15 12:45 Juha Manninen LazTarget => -
2019-03-15 12:45 Juha Manninen Note Added: 0114836
2019-03-15 12:45 Juha Manninen Status assigned => resolved
2019-03-15 12:45 Juha Manninen Resolution open => fixed
2020-06-13 04:20 Richard Woolf Note Added: 0123410
2020-06-13 04:20 Richard Woolf File Added: Popup menu bug.jpg
2020-06-13 04:31 Richard Woolf Issue cloned: 0037210
2020-06-13 04:31 Richard Woolf Relationship added related to 0037210
2020-06-14 10:02 Juha Manninen Status resolved => assigned
2020-06-14 10:02 Juha Manninen Resolution fixed => reopened
2020-06-14 10:02 Juha Manninen Build Lazarus 2.0.0 r55664M FPC 3.3.1 => Lazarus 2.0.0 r55664M FPC 3.3.1
2020-06-14 10:02 Juha Manninen Note Added: 0123428
2020-07-05 08:58 Juha Manninen Status assigned => feedback
2020-07-05 16:16 jamie philbrook Note Added: 0123764
2020-07-06 11:09 Juha Manninen Assigned To Juha Manninen =>
2020-07-06 11:09 Juha Manninen Status feedback => new
2020-07-06 11:09 Juha Manninen Note Added: 0123771
2020-07-06 12:29 jamie philbrook Note Added: 0123775
2020-07-06 13:19 Benito van der Zander Note Added: 0123777
2020-07-06 14:20 jamie philbrook Note Added: 0123779
2020-07-09 22:02 Bart Broersma Note Added: 0123854
2020-07-11 12:22 Bart Broersma Note Edited: 0123854 View Revisions
2021-01-03 11:52 Luis Caballero Note Added: 0128042
2021-01-03 15:26 jamie philbrook Note Added: 0128047
2021-01-04 03:02 jamie philbrook Note Added: 0128061
2021-01-09 16:53 Benito van der Zander Note Added: 0128202
2021-01-09 17:09 jamie philbrook Note Added: 0128204
2021-01-09 23:31 Benito van der Zander Note Added: 0128227
2021-01-10 00:14 jamie philbrook Note Added: 0128228
2021-01-10 00:16 jamie philbrook Note Edited: 0128228 View Revisions