Never permit of using [unassigned] value in SetPersonaState() + deterministic name

[JustArchi]

Please review the changes carefully, this can cause serious regressions if I didn't catch some common-use scenario.

Rationale: #442

[JustArchi]

The biggest advantage of this is fixing two outstanding issues I have with current implementation:

1. It's not possible to tell a difference between pre-initialized name field and user who put [unassigned] as his name in particular.
2. If SetPersonaState() will be called before name initialization, as seen in Player name is changed to [unassigned] on successful login #442 and in my own projects that I've patched, name of the user is changed to [unassigned], which is unwanted. I think exception in this case is justified - user should ensure that nickname is initialized. If needed we could improve it by adding extra documentation and/or requiring from user to put valid name argument instead of using our cached data.

If I didn't miss any use case of those two fields, this is a breaking change as default value of uninitialized user has changed, but I think it's justified if we're able to fix both points above while not generating any major regressions in internal SK2 code. While I can verify fixes, I hope you can help me with SK2 internals knowledge. It looks good to me, but I don't know private SK2 code that good.

[yaakov-h]

I was thinking more along the lines of omitting stateMsg.Body.player_name and only sending stateMsg.Body.persona_state.

We have SetPersonaName and SetPersonaState as two separate functions, but each implicitly does the same role as the other.

I'm assuming though that Steam can handle only one of the two fields being set... I'll have to have a look at how Steam handles it and at what point in the application lifecycle it calls SetPersonaState( k_EPersonaState_Online ) or similar.

[JustArchi]

If you confirm that we can omit it then we can get rid of that ugly exception as well as that name variable in the request itself. This is even more satisfying than current solution, let me know about your results.

Still, I vouch for setting name initially to null, as I'd still want to be able to check it against null in order to determine nickname of the user.

[yaakov-h]

Steam's behaviour is as I remember it:

1. SetPersonaState and SetPersonaName both update client-side records, similar to SteamKit. They then call CCMInterface::SendFriendsStatusToServer(bool, unsigned long long)
2. CCMInterface::SendFriendsStatusToServer appears to have 3 different codepaths that send CMsgClientChangeStatus. I think this is new? I can't quite tell what the difference is between each of them.

For a more practical approach, rather than an analytic one, I just tested with SteamKit and if you send CMsgClientChangeStatus without player_name, the CMs preserve the old name.

On the flip side, if you omit persona_state, Steam interprets it as the default value, i.e. 0, i.e. k_EPersonaStateOffline.

[JustArchi]

Now the only question is if something explicitly depends on default value of account's name. I bet not in SK2 internals, since otherwise it should be marked with const and properly referenced in some other place. Still, this looks like some old code, making such assumption without knowing it could be fatal.

I love how we could get rid of that exception and just avoid setting name entirely 👍.

[yaakov-h]

This means now that GetPersonaName, GetFriendPersonaName and GetClanName can all now return null.

[JustArchi]

Correct, I improved docs a little with this detail, not sure if we can do anything more in terms of this, apart from stating breaking change in the release changelog.

I thought of making those methods deprecated at first with something like return cache.LocalUser.Name ?? "[unassigned]"; but in the end I think it'd bring more bad than good. I don't want to introduce new methods with new names for something like this. Since SK 2.0 is still in alpha stage, I think there won't be a better time than now to decide after such changes.

[voided]

I think we need to do a better job of defining exactly when the local user's persona name will be available. Might have to dig into the disassembly again to see exactly how steamclient handles this.

[JustArchi]

Feel free to update PR with a better explanation if appropriate - GitHub recently added a way for repo maintainers to update PRs if contributor enables such option 🙂.

[voided]

I like the idea of excluding the local name in SetPersonaState, that looks fine. But I'd really like to get a handle on when the local user's name is available and document that a little better before we merge this.

[yaakov-h]

The local user's name is available after AccountInfoCallback, but I think part of the problem is that:

1. The local user's name is only available after SteamFriends handles the account info msg
2. Custom ClientMsgHandlers may also want to handle the account info msg and call code which depends on SteamFriends
3. The AccountInfoCallback is posted by SteamUser, not `SteamFriends.
4. The order in which ClientMsgHandlers are run is undefined because we are enumerating through a dictionary's values, not an ordered collection.

Therefore, code which handles the account info msg or code which handles the account info callback could have the local user name set, or could not. It's one big fat race condition.

For code which handles subsequent messages/callbacks, the local user name should always be available.

[JustArchi]

It's also important to remember that Steam might not always send callbacks in appropriate order, so we might receive AccountInfoCallback at later stage, not making it possible to say "AccountInfoCallback will always arrive before XXX callback".

It's fine to say that local user's name will be available after receiving AccountInfoCallback, but we can't make any more assumptions in terms of callback order. I think it could be possible to iterate over dictionary in a way that SK2 handlers could be evaluated firstly, we could even add a internal bool IsPriority set to true for SK2 handlers and do .OrderByDescending(handler => handler.IsPriority ? 1 : 0).

[yaakov-h]

Or just use a System.Collections.Specialized.OrderedDictionary.

[JustArchi]

How about now? It looks enough documented to me.

[yaakov-h]

Could do with some grammatical tweaks,:

• "user initialisation", not "user's initialisation".
• Should not be left ambiguous, e.g. "is performed prior to AccountInfoCallback rather than "typically happens during AccountInfoCallback". Although in this case we can't be honest as per above, since AccountInfoCallback could be triggered before SteamFriends finishes... perhaps document how it should work, then fix the behaviour.

I'd also remove the initialisation descriptions from Friend and Clan. We go from knowing nothing about them to knowing the exist, and Steam provides us with the name at that point in time (though it's not guaranteed to do so - protocol could change). The old xmldoc is probably sufficient for these two, and we can use the release notes to announce that they could potentially be null in a severe edge case.

[JustArchi]

I think this should work now.

Apart from docs suggestions I changed internal implementation of handlers to ordered dictionary, so SK2 handlers are always first (in our fixed order), and then user handlers are executed last.

I also changed order of SteamUser -> SteamFriends to SteamFriends -> SteamUser during initialization, I stopped for a second wondering if old order was supposed to mean anything, but then, previous dictionary had unpredictable order anyway, so it should not. I checked if all tests were passing after doing this change, looks good to me.

[JustArchi]

As of now I think this PR is in good state to be merged, unless somebody has something else to add.

We could in the future improve handlers and their order, but even current solution is good enough and doesn't result in worse overall health condition than previous one, in fact, it fixes things the way they should be fixed to begin with.

[yaakov-h]

I'm happy with this. @voided?