Virtual TreeView should not react on standard actions for TEdit controls

[vincentparrett]

From CWBudde on August 06, 2014 21:50:10

Can you attach a sample project that replicates the problem, or can you provide detailed steps how to use or modify one of the demo projects that come with VirtualTreeView?

Add a TEditDelete action to the project with a hotkey assigned to [DEL] key. Now focus a node in the virtual treeview and hit [DEL]. As expected, the node is deleted.

Now if one wants to add a warning dialog like 'Really delete this node?, it has to be done in the TEditDelete action handler. While this works, it can be complicated if the same action is shared by other components (ExecuteAction is handled automatically by the component, which has the focus).

What is the expected output? What do you want to see instead?

It would be nice to have a way to filter/cancel the nodes marked for deletion. Something like an OnNodeDeleting (similar to OnNodeCopying). Performance wise this might not be worse as it could be added in a higher level (just after the action is called) What version of the product are you using? On what operating system? I'm using the trunk Can you supply a patch (most appreciated would be a patch file created with TortoiseSVN)? This will significantly increase the likelyhood that the fix is included in the next release. I can do the change and create a patch if this is something that is not possible otherwise (I'm no expert when it comes to VTV, but I couldn't see any chance for this so far).

Original issue: http://code.google.com/p/virtual-treeview/issues/detail?id=457

[vincentparrett]

From joachim....@gmail.com on August 06, 2014 12:17:41

Add a TEditDelete action to the project with a hotkey assigned to [DEL] key. Now focus a node in the virtual treeview and hit [DEL]. As expected, the node is deleted.

I cannot confirm that. It would surprise me, as the TEditDelete action is intended to work on TEdit fields.

While this works, it can be complicated if the same action is shared by other components

I'm sorry, but I disagree. I don't think it is a good design to share actions if they have so less in common. And I don't think this is functionality with that we should bloat Virtual TreeView.

Labels: -Type-Defect Type-Enhancement

[vincentparrett]

From CWBudde on August 07, 2014 00:05:22

Please see the test application attached to this comment.

You can also see how this is happening in:

Unit VirtualTrees, line 28451:

function TBaseVirtualTree.ExecuteAction(Action: TBasicAction): Boolean;

// Some support for standard actions.

begin Result := inherited ExecuteAction(Action);

if not Result then begin Result := Action is TEditSelectAll; if Result then SelectAll(False) else begin Result := Action is TEditCopy; if Result then CopyToClipboard else if not (toReadOnly in FOptions.FMiscOptions) then begin Result := Action is TEditCut; if Result then CutToClipBoard else begin Result := Action is TEditPaste; if Result then PasteFromClipboard else begin Result := Action is TEditDelete; if Result then DeleteSelectedNodes end; end; end; end; end; end;

Attachment: VirtualTreeView TEditDelete.7z

[vincentparrett]

From CWBudde on August 07, 2014 00:11:46

I'm sorry, but I disagree. I don't think it is a good design to share actions if they have so less in common. And I don't think this is functionality with that we should bloat Virtual TreeView.

I wouldn't really mind if it would not react on the standard action. However, it is also fine as it is - as long as there is a way to abort the deletion when it's happening.

As said, I don't want to add code to react on it, but I want it to be limited (filtered). It's not just limited to our application, where I could also write a local workaround. I think it's a common limitation, especially since there are similar events for copying and moving. So why not just add this for deleting as well?

[vincentparrett]

From joachim....@gmail.com on August 13, 2014 13:24:05

I wouldn't really mind if it would not react on the standard action.

Ooops, I never noticed this code! I don't think that this is a good idea and it should be changed.

It would be better to define a set of standard actions for VirtualTreeView itself.

Status: Accepted
Owner: joachim....@gmail.com

[vincentparrett]

From joachim....@gmail.com on August 13, 2014 13:25:09

Summary: Virtual TreeView should not react on standard actions for TEdit controls (was: Feature request: OnNodeDelete)
Labels: -Type-Enhancement Type-Defect

[vincentparrett]

From CWBudde on August 13, 2014 23:52:18

To prevent existing applications (that might rely on this) to stop working, it should probably be configurable. Especially since TEditCopy doesn't directly rely to a TEdit component, but to the typical 'Edit' menu, which - depending on the context - can also mean to edit a tree (e.g. see the Explorer, which also has an 'Edit' menu).

As stated above - adding an event such as OnNodeDeleting (similar to the existing OnNodeCopying) could give a finer control. With this a dialog (similar to the Explorer) like 'Are you sure?' can be realised, which otherwise is not possible.

[vincentparrett]

From joachim....@gmail.com on September 01, 2014 06:35:25

TEditCopy doesn't directly rely to a TEdit component

I cannot confirm that. TEditAction.HandlesTarget() contains the condition: (Target is TCustomEdit)

