Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add arguments support in [openmenu] actions #36

Merged
merged 9 commits into from
Feb 19, 2024
Merged

Add arguments support in [openmenu] actions #36

merged 9 commits into from
Feb 19, 2024

Conversation

BlitzOffline
Copy link
Member

@BlitzOffline BlitzOffline commented Dec 30, 2023

This PR adds support for arguments to the [openmenu] and [openguimenu] actions.

How it works:

  • For backwards compatibility, if no arguments are specified, it works just like it used to.
  • Arguments can be used like so - '[openmenu] menu1 arg1 arg2'
  • To replicate the way arguments work in commands, the number of arguments needs to be the same (or greater since any extra arguments are just concatenated to the last argument) as the number of arguments specified inside the menu that is being opened
  • If the menu that is being opened does not support arguments but they are being specified, menu is not open and warning is shown in console
  • If not enough arguments are being passed, the menu is not open and a warning is shown in console
  • Arguments are still passed from previous menus but if one of the submenus has arguments named the same, they are being overwritten.

Closes #30

EDIT: Should be merged after #46

@BlitzOffline
Copy link
Member Author

BlitzOffline commented Dec 30, 2023

There are still 3 things to figure out before this PR can be merged:

  • For bullet point 3, Should we allow argument length be lower than the required one if arguments are being passed from the previous menu as long as they are overriding arguments in the current menu? So let's say:
    1. There is a menu A that has 2 arguments: player_name, amount
    2. There is a menu B that has 2 arguments: age, player_name
    3. /A PLAYER1 2 is used to open first menu
    4. [openguimenu] B 18 is used to open the second menu
      In this specific case, argument player_name would be given the value PLAYER1 from the previous menu. This would raise another question however. Consider the following case:
    5. There is a menu A that has 2 arguments: player_name, amount
    6. There is a menu B that has 4 arguments: age, player_name, amount, height
    7. /A PLAYER1 2 is used to open first menu
    8. [openguimenu] B 18 2 175 is used to open the second menu
      How do we decide which argument is used? This is easy for only 4 arguments but can get very complicated with more arguments.
  • For bullet point 4, should we still open the menu but do it the same way it would be done if no arguments were specified
  • For bullet point 5, should we still open the menu but do it the same way it would be done if no arguments were specified

@BlitzOffline BlitzOffline marked this pull request as draft December 30, 2023 20:55
@itsme-to
Copy link

I find that it complicates the logic unnecessarily and that it is not very practical or even understandable. The person only needs to put {arg} to retrieve them and can put them in the action, which is much more visual.

Like that: "[openmenu] punish-confirm {target} mute {reason}"

@BlitzOffline
Copy link
Member Author

Which part is complicated exactly? Can you be a bit more specific?

@BlitzOffline BlitzOffline changed the title Add arguments support in [openmenu] actions Add arguments support in [openmenu] actions Dec 31, 2023
@itsme-to
Copy link

To recover part of the arguments if they have the same name, On the configuration side it's not very clear how it works like that.

@BlitzOffline
Copy link
Member Author

To recover part of the arguments if they have the same name, On the configuration side it's not very clear how it works like that.

I'm sorry but I am a bit lost. Can you tell me about which bullet point from my comment are you talking about specifically? Or are you talking about the entire PR?

@itsme-to
Copy link

Keeping part of the arguments, not replacing them if they have the same name, ...

I find that it's not very clear and for example if I want to replace the player_name, how do I do it? I would have simply overwritten the arguments with the new ones, I find it more understandable

@BlitzOffline
Copy link
Member Author

Keeping part of the arguments, not replacing them if they have the same name, ...

I find that it's not very clear and for example if I want to replace the player_name, how do I do it? I would have simply overwritten the arguments with the new ones, I find it more understandable

But this is how my PR currently works. I think I have to give another example because it is pretty hard to explain.

There are 2 menus:

name command arguments
A /menuA player_name, age
B /menuB player_name, amount

You open menu A using /menuA PLAYER1 18
When you open menu B from inside the menu A, you can currently do this in 2 ways:

  • [openmenu] B
  • [openmenu] B PLAYER2 2

If you do it the first way, you will have the following arguments in menu B: {player_name} = PLAYER1 and {age} = 18
If you do it the second way, you will have arguments: {player_name} = PLAYER2, {age} = 18 and {amount} = 2 in menu B.

We need to keep the first option because this is how DeluxeMenus has worked for a long time and changing it would break old menus.

Also, you can always do [openmenu] B {player_name} 2 to open the menu and then you'll have the following arguments in menu B: {player_name} = PLAYER1, {age} = 18 and {amount} = 2.

@itsme-to
Copy link

itsme-to commented Jan 1, 2024

Yes no worries about that, what I found unclear is what happens when you do [openmenu] B 2.
And it's currently don't register argument inside the menu if you don't have command to open it.

@cj89898
Copy link
Contributor

cj89898 commented Jan 2, 2024

For bullet point 3 and your response, I feel the best way would be to just require all arguments and avoid confusion. As itsme-to said, I would just have the player use {arg} to pass it forward.

If there are no arguments supplied, then carry over.

@BlitzOffline BlitzOffline marked this pull request as ready for review January 31, 2024 19:29
@BlitzOffline BlitzOffline merged commit 5505029 into main Feb 19, 2024
@BlitzOffline BlitzOffline deleted the issue/30 branch February 19, 2024 17:36
@itsme-to
Copy link

Hello, it's still missing register argument without command to really have utility to this, like made here #33

It's allow to have menu that cannot been open by player and have the information (args) controled.

@itsme-to
Copy link

And I still find that this implementation is unnecessarily complicated and not at all clear when reading the configuration.

@BlitzOffline
Copy link
Member Author

BlitzOffline commented Aug 30, 2024

Hello, it's still missing register argument without command to really have utility to this, like made here #33

It's allow to have menu that cannot been open by player and have the information (args) controled.

I have raised an issue to allow arguments for menus that have no open_commands set: #137.

And I still find that this implementation is unnecessarily complicated and not at all clear when reading the configuration.

While arguments being passed from one menu to another might confuse things a bit, it is how menus have worked before this change so we'll keep it for backwards compatibility. Here is the best way I was able to simplify arguments:

Arguments can be passed to menus when opening them with commands like so: /menu1 arg1 arg2 etc.
When opening menu2 from menu1 using [openguimenu] menu2, all arguments from menu1 are passed to menu2.
When opening menu2 from menu1 using [openguimenu] menu2 arg1 arg2 etc, the arguments passed in the action will have priority.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Args in OpenGuiMenu action
4 participants