Recent

Author Topic: SOLVED! Virtual listview; validity of "Item" in "Edited" when Selection changed  (Read 5911 times)

d7_2_laz

  • Hero Member
  • *****
  • Posts: 506
Windows (10 x64), virtual listview, nultiselect.   Lazarrus 2.2, but probably earlier too.
First side note: i found the observation a bit curios until i found a workaround. So i only would like to ask if it is normal. Wouldn't like to make an issue of it.
Second: imo it has absolutely nothing to do with the bunch of issues around virtual listview / multiselect as recently discussed. If i remember right it had already been with 2.0.12.
If appears more to be related to the order of message processing (set focus, kill foucs).

Scenario: the inplace editor is open for a selected iten. Now, by click, select another item.
"Item" within "Edited" now will point to the newly selected item, and not to the item just being edited.
"Item" appears to be hijacked (with strange results if one relies on it.

Test project attached.
My question is only for understanding: is it known and is it by design?

« Last Edit: April 26, 2022, 10:13:35 am by d7_2_laz »
Lazarus 3.2  FPC 3.2.2 Win10 64bit

wp

  • Hero Member
  • *****
  • Posts: 11830
I tested the issue with Delphi, and there the edited text always goes into the previously selected, edited item. I cannot imagine that this behaviour is intentional. You should report it as a bug (not sure if someone will pick it up, though).

d7_2_laz

  • Hero Member
  • *****
  • Posts: 506
Yes, it was not with Delphi (7 in my case).  With Laz. i noticed since a longer time sporadically, strange phenomena when editing and change selection, so i did a closer look now and found it at least worthy to communicate.

Happy enough that the really urgent thing(s) had been recently addressed i wouldn't like to annoy again with other issues near by (if desired i could to it of course).
A workaround does exist (using a remembered item index).
Lazarus 3.2  FPC 3.2.2 Win10 64bit

wp

  • Hero Member
  • *****
  • Posts: 11830
It's not annoying. But if you don't report it, the risk that a bug reported in a forum post will be forgotten is quite high.

d7_2_laz

  • Hero Member
  • *****
  • Posts: 506
Probably you are right wp, and it's better to register it for to make it known.
Btw it differs from the behaviour of a regular listview, as:
without any further code (as default action), when the editor is open and another item is clicked,
- the regular listview commits the changed contents,  and selects the clicked item. The "edited" Item still points to the previous one -- ok.
- the virtual listview discards the changed contents, and selects the clicked item (but the "edited" Item  points to the clicked one too  -- wrong).
That may appear to be very unspectular and to be only a matter of personal flavour (which is better..).
Until one starts (withiin "Edited") to do something meaningful with "Item" and has to notice that he's working with a wrong object ...

So:
https://gitlab.com/freepascal.org/lazarus/lazarus/-/issues/39708
Lazarus 3.2  FPC 3.2.2 Win10 64bit

wp

  • Hero Member
  • *****
  • Posts: 11830
It helps for me to end editing before the listview reacts on re-gaining focus again from the editor. For this, I added the lines
Code: Pascal  [Select][+][-]
  1.           // editing
  2.           if FEditor.Visible then
  3.             HideEditor;
to TCustomListview.CNNotify, case LVN_ITEMCHANGED, after "if (nm^.uChanged = LVIF_STATE)".

Code: Pascal  [Select][+][-]
  1. ...
  2.     LVN_ITEMCHANGED:
  3.     begin
  4.       if nm^.iItem < 0 then
  5.         Item := nil
  6.       else
  7.         Item := Items[nm^.iItem];
  8.       //DebugLn('TCustomListView.CNNotify Count=',dbgs(Items.Count),' nm^.iItem=',dbgs(nm^.iItem),' destroying=',dbgs(lifDestroying in Item.FFlags));
  9.       if (Item <> nil) and (not OwnerData) and (lifDestroying in Item.FFlags) then
  10.       begin
  11.         if Item=FFocused then
  12.           FFocused:=nil;
  13.         if Item=FSelected then
  14.           InvalidateSelected;
  15.       end else
  16.       begin
  17.         if (nm^.uChanged = LVIF_STATE) then
  18.         begin
  19.           // checkbox
  20.           if Checkboxes then
  21.             DoItemChecked(Item);
  22.          
  23.           // editing
  24.           if FEditor.Visible then
  25.             HideEditor;
  26.  
  27.           // focus
  28.           if (nm^.uOldState and LVIS_FOCUSED) <> (nm^.uNewState and LVIS_FOCUSED) then
  29.           begin
  30.             ...[/code[
« Last Edit: April 12, 2022, 12:28:23 pm by wp »

d7_2_laz

  • Hero Member
  • *****
  • Posts: 506
Not quite sure yet ..
Yes, the test project does behave correct now.

Tried within the app, i got an access violation. Whcih could be solved: there was also a reference to "Selected" within the Edited callback, and it pointed to Nil.  Could be bypassed, basically no problem.

Within the test project, Selected is _not_ nil (would need to dig deep why this difference, i'm using a custom inplace editor here).
"Selected" points to the currently edited = previous selected item.
Which is correct from the point of view of the "Edited" context.
But maybe not what we would expect from the listview's overall point of view.

I'd formulate the question as follows:
wichin the scenario described, what do we want where "Selected" should point to now, and is it guaranteed that it has a valid contents?
You might take a look applying the following line within the test project, Edited callback:
     Memo1.Lines.Add('Edited: selected Item caption now is   -->  ' + ListView1.Selected.Caption);


Lazarus 3.2  FPC 3.2.2 Win10 64bit

wp

  • Hero Member
  • *****
  • Posts: 11830
There's certainly some ambiguity...

I checked with Delphi: it fires the events in the same order as Lazarus: OnEditing - OnEdited - OnSelectItem(edited item, Selected=false) - OnSelectItem(new item, Selected=true)

The only difference is that Delphi has Item=nil in the OnEditing/OnEdited events - which definitely is a bug. In fact, even Lazarus had it like this, but I had fixed this very recently in commit 9aa8b52 (April 4). You probably do not have this because this would explain your crash.

d7_2_laz

  • Hero Member
  • *****
  • Posts: 506
Due to the latter issues i tested with 2.2 (containing your former prototype patch) as well as with "2.3.0" (as lazgit extract (lazarus-main.zip) from 4. Apr. plus your patches on top. Testing each time with having done full cleanup & build.

And now it's somehow strange. Whereas the test project does behave ok, i get problems to transfer to the application (using the builitin listview editor here, for this test).
Within the application, "item" is still the right one in "Editing". But in "Edited" i get again the next selected item, instead of the previous edited item.  I don't know why yet und need to dig, but it make take some time to find out what went wrong. Sorry for that delay!

Regarding the "Selected" within "Edited": if you don't see a major problem here, it's ok for me so far.
Lazarus 3.2  FPC 3.2.2 Win10 64bit

wp

  • Hero Member
  • *****
  • Posts: 11830
Comitted the code shown above so that we have a common code base.

d7_2_laz

  • Hero Member
  • *****
  • Posts: 506
wp thank you! For now as first quick check i verified that my (2.3.0)  customlistview.inc. Then i traced a bit up from the newly inserted HideEditor statement ... the next step executed would be UpdateMultiSelList. So, purely occasional, i got an idea:

Yesterday i did set the "MultiSelect"  in my demo project (attached in initial post) to "false".  Only for to assure that the original issue described really has nothing specific to do with MultiSelect.
Just for test now i did re-set to MultiSelect true again   -> test project does fail resp,. behaviour not fixed.  You tested against MultiSelect true too?
Lazarus 3.2  FPC 3.2.2 Win10 64bit

wp

  • Hero Member
  • *****
  • Posts: 11830
Ah...

Maybe the solution is to follow Delphi again which does not enter edit mode at all once multiple items are selected, and this is kind of logical because it is not very clear which one of the selected items will be edited. Or maybe even all?

This can be achieved by replacing TCustomListView.CanEdit by this code:
Code: Pascal  [Select][+][-]
  1. function TCustomListView.CanEdit(Item: TListItem): Boolean;
  2. begin
  3.   Result := not (OwnerData and MultiSelect and (SelCount > 1));
  4.   if Result and Assigned(FOnEditing) then
  5.     FOnEditing(Self, Item, Result);
  6. end;
This disables editing in virtual mode when more than 1 item is selected.

I am aware this is inconsistent since editing of multisel items is possible in normal mode. But I don't see a way how the same behaviour could be achieved for virtual mode... At least this work-around would be less destructive than the current situation.

I also tried to check how editing of secondary columns is affected, but it seems that they cannot be edited at all. Or am I missing some parameter?

d7_2_laz

  • Hero Member
  • *****
  • Posts: 506
Oh .... it was meant much more trivial. Not thought here about a real multi-renaming scenario.  But i noticed that when simply setting the MultiSelect property to true, the patch did not work for my demo project (as attached at beginning of this article).

_But_: now i had to notice that this is not true with your demo project That works. Yours behave correct, mine not. So i tried to narrow it down step by step where the crucial difference might be located.

After some tests i thought i found the answer. Setting,  additionally, "RowSelect" to true, made my demo project work ok too. The opposite does not apply: setting RowSelect to false demo in your project does not prevent it to work. Confusing.
I have no final clue now about the different behaviour of both projects regarding the patch. Unfortunately i need to do a break until later the day.
Lazarus 3.2  FPC 3.2.2 Win10 64bit

d7_2_laz

  • Hero Member
  • *****
  • Posts: 506
During all that testing, i did a simple error, ouch. Here the correction:
wp, in your test project (virtual_listview_editing), if you set MultiSelect to true, but set RowSelect to false,
then the patch will not come into effect, right?
(If yest, then the patch depends on "RowSelect", and that's not very satisfying)
Lazarus 3.2  FPC 3.2.2 Win10 64bit

wp

  • Hero Member
  • *****
  • Posts: 11830
Here's a new version of the test project, now extended by MultiSelect and RowSelect options and multiple columns, and I cannot see that anything is wrong with the Listview in current Laz-main.

 

TinyPortal © 2005-2018