getWrap not splitting text correctly

[danielepantaleone]

I noticed (at least under Urban Terror 4.x) that the getWrap method of the Parser class is not splitting text correctly: as you can see from the screenshot below, some words, which I believe B3 consider to be in the first line, are printed on the second line even thought the second line is almost empty:

I think this happens because getWrap doesn't consider the following variables when computing the lines to be set to the server:

• B3 in-game prefix
• Server say/tell prefix

Lines should considered separately: the 1st line will contains also the prefixes, while the other lines just pure text to be sent. Also I believe that a _stripcolor flag needs to be specified (some games discard color codes when computing the length of a single line....iirc UrT 4.2 does that).

Also the current implementation consider 2 parameters: length and min_wrap_length while in my opinion only the maximum sentence length should be considered. I made a possible implementation of a new getWrap function (which takes into consideration also prefixes added by both B3 and game server): https://gist.github.com/FenixXx/daf5d3112b5cbf6c755a

I don't know if this problem affects only Urban Terror or other games so I don't know where to apply the fix :smile:

[thomasleveil]

I always found it difficult to understand that getWrap method implementation :P

where to apply the fix

If you can test only on UrT, then apply the fix to UrT only.

[danielepantaleone]

I know that I can override the method in iourt41.py but the problem is that getWrap is also called from the abstract parser (say method, so if I override it in iourt41.py then I will have a mismatch in the number of parameters passed to the function). I think I will investigate further and apply the patch to the Parser class (if needed)

[danielepantaleone]

I pushed a new version of the function here: https://github.com/FenixXx/big-brother-bot/tree/get-wrap As you can see from the screenshots below it's doing its job quite well. Although some tests are failing (CS:GO parser tests and Ravaged tests....I believe Ravaged ones are failing because of CS:GO). I couldn't find the problem tbh. @thomasleveil could you please have a look?

[thomasleveil]

I'll take a look this we

[thomasleveil]

sorry for the delay.

I thought that the parser msgPrefix was added only on the first line, so instead of

[b3][pm] Available commands: admins, admintest,
[b3][pm] advadd, advlist, advload, advrate, advrem, aliases,
[b3][pm] allaliases, b3, ban, banall, baninfo, calladmin, clear, 
[b3][pm] clientinfo, cmdalias, cmdlevel, deletenotice, die,

we would have:

[b3][pm] Available commands: admins, admintest,
advadd, advlist, advload, advrate, advrem, aliases,
allaliases, b3, ban, banall, baninfo, calladmin, clear, 
clientinfo, cmdalias, cmdlevel, deletenotice, die,

So IMHO the getWrap implementation should not have to worry about msgPrefix. It should be the responsability of the say(), saybig(), message() methods.

Would you agree on usage and results as described in test cases in Test_getWrap and Test_getWrap_integration ?

Finally the native textwrap.TextWrapper might worth the try.

[danielepantaleone]

At current state (before my changes and after) every line has the message prefix. Infact as you can see from commands declarations, the prefix is added just before sending the RCON command, together with some other prefixes (where the skip parameter kicks in):

_commands = {}
_commands['broadcast'] = '%(prefix)s^7 %(message)s'
_commands['message'] = 'tell %(cid)s %(prefix)s ^3[pm]^7 %(message)s'
_commands['deadsay'] = 'tell %(cid)s %(prefix)s [DEAD]^7 %(message)s'
_commands['say'] = 'say %(prefix)s %(message)s'
_commands['saybig'] = 'bigtext "%(prefix)s %(message)s"'

That's why I considered them in my get_wrap implementation. My code does exactly what the old one does, except that mine works (about the old one, I didn't figured out the logic behind). About the native TextWrapper: sure we can make use of it. I didn't know it existed and I didn't search it on the internet since a get_wrap implementation is quite easy to do.

[thomasleveil]

the prefix is added just before sending the RCON command

I see, this is indeed how it is done so far with q3a based parsers.

Take a look at how it is done with Frostbite2, does it look easier to work with this way?

[danielepantaleone]

I'm not sure. Problem is that this lack of consistency. I understand that every parser implements its oen code but the format should be the same, despite of the parser being used. I guess adding the prefix only on the first line will be more than enough (it will also save some space and more text can fit inside lines)

[danielepantaleone]

Again on this: I have been using the default textwrap implementation on a plugin and it seems to work quite good. I'll have a look and fix this in the next few weeks.

[danielepantaleone]

Alright, I created a new branch https://github.com/FenixXx/big-brother-bot/tree/get_wrap in which the new getWrap implementation (which is only in the Parser class) uses textwrap.TextWrapper. I updated all the parsers so that prefixes (bot prefix, pm prefix, dead prefix) are included in the text to be splitted (so they appear only once).

The only problem I got is with test files: 22 tests are failing with error messages like the following

======================================================================
FAIL: test_ban (tests.parsers.test_csgo.Test_parser_API)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/daniele/Documents/workspace/big-brother-bot/tests/parsers/test_csgo.py", line 897, in test_ban
    call('sm_say courgette was banned test')])
  File "build/bdist.macosx-10.9-intel/egg/mock.py", line 863, in assert_has_calls
    'Actual: %r' % (calls, self.mock_calls)
AssertionError: Calls not found.
Expected: [call('sm_addban 0 "STEAM_1:0:1111111" test'), call('sm_kick #2 test'), call('sm_say courgette was banned test')]
Actual: [call('sm_addban 0 "STEAM_1:0:1111111" test'),
 call('sm_kick #2 test'),
 call('sm_say  courgette was banned test')]

@thomasleveil do you have time to have a look?