All Projects FPC fpcprojects fpprofilerfpGUI Lazarus Packages PatchesLazarus CCR Mantis pas2js

View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0034461PatchesLCLpublic2018-10-26 11:452020-03-11 15:57
ReporterBBaz Assigned ToZeljan Rikalo  
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
Summary0034461: TreeItem text is centered instead of left justified
DescriptionThe text of a tree item is drawn in the center of its draw rectangle. Most of the time the draw rectangle is accurately set so the text appears to be left justified but this is not the case. Under certain circumstances this is visible (Qt4 widget set + treeview inside a anchordocking form) because the draw rectangle is bigger than it should.

See pictures showing the problem here https://forum.lazarus.freepascal.org/index.php/topic,42999.msg300279.html#msg300279
Additional InformationPlease include in next Lazarus 2.0 release candidate
TagsNo tags attached.
Fixed in Revision59647
LazTarget-
WidgetsetGTK 2, Win32/Win64, QT, QT5
Attached Files

Relationships

related to 0018285 closedPaul Ishenin Lazarus Treeview do not draw text in middle by default 
related to 0031037 resolvedOndrej Pokorny Lazarus Mac: fixed dpi 72 should be changed to Windows-value 96 
related to 0034625 closedZeljan Rikalo Lazarus Qt/Qt5: Wrong PPI calculation on Mac after r57679 
related to 0034627 assignedZeljan Rikalo Lazarus Wrong treeview tooltip hint position after 0034461 / r34461 

Activities

BBaz

2018-10-26 11:45

reporter  

fix_treeitem_text_centered.patch (1,080 bytes)    
From 0f945726ee5243ed194b313e1ed5531d8f637056 Mon Sep 17 00:00:00 2001
From: Basile Burg <basile.b@gmx.com>
Date: Fri, 26 Oct 2018 11:36:08 +0200
Subject: [PATCH] fix tree item text justification, it was centered

---
 lcl/include/treeview.inc | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lcl/include/treeview.inc b/lcl/include/treeview.inc
index 2802785d..055c27e6 100644
--- a/lcl/include/treeview.inc
+++ b/lcl/include/treeview.inc
@@ -5238,9 +5238,9 @@ var
     end;
 
     if (tvoThemedDraw in Options) then
-      ThemeServices.DrawText(Canvas, Details, AText, NodeRect, DT_CENTER or DT_VCENTER or DT_SINGLELINE or DT_NOPREFIX, 0)
+      ThemeServices.DrawText(Canvas, Details, AText, NodeRect, DT_LEFT or DT_VCENTER or DT_SINGLELINE or DT_NOPREFIX, 0)
     else
-      DrawText(Canvas.Handle, PChar(AText), -1, NodeRect, DT_CENTER or DT_VCENTER or DT_SINGLELINE or DT_NOPREFIX);
+      DrawText(Canvas.Handle, PChar(AText), -1, NodeRect, DT_LEFT or DT_VCENTER or DT_SINGLELINE or DT_NOPREFIX);
 
     if NeedUnderline then
     begin
-- 
2.17.2
fix_treeitem_text_centered.patch (1,080 bytes)   

BBaz

2018-10-26 11:47

reporter   ~0111566

Sorry my description mentioned Qt4 but it's well just Qt5 that was affected.

BBaz

2018-10-26 12:04

reporter   ~0111569

Sorry first patch put items too much to the left. Second patch add a DPI-aware space.

BBaz

2018-10-26 12:05

reporter  

fix_treeitem_text_centered2.patch (1,169 bytes)    
From c9518a8cfae9f22cb35616b1dc1109c457172d85 Mon Sep 17 00:00:00 2001
From: Basile Burg <basile.b@gmx.com>
Date: Fri, 26 Oct 2018 11:36:08 +0200
Subject: [PATCH] fix tree item text justification, it was centered