I agree that this is a breaking change and breaking changes are never a good thing. But Virtual TreeView already has so many option that I am am reluctant to add an option for this odd code.

adding an event such as OnNodeDeleting

The problem is that nodes are not only deleted because of a user interaction, but also programatically. So your code would need to distinguish if a node is deleted because its parent node is deleted, or because the user pressed the DEL key, etc.

I think the best solution would be a unit with standard actions for the virtual treeview, similar to the standard actions for TEdit.

[vincentparrett]

From joachim....@gmail.com on September 09, 2014 08:40:23

When removing ExecuteAction() we should not forget to remove UpdateAction() too.

[vincentparrett]

From joachim....@gmail.com on September 09, 2014 08:47:17

Labels: -Priority-Medium Priority-High

[vincentparrett]

From CWBudde on September 14, 2014 03:04:53

TEditCopy doesn't directly rely to a TEdit component

I cannot confirm that. TEditAction.HandlesTarget() contains the condition: (Target is TCustomEdit)

Actually it only says that it only handles TCustomEdit instances itself. However, in the wild you can find several components that handle this type of actions by custom ExecuteAction() and UpdateAction() code. Since TEditAction does not know anything about 3rd party components it obviously only sticks to what it knows (TEdit).

I don't want to defend to keep this, but it might be breaking quite some code (especially that handling TEditAction seems valid [to me] and at least in the wild).

adding an event such as OnNodeDeleting

The problem is that nodes are not only deleted because of a user interaction, but > also programatically. So your code would need to distinguish if a node is deleted > because its parent node is deleted, or because the user pressed the DEL key, etc.

True, but at the moment there is no chance to fix this (other than by patching the code manually). This said, any change is welcome for us, even if that would involve adding more code around this.

In fact we only want a way to display a confirm dialog prior to actually deleting the nodes. If this can be done in a more general manner (to catch all possible ways of deleting nodes), the better.

Attached I have added a simple patch that adds an OnNodeDeleting event that only triggers for the actions. It's probably not the best solution, but it should do the trick without interference of existing code (haven't tested it deeply though).

Attachment: DeleteNodeEvent.patch

[vincentparrett]

From joachim....@gmail.com on September 14, 2014 11:25:05

True, but at the moment there is no chance to fix this (other than by patching the code manually)

You could simply use a TAction and call DeleteNode() in its OnExecute event. Showing a confirmation dialog is not a big deal then.

but it might be breaking quite some code

This is true, but if there is a clean and proper substitute in form of default Virtual TreeView actions, then the benefit is worth it. I already have some, but nedd to extract them from our code library first.

[vincentparrett]

From CWBudde on September 14, 2014 12:43:44

True, but at the moment there is no chance to fix this (other than by patching the code manually)

You could simply use a TAction and call DeleteNode() in its OnExecute event. Showing a confirmation dialog is not a big deal then.

If it would be a simple application than this might work. But in fact this is a rather complex application already with 3 components that handle TEdit* events. With this I only need to have the edit actions in the action list and each component handles this depending on the focus. However, in order to make this work, I would need to switch to a TAction and make all the required calls manually. In the past we have done it this way, but it always made troubles, because the action sometimes did nothing or invoked an action in a different component (in case of a focus problem).

Only TVirtualTreeview makes problems now, because it deletes the nodes automatically without firing any usefule notification event. Even if I leave out the requirement of showing a confirmation dialog, it deletes the node, but I have no chance to perform synchronization other than for all nodes being freed (OnFinalization). And since I can't hook into the TEditDelete action (handled automatically), I have no way to check what deleted the node.

but it might be breaking quite some code

This is true, but if there is a clean and proper substitute in form of default Virtual TreeView actions, then the benefit is worth it. I already have some, but nedd to extract them from our code library first.

Again, I would welcome any solution, but an extra 'delete' action would cause to add extra code to handle this.

In fact I don't think that this is in particular the 'clean and proper solution', because I still think it is valid to handle TEdit* actions as they are not necessarily tied to TEdit fields, but rather to the 'Edit' category in a main menu. As seen with some other components it can just be treated as general cut/copy/paste/delete actions, which just do cut/copy/paste/delete not related to a TEdit component alone.

One rather prominent example of using the TEdit* actions would be TSynEdit, which - despite of the name - is actually a TCustomControl. It handles the TEdit* actions naturally.

Looking at TEditAction again, I see the property Control: TCustomEdit, but it must not necessarily be set in order to work. Only if it should handle TCustomEdit itself (and not handling the action by the custom component).

However, it might have changed in later implementations of Delphi (only using XE2 here).

[vincentparrett]

From joachim....@gmail.com on September 15, 2014 00:41:56

Looking at TEditAction again, I see the property Control: TCustomEdit, but it must not necessarily be set in order to work.

The implementation of TEditAction clearly shows that its implementors did not have in mind what you are trying to do.

