View Issue Details

IDProjectCategoryView StatusLast Update
0034306LazarusLCLpublic2018-12-27 21:20
ReportertintinuxAssigned ToJesus Reyes 
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
PlatformanyOSWindowsOS Versionany
Product Version1.8.4Product Build 
Target Version2.0Fixed in Version2.1 (SVN) 
Summary0034306: Double validation in TStringGrid
DescriptionThe event handler OnValidateEntry is called twice when you select another cell with the mouse.

When validation fails, users are annoyed with 2 identical messages.


Steps To ReproduceUnzip the small attached project.

In the first editable cell (1,1) enter any letter, and clic the next cell.

You will get twice the exception with message "must be numeric !"
Additional InformationThis doesn't occur when using keyboard to move another cell.
TagsStringGrid
Fixed in Revisionr59207, r59691
LazTarget2.0
WidgetsetWin32/Win64
Attached Files
  • stringgrid.zip (2,095 bytes)
  • TStringGrid_double_validation.patch (115 bytes)
    6521c6521
    <         //// MoveExtend(False, P.x, P.y, False);
    ---
    >         MoveExtend(False, P.x, P.y, False);
    
  • grids.pas.patch (436 bytes)
    Index: grids.pas
    ===================================================================
    --- grids.pas	(revision 59167)
    +++ grids.pas	(working copy)
    @@ -6702,7 +6702,6 @@
             P:=MouseToLogcell(Point(X,Y));
             if gfNeedsSelectActive in GridFlags then
               SelectActive := (P.x<>FPivot.x)or(P.y<>FPivot.y);
    -        MoveExtend(False, P.x, P.y, False);
           end;
         gsColMoving:
           if goColMoving in Options then
    
    grids.pas.patch (436 bytes)
  • patch.diff (448 bytes)
    Index: lcl/grids.pas
    ===================================================================
    --- lcl/grids.pas	(revision 59167)
    +++ lcl/grids.pas	(working copy)
    @@ -6702,7 +6702,6 @@
             P:=MouseToLogcell(Point(X,Y));
             if gfNeedsSelectActive in GridFlags then
               SelectActive := (P.x<>FPivot.x)or(P.y<>FPivot.y);
    -        MoveExtend(False, P.x, P.y, False);
           end;
         gsColMoving:
           if goColMoving in Options then
    
    patch.diff (448 bytes)
  • grids.pas_2.diff (1,452 bytes)
    Index: lcl/grids.pas
    ===================================================================
    --- lcl/grids.pas	(revision 59246)
    +++ lcl/grids.pas	(working copy)
    @@ -6625,9 +6625,6 @@
     
             end
             else if not FixedGrid then begin
    -          // normal selecting
    -          fGridState:=gsSelecting;
    -
               if not EditingAllowed(FCol) or
                 (ExtendedSelect and not EditorAlwaysShown) then begin
     
    @@ -6663,8 +6660,12 @@
     
               include(fGridFlags, gfEditingDone);
               try
    -            if not MoveExtend(False, FGCache.ClickCell.X, FGCache.ClickCell.Y, False) then begin
    -              if EditorAlwaysShown then begin
    +            if MoveExtend(False, FGCache.ClickCell.X, FGCache.ClickCell.Y, False) then
    +              fGridState:=gsSelecting
    +            else
    +            begin
    +              if EditorAlwaysShown then
    +              begin
                     SelectEditor;
                     EditorShow(true);
                   end;
    @@ -6672,9 +6673,7 @@
                 end;
               finally
                 exclude(fGridFlags, gfEditingDone);
    -            fGridState:=gsSelecting;
               end;
    -
             end;
           end;
       end;
    @@ -6702,6 +6701,7 @@
             P:=MouseToLogcell(Point(X,Y));
             if gfNeedsSelectActive in GridFlags then
               SelectActive := (P.x<>FPivot.x)or(P.y<>FPivot.y);
    +        MoveExtend(False, P.x, P.y, False);
           end;
         gsColMoving:
           if goColMoving in Options then
    
    grids.pas_2.diff (1,452 bytes)