---
 lcl/include/treeview.inc | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lcl/include/treeview.inc b/lcl/include/treeview.inc
index 2802785d..7122d391 100644
--- a/lcl/include/treeview.inc
+++ b/lcl/include/treeview.inc
@@ -5237,10 +5237,11 @@ var
         Canvas.Font.Color := FHotTrackColor;
     end;
 
+    NodeRect.Offset(ScaleX(2, 96), 0);
     if (tvoThemedDraw in Options) then
-      ThemeServices.DrawText(Canvas, Details, AText, NodeRect, DT_CENTER or DT_VCENTER or DT_SINGLELINE or DT_NOPREFIX, 0)
+      ThemeServices.DrawText(Canvas, Details, AText, NodeRect, DT_LEFT or DT_VCENTER or DT_SINGLELINE or DT_NOPREFIX, 0)
     else
-      DrawText(Canvas.Handle, PChar(AText), -1, NodeRect, DT_CENTER or DT_VCENTER or DT_SINGLELINE or DT_NOPREFIX);
+      DrawText(Canvas.Handle, PChar(AText), -1, NodeRect, DT_LEFT or DT_VCENTER or DT_SINGLELINE or DT_NOPREFIX);
 
     if NeedUnderline then
     begin
-- 
2.17.2
fix_treeitem_text_centered2.patch (1,169 bytes)   

Zeljan Rikalo

2018-10-26 13:32

developer   ~0111573

Maybe DT_CENTER is there because of icon ? There must be reason why only Qt/Qt5 shows this problem / and / or why eg Gtk draws DT_LEFT while DT_CENTER is added into flags.

BBaz

2018-10-26 13:44

reporter   ~0111574

> Maybe DT_CENTER is there because of icon ?

I don't see the relationship between the icon and the text justification


> There must be reason why only Qt/Qt5 shows this problem / and / or why eg Gtk draws DT_LEFT while DT_CENTER is added into flags.

About gtk i think it's correct because the rectangle to draw has exactly the right width.

About Qt5, yes thee's an unknown here and that's caused by the reparenting by AnchorDocking.


Note that anyway i think that DT_LEFT will be more performant than DT_CENTER.

Zeljan Rikalo

2018-10-26 14:09

developer   ~0111577

IMO, DT_LEFT is more logical to me but must find out why DT_CENTER is there.

Zeljan Rikalo

2018-10-26 14:24

developer   ~0111578

Ok, DT_CENTER is added in r24816 when ThemeServices text drawing is introduced, so maybe, it's just typo.
https://svn.freepascal.org/cgi-bin/viewvc.cgi/trunk/lcl/include/treeview.inc?root=lazarus&r1=24815&r2=24816

Martin Friebe

2018-10-26 14:58

manager   ~0111579

I think the answer lies in revision 24816:

>>>> OLD
        CurTextRect.Right := x + TextWidth(Node.Text) + 1;
        Font.Color := InvertColor(Brush.Color);
        FillRect(CurTextRect);
        TextOut(x + 1, TextY, Node.Text);
<<<<<
The text and background color, where just drawn into the rect.

>>>>> NEW
      CurTextRect.Right := x + TextWidth(Node.Text) + Indent div 2;
      DrawNodeText(NodeSelected, CurTextRect, Node.Text);
<<<<<
The rect is extended by half of indent (indent is the amount of pixels a childnode is moved to the right from its parent)

Together with dt_center, this means, that the background extends by a quarter of indent on either side.

It may make sense to have a bit of extra padding on both sides. Not sure why/if that should rely on "indent"?

BBaz

2018-10-26 17:35

reporter  

0018285_still_ok.png (10,238 bytes)   
0018285_still_ok.png (10,238 bytes)   

BBaz

2018-10-26 17:36

reporter   ~0111581

https://bugs.freepascal.org/view.php?id=18285 is not broken by the patch. Text is drawn after image, whatever is its size

Martin Friebe

2018-10-26 17:44

manager   ~0111582

Last edited: 2018-10-26 17:45

View 2 revisions

 0018285 is about DT_VCENTER. (vertical)
This is about DT_CENTER (horizontal)

I agree DT_CENTER is a bad choice. But removing it, means to restore the horizontal padding by other means.
The image shows, that after the patch, there is more blue padding on the right, than on the left.
 
And maybe at the same time (not really part of this fix), making them independent of "Indent". And making them DPI aware (or bound to font size?)

Zeljan Rikalo

2018-11-18 18:24

developer  

issue34461.diff (1,195 bytes)    
Index: lcl/include/treeview.inc
===================================================================
--- lcl/include/treeview.inc	(revision 59594)
+++ lcl/include/treeview.inc	(working copy)
@@ -5237,10 +5237,11 @@
         Canvas.Font.Color := FHotTrackColor;
     end;
 
+    NodeRect.Offset(ScaleX(2, 96), 0);
     if (tvoThemedDraw in Options) then
-      ThemeServices.DrawText(Canvas, Details, AText, NodeRect, DT_CENTER or DT_VCENTER or DT_SINGLELINE or DT_NOPREFIX, 0)
+      ThemeServices.DrawText(Canvas, Details, AText, NodeRect, DT_LEFT or DT_VCENTER or DT_SINGLELINE or DT_NOPREFIX, 0)
     else
-      DrawText(Canvas.Handle, PChar(AText), -1, NodeRect, DT_CENTER or DT_VCENTER or DT_SINGLELINE or DT_NOPREFIX);
+      DrawText(Canvas.Handle, PChar(AText), -1, NodeRect, DT_LEFT or DT_VCENTER or DT_SINGLELINE or DT_NOPREFIX);
 
     if NeedUnderline then
     begin
@@ -5363,7 +5364,7 @@
     begin
       CurTextRect := NodeRect;
       CurTextRect.Left := x;
-      CurTextRect.Right := x + TextWidth(Node.Text) + RealIndent div 2;
+      CurTextRect.Right := x + TextWidth(Node.Text) + (FDefItemSpace * 2);
       DrawNodeText(NodeSelected, CurTextRect, Node.Text);
     end;
issue34461.diff (1,195 bytes)   

Zeljan Rikalo

2018-11-18 18:25

developer   ~0112049

Please test with my patch. It does not use Indent but internal FDefItemSpace which is applied for x coordinate. Text should be centered in this case.

Zeljan Rikalo

2018-11-24 16:50

developer   ~0112172

Please test and close if ok. This is candidate to be merged into 2.0.RC3 after testing.

Zeljan Rikalo

2018-11-30 18:54

developer   ~0112283

Last edited: 2018-11-30 19:13

View 3 revisions

 It does not look good under qt/qt5 on macOSX. Maybe because of virtual machine, not sure, but maybe because all DARWIN (carbon,cocoa, qt, qt5) target sets default PPI to 96, not 72 so our basic calculation is wrong.
EDIT: I can confirm that problem on MacOS comes from hardcoded PixelsPerInch values for all supported targets (carbon,cocoa,qt and qt5). If it's default from ws (72) then drawing of treeview items is same on all widgetsets.
EDIT 2: Hardcoded values are in all mentioned widgetsets inside Widgetset.InitApp function.

Ondrej Pokorny

2018-12-01 12:00

developer   ~0112288

Just a note: the bug can be observed on win32 as well. Some treeview items are shifted right by 1px. I assume it is the result of DT_CENTER and "CurTextRect.Right := x + TextWidth(Node.Text) + RealIndent div 2;"

Zeljan Rikalo

2018-12-01 14:08

developer   ~0112291

I've found root of problem in Qt/Qt5. It's longstanding bug in Qt with wrong result of QFontMetrics() on Mac - read it as QFontMetrics_width('some text') is wrong when it calculates via QFont_pixelSize() under Mac. I'll fix it in Qt LCL.
Thanks for investigation, but I've already fixed that, treviewitem text drawing does not use DT_CENTER anymore, but DT_LEFT.

