Recent

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

d7_2_laz

  • Hero Member
  • *****
  • Posts: 506
For to go sure. i just recreated my laz-git 2.3 environment (before i couldn't; compilation abort due to missing  unit SimpleWebSrvController, now there had been related changes, as i did see).
Just i could rebuild based on zip download from here: https://gitlab.com/freepascal.org/lazarus/lazarus

Ok, if i activate MultiSelect, but _not_  RowSelect), i think the patch will not get active.

Doing the steps (virtual listview):
Click onto  item 2 and press F2. Enter "Item 2 new".  During the inplace editor is still open, click on item 4
Expected:   Item 2 shows the caption "item 2 new".  Item 4 simply is selected.
What i see: Item 2 shows the caption "Item 2".  Item 4 shows the caption "Item 2 new".   // <=== the same as without patch

Now doing the same steps with "RowSelect" activated. It behaves as expected.
You have a differenz result? Then something basically went wrong with my environment.
« Last Edit: April 13, 2022, 08:32:42 pm by d7_2_laz »
Lazarus 3.2  FPC 3.2.2 Win10 64bit

wp

  • Hero Member
  • *****
  • Posts: 11830
I thought I had checked this...

Here's one more patch in addition to the others, except for CanEdit which can be restored:
Code: Pascal  [Select][+][-]
  1. procedure TCustomListView.HideEditor;
  2. var
  3.   S: String;
  4. begin
  5.   S := FEditor.Text;
  6.   if FEditor.Item <> nil then
  7.   begin                          // add the yellow lines
  8.     if MultiSelect and OwnerData then
  9.       DoEndEdit(Items[FSelectedIdx], S)
  10.     else
  11.       DoEndEdit(FEditor.Item, S);
  12.   end;
  13.   FEditor.Item := nil;
  14.   FEditor.Visible := False;
  15.   FEditor.SetBounds(0, 0, 0, 0);
  16. end;
  17.  
  18. // Replace current code by this
  19. function TCustomListView.CanEdit(Item: TListItem): Boolean;
  20. begin
  21.   Result := True;
  22.   if Assigned(FOnEditing) then
  23.     FOnEditing(Self, Item, Result);
  24. end;

Not committed, yet.


d7_2_laz

  • Hero Member
  • *****
  • Posts: 506
Bingo! I think that's it!  That exactly does the job, on test project level as well as on application level (plus when using a TCustomListViewEditor here).  It's excellent !!   :)


Lazarus 3.2  FPC 3.2.2 Win10 64bit

wp

  • Hero Member
  • *****
  • Posts: 11830
OK, committed then.

d7_2_laz

  • Hero Member
  • *****
  • Posts: 506
Thousand thanks wp for your efforts to improve the listview again!   :)
Lazarus 3.2  FPC 3.2.2 Win10 64bit

d7_2_laz

  • Hero Member
  • *****
  • Posts: 506
After a couple of days using the patch i did encounter a very small, but not nice, remaining lack with the patch. That's life ..
It's just the reciprocal scenario. Not changing a selection. But for the same selected item doing a repeated rename of it.

One could imagine a use case like: rename an item (what might imply a somehow time consuming work , resp. waiting for an OS response when renaming a file).
After that, a corrective action might happen: click onto the item, F2, change the value, press <enter>. After two or three cycles of such action the following might happen:
the selection resp. the focus is lost, the inplace editor might have closed unintendedly.
Additionally on application level I noticed: the listview itself sometimes appeard to have lost the focus …. And pressing an "F2" found itself to open the editor for a node of a treeview attached.

One can see that somehow when using the test project as of reply #14, doing:
- set property "MultiSelect" to true.  Apply something like a "sleep(100)” within "ListView1Edited" for to simulate time consuming action.
- Now do a couple of times very fast:  click onto the item; press F2; change the value; do mouse click onto the item;  again press F2 etc.   I’m seeing now in a reproducable way the selection and or focus go lost. – I think it’s related to HideEditor that might work too greedy in some situations.

Within customlistview.inc,  I refer to this call of HideEditor:
          // editing
          if FEditor.Visible then
            HideEditor;
Maybe it should be applied more restrictive. For to come into effect only for situations where it is needed for.  Probably not for the situation where the selection hadn't been changed.
So I did try a couple of things like:
          if FEditor.Visible then
             if ( (  (nm^.iItem<>-1) And (FSelectedIdx<>-1) ) And (nm^.iItem <> FSelectedIdx)  ) then
               HideEditor;
           
I did experiment with a lot of various combinations here. But none of those attempts had been very robust so far for to be considered reliable.
« Last Edit: April 15, 2022, 10:13:07 pm by d7_2_laz »
Lazarus 3.2  FPC 3.2.2 Win10 64bit

