Add a flags update forward and a override param in set_user_flags #475

OciXCrom · 2017-12-31T18:41:46Z

Added a forward called "client_flags_updated" which will be called when the client's flags are modified using "set_user_flags" or "remove_user_flags". I also added another param in the "set_user_flags" function called "override", which if set to true will remove any other flags from the user - I found this necessary because the name of the function is "SET_user_flags", but it actually adds them to the current ones. This way coders can actually SET the flags instead of add them to the current ones.

Nextra · 2018-01-01T16:23:04Z

amxmodx/amxmodx.cpp

-
-	pPlayer->flags[id] |= flag;
+
+	if(params[4])


You need to check for parameter availability here.

Sorry, I'm fairly new to this. How do I exactly add that check?

Here's an example

amxmodx/amxmodx/amxmodx.cpp

Line 1443 in 7b3646a

if (params[0] / sizeof(cell) >= 12)

Is it actually necessary? The 4th parameter has a default value of "false" if it's not used. Just curious why it's needed.

@OciXCrom you will need to recompile all plugins that using get_players.

@OciXCrom Plugins that don't get compiled against the new *.inc will pass you 3 parameters, and core needs to handle that situation gracefully.

@Nextra @Amaroq7 @WPMGPRoSToTeMa - Thanks, I added the necessary check.

Amaroq7

I added some notes of mine and also as it has been said by @Nextra there's missing check.

Amaroq7 · 2018-01-01T16:09:28Z

plugins/include/amxmodx.inc

@@ -1826,6 +1836,7 @@ native set_user_flags(index, flags = -1, id = 0);
 *
 * @param index     Client index, 0 to set flags of server
 * @param id        Flag set id, ranging from 0 to 31
+ * @param override  If set to true, all other flags will be removed


Misplaced param's description.

Oops, I always get confused when viewing .inc files because of the way the description is placed above the native. Fixed.

Amaroq7 · 2018-01-01T16:27:49Z

plugins/include/amxmodx.inc

+ *
+ * @noreturn
+ */
+forward client_flags_updated(id, flags);


I'd also pass id of set here, so we'll know which set of flags has been updated.

I never knew different sets of flag existed until I saw the actual .cpp file. Are different sets ever used? Because if not, it would be rather unnecessary to have to write the set argument every time we use the forward if we'll never have to use it.

We don't have to write all params in forward, they can be skipped, so if someone doesn't depend on sets, they can safely omit last param.

I knew they existed but I never knew their purpose. The doc says:

AMXX stores multiple sets of flags internally, but only flag set
0 is actively used. You should not change the value of the third
parameter from the default.

I almost feel that we should just ignore flag sets and use a single one.

Even if AMXX doesn't use other values than 0, it still may be used by 3rd plugins for their some specific permission system, so they don't mess up with the AMXX's one. I think including that param wouldn't do much harm, but it's just my opinion.

@Amaroq7 we can add it later if someone really uses sets.