Activities

tintinux

2018-09-20 10:25

reporter  

stringgrid.zip (2,095 bytes)

tintinux

2018-09-20 15:00

reporter  

TStringGrid_double_validation.patch (115 bytes)
6521c6521
<         //// MoveExtend(False, P.x, P.y, False);
---
>         MoveExtend(False, P.x, P.y, False);

tintinux

2018-09-20 15:02

reporter   ~0110897

Last edited: 2018-09-20 15:04

View 3 revisions

I can't see why the event OnValidateEntry is called during MouseMove. This is because of a call to MoveExtend which calls EditorGetValue, calling itself ValidateEntry.

Since I don't see any reason for the call to MoveExtend by MouseMove (some comments would be useful), I have simply removed it. With the options I use, the issue is solved, and I don't find side effects up to now.

But I may be wrong and opinions of authors of the component would be useful.

You can apply the attached patch to grids.pas

tintinux

2018-09-20 15:37

reporter   ~0110898

Same issue on 2.0 RC1

Juha Manninen

2018-09-24 14:57

developer   ~0110994

The patch is invalid. How did you create it?

 $ patch -p0 < ~/patch/TStringGrid_double_validation.patch
 patch: **** Only garbage was found in the patch input.

tintinux

2018-09-25 17:19

reporter   ~0111008

I created it using WinMerge 2.14.0 (last stable version), which is the only tool I have on my Windows workstation.

If needed, I will try to create it on my Linux machine at home, tonight.

However, we can read the patch file : only line 6521 is deleted.

Juha Manninen

2018-09-25 19:58

developer   ~0111013

Last edited: 2018-09-25 20:03

View 3 revisions

> I created it using WinMerge 2.14.0 (last stable version), which is the only tool I have on my Windows workstation.

Then you should report a bug for WinMerge project / product.
This "patch" does not even mention the source file that should be changed. Thus it is quite impossible to apply. Total crap, sorry.

> However, we can read the patch file : only line 6521 is deleted.
Line 6521 of what?
I wonder why we need to have this conversation while there has been a well established diff format for decades.

Patches should ALWAYS be made against Lazarus trunk. A released version will not be patched.
You must install TortoiseSVN, get trunk and then create patches for it.

Juha Manninen

2018-09-25 20:52

developer   ~0111014

Now I can see you mentioned it is for grids.pas.
There is no MoveExtend code near line 6521. There is one such MoveExtend at line 6705.
Uhhh... just create a proper patch against trunk please!

I can reproduce the bug itself. When using keyboard Enter you only get one error message but with mouse you get 2.

tintinux

2018-09-26 10:47

reporter  

grids.pas.patch (436 bytes)
Index: grids.pas
===================================================================
--- grids.pas	(revision 59167)
+++ grids.pas	(working copy)
@@ -6702,7 +6702,6 @@
         P:=MouseToLogcell(Point(X,Y));
         if gfNeedsSelectActive in GridFlags then
           SelectActive := (P.x<>FPivot.x)or(P.y<>FPivot.y);
-        MoveExtend(False, P.x, P.y, False);
       end;
     gsColMoving:
       if goColMoving in Options then
grids.pas.patch (436 bytes)

tintinux

2018-09-26 11:00

reporter  

patch.diff (448 bytes)
Index: lcl/grids.pas
===================================================================
--- lcl/grids.pas	(revision 59167)
+++ lcl/grids.pas	(working copy)
@@ -6702,7 +6702,6 @@
         P:=MouseToLogcell(Point(X,Y));
         if gfNeedsSelectActive in GridFlags then
           SelectActive := (P.x<>FPivot.x)or(P.y<>FPivot.y);
-        MoveExtend(False, P.x, P.y, False);
       end;
     gsColMoving:
       if goColMoving in Options then
