Add surround with feature

[SimonBrandner]

This PR adds surround with feature - a quick way to surround the selected text with the desired character just by pressing it. It's hidden behind a setting. Currently, the following characters are supported:

" _ ` ' * ~ $ ( [ { < 

[jryans]

I think @bwindels is the best reviewer for this on the code side, but the main question to answer here is whether this is desirable product-wise.

[nadonomy]

I think @bwindels is the best reviewer for this on the code side, but the main question to answer here is whether this is desirable product-wise.

@SimonBrandner @jryans apologies for the laggy review on this. The honest answer is we won't know until we test. There's a real risk it could trip users up who expect keyboard input to replace whatever is highlighted. What about if we add this as a setting or labs flag?

[SimonBrandner]

@nadonomy, no worries about the late review! Thanks for catching up with a bunch of my PRs today! I agree hiding this behind a setting makes sense. I'd prefer a classical setting over labs. Do you agree?

[SimonBrandner]

@nadonomy, done, it's a setting now

[ShadowJonathan]

Could maybe markdown sigils (*, _, ~, etc.) also be supported?

[bwindels]

Lgtm, although not sure special casing wrappings with the same character on both sides is worth the extra code?

[SimonBrandner]

Yeah, it probably isn't. I'll change that

[bwindels]

oh, I imagined you'd put everything in one map, but I guess if the 2nd param to toggleInlineFormat is undefined, that will work too.

Although you're allocating and filling an array on every keystroke though. If you'd put everything in SURROUND_WITH_DOUBLE_CHARACTERS, that could become just one map lookup with SURROUND_WITH_DOUBLE_CHARACTERS.has(event.key)

[SimonBrandner]

oh, I imagined you'd put everything in one map, but I guess if the 2nd param to toggleInlineFormat is undefined, that will work too.

Although you're allocating and filling an array on every keystroke though. If you'd put everything in SURROUND_WITH_DOUBLE_CHARACTERS, that could become just one map lookup with SURROUND_WITH_DOUBLE_CHARACTERS.has(event.key)

I did this because repeating the characters in the map felt a bit weird. On the other hand, this might look a bit weird. If you'd prefer the other thing, I am happy to change it

[nadonomy]

Thanks @SimonBrandner, looks good! Just following on from our chat, to reduce ambiguity please can you update the Settings description to:

Surround selected text when typing special characters

[nadonomy]

LGTM, thanks again!