Actually if they can really be skipped when using the forwards (haven't actually checked this), then there's really no problem if that param can be passed too. I'll check this tomorrow and add it if that's the case.

@Amaroq7 @rsKliPPy @WPMGPRoSToTeMa - I added the flag set as the last parameter in the forward, so people can skip it if not needed. I also passed the old flags so people can see which flags were changed.

…modx into admin-flags-forward

OciXCrom · 2018-01-09T14:46:00Z

I added a check whether the new flags are different from the old ones, since there's not really a point in executing the forward if they are the same.

In-line · 2018-01-09T16:36:07Z

I think something like bool addFlags = true will be more clear.

In the first place why does not set_user_flags override?
Perhaps deprecating set_user_flags and adding 2 more natives will be better?
add_user_flags,override_user_flags.

OciXCrom · 2018-01-09T23:32:34Z

It should have added them in the first place, but it's too late now. I don't think there's really a point in deprecating it just because of the name.

WPMGPRoSToTeMa · 2018-01-10T00:26:20Z

I think something like bool addFlags = true will be more clear.

Or SetFlagsMode:mode = SetFlagsMode_Add.

OciXCrom · 2018-01-10T13:40:57Z

Hmm, I'm starting to like that idea. Maybe we can deprecate both set_user_flags and remove_user_flags and add a new one that will cover all 3 cases (set, add, remove) with the bool mentioned by @WPMGPRoSToTeMa. This bool can also be added to many other functions, e.g. set_user_health for easier usage. Any suggestions for the new flags function name?

asherkin · 2018-02-10T10:57:59Z

This PR makes two independent changes, and you're talking about adding even more - each change should be a separate PR. I suggest retargeting this to be only the forward, and make a separate PR if you want to change how set_user_flags works.

OciXCrom · 2018-02-10T13:14:31Z

Well, the PR is mainly about the forward, and the other change is just a small improvement to a function that's calling it. If I made them separate, one would become incompatible if the other one got merged, so I found it to be better this way. I won't be adding any more stuff until this gets approved (if it does). If it's really a problem I'll remove the set_user_flags change and make a separate PR after this one gets reviewed.

twisterniq · 2019-04-28T15:50:53Z

@Arkshine will it be merged?

In-line · 2019-05-19T23:11:10Z

plugins/include/amxmodx.inc

+ *
+ * @noreturn
+ */
+forward client_flags_updated(id, oldflags, newflags, set);


Maybe use same terminology that is used in set_user_flags?
Client ID is index
Set ID is just id

voed · 2019-11-21T13:13:12Z

@Arkshine bump. Should it be splitted into 2 PRs?

OciXCrom · 2020-05-30T12:06:12Z

@Arkshine if you wouldn't mind.

OciXCrom added 2 commits December 31, 2017 19:38

Add a flags update forward and a override param in set_user_flags

8659c90

Update amxmodx.inc

163b4e8

Nextra reviewed Jan 1, 2018

View reviewed changes

Amaroq7 suggested changes Jan 1, 2018

View reviewed changes

OciXCrom added 5 commits January 1, 2018 19:47

Fixed misplaced param.

e2c7ecb

Fixed misplaced param

b4170b4

Added param availability check + 2 more params in the forward

a3f5769

Merge branch 'admin-flags-forward' of https://github.com/OciXCrom/amx…

6edebc9

…modx into admin-flags-forward

Added check for if the new flags are different

9b26923

In-line reviewed May 19, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a flags update forward and a override param in set_user_flags #475

Add a flags update forward and a override param in set_user_flags #475

OciXCrom commented Dec 31, 2017

Nextra Jan 1, 2018

OciXCrom Jan 1, 2018

Amaroq7 Jan 1, 2018

OciXCrom Jan 1, 2018

WPMGPRoSToTeMa Jan 1, 2018 •

edited

Loading

Nextra Jan 1, 2018

OciXCrom Jan 2, 2018

Amaroq7 left a comment

Amaroq7 Jan 1, 2018

OciXCrom Jan 1, 2018

Amaroq7 Jan 1, 2018

OciXCrom Jan 1, 2018 •

edited

Loading

Amaroq7 Jan 1, 2018

rsKliPPy Jan 1, 2018 •

edited

Loading

Amaroq7 Jan 1, 2018

WPMGPRoSToTeMa Jan 1, 2018

OciXCrom Jan 1, 2018

OciXCrom Jan 2, 2018

OciXCrom commented Jan 9, 2018

In-line commented Jan 9, 2018 •

edited

Loading

OciXCrom commented Jan 9, 2018

WPMGPRoSToTeMa commented Jan 10, 2018

OciXCrom commented Jan 10, 2018 •

edited

Loading

asherkin commented Feb 10, 2018

OciXCrom commented Feb 10, 2018

twisterniq commented Apr 28, 2019

In-line May 19, 2019

voed commented Nov 21, 2019

OciXCrom commented May 30, 2020

Add a flags update forward and a override param in set_user_flags #475

Are you sure you want to change the base?

Add a flags update forward and a override param in set_user_flags #475

Conversation

OciXCrom commented Dec 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WPMGPRoSToTeMa Jan 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Amaroq7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OciXCrom Jan 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsKliPPy Jan 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OciXCrom commented Jan 9, 2018

In-line commented Jan 9, 2018 • edited Loading

OciXCrom commented Jan 9, 2018

WPMGPRoSToTeMa commented Jan 10, 2018

OciXCrom commented Jan 10, 2018 • edited Loading

asherkin commented Feb 10, 2018

OciXCrom commented Feb 10, 2018

twisterniq commented Apr 28, 2019

Choose a reason for hiding this comment

voed commented Nov 21, 2019

OciXCrom commented May 30, 2020

WPMGPRoSToTeMa Jan 1, 2018 •

edited

Loading

OciXCrom Jan 1, 2018 •

edited

Loading

rsKliPPy Jan 1, 2018 •

edited

Loading

In-line commented Jan 9, 2018 •

edited

Loading

OciXCrom commented Jan 10, 2018 •

edited

Loading