Konsole - backport of some patches from KDE #184

I backported some KDE patches for Konsole in the hope that some of them might be related to bug 3028. But from my testing, it doesn't seem to be getting the desired result.

Hi Slavek, I will test at some point. Long ago I looked at some of those patches on bug 3028 and I remember some of the math there seemed wrong to me. I would suggest we investigate a bit further before merging, especially if you don't see improvomentns with the patches applied.

I was picking patches that I hoped could be related to the problem – b267f6c60a and 429311a2ce and also those that seemed useful and reasonable.

Nothing of value here by any chance? https://src.fedoraproject.org/rpms/kdebase3/tree/rawhide

Nothing of value here by any chance? https://src.fedoraproject.org/rpms/kdebase3/tree/rawhide

Interesting, worth taking a look, maybe not only specifically for this bug 👍

I am going to look at this so we can merge before R14.0.11.

Commit 45860f8b6a seems to be only a partial fix. See attached file, left is old behavior, right is new behavior with mentioned commit. BOLD should be orange and not yellow.

commit_45860f8b6a.png

14 KiB

PR #233 is a replacement fix for commit 45860f8b6a

PR #233 is a replacement fix for commit 45860f8b6a

Ignore this, it was wrong. Original commit 45860f8b6a behaves correctly.

I did further research and testing nad I believe commit 45860f8b6a should not be merged. It causes "high intensity" and "high intensity bold" text to become indistinguishible.
To test use:

echo -e "\e[0;31m Normal text \e[0m" && echo -e "\e[1;31m BOLD text \e[0m" && echo -e "\e[0;91m High intensity text \e[0m" && echo -e "\e[1;91m High intensity BOLD text\e[0m" && echo -e "\e[0;91m Low \e[1;91mintensity\e[0;91m text \e[0m" && echo -e "\e[1;91m High \e[0;91mintensity\e[1;91m text \e[0m"

See attached screeshot. Left console with code from current master. Right console with commit 45860f8b6a. Note how in the last 2 lines, the word "intensity" is indistinguishible from the rest of the line.

What do you think? IMO that commit should not be merged.

commit_45860f8b6a-2.png

54 KiB

Yes, I agree, an awkward color, is the problem on the side of the script author, and it seems better when bold has some reaction than when it is ignored.

Yes, I agree, an awkward color, is the problem on the side of the script author, and it seems better when bold has some reaction than when it is ignored.

In that case we can discard commit 45860f8b6a. Will look into the others and feedback.

I can't reproduce the problem described in commit f2272ab117, I see same behavior with and without the fix. I guess we can still merge it since it seems it once caused issues in KDE 4.x Konsole. Whether that was caused by Konsole code or older ncurses version, I don't know.

I tested also in Trusty and again I see no difference with/without commit f2272ab117. I used the test program from KDE bug report.
Given that even Trusty seems unaffected, I wonder whether the problem in KDE 4.x was caused by some other issues in that code base. Shall we still backport this commit? I am not quite sure it is needed at this point and rather than merging something for the sake of it, I would skip it.
What do you think?

I am not able to reproduce the problem mentioned by commit b267f6c60a, but given it is an additional escape sequence it seems to make sense to add the commit even if I can't reproduce the problem.

I can reproduce the problem described in commit 1ad768b613 and the mentioned commit fixes it. With the commit, "clear" and "CSI 2J" sequence behaves in the same way.

I tested also in Trusty and again I see no difference with/without commit f2272ab117. I used the test program from KDE bug report.
Given that even Trusty seems unaffected, I wonder whether the problem in KDE 4.x was caused by some other issues in that code base. Shall we still backport this commit? I am not quite sure it is needed at this point and rather than merging something for the sake of it, I would skip it.
What do you think?

You did a test on Konsole with width <= 80? According to the bug report, this manifests itself only in these cases.

Commit 429311a2ce does fix the problem mentioned in the KDE bug (scroll up too many lines being ignored) and should be merged.
Nevertheless the behavior of scroll up is inconsistent (regardless of this commit) because it does remove lines from the scroll back. This should be a separate issue probably.

In summary:

commit 45860f8b6a: to be ignored
commit f2272ab117: do we need it? Probably not.
commit b267f6c60a: to be merged, although I can't reproduce the issue.
commit 1ad768b613: ok
commit 429311a2ce: ok

@SlavekB: if you agree with the above, please update the PR and feel free to merge it.

I am not able to reproduce the problem mentioned by commit b267f6c60a, but given it is an additional escape sequence it seems to make sense to add the commit even if I can't reproduce the problem.

Yes, it also seems to me as a good idea to add it when it adds support for additional escape sequence.

You did a test on Konsole with width <= 80? According to the bug report, this manifests itself only in these cases.

yes, less than 80, more than 80 and exactly 80

Looks good!

Konsole - backport of some patches from KDE #184

Reviewers