Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Columns EndUpdate and duplicate position #1228

Closed
Dzyszla opened this issue Nov 16, 2023 · 14 comments
Closed

Columns EndUpdate and duplicate position #1228

Dzyszla opened this issue Nov 16, 2023 · 14 comments
Assignees
Milestone

Comments

@Dzyszla
Copy link

Dzyszla commented Nov 16, 2023

If I have 3 columns, last invisible, but set for two latest same position during columns are under updating, freezing on EndUpdate:

	with VirtualStringTree1.Header.Columns do
	begin
		BeginUpdate;
		Items[0].Position := 0;
		Items[1].Position := 1;
		Items[2].Options := Items[2].Options - [coVisible];
		Items[2].Position := 1;
		EndUpdate;
	end;

bug is in the VirtualTrees.Header file in TotalWidth method, especially on:

    if not (coVisible in Items[LastColumn].Options) then
      LastColumn := GetPreviousVisibleColumn(LastColumn);

But have same position, so still return same column.

Checked on the last version of VST.
Sample project attached.
Projects.zip

@joachimmarder
Copy link
Contributor

Please respect our guidelines on the project homepage for submitting bugs. Please include your version of Virtual TreeView and Delphi, and attach a sample compiling project as ZIP to your report, or instructions how to modify one of the included sample projects to reproduce the problem. If only small changes are required, a description is sufficient how a demo projects needs to be changed in order to replicate the bug. If you already have a solution, please supply a patch file.

@Dzyszla
Copy link
Author

Dzyszla commented Nov 16, 2023

Please respect our guidelines on the project homepage for submitting bugs. Please include your version of Virtual TreeView and Delphi, and attach a sample compiling project as ZIP to your report, or instructions how to modify one of the included sample projects to reproduce the problem. If only small changes are required, a description is sufficient how a demo projects needs to be changed in order to replicate the bug. If you already have a solution, please supply a patch file.

Done (issue has been edited). I think this is very simple case. Only VST with 3 columns and my code - that's all.

@joachimmarder
Copy link
Contributor

joachimmarder commented Nov 16, 2023

@Dzyszla : Please check if the recent commit fixes the problem for you.

@Dzyszla
Copy link
Author

Dzyszla commented Nov 17, 2023

Problem was fixed, but... Now no change column positions :( Try change (manually, run-time) column c2 to position 0 (first) and press button - no error, but nothing change (c2 will back to second place).

joachimmarder added a commit that referenced this issue Nov 18, 2023
@joachimmarder
Copy link
Contributor

You are right. Could you give it another try?

By the way, it will always work if you don't use BeginUpdate/EndUpdate on the columns.

@Dzyszla
Copy link
Author

Dzyszla commented Nov 20, 2023

In first repeat-until loop iteration for positions [0, 1, 1] second column got position 0. Then in second iteration sets to [-1 (without sign), 0, 1] so... Access Violation :)

I think it works this way: write all position into array, sort it by values (positions) and rewrite from index to position.

Maybe something like this:

procedure TVirtualTreeColumns.FixPositions;
// Fixes column positions after loading from DFM or Bidi mode change.
var
  lPositions: System.Generics.Collections.TList<TVirtualTreeColumn>;
  I : Integer;
begin
  lPositions := TList<TVirtualTreeColumn>.Create;
  try
	lPositions.Capacity := Count;
	  for i := 0 to Count - 1 do
		  lPositions.Add(Items[i]);
	lPositions.Sort(
		System.Generics.Defaults.TComparer<TVirtualTreeColumn>.Construct(
			function(const aLeft, aRight: TVirtualTreeColumn): Integer
			begin
				Result := aLeft.Position - aRight.Position;
				if Result = 0 then
					Result := aLeft.Index - aRight.Index; // for not randomize order with same position
			end));
	for i := 0 to Count - 1 do
	begin
		lPositions[i].FPosition := i;
		FPositionToIndex[i] := lPositions[i].Index; // (?)
	end;
  finally
	  lPositions.Free;
  end;

  FNeedPositionsFix := False;
  UpdatePositions(True);
