Fix #107422: Mesh context menus do not open with the last used option #107427

Philipp Oeser · 2023-04-28T11:40:17+02:00

Philipp Oeser commented

2023-04-28 11:40:17 +02:00

Caused by 99e5024e97

Above commit changed behavior in that it only flagged the uiBlock with
UI_BLOCK_POPUP_MEMORY in case a Menu's title was provided to
ui_popup_menu_create_block. Now when is the title provided? This
depends on the bl_label set in the menu class. But this is not always
the case, there are some menus that have a blank bl_label (as seen
with VIEW3D_MT_edit_mesh_context_menu for example -- this specifies its
own label inside the draw function depending on vertex/edge/face mode).
Thus the provided title is not really reliable as source information to
do the flagging (also see for example
a38b98478a), flagging should always be
done [and only check a title for actually inserting in the puphash].

Choosing an entry in a menu will still handle puphash later (see
button_activate_exit) though multiple menus without a label might fight for the
same storage of the menu memory. Using idname instead (or in combination with
the label) for the hash could be looked at to solve this.

Caused by 99e5024e97f1c34697885b1a93935e72c019da5c Above commit changed behavior in that it only flagged the `uiBlock` with `UI_BLOCK_POPUP_MEMORY` in case a Menu's title was provided to `ui_popup_menu_create_block`. Now when is the title provided? This depends on the `bl_label` set in the menu class. But this is not always the case, there are some menus that have a blank `bl_label` (as seen with `VIEW3D_MT_edit_mesh_context_menu` for example -- this specifies its own label inside the draw function depending on vertex/edge/face mode). Thus the provided title is not really reliable as source information to do the flagging (also see for example a38b98478a61815f57dee29c226de61d9cd338e1), flagging should **always** be done [and only check a title for actually inserting in the puphash]. Choosing an entry in a menu will still handle puphash later (see `button_activate_exit`) though multiple menus without a label might fight for the same storage of the menu memory. Using idname instead (or in combination with the label) for the hash could be looked at to solve this.

Philipp Oeser added 1 commit 2023-04-28 11:40:27 +02:00

f26fc831f8 Fix #107422 : Mesh context menus do not open with the last used option

Caused by 99e5024e97

Above commit changed behavior in that it only flagged the `uiBlock` with
`UI_BLOCK_POPUP_MEMORY` in case a Menu's title was provided to
`ui_popup_menu_create_block`. Now when is the title provided? This
depends on the `bl_label` set in the menu class. But this is not always
the case, there are some menus that have a blank `bl_label` (as seen
with `VIEW3D_MT_edit_mesh_context_menu` for example -- this specifies its
own label inside the draw function depending on vertex/edge/face mode).
Thus the provided title is not really reliable as source information to
do the flagging (also see for example
a38b98478a), flagging should **always** be
done [and only check a title for actually inserting in the puphash].

Now why does this still work for menus **not** providing a title? This
is because `button_activate_exit` will also handle the puphash when
choosing a menu entry and does `ui_popup_menu_memory_set`. So we are all
good here.

Philipp Oeser requested review from Julian Eisel 2023-04-28 11:40:37 +02:00

Philipp Oeser added this to the User Interface project 2023-04-28 11:40:46 +02:00

Philipp Oeser requested review from Harley Acheson 2023-04-28 11:40:55 +02:00

Harley Acheson commented

2023-04-28 20:20:57 +02:00

Hey that works quite well!

The comment has a typo ("alwas"), and it should probably reference "#107422". The following is just a suggestion:

  /* Add UI_BLOCK_POPUP_MEMORY flag even if we don’t have a title as this might be created
   * later, ie: VIEW3D_MT_edit_mesh_context_menu. A puphash entry will be inserted when a
   * menu item is selected (`button_activate_exit`). #107422. */

Hey that works quite well! The comment has a typo ("alwas"), and it should probably reference "#107422". The following is just a suggestion: ``` /* Add UI_BLOCK_POPUP_MEMORY flag even if we don’t have a title as this might be created * later, ie: VIEW3D_MT_edit_mesh_context_menu. A puphash entry will be inserted when a * menu item is selected (`button_activate_exit`). #107422. */ ```

Harley Acheson approved these changes 2023-04-28 20:21:07 +02:00

Julian Eisel commented

2023-05-01 13:12:44 +02:00

Problem is that the block's puphash will always be 0 for menus without title. So if you have multiple menus without a label, they will fight for the same storage of the menu memory. Need to think about what's the best solution here. Maybe we shouldn't use the label, but an id-name for this (there's already a menu type id-name separate from the label). But it's a bit tricky to get this information at this level.

Problem is that the block's `puphash` will always be 0 for menus without title. So if you have multiple menus without a label, they will fight for the same storage of the menu memory. Need to think about what's the best solution here. Maybe we shouldn't use the label, but an id-name for this (there's already a menu type id-name separate from the label). But it's a bit tricky to get this information at this level.

Philipp Oeser commented

2023-05-02 08:58:00 +02:00

Problem is that the block's puphash will always be 0 for menus without title. So if you have multiple menus without a label, they will fight for the same storage of the menu memory. Need to think about what's the best solution here. Maybe we shouldn't use the label, but an id-name for this (there's already a menu type id-name separate from the label). But it's a bit tricky to get this information at this level.

Afaict, that situation is not new (was the same prior to 99e5024e97 -- and wasnt really a problem then), but yeah, theoretically, true...

> Problem is that the block's `puphash` will always be 0 for menus without title. So if you have multiple menus without a label, they will fight for the same storage of the menu memory. Need to think about what's the best solution here. Maybe we shouldn't use the label, but an id-name for this (there's already a menu type id-name separate from the label). But it's a bit tricky to get this information at this level. Afaict, that situation is not new (was the same prior to 99e5024e97 -- and wasnt really a problem then), but yeah, theoretically, true...

Julian Eisel commented

2023-05-05 15:49:37 +02:00

A few possibilities:

Split the menu into multiple and set the bl_label for each. This would mean menus require a label to support memory.
Use the idname of the menu type instead of the label. Additionally, the hash could be a combination of the idname + label hash. This would be the proper way to address this I think, but not sure how trivial/involved it is to let the menu creation API accept a idname in addition to the label (and if this can be passed around cleanly).
Accept this patch and consider the memory "stealing" a known issue.

Note that there are more broken cases. For example I can see the Grease Pencil edit mode context menu has the same issue. So it's probably worth solving this properly, with

A few possibilities: 1. Split the menu into multiple and set the `bl_label` for each. This would mean menus require a label to support memory. 1. Use the idname of the menu type instead of the label. Additionally, the hash could be a combination of the idname + label hash. This would be the proper way to address this I think, but not sure how trivial/involved it is to let the menu creation API accept a idname in addition to the label (and if this can be passed around cleanly). 1. Accept this patch and consider the memory "stealing" a known issue. Note that there are more broken cases. For example I can see the Grease Pencil edit mode context menu has the same issue. So it's probably worth solving this properly, with

Philipp Oeser commented

2023-05-09 11:08:13 +02:00

A few possibilities:

Split the menu into multiple and set the bl_label for each. This would mean menus require a label to support memory.

Use the idname of the menu type instead of the label. Additionally, the hash could be a combination of the idname + label hash. This would be the proper way to address this I think, but not sure how trivial/involved it is to let the menu creation API accept a idname in addition to the label (and if this can be passed around cleanly).

Accept this patch and consider the memory "stealing" a known issue.

Note that there are more broken cases. For example I can see the Grease Pencil edit mode context menu has the same issue. So it's probably worth solving this properly, with

Passing around idname + label/title might be a better way, but there are a couple of places where an idname is hard to get to (e.g. since this is also used with PieMenus, we might only have PieMenuLevelData at hand -- which only has a title), in these cases, we could probably get away with reusing the title for "both" label & idname, but this is also not very clean.

My vote would be to not sink a lot of time at this moment (like I said, afaict the danger of "stealing" was there before and apparently never really caused too much breakage in practice), and note this in both the commit message and the comment [I can update that], but still just go forward with this patch.

Of course, this is a decision the UI module has to make though.

> A few possibilities: > 1. Split the menu into multiple and set the `bl_label` for each. This would mean menus require a label to support memory. > 1. Use the idname of the menu type instead of the label. Additionally, the hash could be a combination of the idname + label hash. This would be the proper way to address this I think, but not sure how trivial/involved it is to let the menu creation API accept a idname in addition to the label (and if this can be passed around cleanly). > 1. Accept this patch and consider the memory "stealing" a known issue. > > Note that there are more broken cases. For example I can see the Grease Pencil edit mode context menu has the same issue. So it's probably worth solving this properly, with Passing around idname + label/title might be a better way, but there are a couple of places where an idname is hard to get to (e.g. since this is also used with PieMenus, we might only have PieMenuLevelData at hand -- which only has a title), in these cases, we could probably get away with reusing the title for "both" label & idname, but this is also not very clean. My vote would be to not sink a lot of time at this moment (like I said, afaict the danger of "stealing" was there before and apparently never really caused too much breakage in practice), and note this in both the commit message and the comment [I can update that], but still just go forward with this patch. Of course, this is a decision the UI module has to make though.

Julian Eisel commented

2023-05-16 17:27:14 +02:00

My vote would be to not sink a lot of time at this moment (like I said, afaict the danger of "stealing" was there before and apparently never really caused too much breakage in practice), and note this in both the commit message and the comment [I can update that], but still just go forward with this patch.

I'm fine with this for now, just note this clearly in code comments.

I did work on a patch to use the idname for hashing last week, don't remember if it's finished though, would have to test again.

> My vote would be to not sink a lot of time at this moment (like I said, afaict the danger of "stealing" was there before and apparently never really caused too much breakage in practice), and note this in both the commit message and the comment [I can update that], but still just go forward with this patch. I'm fine with this for now, just note this clearly in code comments. I did work on a patch to use the idname for hashing last week, don't remember if it's finished though, would have to test again.

Philipp Oeser added 2 commits 2023-05-17 10:04:24 +02:00

4a9becab7f Merge branch 'main' into 107422

6e0d60f9a1 improve comment

Philipp Oeser merged commit eac3d37ab1 into main

2023-05-17 10:41:23 +02:00

Philipp Oeser deleted branch 107422

2023-05-17 10:41:26 +02:00

Philipp Oeser referenced this issue from a commit

2023-05-17 10:41:32 +02:00

Fix #107422: Mesh context menus do not open with the last used option

Falk David referenced this issue from a commit

2023-05-23 14:35:34 +02:00