Bug 199738

Summary: A slight reduction in konsole default line spacing
Product: [Applications] konsole Reporter: John Stanley <jpsinthemix>
Component: generalAssignee: Konsole Developer <konsole-devel>
Status: RESOLVED DUPLICATE    
Severity: wishlist CC: robertknight
Priority: NOR    
Version: unspecified   
Target Milestone: ---   
Platform: Compiled Sources   
OS: Linux   
Latest Commit: Version Fixed In:
Sentry Crash Report:
Attachments: screenshot illustrating konsole linespacing

Description John Stanley 2009-07-11 02:02:58 UTC
Version:            (using KDE 4.2.4)
Compiler:          gcc-4.4.0 linux-2.6.30.1 glibc-2.10.1
OS:                Linux
Installed from:    Compiled From Sources

This may simply be a matter of taste, of concern to no one but me, but I find the default konsole line spacing to be a bit to large in login bash shells and in vim. To reduce the spacing a bit, I use the following patch (any chance of incorporating this into konsole?):

 --- kdebase-4.2.96.old/apps/konsole/src/TerminalDisplay.cpp     2009-05-14 13:26:16.000000000 -0400
+++ kdebase-4.2.96.new/apps/konsole/src/TerminalDisplay.cpp     2009-07-09 20:28:04.682453580 -0400
@@ -209,7 +209,7 @@
 void TerminalDisplay::fontChange(const QFont&)
 {
   QFontMetrics fm(font());
-  _fontHeight = fm.height() + _lineSpacing;
+  _fontHeight = fm.lineSpacing() + _lineSpacing;

   // waba TerminalDisplay 1.123:
   // "Base character width on widest ASCII character. This prevents too wide
Comment 1 Kurt Hindenburg 2009-07-11 06:23:51 UTC
A dup; but this one has a patch.  I'm not sure why the change for KDE4 w/ spaceing.

*** This bug has been marked as a duplicate of bug 161495 ***
Comment 2 Robert Knight 2009-07-11 13:15:59 UTC
Hi John,

Can you check what _fm.leading() returns with your font?  Looking at the documentation _fm.lineSpacing() returns _fm.height()+_fm.leading() so it must be negative in your case.

Either way, since _lineSpacing is 0, separating lines by QFontMetrics::lineSpacing() should give results more like other Qt programs rendering the same font.
Comment 3 John Stanley 2009-07-12 05:20:03 UTC
Created attachment 35242 [details]
screenshot illustrating konsole linespacing
Comment 4 John Stanley 2009-07-12 05:21:14 UTC
(In reply to comment #2)
> Hi John,
> 
> Can you check what _fm.leading() returns with your font?  Looking at the
> documentation _fm.lineSpacing() returns _fm.height()+_fm.leading() so it must
> be negative in your case.
> 
> Either way, since _lineSpacing is 0, separating lines by
> QFontMetrics::lineSpacing() should give results more like other Qt programs
> rendering the same font.
Comment 5 John Stanley 2009-07-12 05:25:35 UTC
(In reply to comment #2)
> Hi John,
> 
> Can you check what _fm.leading() returns with your font?  Looking at the
> documentation _fm.lineSpacing() returns _fm.height()+_fm.leading() so it must
> be negative in your case.
> 
> Either way, since _lineSpacing is 0, separating lines by
> QFontMetrics::lineSpacing() should give results more like other Qt programs
> rendering the same font.

Hi Robert,
Yup, fm.leading() is -2 for the Monospace and DejaVu Sans Mono fonts. This is what I get (kde-4.2.96):

font().toString(): DejaVu Sans Mono,7,-1,5,50,0,0,0,0,0
fm.leading():     -2
fm.height():       13
_lineSpacing:      0
fm.lineSpacing():  11

I also attached a .png screenshot: the upper right shell is with the patch I initially sent, the lower right shell is with it, and the left shell shows the fm.leading/etc. values for the upper right shell.

An unrelated note: Notice the message in the left shell:
   "Undecodable sequence: \001b(hex)[?1034h

This is the first I've seen of this one... Any thought on this?

(In reply to comment #2)
> Hi John,
> 
> Can you check what _fm.leading() returns with your font?  Looking at the
> documentation _fm.lineSpacing() returns _fm.height()+_fm.leading() so it must
> be negative in your case.
> 
> Either way, since _lineSpacing is 0, separating lines by
> QFontMetrics::lineSpacing() should give results more like other Qt programs
> rendering the same font.
Comment 6 Robert Knight 2009-07-12 17:12:14 UTC
> This is the first I've seen of this one... Any thought on this?

This means that the terminal application sent some command to Konsole which it does not support.  I'm not sure what (Esc)[?1034 does.

The suggested patch looks sensible - although some additional refactoring of the code is needed first:

1.  If the variable _fontHeight no longer contains fontMetrics.height() but the actual spacing between lines then it needs to be renamed accordingly.

2.  The _lineSpacing variable and setLineSpacing()/lineSpacing() need to either be renamed (to indicate they are extra spacing added to or removed from the default spacing) or removed (because they are not used right now I believe)

3.  We need to check that the variable which is now called _fontHeight is actually the distance between lines of text and that nothing else is added to or removed from it before rendering.