Opened 11 years ago
Closed 11 years ago
#2062 closed patch (Fixed)
Screen.add_line.get_line_chunks() discards everything before first colour string.
Reported by: | eirikba | Owned by: | |
---|---|---|---|
Priority: | minor | Milestone: | 1.3.6 |
Component: | Console UI | Version: | 1.3-stable |
Keywords: | Cc: |
Description
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.
Attachments (2)
Change History (9)
Changed 11 years ago by eirikba
comment:1 Changed 11 years ago by Cas
comment:2 Changed 11 years ago by 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.)
Changed 11 years ago by eirikba
comment:3 Changed 11 years ago by 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.
comment:4 Changed 11 years ago by 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.
comment:5 Changed 11 years ago by Cas
- Component changed from other to console
- Milestone changed from Future to 1.3.5
comment:6 Changed 11 years ago by eirikba
I think your version will chunk 'some{!text' to [(,'some'), ('{', '!text')]. Was that intended?
comment:7 Changed 11 years ago by Cas
- Resolution set to fixed
- Status changed from new to closed
Fixed 1.3-stable: d08ea7da
Does it need such drastic code change, this would be simpler and i think should work just as well.
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.
This would be valid because '{!}' will match both '{!' and '!}' when parsing.