patch.diff (448 bytes)

tintinux

2018-09-26 11:08

reporter   ~0111026

Hi

I have uploaded patch.diff and hope it will be valid.
Please remove the other *.patch attachments.

Again, I am not sure it contains the right correction. Someone involved in grids.pas development must check.

Regards

PS : could you remove the old out of date trunk sources hosted on sourceforge, and change the wiki pages linked to it. This is confusing and do not help us to help.

Regards

Juha Manninen

2018-09-26 16:44

developer   ~0111034

Which wiki page links to outdated sources? Wiki can be edited by anybody but I can also fix it if you don't want to.

tintinux

2018-10-01 08:57

reporter   ~0111144

The lazarus wiki was updated, I find no more links.
In my opinion, it would be better to remove the sourceforge repository for those who browse sourceforge other pages and may ignore it has been moved.

Juha Manninen

2018-10-01 10:45

developer   ~0111150

Last edited: 2018-10-01 12:05

View 3 revisions

Thanks for the patch with a valid format. It fixes the problem in my tests.
I was hoping a Grid-knowledgeable person would test and apply it. Now I applied it. It must be tested in trunk for possible regressions. Can be merged for 2.0 later if all works well.

I wrote to Lazarus dev list about the outdated Sourceforge repository. It is a valid note, something should be done to the sources.

Jesus Reyes

2018-10-02 09:56

developer   ~0111182

Juha I don't think this patch is good, for a start, I think it will affect grids with editing=false (where you don't have validation). I'm don't saying the bug does not exists (in fact I have not tried to reproduce it yet) but if it exists it surely exists since validation was implemented.

The removed line should notify that selection has changed but the action is still ongoing, maybe it's not a very used feature but anyway in general lets try to not remove features without a discussion first. And the problem I see here is that there is not an acknowledge that a feature may be affected. I guess first a solution that doesn't affect features should be explored and if there is no other remedy then at least lets make a notice that a feature will be affected.

Juha Manninen

2018-10-02 10:13

developer   ~0111184

Assigning to you. I don't know how to fix it properly.

tintinux

2018-10-04 16:08

reporter  

grids.pas_2.diff (1,452 bytes)
Index: lcl/grids.pas
===================================================================
--- lcl/grids.pas	(revision 59246)
+++ lcl/grids.pas	(working copy)
@@ -6625,9 +6625,6 @@
 
         end
         else if not FixedGrid then begin
-          // normal selecting
-          fGridState:=gsSelecting;
-
           if not EditingAllowed(FCol) or
             (ExtendedSelect and not EditorAlwaysShown) then begin
 
@@ -6663,8 +6660,12 @@
 
           include(fGridFlags, gfEditingDone);
           try
-            if not MoveExtend(False, FGCache.ClickCell.X, FGCache.ClickCell.Y, False) then begin
-              if EditorAlwaysShown then begin
+            if MoveExtend(False, FGCache.ClickCell.X, FGCache.ClickCell.Y, False) then
+              fGridState:=gsSelecting
+            else
+            begin
+              if EditorAlwaysShown then
+              begin
                 SelectEditor;
                 EditorShow(true);
               end;
@@ -6672,9 +6673,7 @@
             end;
           finally
             exclude(fGridFlags, gfEditingDone);
-            fGridState:=gsSelecting;
           end;
-
         end;
       end;
   end;
@@ -6702,6 +6701,7 @@
         P:=MouseToLogcell(Point(X,Y));
         if gfNeedsSelectActive in GridFlags then
           SelectActive := (P.x<>FPivot.x)or(P.y<>FPivot.y);
+        MoveExtend(False, P.x, P.y, False);
       end;
     gsColMoving:
       if goColMoving in Options then
grids.pas_2.diff (1,452 bytes)

tintinux

2018-10-04 16:15

reporter   ~0111243

I have found another way to fix the issue, probably avoiding side effects mentioned by Jesus. Instead of removing MoveExtend it doesn't assign gsSelecting to fGridState when the validation fails.