end;

I have one more ask - it's possible, when official fix will be available, to apply it to the 7.5.x sable version as fix the bug? Or 8.0 will be soon?

@joachimmarder
Copy link
Contributor

Or 8.0 will be soon?

Yes. Actually, I wanted to release V8 this week. This is the last remaining issue for V8.0.
See: https:/JAM-Software/Virtual-TreeView/milestone/15

joachimmarder added a commit that referenced this issue Nov 29, 2023
@joachimmarder
Copy link
Contributor

You should use BeginUpdate() / EndUpdate() only in case you are setting the entire columns with consistent data. Otherwise just omit it. I will check back to this issue in V8.1.

@joachimmarder joachimmarder modified the milestones: V8.0, V8.1 Nov 29, 2023
@Dzyszla
Copy link
Author

Dzyszla commented Nov 30, 2023

Everywhere using this two commands are used for block unnecessary operations during changing several items of list, so this will works in all case ;) I keep waiting for fix.

@tecmatia-dp
Copy link

tecmatia-dp commented Jan 17, 2024

I was expieriencing a lot of problems with version 8.0 in my project, with a tree with 45 columns that the user can move, hide, save multiple views. In some cases, resulting in exceptions, columns that are not displayed, different widths in header and content, etc.
I found that problem is repeated Position, including design-time.
I tried to fix positions in my own code, outside the library, but no success.
Finding this open issue, I have seen the way, and fixed method TVirtualTreeColumns.FixPositions. Based on @Dzyszla proposal, this is my final code, that is working fine at runtime:

procedure TVirtualTreeColumns.FixPositions;
// Fixes column positions after loading from DFM or Bidi mode change.
var
  LColumnsByPos: TList<TVirtualTreeColumn>;
  I: Integer;
begin
  LColumnsByPos := TList<TVirtualTreeColumn>.Create;
  try
    LColumnsByPos.Capacity := Self.Count;
    for I := 0 to Self.Count-1 do
      LColumnsByPos.Add(Items[I]);

    LColumnsByPos.Sort(
      TComparer<TVirtualTreeColumn>.Construct(
        function(const A, B: TVirtualTreeColumn): Integer
        begin
          Result := CompareValue(A.Position, B.Position);
          if Result = 0 then
            Result := CompareValue(A.Index, B.Index);
        end)
    );

    for I := 0 to LColumnsByPos.Count-1 do
    begin
      LColumnsByPos[I].FPosition := I;
      Self.FPositionToIndex[I] := LColumnsByPos[I].Index;
    end;

  finally
	  LColumnsByPos.Free;
  end;

  FNeedPositionsFix := False;
  UpdatePositions(True);
end;

It's the same idea, but using CompareValue in Sort (easy to read), and fixing the assignment to the array FPositionToIndex.
Thank you @Dzyszla

@Dzyszla
Copy link
Author

Dzyszla commented Jan 17, 2024

And thank's You too, @tecmatia-dp
I see, that line with question mark in my code is a little bit different. Maybe that's why my code wasn't working properly always.
Now waiting for official release with fix :)

@tecmatia-dp
Copy link

Oops, @Dzyszla, checking that line again, I think the correct one is yours, mine always assign Position=I to Index=I...
Correct is FPositionToIndex[I] := LColumnsByPos[I].Index
But, why wasn't it working properly always for you?
Now, I think it is working for me, apparently.

Edited my previous code, to fix that line, and change var names to be more easy to read and understand the algorithm (yours).

@joachimmarder
Copy link
Contributor

Thanks for the code, which I just published as part of official release V8.0.1

@Dzyszla
Copy link
Author

Dzyszla commented Jan 22, 2024

@tecmatia-dp

But, why wasn't it working properly always for you? Now, I think it is working for me, apparently.

I don't know :( Had some problem with correct changing position... I don't remember now, what exactly case. I try again now after update to 8.0.1, but hope now is all fine :)

salvadorbs pushed a commit to salvadorbs/VirtualTreeView-Lazarus that referenced this issue Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants