View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0001725 | Double Commander | Viewer | public | 2017-02-07 08:06 | 2021-10-29 23:21 |
| Reporter | halmai | Assigned To | Alexx2000 | ||
| Priority | normal | Severity | major | Reproducibility | always |
| Status | closed | Resolution | fixed | ||
| Projection | none | ETA | none | ||
| Platform | MacOSX | ||||
| Product Version | 1.0.0 (trunk) | Product Build | 7369 | ||
| Target Version | 0.9.0 | Fixed in Version | 0.9.0 | ||
| Summary | 0001725: Selecting a text in viewer by mouse has problems with X coordinate | ||||
| Description | I open a txt file with 0000241:0000300 characters in a line. - I select the very first characters of the line. It works fine. - I select characters from 20th position. It selects not from not the character under the mouse but from the next one - I select characters from the 40th position. It selects not from the character under the mouse cursor but from 2 characters after it. - I select characters from the 100th position. It selects from 104th character. It seems that the conversion from the horizontal mouse position (measured in pixels) to character position is wrong. The attached screenshot shows the following: - I started to select the text by pressing the mouse right before the first character of the row - I kept the mouse button pressed while I moved right - During moving right, the difference between the right edge of the blue selection area and the mouse position was continuously growing and it was proportional to the X position. - When I move back to left, the gap is decreasing again. (The same bug was present with the ~6 months old version as well, I was just waiting for the new version before I reported it.) | ||||
| Tags | No tags attached. | ||||
| Attached Files | viewer-selection.patch (2,393 bytes)
Index: viewercontrol.pas
===================================================================
--- viewercontrol.pas (revision 8450)
+++ viewercontrol.pas (working copy)
@@ -333,7 +333,7 @@
}
function XYPos2Adr(x, y: Integer; out CharSide: TCharSide): PtrInt;
- procedure OutText(x, y: Integer; sText: String; StartPos: PtrInt; DataLength: Integer);
+ procedure OutText(x, y: Integer; const sText: String; StartPos: PtrInt; DataLength: Integer);
procedure OutBin(x, y: Integer; sText: string; StartPos: PtrInt; DataLength: Integer);
procedure OutCustom(x, y: Integer; sText: string;StartPos: PtrInt; DataLength: Integer); // render one line
@@ -1901,11 +1901,9 @@
end;
end;
-procedure TViewerControl.OutText(x, y: Integer; sText: String;
+procedure TViewerControl.OutText(x, y: Integer; const sText: String;
StartPos: PtrInt; DataLength: Integer);
var
- sBefore: String;
- iBefore: Integer = 0;
pBegLine, pEndLine: PtrInt;
iBegDrawIndex, iEndDrawIndex: PtrInt;
begin
@@ -1933,25 +1931,11 @@
else
iEndDrawIndex := pEndLine;
- // Text before selection
- if iBegDrawIndex - pBegLine > 0 then
- begin
- sBefore := GetText(StartPos, iBegDrawIndex - pBegLine, 0);
- iBefore := Canvas.TextWidth(sBefore);
- end;
-
// Text before selection + selected text
- sText := GetText(StartPos, iEndDrawIndex - pBegLine, 0);
-
Canvas.Brush.Color := clHighlight;
Canvas.Font.Color := clHighlightText;
- // Cannot simply draw text with brush with TextOut
- // because it differs between widgetsets.
- Canvas.Brush.Style := bsSolid;
- Canvas.FillRect(Bounds(X + iBefore, Y, Canvas.TextWidth(sText) - iBefore, FTextHeight));
- Canvas.Brush.Style := bsClear;
- Canvas.TextOut(X, Y, sText);
+ Canvas.TextOut(X, Y, GetText(StartPos, iEndDrawIndex - pBegLine, 0));
// Restore previous canvas settings
Canvas.Brush.Color := Color;
@@ -1958,13 +1942,8 @@
Canvas.Font.Color := Font.Color;
// Text before selection
- if iBefore > 0 then
- begin
- Canvas.Brush.Style := bsSolid;
- Canvas.FillRect(Bounds(X, Y, iBefore, FTextHeight));
- Canvas.Brush.Style := bsClear;
- Canvas.TextOut(X, Y, sBefore);
- end;
+ if iBegDrawIndex - pBegLine > 0 then
+ Canvas.TextOut(X, Y, GetText(StartPos, iBegDrawIndex - pBegLine, 0));
end;
procedure TViewerControl.OutCustom(x, y: Integer; sText: string;
viewer-selection-with-workaround.patch (1,714 bytes)
Index: components/viewer/viewercontrol.pas
===================================================================
--- components/viewer/viewercontrol.pas (revision 8456)
+++ components/viewer/viewercontrol.pas (working copy)
@@ -1905,7 +1905,9 @@
StartPos: PtrInt; DataLength: Integer);
var
sBefore: String;
+{$IF DEFINED(DARWIN) and DEFINED(LCLQT)}
iBefore: Integer = 0;
+{$ENDIF}
pBegLine, pEndLine: PtrInt;
iBegDrawIndex, iEndDrawIndex: PtrInt;
begin
@@ -1937,7 +1939,9 @@
if iBegDrawIndex - pBegLine > 0 then
begin
sBefore := GetText(StartPos, iBegDrawIndex - pBegLine, 0);
+{$IF DEFINED(DARWIN) and DEFINED(LCLQT)}
iBefore := Canvas.TextWidth(sBefore);
+{$ENDIF}
end;
// Text before selection + selected text
@@ -1946,11 +1950,10 @@
Canvas.Brush.Color := clHighlight;
Canvas.Font.Color := clHighlightText;
- // Cannot simply draw text with brush with TextOut
- // because it differs between widgetsets.
- Canvas.Brush.Style := bsSolid;
+{$IF DEFINED(DARWIN) and DEFINED(LCLQT)}
+ // Workaround for the unpainted background bug on macOS with Qt widgetset
Canvas.FillRect(Bounds(X + iBefore, Y, Canvas.TextWidth(sText) - iBefore, FTextHeight));
- Canvas.Brush.Style := bsClear;
+{$ENDIF}
Canvas.TextOut(X, Y, sText);
// Restore previous canvas settings
@@ -1958,11 +1961,11 @@
Canvas.Font.Color := Font.Color;
// Text before selection
- if iBefore > 0 then
+ if iBegDrawIndex - pBegLine > 0 then
begin
- Canvas.Brush.Style := bsSolid;
+{$IF DEFINED(DARWIN) and DEFINED(LCLQT)}
Canvas.FillRect(Bounds(X, Y, iBefore, FTextHeight));
- Canvas.Brush.Style := bsClear;
+{$ENDIF}
Canvas.TextOut(X, Y, sBefore);
end;
end;
| ||||
| Fixed in Revision | 8444, 8497, 8500-8501, 8531 | ||||
| Operating system | MacOSX | ||||
| Widgetset | |||||
| Architecture | 64-bit | ||||
|
|
I don't know where the 0000241:0000300 came in my comment, I wrote a tilde (wave) character and 300 right after it (which should be read as "about 300"). It was converted automatically. :) |
|
|
This is still a very annoying bug. Could you check if you can reproduce it, please? |
|
|
Please report your font and try with another one. |
|
|
I checked the Configuration => Options => Fonts => Viewer Font which shows Courier and font size = 13. Then I changed it to Osaka, Regular-Mono, 14 and then to PT Mono, Regular, 9 and then to some others. I experienced the problem in each case. Sometimes the selection goes behind the mouse (on the left from the mouse) and sometimes on the other side. |
|
|
I have had the issue for a long time now. Simply put, long lines have problems. When you select some portion of text that is far from the start of the line the actual selection starts at a different place (for me before) then the actual mouse cursor is. The further you go (drag the selection) the bigger the gap between your mouse cursor and the text selection is. I can reproduce it any time and on any Ubuntu based machine (14.04-17.10) It has also something to do with the vertical scroller. When you scroller position is not at the beginning the actual selection starts way before the cursor. Another observation is that it does not really differ if you change the font. But if you change the font size (I changed it to Monospace 15) it actually works properly. If I change it back to size 12 it stops working again. So it is almost as if the font width is hard coded and not retrieved from the current font and size. That should help to troubleshoot it I can help in any way Thank you |
|
|
jklos, do you have Qt or GTK version of Double Commander? Could you test the other one? |
|
|
Ahh, exactly. I use Qt (which suffers from this issue), with GTK2 it works properly. I used to run the GTK2 version in the past but then was forced to use the Qt for a reason I don't remember any more :) Thank you |
|
|
@author To solve: use ATBinHex in viewer. It has most problems fixed long ago. It is OK on Linux too. and QT. |
|
|
I checked it and see that it's text painting is much slower under Linux. |
|
|
I checked it on the newest version but the problem is still the same. The version is: Double Commander Version: 0.9.0 alpha Revision: 8189 Build date: 2018/06/30 Lazarus: 1.8.4.0-58419 FPC: 3.0.4 Platform: i386-Darwin-carbon OS version: Mac OS X 10.11.6 |
|
|
Checked in QT version too, unfortunately the result is the same: Double Commander Version: 0.9.0 alpha Revision: 8189 Build date: 2018/06/30 Lazarus: 1.8.4.0-58419 FPC: 3.0.4 Platform: x86_64-Darwin-qt4 OS version: Mac OS X 10.11.6 Very annoying... :( Widgetset library: Qt 4.8.6, libQt4Pas 4.5.3 |
|
|
Can we hope solving this issue in the near future, please? |
|
|
I can reproduce this on Linux with both Qt4 and Qt5 interfaces |
|
|
I've commited a fix for Text mode (not Bin or Hex), rev.8444. Checked on Qt on Linux, please report whether it's fixed on Mac, especially with Carbon interface. Bin and Hex are still broken, so not closing as resolved/fixed for now. The problem is in rounding, length of multiple chars is not equal to the sum of lengths of separate chars. |
|
|
It should help, I tested same code change some time ago. But under macOS there is a second problem: drawing normal/selected text. Intersymbol distance is random and depends on drawing start X point. So when user selecting text, the symbols are "dancing". |
|
|
> But under macOS there is a second problem: drawing normal/selected text. Intersymbol distance is random and depends on drawing start X point. So when user selecting text, the symbols are "dancing". I've seen "dancing" of the unselected part that goes after the selection on Linux/Qt. Not sure if this is the same thing you're talking about. The solution for this would be to not draw by chunks (unselected, selected, unselected again), rather display the full unselected string and then draw the selection over it. As an optimization it could be only for the last line of the selection, the only line that can have text after the selection. Also the selection algorithm doesn't seem to take into account the possibility of combining characters on (after) the last selected symbol. It stops on the first character whose right position is bigger than mouse X pos, instead it should stop on the first character whose left position is bigger than mouse X pos and not include it. But if the viewer doesn't support combining characters at all, that is not an issue. Relevant discussion: https://www.reddit.com/r/programming/comments/3kitlm/when_monospace_fonts_arent_the_unicode_character/ The support of combining characters in monospace may depend on the font. |
|
|
>>>The solution for this would be to not draw by chunks (unselected, selected, unselected again) Yes >>>rather display the full unselected string and then draw the selection over it I tried, this does not help, intersymbol distance in selection is different (as I remember). I think it is needed to display the full unselected string, then draw full string as selected in invisible bitmap and copy rect of real selection to canvas. I hope this will help. |
|
|
> I think it is needed to display the full unselected string, then draw full string as selected in invisible bitmap and copy rect of real selection to canvas. Another option that should work: draw full unselected, then selected from the line start to the selection end, then unselected from the line start to the selection start. |
|
|
>>draw full unselected, then selected from the line start to the selection end, then unselected from the line start to the selection start. I checked it, at first glance it works. I will test some more and commit if all good. |
|
|
I committed above algorithm implementation (for text mode only). Checked it under macOS and Windows, both works fine. I updated macOS snapshots. I also added native Cocoa version, Lazarus 2.0 positioning Cocoa support as a beta. |
|
|
After the changes in the selection algorithm, the bug on XP with Fixedsys font https://doublecmd.sourceforge.io/forum/viewtopic.php?t=3838 (reported by Grishanenko) became more annoying, as the text has started painting right, but the background of the selection has not (as it still depends on TextWidth) and that leads to white text on white background for the end of the selection on the rows with the 'я' character. I've removed the separate selection background painting in the attached viewer-selection.patch. It doesn't fix the issue entirely, but mitigates it to a good extent, as the selection starts being drawn absolutely correctly, just the mouse X coordinate is now wrong on the rows with this character. That's much better than the previously "eaten" letters. And the mouse shift becomes really noticeable only after the fifth 'я' occurrence in the row. But I'm not committing it right away because there was a reason for the background to be drawn separately. The comment says "Cannot simply draw text with brush with TextOut because it differs between widgetsets". It was implemented by cobines in 2009, so there's a chance that it's not needed anymore since the bugs might have been fixed long ago. I have tested the patch on Windows with Lazarus 2.1.0 r59527 and on Linux with Lazarus 2.0.0RC1 Qt4 and GTK2 backends and found no issues or differences. Alexx2000, could you please test the patch on Mac with different widgetsets so I can safely commit? If there are any problems with a particular widgetset, I would remake the patch to leave the workaround for that widgetset only. |
|
|
Under macOS it works with Carbon and Cocoa, but does not work with Qt4 which is used now as 64 bit version, TextOut does not draw background in this case. |
|
|
Thank you for taking your time testing this. That's a pity that the workaround has to be left, the original patch drastically simplified the code. I've found the bug by cobines at https://bugs.freepascal.org/view.php?id=21863 It's fixed long ago, I wonder whether DC still builds with that Lazarus version at all. So I haven't included the workaround and defines for that bug, only for the Qt on Mac. Please test whether the new patch fixes that build. Also, would you report the bug to Lazarus? There's a chance they'll fix it before 2.0 release. |
|
|
I've re-tested Win32, Qt4 and GTK2 with the stable Lazarus 1.8.4, all OK. I'm having troubles building Lazarus with Qt5 widgetset, so that should be tested also. |
|
|
Works fine under macOS now. Under Linux with Qt5 works too. https://bugs.freepascal.org/view.php?id=34587 |
|
|
Fixed in Lazarus fixes_2_0 branch. So we can use previous patch without workaround for Qt4. |
|
|
Fantastic, it works, thank you. :) |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2017-02-07 08:06 | halmai | New Issue | |
| 2017-02-07 08:06 | halmai | File Added: screenshot.png | |
| 2017-02-07 08:07 | halmai | Note Added: 0002079 | |
| 2017-02-20 07:31 | halmai | Note Edited: 0002079 | |
| 2017-07-02 06:07 | halmai | Note Added: 0002289 | |
| 2017-07-05 20:00 | cordylus | Note Added: 0002293 | |
| 2017-08-17 10:43 | halmai | Note Added: 0002329 | |
| 2017-12-27 14:53 | jklos | Note Added: 0002478 | |
| 2017-12-27 18:15 | cordylus | Note Added: 0002479 | |
| 2017-12-29 16:58 | jklos | Note Added: 0002482 | |
| 2017-12-29 17:02 | jklos | Note Edited: 0002482 | |
| 2018-01-08 14:24 | Alextp | Note Added: 0002499 | |
| 2018-01-08 14:25 | Alextp | Note Edited: 0002499 | |
| 2018-01-08 15:32 | Alexx2000 | Note Added: 0002500 | |
| 2018-07-06 09:20 | halmai | Note Added: 0002666 | |
| 2018-07-06 09:22 | halmai | Note Added: 0002667 | |
| 2018-11-14 03:53 | halmai | Note Added: 0002921 | |
| 2018-11-15 23:18 | cordylus | Note Added: 0002929 | |
| 2018-11-16 16:12 | cordylus | Fixed in Revision | => 8444 |
| 2018-11-16 16:24 | cordylus | Note Added: 0002930 | |
| 2018-11-16 21:07 | Alexx2000 | Note Added: 0002931 | |
| 2018-11-16 22:06 | cordylus | Note Added: 0002932 | |
| 2018-11-16 22:10 | cordylus | Note Edited: 0002932 | |
| 2018-11-16 22:30 | Alexx2000 | Note Added: 0002933 | |
| 2018-11-16 22:34 | Alexx2000 | Note Edited: 0002933 | |
| 2018-11-16 23:03 | cordylus | Note Added: 0002934 | |
| 2018-11-16 23:38 | Alexx2000 | Note Added: 0002935 | |
| 2018-11-17 00:27 | Alexx2000 | Note Edited: 0002935 | |
| 2018-11-17 11:47 | Alexx2000 | Note Added: 0002936 | |
| 2018-11-17 12:34 | Alexx2000 | Note Edited: 0002936 | |
| 2018-11-19 01:07 | cordylus | File Added: viewer-selection.patch | |
| 2018-11-19 02:12 | cordylus | Note Added: 0002937 | |
| 2018-11-19 02:20 | cordylus | Note Edited: 0002937 | |
| 2018-11-20 22:51 | Alexx2000 | Note Added: 0002938 | |
| 2018-11-21 03:26 | cordylus | File Added: viewer-selection-with-workaround.patch | |
| 2018-11-21 03:37 | cordylus | Note Added: 0002939 | |
| 2018-11-21 03:38 | cordylus | Note Edited: 0002939 | |
| 2018-11-21 03:49 | cordylus | Note Edited: 0002939 | |
| 2018-11-21 23:51 | cordylus | Note Added: 0002940 | |
| 2018-11-22 19:30 | Alexx2000 | Note Added: 0002941 | |
| 2018-11-22 20:12 | Alexx2000 | Note Edited: 0002941 | |
| 2018-12-08 19:00 | Alexx2000 | Note Added: 0002967 | |
| 2018-12-08 19:27 | Alexx2000 | Fixed in Revision | 8444 => 8444, 8497 |
| 2018-12-08 19:27 | Alexx2000 | Widgetset | QT4 => |
| 2018-12-09 01:18 | Alexx2000 | Fixed in Revision | 8444, 8497 => 8444, 8497, 8500-8501 |
| 2018-12-10 09:02 | Alexx2000 | Status | new => confirmed |
| 2018-12-10 09:02 | Alexx2000 | Target Version | => 0.9.0 |
| 2018-12-22 00:20 | Alexx2000 | Assigned To | => Alexx2000 |
| 2018-12-22 00:20 | Alexx2000 | Status | confirmed => assigned |
| 2018-12-22 00:20 | Alexx2000 | Fixed in Revision | 8444, 8497, 8500-8501 => 8444, 8497, 8500-8501, 8531 |
| 2018-12-22 00:25 | Alexx2000 | Status | assigned => resolved |
| 2018-12-22 00:25 | Alexx2000 | Fixed in Version | => 0.9.0 |
| 2018-12-22 00:25 | Alexx2000 | Resolution | open => fixed |
| 2019-01-15 03:32 | halmai | Note Added: 0003003 | |
| 2021-10-29 23:21 | Alexx2000 | Status | resolved => closed |
