sizepolicy param not implemented for mod_menu.menu() despite documentation saying so

[mbenkmann]

The documentation for mod_menu.menu()

https://raboof.github.io/notion-doc/notionconf/node7.html#SECTION00740000000000000000

says

The position can be set using the field sizepolicy.

However this is not actually implemented in mod_menu/mkmenu.c. A patch for the implementation was submitted long ago:

https://sourceforge.net/p/notion/mailman/notion-devel/thread/op.wix8xumbxzm3zy%40hedin.hednings/#msg29667549

I consider this feature essential since on ultra-widescreen monitors the bottom left corner is not a very user friendly place for a menu to appear and "center" makes a lot more sense.

Please apply the patch.

[mbenkmann]

It looks like the patch was applied in commit eae8a8c but the code got lost during commit 35a2099 when several files, including mkmenu.c were overwritten with older versions.

[raboof]

Sadly I suspect that means that improvement was made during the period where ion3 was licensed "LGPL with exceptions", which we don't want for Notion.

Could you do a clean-room reimplementation, or ask Tor if he's OK with sharing the patch under 'regular' LGPL?

[mbenkmann]

1. He submitted the patch as an email to notion-devel, not Ion, in 2012
2. He submitted pull request Add sizepolicy support for menus #17 to Notion in 2016
3. From 1 and 2 we can safely assume he wants it in Notion and doesn't care about licensing shenanigans of Ion
4. The patch cannot be copyrighted because the code part cannot be written in any other way and the human readable comment part is trivial and lacks any creativity

I think you should re-apply his old commit. That way authorship is properly recorded in git. It's silly to bother the original author for these 3 lines of code after all these years.

[mbenkmann]

sigh I just checked and the patch doesn't apply anymore because of the (also lost) change to the stacking order of menus. Let's cut this short. I'll submit my own pull request.

[raboof]

1. He submitted the patch as an email to notion-devel, not Ion, in 2012

   2. He submitted pull request Add sizepolicy support for menus #17 to Notion in 2016

   3. From 1 and 2 we can safely assume he wants it in Notion and doesn't care about licensing shenanigans of Ion

Sadly at that point Notion was still under the inherited LGPL-with-exceptions - though it seems I had the foresight to ask for permission to also use it under the LGPL, which we got in #17 (comment)

4. The patch cannot be copyrighted because the code part cannot be written in any other way and the human readable comment part is trivial and lacks any creativity

While this may be true, I'd prefer not to have to make such an assumption.

Let's cut this short. I'll submit my own pull request.

Thanks!

[mbenkmann]

First part is done: Refactor code to have a re-usable function to read sizepolicy:

main...mbenkmann:notion:sizepolicy-for-menus

Although there is very little to get wrong here, I would like to test that I didn't cause any regression. What's the simplest way to test group.c:get_params() and mplex.c:get_params()? What would I have to configure to exercise these 2 code positions?

[mbenkmann]

Second part is done, too. sizepolicy implemented on grabmenu and menu. Tested with my own configuration. As said in the prior comment, before I open the pull request I'd like to test for regression in group.c and mplex.c but you'll have to give me a hint how.

[raboof]

Second part is done, too. sizepolicy implemented on grabmenu and menu. Tested with my own configuration.

Awesome!

As said in the prior comment, before I open the pull request I'd like to test for regression in group.c and mplex.c but you'll have to give me a hint how.

If I'm honest I'm not sure how... maybe @wilhelmy ?

[mbenkmann]

sizepolicy (the exposed form that gets parsed) isn't used much. Probably because it's not documented. So only people who look deeper into the code know how to use it. The only uses grep turns up in the source tree are xkbbell.lua which is part of a module that's not being built by default, 3 contrib/ scripts (float-sb.lua, named_scratchpad.lua, show_submap.lua) and 2 contrib/legacy scripts (notifybox.lua, cwin_sp.lua).

I tested float-sb.lua, named_scratchpad.lua and notifybox.lua. They work with both numeric and string sizepolicy params. I didn't attempt xkbbell.lua because I don't even know if the module still works. I didn't attempt cwin_sp.lua because it looks very complex and being in legacy I'm not sure if it's supposed to still work. I tried show_submap.lua but it doesn't seem to work anymore. It doesn't pick up any submaps. In any case its failure is unrelated to the change because it doesn't even hit the code to create the window.

I don't know if these tests exercised both group.c and mplex.c. My suspicion is that they only go through one of the 2 code paths, but I'm not going to set up a debug environment for Notion to test this further. I think this small change has received more than adequate testing.

[mbenkmann]

Implemented in pull request #381

[mbenkmann]

Merged.