wp

  • Hero Member
  • *****
  • Posts: 11830
As usual I cannot reproduce...

Please post your version of that test project modified such that the error occurs. You do give a description but there are still some possibilities how to implement it exactly (e.g. where to put the Sleep(100)).

Also, please edit your message and remove the typos because the important parts are hard to understand.

If you feel that the HideEditor in CNNotify is too aggressive move or copy it to other places, e.g. inside the "// focus" or "// select" parts, and test again.

d7_2_laz

  • Hero Member
  • *****
  • Posts: 506
For to explain a bit why i did write this: it's like so often: you see something strange and wonder. After some time it might occur again, and one begins to search. Im my case I got aware when I dis notice that after doing “F2” the editor of the current node of a treeview opened(*). My feeling had been, oh, you did run into a defocusing problem (*)

Oh yes too much typos. I'll try to avoid this and did an edit.
The project as used for the test is attached.
Also already tried to move the placing of "if ... then HideEditor" but had no success yet, but i'll continue.

Important for the test is the sequence (done very fast):  for the same item: do mouse click onto the item, press "F2", edit the value, press <Enter>  … and the same again a couple of times (the Click is a redundant action, but it might happen though).   -  “F2” alone does not raise problems
At least you should see that the editor might close unintendedly.

(*) Besides: I already had noticed such in my first attempts (some time before I opened this topic), where I tried to intercept the WMKillFocus of a TCustomListViewEditor and did here:
   If somecondition then begin
      DoExit;
     // inherited;
« Last Edit: April 15, 2022, 10:15:06 pm by d7_2_laz »
Lazarus 3.2  FPC 3.2.2 Win10 64bit

wp

  • Hero Member
  • *****
  • Posts: 11830
OK, I could see now a few times that the selected item moved to the top after editing the third item. Is this the problem?

But seeing your modification of the OnEdited event I think that this in issue in this event handler rather than of the TListView.
You put the Sleep (the "long operation") BEFORE the assignment of the new string to the FList user structure. Since the Listview does not contain the data in virtual mode the data are kept in some kind of user structure; in case of the demo, this is the StringList FList. This is where the OnData event gets the displayed strings from. And the OnEdited handler must move the edited string back into this list. If you do a lengthy calculation before this step there's a risk that the selection may be moved to another item before that. Please put the calculation (the Sleep) AFTER FList[Item.Index] := S + subitems, and check again. (Sorry I did not think that this might be an issue when I set up the demo and put the logging code before the assignment of the new string).

d7_2_laz

  • Hero Member
  • *****
  • Posts: 506
Hello wp, yes. i see this point.
The reason behind why i placed, within the “Edited” event handler, the "sleep" ("lengthy operation") before the assignment to the data structure, is, that in the real use case behind it represents a file i/o which might fail (“file cannot be renamed” as it is read-only etc.).
And so one doesn't want to write potentially obsolete data to the structure.  Wait for OS's response first, then write to the data ..
Ok, one could write the data first, and revert afterwards if needed, that is feasible of course.

However: the placement imo does not really hinder the patch to work in the main respect. Ie. when editing item_3 and clicking onto item_5 during this action  - the patch does this job fine.
But: whereever the "sleep" is placed (and that happens even completely independent from this "sleep", you might remove it):  a second or third Click + F2 + edit will make the item editor disappear (close) unintendedly, just during an attempt to edit. What you additionally did see (that the selection moved to the top), appeared within my app in that way, that the focus went to another control (treeview) and via F2 the current node's editor opened. Obviously focus? selection? are lost and user interaction routed to somewhere else. I regret that i saw this side-effect so late.

Hhhmm ... .  Maybe one can find a combination of various conditions for to make the HideEditor only happen when really needed and else not.
But that could end up to make customlistview.inc coding partially non-intuitive and hard to maintain. The more as additional layers might occur which can influence. I’m thinking here of an app option “sort after rename” which uses ColClick which internally does an HideEditor again, showing up other effects again.

Meanwhile i'm a bit doubting if it is very reasonable to take all those various situations into account which can occur in special situations. Maybe it’s better here to let it up to the programmer to find a specific solution for a specific context.
Ie., for this topic. on app level it's possible, by using a remembered index of the item that is currently edited. That would fully serve all needs, but would work  only without the HideEditor call. Sigh.
I'm pretty unsure what's the best manner here. What do you think?
But I had to learn the hard way that one might easily enter treacherous terrain.
Lazarus 3.2  FPC 3.2.2 Win10 64bit

wp

  • Hero Member
  • *****
  • Posts: 11830
