Skip to content

Fixed behavior difference in SetPlayerName():#672

Closed
dev-karpov0 wants to merge 5 commits intoopenmultiplayer:masterfrom
dev-karpov0:fix-SetPlayerName
Closed

Fixed behavior difference in SetPlayerName():#672
dev-karpov0 wants to merge 5 commits intoopenmultiplayer:masterfrom
dev-karpov0:fix-SetPlayerName

Conversation

@dev-karpov0
Copy link
Copy Markdown
Contributor

@dev-karpov0 dev-karpov0 commented Jun 13, 2023

If the specified name has already been taken by player - it is set anyway, then native returns 1. (should 0 and the same name cannot be re-set).
If name has already been taken by another player, native returns 0 (should -1).

Docs

If the specified name has already been taken by player - it is set anyway, then native returns 1. (should 0 and the same name cannot be re-set).
If name has already been taken by another player, native returns 0 (should -1).
Comment thread SDK/include/utils.hpp Outdated
}

/// Makes Case Insensitive comparison of StringView strings
inline bool strIsEqualCI(const StringView& str1, const StringView& str2)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StringView should be passed by value, refer to CODING.md

@@ -253,8 +253,21 @@ SCRIPT_API(ResetPlayerMoney, bool(IPlayer& player))

SCRIPT_API(SetPlayerName, int(IPlayer& player, const std::string& name))
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add SCRIPT_API_RETVAL(-1 here to return -1 if player is invalid

Copy link
Copy Markdown
Contributor Author

@dev-karpov0 dev-karpov0 Jun 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean SCRIPT_API_FAILRET?

In SA:MP SetPlayerName returns 0 if player is invalid.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK no change needed then

@dev-karpov0
Copy link
Copy Markdown
Contributor Author

There's an error: is SA:MP if name is already taken by ANOTHER player - SetPlayerName returns 0. But the doc says -1

For backward compatibility, i think, the docs must be edited.

@dev-karpov0 dev-karpov0 marked this pull request as draft June 14, 2023 04:48
@AmyrAhmady
Copy link
Copy Markdown
Member

Is there any reason you marked it as a draft?

@Hual
Copy link
Copy Markdown
Collaborator

Hual commented Jun 14, 2023

There's an error: is SA:MP if name is already taken by ANOTHER player - SetPlayerName returns 0. But the doc says -1

For backward compatibility, i think, the docs must be edited.

The samp doc itself says that but if you're sure the server does the opposite then yeah docs should be updated

@dev-karpov0
Copy link
Copy Markdown
Contributor Author

dev-karpov0 commented Jun 14, 2023

Is there any reason you marked it as a draft?

Yes, see comment above. Now (in last commit) SetPlayerName returns -1 if name has already taken by another player.

@dev-karpov0 dev-karpov0 marked this pull request as ready for review June 14, 2023 05:21
@dev-karpov0
Copy link
Copy Markdown
Contributor Author

I have a question.
In omp_player.inc native SetPlayerName has bool tag, although it may return -1.
Can i open a PR in omp-stdlib repository to fix that?

@Hual
Copy link
Copy Markdown
Collaborator

Hual commented Jun 14, 2023

I have a question.
In omp_player.inc native SetPlayerName has bool tag, although it may return -1.
Can i open a PR in omp-stdlib repository to fix that?

Of course

@Hual
Copy link
Copy Markdown
Collaborator

Hual commented Jun 14, 2023

Now that we figured the docs are wrong, what does this PR fix?

@dev-karpov0
Copy link
Copy Markdown
Contributor Author

It fix ability to re-set the name that player already has.

If the specified name has already been taken by player - it is set anyway, then native returns 1. (should 0 and the same name cannot be re-set).

@Hual
Copy link
Copy Markdown
Collaborator

Hual commented Jun 14, 2023

It fix ability to re-set the name that player already has.

If the specified name has already been taken by player - it is set anyway, then native returns 1. (should 0 and the same name cannot be re-set).

I think that's deliberate to allow for changing upper and lower cases

@Y-Less
Copy link
Copy Markdown
Collaborator

Y-Less commented Jun 14, 2023

It is yes. Could maybe be even stricter and return 0 if the passed and current names are identical down to case, but then there's a semantic question: does return 1 mean "the name was changed" or "the name is now what you passed". Because in the case of passing in the same name, no change took place, but the name is now what you wanted. If you're using that return value for script logic you probably don't want to retry, you probably don't want to show an error to a user, because although nothing changed, the final result is still correct. This could be a case for -1 return since most if (SetPlayerName(pid, name)) checks will still just pass, but you can go better information if desired. 0 - error, 1 - name changed, -1 - name already that. Maybe.

@dev-karpov0
Copy link
Copy Markdown
Contributor Author

but then there's a semantic question: does return 1 mean "the name was changed" or "the name is now what you passed"

I think it is necessary to proceed from the purpose of the function. SetPlayerName explicitly says that the name must be set. If it is not set, return value must be different from 1, because the function did not fulfill its purpose.

0 - error, 1 - name changed, -1 - name already that.

It's better, but backward compatibility will be broken.

@dev-karpov0
Copy link
Copy Markdown
Contributor Author

Maybe, if names are identical (with case) then return 0.

@Hual
Copy link
Copy Markdown
Collaborator

Hual commented Jun 14, 2023

Is the return value such an issue at all? Returning 1 when changing player name with only case sensitivity differences seems fine as the name is actually changed, don't see why it's an issue

@Y-Less
Copy link
Copy Markdown
Collaborator

Y-Less commented Jun 14, 2023

Is the return value such an issue at all? Returning 1 when changing player name with only case sensitivity differences seems fine as the name is actually changed, don't see why it's an issue

I fully agree with that - as you say, the name has changed. I was just thinking about the other situation of no case change at all.

dev-karpov0 added a commit to dev-karpov0/web that referenced this pull request Jun 15, 2023
@dev-karpov0
Copy link
Copy Markdown
Contributor Author

I created the PRs to update dosc in omp-stdlib and web.
This PR is closed. Thank you.

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.

4 participants