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

Create base class for EditLink and few most used implementations #1063

Open
Fr0sT-Brutal opened this issue Sep 22, 2021 · 7 comments
Open

Create base class for EditLink and few most used implementations #1063

Fr0sT-Brutal opened this issue Sep 22, 2021 · 7 comments
Labels
Pull Requests Invited There are no current plans to address the issue, but we would be happy if someone supplies a PR.

Comments

@Fr0sT-Brutal
Copy link
Contributor

Somewhat related to #799. Would be nice to have basic EditLink class to inherit from and few ready-to-use most used implementations (plain edit, combobox, mask edit, spin edit...).
I could do it but first I want to be sure changes will be applied

@joachimmarder
Copy link
Contributor

I could do it but first I want to be sure changes will be applied

If they come in new, separate units I don't see why not. My concerns in #799 were mainly about a read/write Data[] property in the base class. I will be happy to accpet a derived TVirtualTreeGrid control and appropriate editors.

@joachimmarder joachimmarder added the Pull Requests Invited There are no current plans to address the issue, but we would be happy if someone supplies a PR. label Sep 22, 2021
@Fr0sT-Brutal
Copy link
Contributor Author

Well, I thought about extending/modifying VirtualTrees.EditLink.pas so that other links could inherit most of generic methods and implement just specific things

@joachimmarder
Copy link
Contributor

@Fr0sT-Brutal: That sounds good, a well designed base class for editors will be helpful.

@Fr0sT-Brutal
Copy link
Contributor Author

Done.
Combobox/datetimepicker/etc editlinks are probably to come.
@joachimmarder I've inspected TStringEditLink.SetBounds but it contains too much black magic so I left it for now but I guess some of that code will be needed for classes with controls of other types. In the same time Advanced demo only has 2 lines for this.

Is it really necessary to reset FStopping on exception in TStringEditLink.EndEdit?

@joachimmarder
Copy link
Contributor

Is it really necessary to reset FStopping on exception in TStringEditLink.EndEdit?

Sorry, I can't comment much on that, I never worked on that code after taking over the project. But it looks strange. I wonder which kind of exceptions are to be expected here? Catching untyped exceptions isn't best practice anyway, and the covered code block seems to be too large. Maybe in case FEdit.Hide() fails, the edit process should not be considered as ended because the editor may still be visible.

I would prefer to catch expected exceptions here only.

@joachimmarder
Copy link
Contributor

Thank you for the pull request, it is now merged, but without an extended code review.

@Fr0sT-Brutal
Copy link
Contributor Author

I'm a little puzzled here too... FStopping looks like mechanism to avoid reentrance but I guess the tree itself won't let it happen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Pull Requests Invited There are no current plans to address the issue, but we would be happy if someone supplies a PR.
Projects
None yet
Development

No branches or pull requests

2 participants