Some pessimistic notes:

Some LCL components, in particular those with heavy interaction with the many widgetsets, have become extremely complicated. Assuming that nobody understands their inner workings in every detail, and knowing that a perfection of 100% cannot be achieved, it follows that every bug fix very likely will introduce an issue somewhere else...

I'm not even sure whether adding HideEditor in the first fix of this series is correct. It does not address the root cause of the issue, namely that virtual mode cannot handle the case that another item is selected while being in edit mode. Although this is not delphi-compatible it seems that the ListView is constructed to work this way - see the behaviour in normal mode. So, ending edit mode by calling HideEditor "out of the row", may introduce other issues...

d7_2_laz

  • Hero Member
  • *****
  • Posts: 506
Yes wp, i ended up to see it just in the same way. Nevertheless i see it positive in so far that helped to gain more awareness about this potential field of issues.
I’d propose as a compromise simply to revert the two-liner (if … then HideEditor) and to leave it to the application specifically to deal with, which is easily possible (using a remembered item index).
Lazarus 3.2  FPC 3.2.2 Win10 64bit

wp

  • Hero Member
  • *****
  • Posts: 11830
I am hesitant to revert the "HideEditor" because I think the probability that a user of the virtual listview traps into the "edit the wrong line" bug (the initial concern) is much greater than the current issue with some delay in the OnEdited event.

But now I found a bug which is even more severe and it happens even in the old version (before introducing "HideEditor"), even in Laz 1.8.4: Just select an item in the virtual listview and press any key (e.g. "a") - the selection jumps to the first item (Windows only). Another problem is that the behaviour is different depending on whether ListView.Selected is called in the OnEdited handler or not. What a mess!

ATM, I cannot recommend using the TListview in virtual mode to anybody.

d7_2_laz

  • Hero Member
  • *****
  • Posts: 506
Yes,. i already noticed that too.  Previously (i assumed a special keyboard navigation routine would be needed to be written attached to the keypress (*)), and now again actually, as you told the your selection jumped to the top of the list.
My interpretation had been: ok. one thinks he's still in the editor, but in fact the editor already did close underneath, and so the key did apply to the listview, not the editor.

(*) Checked against my old app (Delphi 7) how that did behave (without special coding): typing a character, it did stay on the currently selected item. If none selected, it does nothing. I'd doubt that there is a builtin mechanism here

About "Selected" within "Edited" i've no clear picture yet, but i did wonder a bit why (in your demo) OnSelectItem had been called twice, the first with Selected nil resp. itemindex -1.
(that's probably just the nm^.iItem = -1 see below). No real opinion yet.

Meanwhile .. the matter didn't let me rest... I took a closer look at the "end editing" process and values and maybe there is a less aggressive / more specific solution that does cover the logics discussed.
If you should have the time to after Easter, could you give it a short look and try?

// Does relate to: https://gitlab.com/freepascal.org/lazarus/lazarus/-/issues/39708

Code: Pascal  [Select][+][-]
  1.       // end editing
  2.       if IsEditing then
  3.         if (nm^.iItem = -1) And (FSelectedIdx<>-1) then  // specifically when item outside the editor clicked
  4.           if Not ((FFocused <> nil) And FFocused.Selected) then  // but prevent known side effect (defocusing at repeated /click/edit with Click on the same item)
  5.                HideEditor;

I did already test, with special regard to the side effect, with the application some time and it did meet the expectation.
I think it’s at least much more better than before
If you find it promising and worthy, I would subject it to a more extensive test here.

Generally, i did struggle with the virtual listview since a longer time, as you probably remember, but meanwhile i felt that the main hurdles had been resolved. There are more use cases and so more hurdles though.
Lazarus 3.2  FPC 3.2.2 Win10 64bit

wp

  • Hero Member
  • *****
  • Posts: 11830
Thank you. This modification works better, although it does look like a real workaround now ;). Seriously: Shouldn't we add an "If OwnerData" because normal mode has been working fine all the time, and this way we would restrict the range of possible side-effects? I will commit it after a few more tests.

I cannot see an effect any more when ListView.Selected is called from the OnEdited handler.

Still something is going on with selection. Because, when I select an item in the virtual listview, press F2, the editor box appears, flahes away, and immediately comes back. This is very clearly seen in Win 11 where the edit has a thick bright blue underline.

And the "jump to first" issue is still there. Simply select any virtual listview item down the list, press any (standard) key, no edit involved, and see the selection jump to the first item. Very probably this is unrelated to the current issues because it has been there for a long time.
« Last Edit: April 17, 2022, 01:31:21 pm by wp »

 

TinyPortal © 2005-2018