The patch grids.pas_2.diff can be applied to the last version Juha has commited after my previous patch.

Again, it must be tested much more and the advice of anyone knowing well grids.pas would be appreciated.

Jesus Reyes

2018-11-29 02:49

developer   ~0112252

Thanks, I tried to fix the problem by my own and arrived to almost the same patch, please test.

tintinux

2018-12-26 15:31

reporter   ~0112890

For me, the issue is solved on Lazarus trunk (Not in 2.0-RC3).
Thanks !

Jesus Reyes

2018-12-27 21:20

developer   ~0112920

Is now queued

Issue History

Date Modified Username Field Change
2018-09-20 10:25 tintinux New Issue
2018-09-20 10:25 tintinux File Added: stringgrid.zip
2018-09-20 10:26 tintinux Tag Attached: StringGrid
2018-09-20 15:00 tintinux File Added: TStringGrid_double_validation.patch
2018-09-20 15:02 tintinux Note Added: 0110897
2018-09-20 15:03 tintinux Note Edited: 0110897 View Revisions
2018-09-20 15:04 tintinux Note Edited: 0110897 View Revisions
2018-09-20 15:37 tintinux Note Added: 0110898
2018-09-24 14:57 Juha Manninen Note Added: 0110994
2018-09-24 14:57 Juha Manninen LazTarget => -
2018-09-24 14:57 Juha Manninen Status new => feedback
2018-09-25 17:19 tintinux Note Added: 0111008
2018-09-25 17:19 tintinux Status feedback => new
2018-09-25 19:58 Juha Manninen Note Added: 0111013
2018-09-25 20:01 Juha Manninen Note Edited: 0111013 View Revisions
2018-09-25 20:03 Juha Manninen Note Edited: 0111013 View Revisions
2018-09-25 20:52 Juha Manninen Note Added: 0111014
2018-09-26 10:47 tintinux File Added: grids.pas.patch
2018-09-26 11:00 tintinux File Added: patch.diff
2018-09-26 11:08 tintinux Note Added: 0111026
2018-09-26 16:44 Juha Manninen Note Added: 0111034
2018-10-01 08:57 tintinux Note Added: 0111144
2018-10-01 10:41 Juha Manninen Assigned To => Juha Manninen
2018-10-01 10:41 Juha Manninen Status new => assigned
2018-10-01 10:45 Juha Manninen Fixed in Revision => r59207
2018-10-01 10:45 Juha Manninen Note Added: 0111150
2018-10-01 10:45 Juha Manninen Status assigned => resolved
2018-10-01 10:45 Juha Manninen Resolution open => fixed
2018-10-01 10:46 Juha Manninen Note Edited: 0111150 View Revisions
2018-10-01 12:05 Juha Manninen Note Edited: 0111150 View Revisions
2018-10-02 09:56 Jesus Reyes Note Added: 0111182
2018-10-02 10:11 Juha Manninen Assigned To Juha Manninen => Jesus Reyes
2018-10-02 10:11 Juha Manninen Status resolved => assigned
2018-10-02 10:13 Juha Manninen Note Added: 0111184
2018-10-04 16:08 tintinux File Added: grids.pas_2.diff
2018-10-04 16:15 tintinux Note Added: 0111243
2018-11-29 02:49 Jesus Reyes Fixed in Revision r59207 => r59207, r59691
2018-11-29 02:49 Jesus Reyes LazTarget - => 2.0
2018-11-29 02:49 Jesus Reyes Note Added: 0112252
2018-11-29 02:49 Jesus Reyes Status assigned => resolved
2018-11-29 02:49 Jesus Reyes Fixed in Version => 2.1 (SVN)
2018-11-29 02:49 Jesus Reyes Target Version => 2.0
2018-12-26 15:31 tintinux Note Added: 0112890
2018-12-27 21:20 Jesus Reyes Note Added: 0112920