User privacy enhancements

scheibo commented 4 years ago

Add a `/nofeedback` command

I've been thinking about some sort of command like /nofeedback /hidenext so it doesn't print a response hence /nofeedback as a command that runs the next command, but hides all sendReply responses

Add sticky checkboxes to the Find Battle/Challenge UIs which will cause the battle to be hidden

<abbr title="You can still invite spectators by giving them the URL or using the /invite command" class="checkbox">Ban spectators</abbr>

I would put it between the team selector and "Battle!" button & above challenge/cancel and below the team selector

The state of the Ban Spectators checkbox can be stored in local storage to ensure it is sticky.

Support renaming rooms

apparently the rename functionality just adds a broadcast linking to the new room but we do also have protocol infrastructure for actually renaming a room it's used by room aliases to redirect to the actual roomid

Passwords

When a room is made invite only (either from the beginning or mid battle), kick out all guests and randomize the URL

the new room id (and thus URL) generated by the sim server should follow the same pattern as replays which were made private: https://replay.pokemonshowdown.com/gen7randombattle-956127160-fz0wmd2n1wu7gorbay9jtvqxpc3tkl4pw. ie/ normal room id plus a random character string that ends in 'pw' (for 'password')
if a replay is saved of the invite only room, the password generated by the sim server is to be used instead of the password generated by the replay server
the password generation on the replay server is still needed in case a replay is made private after the fact, though the password generated may be different than the password generated by the sim server (though will follow the same format)

Zarel commented 4 years ago

I did some edits to remove things that were a combination of my typos and some miscommunications.

I have no intention of removing the distinction between /hideroom and /inviteonly. Both should exist, but the UI should only surface /hideroom because the difference is too minor to have both.

AnnikaCodes commented 4 years ago

apparently the rename functionality just adds a broadcast linking to the new room

While there is a banner stating the room has been renamed, it's not a link -- it seems that all user connections are moved to the new room, as is the modlog. There is, however, a bug in /renameroom, which I opened a PR to fix (#6952): the chatroom's new name is never saved to chatrooms.json, and thus the rename partially reverts after restarting.

Zarel commented 4 years ago

The client part of manual password specification is now done:

https://github.com/smogon/pokemon-showdown-client/commit/fdca8fe5dce755dd8390bc781dc5e5a3d9708cb5

You need only include the password in the JSON blob in |queryresponse|savereplay|:

https://github.com/smogon/pokemon-showdown/blob/a470fc48ca4fa8478473c32cc999b1b37daaa8bf/server/rooms.ts#L1595-L1599

Zarel commented 4 years ago

/nofeedback is also done (by @AnnikaCodes) in #6953. The command name still needs some workshopping (we can merge it whenever it's needed if we don't think of a better command name, but it's worth waiting a bit to see if anyone comes up with anything).

scheibo commented 4 years ago

https://github.com/smogon/pokemon-showdown/pull/6947 has partial /hidenext support in that it hides the room but doesnt implement the password/renaming/kicking out guests. The following added to BasicChatRoom should help with some of that:

    // TODO kick out guests
    makePrivate(privacy: true | 'hidden' | 'voice') {
        // This is the same password generation approach as genPassword in the client replays.lib.php
        // but obviously will not match given mt_rand there uses a different RNG and seed.
        let password = '';
        for (let i = 0; i <= 31; i++) password += ALPHABET[Math.floor(Math.random() * 36)];

        this.settings.isPrivate = privacy;
        if (this.roomid.endsWith('pw')) return Promise.resolve(true);
        return this.rename(this.title, `${this.roomid}-${password}pw` as RoomID);
    }
    makePublic() {
        this.settings.isPrivate = false;
        if (!this.roomid.endsWith('pw')) return Promise.resolve(true);
        return this.rename(this.title, this.getReplayData().id as RoomID);
    }
    getReplayData() {
        if (!this.roomid.endsWith('pw')) return {id: this.roomid.slice(7)};
        const end = this.roomid.length - 2;
        const lastHyphen = this.roomid.lastIndexOf('-', end);
        return {id: this.roomid.slice(7, lastHyphen), password: this.roomid.slice(lastHyphen, end)};
    }

but Battle rooms don't currently support renaming and I think thats perhaps hairy to handle. I'm going to look at the client side UI changes because even in the worst case 'Ban spectators' just doing the current /ionext behavior isnt a bad incremental step

NOTE: when renaming battle rooms, we will want to keep the same battle logs name to avoid polluting the logs directories used by usage stats - the files should still have the id without the password, even if the URL of the battle changes names

Zarel commented 4 years ago

I'd be fine with /ionext has a stopgap.

scheibo commented 4 years ago

OK, so i've been beating my head against the wall for a while getting things hooked up with BattleReady, and I think the main crux of my dilemma is:

For the purposes of the UI toggle I would like the ability to scope the privacy of the battle to what the state of the toggle was when the button was clicked and the search/challenge was issued. I think this is most intuitive for the user.
/hidenext and /ionext have different semantics in that they are scoped to the next battle that is created, not search/challenge that is issued.

It is the interplay of these two elements and the discrepancy of what they should intutitively apply to that is most difficult for me to reconcile. My BattleReady solution makes things simpler for the UI's use case, but changes the semantics of the chat commands to apply to the next search/challenge instead of next battle. Consider interactions such as:

the user kicks of a search without 'Ban spectators' selected, but then types /ionext while waiting for a response

Here it seems sort of like the user changed their mind and maybe wanted the battle to be private? But if we go with BattleReady-scoped options, the battle will be public, even though it is the next battle and /ionext was used. Next:

the user challenges friend Alice with 'Ban spectators' selected, but then challenges user Bob with 'Ban spectators' not selected, and Bob accepts first, followed by Alice.

If options are not BattleReady scoped, both battles will be public, even though it seems fairly clear the battle with Alice was meant to be private.

smogon / pokemon-showdown