Bug 3096 - menu_addblank slot=1 does not count in prop per page
menu_addblank slot=1 does not count in prop per page
Status: RESOLVED FIXED
Product: AMX Mod X
Classification: Unclassified
Component: Core
trunk
PC All
: P4 minor
Assigned To: amxmodx-bugs@alliedmods.net
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-11-16 17:04 PST by vittu
Modified: 2014-02-08 23:52 PST (History)
3 users (show)

See Also:


Attachments
menu_addblank2, menu_addtext2 (2.38 KB, patch)
2013-07-01 08:03 PDT, Valentin Grünbacher [:Nextra]
hv.contact: review+
Details | Diff | Splinter Review
Test plugin with verifier for patch (3.11 KB, application/octet-stream)
2013-07-01 08:04 PDT, Valentin Grünbacher [:Nextra]
no flags Details
menu_addblank2, menu_addtext2 (new format) (3.40 KB, patch)
2013-07-01 12:08 PDT, Valentin Grünbacher [:Nextra]
nextra.24: review+
Details | Diff | Splinter Review
Proposed patch (splinter view ready) (2.29 KB, patch)
2013-07-02 03:10 PDT, Vincent Herbet [:Arkshine]
hv.contact: review+
Details | Diff | Splinter Review
Proposed fixed patch (3.39 KB, patch)
2013-07-02 03:44 PDT, Vincent Herbet [:Arkshine]
hv.contact: review+
Details | Diff | Splinter Review
Proposed fixed patch (4.62 KB, patch)
2013-07-02 05:50 PDT, Vincent Herbet [:Arkshine]
hv.contact: review+
Details | Diff | Splinter Review

Description vittu 2007-11-16 17:04:51 PST
I don't know if I can explain this well or even if this is really an issue.

When using menu_addblank with default slot 1, it uses up a number in the list but does not count as a prop per page. Whereas menu_addblank with slot 0 seems to be fine not counting as a prop because the list gets done right.

ie.
menu_addblank with slot 1 and default prop per page at 7, with a blank using number 4:
<code>1. blah
2. blah
3. blah

5. blah
6. blah
7. blah
8. blah

9. Back
0. Next
11. Exit</code>

Thing is I'm not completely sure how this would effect use of a blank as a prop per page in other situations. I haven't really done any testing with this.
Comment 1 Valentin Grünbacher [:Nextra] 2013-07-01 07:21:49 PDT
Having looked into the issue I think this is a design flaw that can't be fixed easily with the old functions.

Blanks (and text, which also exhibits this bug) are affecting the ordering of actual items when slot is set to 1.  The issue is that slot eating blanks are a special type of blanks when probably they should be a special type of item - i.e. an item that has no text.

This is also shown by the fact that addblank and addtext are checking for GetItemCount() >= 10 if slot is 1 when they don't even contribute to m_Items growing. You can add an indefinite amount of them, screwing up the menus.

To fix this I propose to have a special type of item to achieve this.

Changing blanks to be items all the time or only when slot is 1 would change what old plugin code has to expect about menu_items or menu_pages, so it is likely that this change would break compatibility somewhere.

The way I see it we have two options:
1) Add a new parameter 'blank' to menu_additem; I think this is inconvenient because menu_additem already has three optional parameters that would not even matter to blanks because they can't be selected anyway

2) Add new functions like menu_add[blank|text]2 or menu_addtextitem to add this type of item


This would add the desired functionality for new plugins - that then have to deal with the fact that slot eating blanks and texts are real items and affect the menus item count and pagination - while leaving old plugins alone.


I hope I'm not missing stuff here :)
Comment 2 Valentin Grünbacher [:Nextra] 2013-07-01 08:03:08 PDT
Created attachment 3407 [details] [review]
menu_addblank2, menu_addtext2

Example patch if adding menu_addblank2/addtext2 is an acceptable solution to this problem.
Comment 3 Valentin Grünbacher [:Nextra] 2013-07-01 08:04:49 PDT
Created attachment 3408 [details]
Test plugin with verifier for patch

Commands singleblank/manyblanks display faulty behavior for two different kinds of blank/text usage.

Commands singleblank_fix/manyblanks_fix display how the proposed patch fixes the issues.
Comment 4 Vincent Herbet [:Arkshine] 2013-07-01 08:12:20 PDT
Comment on attachment 3407 [details] [review]
menu_addblank2, menu_addtext2

Review of attachment 3407 [details] [review]:
-----------------------------------------------------------------

Looks good for me. After checking the issue, I would agree with Valentin. Fixing it without new natives would be tricky, adding unnecessary complexity in the code and would lead most likely to break things. Simple & safer to rely on new natives dealing properly with blanks as empty string. 
Patch tested and is working as espected.

Asking feedback? from joropito.
Comment 5 David Anderson [:dvander] 2013-07-01 11:37:58 PDT
Splinter is having trouble reading these patches. Could you add this to your hgrc?

[defaults]
diff = -U 8 -p
qdiff = -U 8 -b

[alias]
qstatus = status --rev -2:.

[diff]
git = 1
showfunc = true
unified = 8
Comment 6 David Anderson [:dvander] 2013-07-01 11:38:13 PDT
Err, ignore the [alias] section.
Comment 7 Valentin Grünbacher [:Nextra] 2013-07-01 12:08:09 PDT
Created attachment 3410 [details] [review]
menu_addblank2, menu_addtext2 (new format)

Does this work?
Comment 8 Vincent Herbet [:Arkshine] 2013-07-02 03:08:07 PDT
It seems not. :P 
Let me try.
Comment 9 Vincent Herbet [:Arkshine] 2013-07-02 03:10:48 PDT
Created attachment 3415 [details] [review]
Proposed patch (splinter view ready)

Patch looks good.
I've also tested and is working as expected.
Comment 10 Vincent Herbet [:Arkshine] 2013-07-02 03:44:25 PDT
Created attachment 3416 [details] [review]
Proposed fixed patch

Actually it misses the include modification.
Comment 11 Vincent Herbet [:Arkshine] 2013-07-02 05:50:40 PDT
Created attachment 3418 [details] [review]
Proposed fixed patch

Better documentation (by Nextra).
Comment 12 AM Bugzilla Bot 2013-07-04 11:45:30 PDT
Pushed changeset: http://hg.alliedmods.net/amxmodx-central/rev/51bf0d8e210b
Changelog:
	Add menu_addblank2 and menu_addtext2 to fix unexpected behavior with the original ones when slot=1 (bug 3096, r=Arkshine)
Comment 13 AM Bugzilla Bot 2014-02-08 23:52:23 PST
Commit pushed to master at https://github.com/alliedmodders/amxmodx

https://github.com/alliedmodders/amxmodx/commit/21a00e00a27ee31311f6d51e62388c94069bd930
Add menu_addblank2 and menu_addtext2 to fix unexpected behavior with the original ones when slot=1 (bug 3096, r=Arkshine)


Former-commit-id: b4f84a5cee58d4a8fad716af82bca5b8aa0f5ff2

Note You need to log in before you can comment on or make changes to this bug.