SEServerExtender / EssentialsPlugin

Dedicated Essentials Plugin
GNU Lesser General Public License v3.0
18 stars 18 forks source link

Improved chat commands parsing. #13

Closed LeShred closed 9 years ago

LeShred commented 9 years ago

Allowing using double quotes to set commands parameters with spaces. eg: /admin move player position "John Doe" 100 100 100

Use double quotes twice in order to set include a double quote in the parameter. eg: /admin move player position "John""Doe" 100 100 100 (for player John"Doe)

dodexahedron commented 9 years ago

Thanks for this. I was going to write the command parser as a state machine, but you went ahead and did it. :) Have you tested it in all commands and in the automated cleanups, yet?

LeShred commented 9 years ago

You're welcome! I didn't tested it with the automated cleanup. I wasn't aware there is an automated cleanup, I started working on the plugin sources this morning and didn't read all the sources currently. How can I test with the automated cleanup?

dodexahedron commented 9 years ago

Automated cleanups can be found in the settings for Essentials. They're timed or triggered events that use scan commands to delete objects based on what the scan returns. Any change in behavior to that will result in a lot of people having unexpected ship deletions, which is always fun. So we just need to be sure that the behavior of the new parsing logic behaves the same as the old logic, for unquoted values.

LeShred commented 9 years ago

OK I see, it's the Timing and triggered cleanups. I will check the behavior with those.

dodexahedron commented 9 years ago

Awesome. Thanks

dodexahedron commented 9 years ago

I tested this out myself, and it seems to work well. I then merged it and realized that it would be useful to standardize command parsing among plugins and SESE itself, so I backed the change out and then put a modified version of the code in SESE, in a utility class. The behavior is the same, but it is just a little more efficient. Thanks for this contribution!

LeShred commented 9 years ago

I just had a look before seeing your message and realized that timed and triggered cleanups are using command.Split( ' ' ) and thus the parsing has to be moved to an utility class :) Did you also modified ProcessTimedItem and ProcessTriggerItem in order to use the new parser you implemented in SESE?

LeShred commented 9 years ago

Oh and thanks for the code improvement! I did saw that you used a StringBuilder and just found the performance improvement it makes compared to simple string manipulation. I wasn't aware of that!

LeShred commented 9 years ago

I drilled into the code of the plugin and found that some utility classes like CubeGrids use a string splitter - General.SplitString() - with double-quotes management and using the new parser with some commands can lead to unexpected behaviors. The included parser handles double-quotes but it doesn't handles double double-quotes which can be a problem.

For example in CubeGrid.ScanGrids() the command string is rebuilt in the first 2 lines and then analyzed. This will cause a problem with the command - /admin scan grids functional "hasdisplayname:My ""Super ship"":exact" - as it will split the displayname parameter into 3 chunks and so the command will not work as expected.

Please have a look at that and if you agree I would recommend removing the use of this string splitter. I can do it if you want.