View Issue Details

IDProjectCategoryView StatusLast Update
0035219PatchesPatchpublic2021-01-18 03:08
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.

Benito van der Zander

2021-01-17 13:54

reporter   ~0128387

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

That would have made sense if Borland did that 20 years ago.

But they did not.

It is also in their docs: http://docwiki.embarcadero.com/Libraries/Sydney/en/Vcl.Menus.TMenuItem.OnClick

>Write an OnClick event handler to implement the desired behavior for when the user selects the menu item.



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

In gtk2 it works with onclick

jamie philbrook

2021-01-17 14:11

reporter   ~0128388

I am sorry but I highly reject to the idea of how Delphi does it, just because they do something does not make it right.. They are in fact full of bugs that hamper coders and what ever hacks that coders had to do over the years to get around their bugs should not also be brought here, too.

  I have already fully demonstrated in the forums what to do but either you are hell bent on copying bad coding practices from other tools or just being na├»ve about the situation.

  a simple example of how Delphis method of control falls flat on its face is when you dynamically manipulate the child menus, if the menu you click on has no children then that onclick event should fire but later on if a child menu dynamically gets added you can't have that parent calling the click..

  Delphi did that just as a short cut to get around a real problem, that is they didn't implement a SelectMenu.. There is a huge difference between Menu selection where you simply are viewing it but not enabling its option over the fact that you actually click on it..

  You seem to think its ok to keep driving the Model-T and if that is what you think then may I can offer you a crank handle to start your app and run all around it to hinder the all the known problems that come with it and hope it starts today without issues.


 I vote that we drop this crazzy idea and think more about implementing a OnSelectmenu event and leave the Onclick as is.

 Please offer up a patch for the OnSelectmenu option so you can have your provided hack you are looking for.

jamie philbrook

2021-01-18 03:08

reporter   ~0128393

Apparently its true that Delphi does not do it correctly..

Checking the MS Docs for their MEnuItem Class it contains both a OnClick and OnSelect event in their class.

The OnClick only fires when the last item on the tree is clicked! just like how it is now and not on every parent but the OnSelect is called on the whole tree of menus which makes sense.

So some one needs to make a decision on how to fix this ..

 1. Add the OnSelect event to TMenuItem ?

 2. Modified the OnClick behavior in the LCL to check for Child Menus before toggling the Auto check mark, only the last child should be toggled when clicked.

In either case 1 or 2, the Patch that was offered here which looks like an original chunk of code that got removed from an earlier patch needs to be put back in for the Windows side.

 Currently without the patch or original code put back in, the ACTIONS features are not working correctly anymore, or they are only firing on the last child so I am not sure exactly how those were intended to work, the whole tree or the last child ?


Need some suggestions so patch work can be done. it has to be two, the original patch put back in and a new patch to either add a Onselect or modified the Onclick and have no OnSelect..

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
2021-01-17 13:54 Benito van der Zander Note Added: 0128387
2021-01-17 14:11 jamie philbrook Note Added: 0128388
2021-01-18 03:08 jamie philbrook Note Added: 0128393