One rather prominent example of using the TEdit* actions would be TSynEdit, which - despite of the name - is actually a TCustomControl. It handles the TEdit* actions naturally.

Oh come on, you seriously want to compare a multiline text edit control like SynEdit to a treeview control in this question?

I can offer to do changes that make it easier it easier for you to do this in a derived class. So if you need any methods declared virtual, just let me know.

[vincentparrett]

From CWBudde on September 15, 2014 01:52:01

Looking at TEditAction again, I see the property Control: TCustomEdit, but it must not necessarily be set in order to work.

The implementation of TEditAction clearly shows that its implementors did not have in mind what you are trying to do.

I don't say that they didn't have that in mind, but they made it that loose, that others are possible as well. Just look at the documentation http://docwiki.embarcadero.com/Libraries/XE6/en/Vcl.StdActns.TEditAction , which only states 'edit control' and not TCustomEdit or TEdit. Also given the fact that there is no link in the 'See also' section would hint for the allowance of other.

Actually it's not me who had implemented this in neither TVirtualTreeview or TSynEdit or others, but I'd like to see a solution that hardly breaks existing code.

In our case I can live with a custom modification. I just wanted to note that adding a TAction to perform the delete isn't sufficient, as the existing TEditDelete must be disabled to not jump in. And by disabling the TEditDelete manually, it must also be enabled manually for the places where it should be used (see example, where it gets automatically enabled/disabled).

This means that the existing code should be adjusted somehow, which should be done in the least destructive way to keep existing code still working.

One rather prominent example of using the TEdit* actions would be TSynEdit, which - despite of the name - is actually a TCustomControl. It handles the TEdit* actions naturally.

Oh come on, you seriously want to compare a multiline text edit control like SynEdit to a treeview control in this question?

From the compiler/RTL perspective both are non-TCustomEdit controls. From the user perspective both can be considered as 'edit control[s]'. A virtual Treeview is probably not the typical edit control, but given the wide use case possibilities, it can pretty much look like an edit control.

I can offer to do changes that make it easier it easier for you to do this in a derived class. So if you need any methods declared virtual, just let me know.

Since ExecuteAction/UpdateAction and DeleteSelectedNodes are already virtual, it should already be possible to derive from the TVirtualStringTree and override this locally.

It could also be useful to have an option which reads 'DisableAutoActionHandling' or similar, which disables the action handling for TEditActions. If set to 'false' by default, the users still relying on these can be made compatible by re-enabling this.

[vincentparrett]

From joachim....@gmail.com on September 15, 2014 03:57:18

which only states 'edit control' and not TCustomEdit or TEdit.

And Virtual treeview clearly is not an edit control, while SynEdit is.

It could also be useful to have an option which reads 'DisableAutoActionHandling' or similar

When this issue is implemented, this will be handled by the actions, like it is done in similar cases in the VCL. So there won't be the need for such a property, and it doesn't make sense to add it just for a few weeks.

[vincentparrett]

From CWBudde on September 15, 2014 04:25:55

which only states 'edit control' and not TCustomEdit or TEdit.

And Virtual treeview clearly is not an edit control, while SynEdit is.

I agree that it's probably not the typical edit control, but since 'edit' does not imply 'text edit', I would not exclude it.

It could also be useful to have an option which reads 'DisableAutoActionHandling' or similar

When this issue is implemented, this will be handled by the actions, like it is done in similar cases in the VCL. So there won't be the need for such a property, and it doesn't make sense to add it just for a few weeks.

I'd rather have in mind to have this option still in place for the existing code. This means the existing code should stay in place, as I don't consider this as a defect (see above).

In the end it might boil down to the understanding of the word edit (see http://www.thesaurus.com/browse/edit ), which I understand probably a bit wider ('bearbeiten' in the German language).

This however is opposed to a static control, only serving for display purpose (such as a TLabel). While a treeview (containing at least 'view') is typical (historically) used to view a tree, I can however also understand your point of view.

[vincentparrett]

From joachim....@gmail.com on September 15, 2014 11:55:34

In the end it might boil down to the understanding of the word edit

No, it is not important which understanding you or I have of the word "edit". In the end it is important which ideas and principles the makers of the VCL framework had in mind. The fact that that they didn't implement the TEditAction to work with list and tree controls, but on the other hand implemented standard actions like TListControlSelectAll and TListControlDeleteSelection tells everything.

[vincentparrett]

From CWBudde on September 16, 2014 05:39:24

It is at least also important how others (including you and me) understand by it if they used the TEdit* actions for other purpose based on the documentation (and thus not tied only to TCustomEdit). In case they rely on the automatic behaviour, which is in place in the virtual treeview right now, all they will notice is that it doesn't work anymore.

But never mind. Just wanted to raise potential problems and solutions that can be the least problematic for existing code.

In our case we have found a custom solution now, independent from any possible changes in this regards.