Bug 362171 - Patch review requested for implementation SGR 2/8/9/53 (dim/conceal/strikeout/overline)
Summary: Patch review requested for implementation SGR 2/8/9/53 (dim/conceal/strikeout...
Status: RESOLVED FIXED
Alias: None
Product: konsole
Classification: Applications
Component: emulation (show other bugs)
Version: master
Platform: Other Linux
: NOR wishlist
Target Milestone: ---
Assignee: Konsole Developer
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-04-24 01:01 UTC by Antonio Russo
Modified: 2016-07-16 17:45 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
SGR Implementation (10.00 KB, patch)
2016-04-24 01:06 UTC, Antonio Russo
Details
Revised complete patchset (9.49 KB, application/octet-stream)
2016-07-07 16:08 UTC, Antonio Russo
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Antonio Russo 2016-04-24 01:01:28 UTC
Escape codes SGR 2, SGR 8, SGR 9, and SGR 53 are not implemented in Konsole. All except SGR 53 are implemented in xterm.

Reproducible: Always

Steps to Reproduce:
echo -e 'D\e[2mD\e[9mD\e[53mD\e[8mD'

Actual Results:  
5 D symbols, all identical

Expected Results:  
4 D symbols, the last three with "dim," "faint" or "half" intensity, the last two with "strikeout" font, and the last one overlined. The final, fifth D should not be visible.

I posted an earlier, less-complete patch set to the development mailing list. I don't know if that was the correct venue---or if this one is either.
Comment 1 Antonio Russo 2016-04-24 01:06:49 UTC
Created attachment 98546 [details]
SGR Implementation
Comment 2 Kurt Hindenburg 2016-05-21 16:52:38 UTC
What is this attachment?  gz or bz2?
Comment 3 Antonio Russo 2016-05-21 17:59:07 UTC
The attachment should be a tar.gz with 7 patches. On my debian machine it extracts with tar -xf .

I've been using a version of konsole with these patches applied for about a month now, and haven't hit any bugs.
Comment 4 Kurt Hindenburg 2016-05-23 21:06:47 UTC
Even w/ all the patches, I only get 4 D - does it work for you w/ these patches?  What color scheme to you use?
Comment 5 Antonio Russo 2016-05-23 23:06:02 UTC
The patch depends on a color style to implement the "faint" intensities for each color. I did this specifically for Linux.colorscheme (and it
should is included in the patch) but did not get around to the other themes. My eye is not particularly suited for aesthetic choices, and the
choice of faint colors for the themes is such a choice (in particular the solarized and light themes seemed to require some thought). Assuming
this bug gets enough attention to be included, but not enough for other people with better aesthetics than me to work on the themes, I'll hack
up some faint colors.

I'll point out that the patch also includes in a fully functional (but completely untested) gui for setting the faint color theme---right next
to setting the other colors and intense colors.

Notwithstanding that, the overline and strikeout font effects should function correctly. Are you seeing those with the patch applied? If not,
could you confirm that your terminal font supports overline and strikeout effects?
Comment 6 Antonio Russo 2016-07-07 16:08:45 UTC
Created attachment 99927 [details]
Revised complete patchset

tar.gz of 6 patches
Comment 7 Antonio Russo 2016-07-07 16:12:11 UTC
The revised patchset has support for all default color schemes. There should be no issues using these color schemes.
Comment 8 Martin Sandsmark 2016-07-07 19:56:06 UTC
Hi!

https://git.reviewboard.kde.org/dashboard/ is the preferred method of submitting patches for Konsole.
Comment 9 Martin Sandsmark 2016-07-07 20:04:46 UTC
The patches works for me, I get the correct strikethrough/overline, and the last one is invisible. The first one is with the normal color, the three remaining are in the faint color defined in the color scheme.

However it seems like neither urxvt nor uxterm here handles it correctly.
Comment 10 Antonio Russo 2016-07-07 21:32:43 UTC
I am glad that the patch works for others.

Sorry if this is turning into spam. I made a review request

https://git.reviewboard.kde.org/r/128390/

but since I have a series of patches that depend on
the previous one, I was not sure how to upload the
sequence without squashing all the commits. What
should I now do? Wait for someone to review my draft
proposal?

Thanks,
Antonio
Comment 11 Martin Sandsmark 2016-07-07 23:17:37 UTC
Reviewboard isn't very good with series of patches, though you can mark patches as dependent on others I haven't really figured out the details.

FWIW I don't seem to have access to that review request, but I guess hindenburg has?
Comment 12 Antonio Russo 2016-07-07 23:39:04 UTC
I really appreciate your getting back to me. Thank you.

I cannot "publish" the patch without it having a
"review"---and I don't think I am allowed to review it
myself (?). The first step in the patch creation
process required uploading a diff---which for
subsequent patches is rejected because the parent
revision is not yet known to reviewboard (or, it was
not properly detected). I think.

Hindenburg glanced at this bug a couple months ago.
Could you possibly contact him, or should I maybe ping
him directly?
Comment 13 Martin Sandsmark 2016-07-08 02:31:37 UTC
He is very active reviewing patches, he'll probably get to this very soon.
Comment 14 Kurt Hindenburg 2016-07-16 16:36:46 UTC
Git commit 84b43dfb2108eab47fa1dfcafbf1b94a410d6cbf by Kurt Hindenburg.
Committed on 16/07/2016 at 16:34.
Pushed by hindenburg into branch 'master'.

Add rendition flags SGRs 2, 8, 9, 53

Adds faint intensity, strikeout, conceal and overline support.

echo -e 'D\e[2mD\e[9mD\e[53mD\e[8mD'

Thanks to Antonio Russo antonio e russo gmail com for patch

REVIEW: 128405

M  +30   -0    data/color-schemes/BlackOnLightYellow.colorscheme
M  +30   -0    data/color-schemes/BlackOnRandomLight.colorscheme
M  +30   -0    data/color-schemes/BlackOnWhite.colorscheme
M  +30   -0    data/color-schemes/BlueOnBlack.colorscheme
M  +30   -0    data/color-schemes/Breeze.colorscheme
M  +30   -0    data/color-schemes/DarkPastels.colorscheme
M  +30   -0    data/color-schemes/GreenOnBlack.colorscheme
M  +30   -0    data/color-schemes/Linux.colorscheme
M  +30   -0    data/color-schemes/RedOnBlack.colorscheme
M  +30   -0    data/color-schemes/Solarized.colorscheme
M  +30   -0    data/color-schemes/SolarizedLight.colorscheme
M  +30   -0    data/color-schemes/WhiteOnBlack.colorscheme
M  +16   -11   src/Character.h
M  +22   -4    src/CharacterColor.h
M  +40   -3    src/ColorScheme.cpp
M  +19   -5    src/ColorSchemeEditor.cpp
M  +1    -1    src/History.h
M  +25   -33   src/Screen.cpp
M  +6    -5    src/Screen.h
M  +1    -1    src/TerminalCharacterDecoder.h
M  +13   -3    src/TerminalDisplay.cpp
M  +10   -1    src/Vt102Emulation.cpp
M  +13   -1    src/autotests/CharacterColorTest.cpp

http://commits.kde.org/konsole/84b43dfb2108eab47fa1dfcafbf1b94a410d6cbf
Comment 15 Kurt Hindenburg 2016-07-16 17:45:10 UTC
Git commit c8da8f3b4745c9bd7ef24890a15457342d2ed860 by Kurt Hindenburg.
Committed on 16/07/2016 at 17:40.
Pushed by hindenburg into branch 'Applications/16.08'.

Add rendition flags SGRs 2, 8, 9, 53

Adds faint intensity, strikeout, conceal and overline support.

echo -e 'D\e[2mD\e[9mD\e[53mD\e[8mD'

Thanks to Antonio Russo antonio e russo gmail com for patch

REVIEW: 128405
(cherry picked from commit 84b43dfb2108eab47fa1dfcafbf1b94a410d6cbf)

M  +30   -0    data/color-schemes/BlackOnLightYellow.colorscheme
M  +30   -0    data/color-schemes/BlackOnRandomLight.colorscheme
M  +30   -0    data/color-schemes/BlackOnWhite.colorscheme
M  +30   -0    data/color-schemes/BlueOnBlack.colorscheme
M  +30   -0    data/color-schemes/Breeze.colorscheme
M  +30   -0    data/color-schemes/DarkPastels.colorscheme
M  +30   -0    data/color-schemes/GreenOnBlack.colorscheme
M  +30   -0    data/color-schemes/Linux.colorscheme
M  +30   -0    data/color-schemes/RedOnBlack.colorscheme
M  +30   -0    data/color-schemes/Solarized.colorscheme
M  +30   -0    data/color-schemes/SolarizedLight.colorscheme
M  +30   -0    data/color-schemes/WhiteOnBlack.colorscheme
M  +16   -11   src/Character.h
M  +22   -4    src/CharacterColor.h
M  +40   -3    src/ColorScheme.cpp
M  +19   -5    src/ColorSchemeEditor.cpp
M  +1    -1    src/History.h
M  +25   -33   src/Screen.cpp
M  +6    -5    src/Screen.h
M  +1    -1    src/TerminalCharacterDecoder.h
M  +13   -3    src/TerminalDisplay.cpp
M  +10   -1    src/Vt102Emulation.cpp
M  +13   -1    src/autotests/CharacterColorTest.cpp

http://commits.kde.org/konsole/c8da8f3b4745c9bd7ef24890a15457342d2ed860