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 ability to escape commands with a back-slash #1768

Closed
wants to merge 1 commit into from

Conversation

ShadowNinja
Copy link
Member

Rationale: Useful when telling a user to run a specific command.

Older clients will see the back slash on their display when they send a message, and older servers will show the backslash on all other displays.

@sfan5
Copy link
Member

sfan5 commented Oct 25, 2014

👎
We don't need this, typing " /some_command" works too and everyone does this.
Just because you chose the double-forward-slash escape because that's how my IRC client does it doesn't mean everyone else wants that.

@SmallJoker
Copy link
Member

👎 Same reason as sfan5

@ShadowNinja
Copy link
Member Author

@sfan5: That prepends a space to your message though. I don't think IRC clients use this method while all of their users want a different method, they didn't just chose a random method. The fact that this is in IRC clients means that users want it and find that this method is a good way to escape it.

@Calinou
Copy link
Member

Calinou commented Oct 25, 2014

👍

It makes sense, many IRC clients do that already.

@kaeza
Copy link
Contributor

kaeza commented Oct 26, 2014

👍

Apart from Calinou's reason, it adds a way to discourage silly command names like WE had.

@ghost
Copy link

ghost commented Oct 26, 2014

I'd much prefer the ./command style, for some reason it just feels better. Then again, I've probably played too much MC.

@Calinou
Copy link
Member

Calinou commented Oct 26, 2014

./command looks bad and looks like you forced yourself some random workaround.

@sfan5
Copy link
Member

sfan5 commented Oct 30, 2014

@sfan5: That prepends a space to your message though.

Holy shit. a space. What is so bad about a space?

@ShadowNinja
Copy link
Member Author

@sfan5: It's a hack, a workaround rather than a proper fix.

@est31
Copy link
Contributor

est31 commented Mar 1, 2015

I'm for backslash \ as escape character. Double slash would break worldedit and is inconsistent with most programming languages.

@ShadowNinja ShadowNinja changed the title Add ability to escape commands with a second forward-slash Add ability to escape commands with a back-slash Aug 24, 2015
@ShadowNinja
Copy link
Member Author

Rebased and changed to back-slash.

@est31
Copy link
Contributor

est31 commented Aug 24, 2015

👍

LocalPlayer *player = m_env.getLocalPlayer();
assert(player != NULL);
std::wstring name = narrow_to_wide(player->getName());
m_chat_queue.push((std::wstring)L"<" + name + L"> " + message);
m_chat_queue.push((std::wstring)L"<" + name + L"> " +
(message[0] == L'\\' ? message.substr(1) : message));
Copy link
Member

@sfan5 sfan5 Aug 29, 2015

Choose a reason for hiding this comment

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

Actually this code does not do what the description says.
Currently if I were to type in "\foobar" it would get changed to "foobar".
What this is supposed to do is "\/foobar" -> "/foobar" and only that.

@est31
Copy link
Contributor

est31 commented Mar 18, 2016

@paramat there is one disapproval by @sfan5 makes 0 approvals in total (or one if SN approves)

@paramat
Copy link
Contributor

paramat commented Mar 18, 2016

Okay, but i doubt we add them up like that, 2 approvals remain 2 approvals despite dissapprovals.

@ShadowNinja
Copy link
Member Author

I have to fix the bit sfan5 reported before this is ready. It's fairly trivial, I've just been busy with other things.

@0-afflatus
Copy link

I'm with @sfan5 on this, but I guess I could live with it. Non-issue for me.

@paramat paramat added the WIP label Mar 21, 2016
@paramat paramat added the Rebase needed The PR needs to be rebased by its author label Oct 27, 2016
@paramat
Copy link
Contributor

paramat commented Oct 27, 2016

@ShadowNinja ?

@ShadowNinja
Copy link
Member Author

Sorry, I still don't have much time for this. Someone else could fix this quite easily and merge it since it's already approved.

@ShadowNinja ShadowNinja added the Trivial The change is a trivial bug fix, documentation or maintenance change, as per the Git Guidelines label Oct 30, 2016
@paramat paramat added the Adoption needed The pull request needs someone to adopt it. Adoption welcomed! label Oct 30, 2016
@sfan5
Copy link
Member

sfan5 commented Oct 30, 2016

since it's already approved.

disapprovals don't count anymore?

@ShadowNinja
Copy link
Member Author

Er, I'm not sure what it adds up to.

@paramat
Copy link
Contributor

paramat commented Nov 25, 2016

Disapprovals don't subtract from approvals, so this technically has 2 approvals, counting one from the core dev author. However it is good dev practice to address objections.

@HybridDog
Copy link
Contributor

Players shouldn't make the command be shown as their chat message just to tell other players how to use the command.
Also, adding a space before the command is a lot easier than \ (on qwertz keyboards, that is) and players don't become baffled because of not knowing how other players can write the command without being executed.
It also confuses modders because the chat message which is shown is not the same as the string in lua.

@nerzhul
Copy link
Member

nerzhul commented Apr 11, 2017

@HybridDog adding space is what i do on... skype, IRC, gitlab, slack... everywhere, and it's more natural

@Zughy Zughy removed the Adoption needed The pull request needs someone to adopt it. Adoption welcomed! label May 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Low priority One approval ✅ ◻️ Rebase needed The PR needs to be rebased by its author @ Script API Trivial The change is a trivial bug fix, documentation or maintenance change, as per the Git Guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.