Ondrej Pokorny

2018-12-01 15:31

developer   ~0112296

> Thanks for investigation, but I've already fixed that, treviewitem text drawing does not use DT_CENTER anymore, but DT_LEFT.

Yes, I know. I should have better used past tense: "the bug COULD be observed".

Btw. why do you use "NodeRect.Offset(ScaleX(2, 96), 0);" in your patch - a scaled value of 2 instead of FDefItemSpace:

OffsetRect(NodeRect, FDefItemSpace, 0);

Zeljan Rikalo

2018-12-01 17:59

developer   ~0112305

NodeRect.Offset(ScaleX(2, 96), 0); is just copy/pasted from reporter's patch, and I forgot to fix it. Thanks for mentioning that.

Issue History

Date Modified Username Field Change
2018-10-26 11:45 BBaz New Issue
2018-10-26 11:45 BBaz File Added: fix_treeitem_text_centered.patch
2018-10-26 11:47 BBaz Note Added: 0111566
2018-10-26 12:04 BBaz Note Added: 0111569
2018-10-26 12:05 BBaz File Added: fix_treeitem_text_centered2.patch
2018-10-26 13:30 Zeljan Rikalo Assigned To => Zeljan Rikalo
2018-10-26 13:30 Zeljan Rikalo Status new => assigned
2018-10-26 13:32 Zeljan Rikalo Note Added: 0111573
2018-10-26 13:44 BBaz Note Added: 0111574
2018-10-26 14:08 Zeljan Rikalo Relationship added related to 0018285
2018-10-26 14:09 Zeljan Rikalo Note Added: 0111577
2018-10-26 14:24 Zeljan Rikalo Note Added: 0111578
2018-10-26 14:58 Martin Friebe Note Added: 0111579
2018-10-26 17:35 BBaz File Added: 0018285_still_ok.png
2018-10-26 17:36 BBaz Note Added: 0111581
2018-10-26 17:44 Martin Friebe Note Added: 0111582
2018-10-26 17:45 Martin Friebe Note Edited: 0111582 View Revisions
2018-11-18 18:24 Zeljan Rikalo File Added: issue34461.diff
2018-11-18 18:25 Zeljan Rikalo LazTarget => -
2018-11-18 18:25 Zeljan Rikalo Note Added: 0112049
2018-11-18 18:25 Zeljan Rikalo Status assigned => feedback
2018-11-24 16:50 Zeljan Rikalo Fixed in Revision => 59647
2018-11-24 16:50 Zeljan Rikalo Widgetset QT => GTK 2, Win32/Win64, QT, QT5
2018-11-24 16:50 Zeljan Rikalo Note Added: 0112172
2018-11-24 16:50 Zeljan Rikalo Status feedback => resolved
2018-11-24 16:50 Zeljan Rikalo Resolution open => fixed
2018-11-30 18:54 Zeljan Rikalo Note Added: 0112283
2018-11-30 19:05 Zeljan Rikalo Note Edited: 0112283 View Revisions
2018-11-30 19:13 Zeljan Rikalo Note Edited: 0112283 View Revisions
2018-11-30 19:18 Zeljan Rikalo Relationship added related to 0031037
2018-12-01 09:56 Zeljan Rikalo Relationship added related to 0034625
2018-12-01 12:00 Ondrej Pokorny Note Added: 0112288
2018-12-01 14:08 Zeljan Rikalo Note Added: 0112291
2018-12-01 15:31 Ondrej Pokorny Note Added: 0112296
2018-12-01 17:52 Zeljan Rikalo Relationship added related to 0034627
2018-12-01 17:59 Zeljan Rikalo Note Added: 0112305
2020-03-11 15:57 BBaz Status resolved => closed