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

Easier Inheritance #12

Closed
ColinCren opened this issue Jul 24, 2014 · 7 comments
Closed

Easier Inheritance #12

ColinCren opened this issue Jul 24, 2014 · 7 comments

Comments

@ColinCren
Copy link

ATF is a great package, and I'm really stoked it's now open for everyone to use.

I've found that a lot of the classes I've been using so far (PropertyGrid, PropertyEditor, PropertyGridView, etc) use restrictive access modifiers. I'd like to suggest "internal" be removed from codebase, and "private" be used minimally in favor of protected. These access modifiers make it difficult to re-use existing code and either force a lot of copy/paste or unnecessary forking.

@Ron2
Copy link
Member

Ron2 commented Jul 24, 2014

Hello,

Thank you for the kind words and I'm glad if ATF is useful to you.

You bring up a very good point. The request to expose private members is very common. We've even been asked before to make literally every private and internal member be either protected or public.

The problem for us is that once a method or property or class or (rarely) a field is public, then it is frozen and we are constrained by it when trying to add new features or to fix problems, because we are very reluctant to make breaking changes. We are free to change private members. So, we tend to err on the side of keeping things private because we can always make it public in the future, but that's a one-way conversion and we can essentially never make a public member be private again or change it.

We try to imagine all the ways that a class might need to be customized and we make an effort to add customization points (events and protected virtual methods, for example) but we can't anticipate all possible uses.

Would you be willing to list specific members that you would like to see be made protected or public? This is normally an easy change for us to do, and then you won't have to duplicate the code.

Thank you!

--Ron

@ColinCren
Copy link
Author

PropertyGridView's SelectProperty() I'd vote be made not internal.

I understand your argument regarding locking yourself in when making things more visible than private. I agree completely, especially with code deeper and deeper in the core of ATF. There are certain classes though that I'd expect to be more often inherited from than others like controls or services that are often more specific for tools. In those cases I'd rather see the properties protected instead of private, and if large scale changes are needed branch to a new class that you replace the old one with.

If users have to copy/paste to make changes to an existing control to make any changes then any concern about changing things out from underneath them is moot anyway.

Thanks for being so open to discussion.

  • Colin

@Ron2
Copy link
Member

Ron2 commented Aug 7, 2014

Good suggestion, Colin. Thank you. I made that one public. Do you have any other members that you think should be public?

--Ron

@ColinCren
Copy link
Author

In PropertyGrid.cs DescriptionControl and m_descriptionTextBox being private made it hard to make changes to the PropertyGrid class's PropertyGridView. I basically had to copy-paste the whole PropertyGrid to change a couple small behaviors in the PropertyGridView.

@Ron2
Copy link
Member

Ron2 commented Aug 11, 2014

Did you need access to the DescriptionControl's TextBrush, for a customized appearance? I could expose that. Or did you want to use your own bold-faced font? The description text can already be set. I don't see anything else in that private class that is worth customizing.

@ColinCren
Copy link
Author

Looking back at the changes I made I think I was just trying running into the fact that m_propertyGridView is just so core to how the PropertyGrid class works, it's touched everywhere, and the m_descriptionTextBox was just one of the first things I ran into where there was a direct interaction. I guess scratch my last remark.

@ColinCren
Copy link
Author

PropertyGridView.Pick might be another good candidate.

Ron2 added a commit that referenced this issue Aug 20, 2014
PropertyEditorControlContext: when editing a property, the property's
name is now included in the undo/redo command's name. #13
PropertyEditingCommands: made the undo/redo text include the property's
name. #13
PropertyGridView: made Pick() be public instead of private. #12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants