search

SonyWWS / ATF

Authoring Tools Framework (ATF) is a set of C#/.NET components for making tools on Windows. ATF has been in continuous development in Sony Computer Entertainment's (SCE) Worldwide Studios central tools group since early 2005. ATF has been used by most SCE first party studios to make many custom tools such as Naughty Dog’s level editor and shader editor for The Last of Us, Guerrilla Games’ sequence editor for Killzone games (including the Killzone: Shadow Fall PS4 launch title), an animation blending tool at Santa Monica Studio, a level editor at Bend Studio, a visual state machine editor for Quantic Dream, sound editing tools, and many others.
Apache License 2.0
1.89k stars 262 forks source link

Easier Inheritance #12

Closed ColinCren closed 10 years ago

ColinCren commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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.

Ron2 commented 10 years ago

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

--Ron

ColinCren commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

PropertyGridView.Pick might be another good candidate.