Screen.add_line.get_line_chunks() discards everything before first colour string.

[eirikba]

Screen.add_line.get_line_chunks() (in deluge/ui/console/Screen.py) will discard all the data before the first occurrence of '{!'.

Patch attached that fixes this issue, but also changes the chunking of '{!1!}1{!2'. The old version would return [('{!1!}', '1'), ('{', '!2')]. The new version returns [('{!1!}', '1'), ('{!2', )]. I'm not sure that is correct, but I don't think it is worse :)

On the other hand, both the new and the old chunks '{!a!}a{!}other' to [('{!a!}', 'a'), ('{!}', 'other')]. I don't know if that's correct either.

[Cas]

Does it need such drastic code change, this would be simpler and i think should work just as well.

@@ -172,6 +174,9 @@ def get_line_chunks(line):
                 end = line.find("!}") + 2
                 color = line[beg:end]
+                if not line.startswith("{!"):
+                    # Found text with no color tag
+                    chunks.append(('', line[:line.find("{!")]))
                 line = line[end:]

Patch attached that fixes this issue, but also changes the chunking of '{!1!}1{!2'. The old version would return [('{!1!}', '1'), ('{', '!2')]. The new version returns [('{!1!}', '1'), ('{!2', )].

If both opening and closing braces are not supplied then it is not valid and I do not think it should be parsed and instead should return an error/exception.

On the other hand, both the new and the old chunks '{!a!}a{!}other' to [('{!a!}', 'a'), ('{!}', 'other')]. I don't know if that's correct either.

This would be valid because '{!}' will match both '{!' and '!}' when parsing.

[eirikba]

No, it doesn't need such drastic change. It would be enough to handle the beginning of the line before the loop. However, your change wouldn't work. If the input doesn't contain any occurrences of '{!', then the loop won't be entered at all!

The reason for the complete rewrite was that I didn't particularly like the old code :)

Unfortunately, my version ended up less elegant than I had hoped for, too. Still, I think my version is simpler, less magic and thus "more obviously correct" than the old one. But maybe I should have gone for a simpler "split off the beginning per iteration" loop instead. I'll write that up and see what it looks like.

I don't think it is particularly useful for this code to raise an exception. The result of chunking will be passed on to the real parser afterwards. It is probably better to leave any error detection to that one. (And that is exactly the point where I don't know how '{!}' will be interpreted.)

[eirikba]

Ok, here's an alternative version. I think I prefer this one to my first attempt. It is a little longer and a little less efficient, but it is also more straightforward and easier to understand.

It produces the exact same results as my previous version (at least for the set of test cases I used). But it should be trivial to modify its behaviour for the two cases I was uncertain how to deal with correctly.

[Cas]

The change I suggested does work for the case that this bug was opened but I see that it fails with completely uncoloured lines.

I have modified the patch slightly and added a 'continue' that will discard lines which are not correct.

 https://github.com/cas--/Deluge/commit/33cc73fc89

I had a look in colors.py and '{!}' will also be discarded as invalid but not at the correct point so that will need fixing separately.

[eirikba]

I think your version will chunk 'some{!text' to [(,'some'), ('{', '!text')]. Was that intended?

[Cas]

Fixed 1.3-stable:  d08ea7da