SOLVED! Virtual listview; validity of "Item" in "Edited" when Selection changed

[d7_2_laz]

Once again thousane thanks for your attention wp!

- Shouldn't we add an "If OwnerData" -> Does make sense of course. -  "if (IsEditing and OwnerData)"  ?

- Modification works better ->  It's certainly better than before (although presumingly not perfect), but i'd propose too to set it on a "hold until fully tested" for to be aware of any further deviations

- No more effect when ListView.Selected is called from the OnEdited -> i supposed this, but it's fine to hear that it really helps here

- "Jump to first" issue -> i believe it's a really different thing. Without further digging i'd assume that something like FindCaption  could be implemented Keyprss or KeyDown.
Basically it think it's not much different to D7, which seems to do a "stay at current selected".  - I think it's not a bug but a not yet implemnted feature.
And an implementation might not necessarily be easy (what, when typing a sequence of characters. likr "bbb"; implement the belonging sub-iteration too ? How to behave on a reversed sort order? How, if th sort order is not given by column 0, but by column 4?  Follow such sort orders, or simply find the next match downwards?
 
- Pess F2, the editor box appears, flahes away, and immediately comes back
  -> I cannot see it here (your demo, Win 10, unfortunately i have no access to a Win 11). Is it specifically due to the "HideEditor" patch, or due to it's mdification?  Or did it occur independent from those already before?

[wp]

- "Jump to first" issue -> i believe it's a really different thing. Without further digging i'd assume that something like FindCaption  could be implemented Keyprss or KeyDown.
Basically it think it's not much different to D7, which seems to do a "stay at current selected".  - I think it's not a bug but a not yet implemnted feature.

I don't think so. I would consider a built-in automatic incremental search feature to be very annoying. And Linux and mac don't have it, and normal mode neither. There is a KeyDown method which opens the editor and calls the inherited method when the key is not F2. But diving into the inherited KeyDown I am soon lost in the myriads of system messages. Beyond that, I did not see anything catching the key press. So why does the Listview select the first item? I can fix the issue by killing all "normal" keys in that KeyDown method, but this certainly is too large an impact on user code.

I filed a bug report so that the issue is not forgotten (https://gitlab.com/freepascal.org/lazarus/lazarus/-/issues/39715).
 

- Pess F2, the editor box appears, flahes away, and immediately comes back
  -> I cannot see it here (your demo, Win 10, unfortunately i have no access to a Win 11). Is it specifically due to the "HideEditor" patch, or due to it's mdification?  Or did it occur independent from those already before?

I see it also when I compile with Laz 1.8.4. Thus, it has nothing to do with the recent modifications.

[d7_2_laz]

About the "jump to top", yes i think too it's earlier than CNNotify, within the message loop.
CNNotify itself will behave correct. It will receive a twofold LVN_ITEMCHANGED, the first, on the currently selected item, with iItem -1 (for deselection), and a second with iItem 0, and it is not clear, why 0 (might it be orphaned from an earlier initialization of the data structure?).
Even if one tries, purely experimental, to intercept the latter (and for the first call that's not possible), the selection will jump to top. On the one hand, the question appears to be, why this doubled LVN_ITEMCHANGED is sent here (for the regular listview it is not). On the other hand: CNNotify doesn’t play a role here, it may be exited directly at the beginning and nothing changes (jumps to top). It happens earlier, right.

Back to the modification of the patch. I tried it out a couple of times, and my experience is:
the side-effect (= unintended closure of edit box when doing fast and repeatedly a Click / F2 / edit on the same item) in rare cases still did occur, but significantly less often.
And: the very irritating effect, that with F2 the editor does open within another control (tree node), as the listview passed the focus to another control. never did happen again since having applied the patch modification.
So far I was satisfied, but not fully, and asked myself, if it’s possible to intercept the remaining few deviants too. With this little addition they appear to be removed:

Code: Pascal  [Select][+][-]

1.           // end editing
2.           if (IsEditing And OwnerData) 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.                   if FSelectedIdx <> FFocused.Index then    // Eliminate remaining deviations
6.                      HideEditor;

The syntax can be more compacted of course, but I kept it as it is for better readability.

[d7_2_laz]

Occasionally encounted situations where the item editor still did close unintendedly. This is a real beast. Meanwhile i'm missing ideas how to reliably intercept this.

The problem is, that on item change there are each slices of deselections sent within (nm^.iItem = -1) that
make it very hard, maybe impossible, to find the right hook to decide, when the editor needs to be closed and when it must not be closed.

With other words: what's missing is a persistent variable that could be used to store the information about the index of the item that just is edited.
(That's just the strategy of the workaround that i mentioned initially), Something like FEditedItemIndex.

For instance, if i misuse - just for test - the Listview's "Tag" for to store - within Editing - the index of the item that is edited, then it would be faiirly easy enough to do later a simple compare of this variable with nm^.iItem for to decide if HideEditor has to be done because the indices differ. Easy, understandable and reliable.
Every idea would be very welcome.

[wp]

There's no reason why an "FEditedIdx" should not be introduced... I had attempted it myself but was lead into more desaster. But maybe I gave up too early.

[d7_2_laz]

Sadly, the assumption was too optimistic, the principle is not fully transferable.

Because the comparison of the indices needs to be done when nm^.iItem -1 is passed to CNNotify, otherwise, when HideEditor is done after selection is updated,  it's too late (EndEdit will point to the wrong item). But at that stage (when iItem is -1) one cannot reliably determine which will be the upcoming next index).
So it seems to be the same vice versa, a dilemma.

So perhaps  it's the best to stay at the version as from replay #32, as it is not fully perfect, but mostly does the job well.

[d7_2_laz]

Observation on this topic: the remaining irregularities probably are related to the position of the mouse cursor.

Example (using the demo project)
When doing fast: Click / F2 / edit modify value / <Enter>  ... repeatedly a couple of times eg. on the same item 3 (index 3), then it may happen that sometimes the mouse did move away a bit from the item 3 when pressing F2.

And then:
FSelectedIdx is: 3;   FSelected.Index, FFocused.Index, FEditor.Item.Index are, all uniquely identical here:  4, or 5, or ..
Having inspected those values directly before HideEditor is called.
And the logic does what it has been told to do (HideEditor). Because it does not have the right information available.

I wouldn't have expected that. That, at the click, Selected & Co. do behave as being controlled by the mouse position.

With the demo project having applied the workaround as above:
Try: on item 3, press F2 and simultaneously move the mouse a little bit down, and you'll see it easily.
The editor will be closed immediately (HideEditor called) although it shouldn't.
That should not happen. It should be possible to move the mouse outside of the boundaries of the item when pressing F2, it's a normal gesture.

The challenge here is to determine - within CNNotify/LVN_ITEMCHANGED, and here: when nm^.iItem is -1, not later - which item had been clicked.
And not being falsified by mouse movements later.
Had tried to to a test which remembers the item index when WM_LBUTTONDOWN is called, but had to learn that sometimes this is called later than CNNotify itself.

This thing appears to be very recalcitrant.
Maybe somebody has a clever idea.

[d7_2_laz]

I had been still trying to find the appropriate coordinates for to decide when HideEditor should be called and when not.
The last appearing caveat had been:  when moving the cursor outside the item's bounds whilst pressing F2, the coordinates are going wrong.

It's a different issue, but exactly the same situation that is described here (here: concerning StartDrag).
https://delphi.cjcsoft.net/viewthread.php?tid=49546

"Windows sends a WM_LBUTTONDOWN message to the application, with the current cursor coordinates. In the meantime, you move the cursor to a different location.
Windows interrupts the running of your application to update the position of the cursor. VCL still didn't finish processing your events."

I had already tried before to solve the issue described recently by remembering the cursor position (or the item index) at WndProc / WM_LBUTTONDOWN.
Unfortunately: the problem is that CNNotify sometimes arrives earlier than WM_LBUTTONDOWN. and so the relevant info arrived too late. At least using my subclassed WndProc here. So i hadn't pursue it.
 
But after having read that article, i retried
I did a restart by using a little modification within win32wscustomlistview:

// win32wscustomlistview, ListViewProc:

Code: Pascal  [Select][+][-]

1. // There is already a nice to use "ListItem" retrieved, so it wouldn't cost additional overhead for to apply a simple assignment:
2.         if Msg = WM_LBUTTONDOWN then
3.            if ListItem = nil then
4.              ListView.Tag := -1
5.            else
6.              ListView.Tag := PtrInt(ListItem.Index);   // Only for proof-of-concept. Should be better something like: FClickedItemIdx or similar
7. // Wanted to have a real variable for that, like (whatever it is named):
8.         //FClickedItemIdx := ListItem.Index;

Here, "ListView.Tag" is only used as a placeholder persistent storage just for testing, as there is no available variable yet..  It should be a F-Variable of course.

And when using this information within customlistview.inc, the result is just i would have expected:
straightforward and no side-effects! Does work flawlessly:

// customlistview.inc:

Code: Pascal  [Select][+][-]

1.       // end editing
2.       if (IsEditing And OwnerData) then
3.         if (nm^.iItem = -1) And (FSelectedIdx<>-1) then
4.             if Integer(Self.Tag) <>  FSelectedIdx then         
5.  // Would be nice to have something like that instead::  if FClickedItemIdx <> FSelectedIdx then
6.                 HideEditor;

That's it. As it should be. No more unexpected closurese of the editor. Easy to understand.
What do you think??

[wp]

Can you send me a patch relative to the current version in "main"? This thread contains many ideas for possible changes, and it's too easy to do something wrong when applying them from the description and code fragments.

Never use the Tag of a component for internal purposes. It is reserved for the user.

[d7_2_laz]

The tag was misused for testing purposes only as a placeholder, until there would be a basic positive feedback about the approach itself.
A variable should be to be applied of course, probably within comctrls.pp. i think.
I did that now.

Just efetched the recent files related (comctrls.pp, win32wscustomlistview.inc, customlistview.inc) as of today from here:
https://gitlab.com/freepascal.org/lazarus/lazarus/-/tree/main/lcl
(Remark besides: could it be that a recent patch from you about TCustomListView.CanEdit is not yet in there ??)

As i don't know the gitlab's instruments i created three patch-like diff-files generated locally with the help of WinMerge.

wp, what do you think of the fix proposal?

[d7_2_laz]

Late idea: within win32wscustomlistview, it wouldn't be bad to apply it to WM_RBUTTONDOWN as well..
    if ( (Msg = WM_LBUTTONDOWN) or (Msg = WM_RBUTTONDOWN) )  then

[wp]

Hmmm...

You introduce a FClickedIdx and make a public property out of it. This means it can be set/read for all purposes. But it gets its regular value only in the win32 widget. And the reaction on the value of FClickedIdx is in common code again. My stomach tells me that this concept calls for trouble...

I'd have a better feeling if we could keep all that FClickedIdx thing inside the widgetset. Here's how I would implement your idea:

In win32wscustomlistview.inc there is a data record ListViewWindProcInfo which provides some intermediate storage of message data. Its elements are populated in the ListViewParentMsgHandler and the ListViewProc procedure - the latter one is the one in which you intercept the WM_LBUTTONDOWN message. So, why don't we add ClickedIdx element to that record and write the ListItem.Index to that element rather than to a new, globally available ClickedIdx property of the listview?

Code: Pascal  [Select][+][-]

1. var
2.   ListViewWindProcInfo: record
3.     ActiveListView: TCustomListView;
4.     NoMouseUp: Boolean;
5.     ClickedIdx: Integer;         // <------ wp: added
6.   end;
7.  
8. function ListViewProc(Window: HWnd; Msg: UInt; WParam: Windows.WParam;
9.     LParam: Windows.LParam): LResult; stdcall;
10. ...
11. begin
12.   case Msg of
13.   ...
14.     WM_LBUTTONDOWN, WM_RBUTTONDOWN:
15.       begin
16.         WindowInfo := GetWin32WindowInfo(Window);
17.         ListView := TListView(WindowInfo^.WinControl);
18.         ListItem := ListView.GetItemAt(GET_X_LPARAM(LParam), GET_Y_LPARAM(LParam));
19.  
20.        { wp: next lines replace your code }
21.         if (Msg = WM_LBUTTONDOWN) or (Msg = WM_RBUTTONDOWN) then
22.           if ListItem = nil then
23.             ListViewWindProcInfo.ClickedIdx := -1
24.           else
25.             ListViewWindProcInfo.ClickedIdx := ListItem.Index;
26.   ...

Now, how do we get this ListViewWindProcInfo.ClickIdx into the listview? We could add a widgetset method MustHideEditor() which returns true when that ClickedIdx is different from the SelectedIdx passed as a parameter:

In Win32WSComCtrls:

Code: Pascal  [Select][+][-]

1. type
2.     TWin32WSCustomListView = class(TWSCustomListView)
3.       ...
4.     public
5.       ...
6.       class function MustHideEditor(const ALV: TCustomListView; ASelectedIdx: Integer): Boolean; override;  // <--- wp: add
7.     end;

and in win32wscustomlistview.inc:

Code: Pascal  [Select][+][-]

1. class function TWin32WSCustomListView.MustHideEditor(const ALV: TCustomListview;   // <--- wp: add this method
2.   ASelectedIdx: Integer): Boolean;
3. begin
4.   result := ASelectedIdx <> ListViewWindProcInfo.ClickedIdx;
5. end;

We also must define the method in the ancestor, TWSCustomListView, where it simply can return false to be ignored by all non-windows widgetsets:

In WSComCtrls.pas: (folder widgets)

Code: Pascal  [Select][+][-]

1. type
2.   TWSCustomListView = class(TWSWinControl)
3.   ...
4.   public
5.   ...
6.     class function MustHideEditor(const ALV: TCustomListView; ASelectedIdx: Integer): Boolean; virtual;  // <--- wp: add
7.   end;
8.  
9. class function TWSCustomListView.MustHideEditor(const ALV: TCustomListView;   // wp: add this method
10.   ASelectedIdx: Integer): Boolean;
11. begin
12.   Result := false;
13. end;

Now almost done... What's left is to call the new MustHideEditor method. This happens in TCustomListView.CNNotify at the place where you compare the ClickedIdx property with the SelectedIdx:

Code: Text  [Select][+][-]

1.           // end editing
2.           if (IsEditing and OwnerData) then
3.            if (nm^.iItem = -1) And (FSelectedIdx<>-1) then
4.              if TWSCustomListViewClass(WidgetSetClass).MustHideEditor(self, FSelectedIdx) then   // <--- wp: replace your code by this line
5.                HideEditor;

As far as I can see, this is working. But I would appreciate if you could test this as well before I commit it (of course, I'll commit it nevertheless if you have difficulties following my description).

[wp]

(Remark besides: could it be that a recent patch from you about TCustomListView.CanEdit is not yet in there ??)

IIRC, the CanEdit modification became obsolete after a later modification in HideEditor itself.

[d7_2_laz]

>>   I'd have a better feeling if we could keep all that FClickedIdx thing inside the widgetset.
Yes, right .. it had really not been planned by me to see this variable as public (it simply had turned out that, as private, it was not known within ListViewProc).
So my full support of your concern and the idea behind the more protective approach.

For testing i replayed the well-known problematic scenarios again (demo project and app) and found it did behave just as expected. Couldn't find anything strange so far. For to go sure i'll repeat some tests tomorrow, but i believe this strange thing should be fully covered now.

Congratulations for your implementation, wp! (i'd never had foound that).

[wp]

The strange thing here is that the more straight-forward idea of reading the clicked index in the MouseDown method does not work because the listview seems to send the LVN_ITEMCHANGED message first, and the mouse messages come later! Mysterious Windows!