Skip to content
This repository has been archived by the owner on Oct 1, 2024. It is now read-only.

generator: Overhaul GInterface type naming approach #113

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

knocte
Copy link
Contributor

@knocte knocte commented Aug 5, 2014

In the past, for the GInterface Foo we used to have:
a) FooAdapter: the helper class to use GInterface implementations.
b) Foo: the interface with the methods from native side.
c) FooImplementor: the interface to inherit from .NET classes.

Learning about these special suffixes "Adapter" and "Implementor" was
a bit cumbersome and not very API-consumer friendly.

Some months ago we modified gtk-sharp's generator to follow the "I"
prefix convention for .NET interfaces, which leaves us as:

a) FooAdapter.
b) IFoo.
c) IFooImplementor.

This is good because now the name "Foo" is free, so we can rename
"FooAdapter" to "Foo", and there's one less thing to learn (so this
is one of the things this commit achieves).

The next step is, then, getting rid of the "Implementor" suffix, how?
Simply switching the way this is exposed, as:

b) IFoo -> IFooBase
c) IFooImplementor -> IFoo

This way, if one wants to create a .NET class that implements the
GInterface Foo, she simply needs to inherit from "IFoo". And when
consuming, from .NET, native classes that implement the Foo GInterface,
the interface they will implement will be IFooBase instead of IFoo
(but there's no real need to learn about this latter suffix, as by using
the API you will simply be exposed to it).

Discussed with and reviewed by: Stephan Sundermann <[email protected]>

@monoadmin
Copy link

Hello! I'm the build bot for the Mono project. I need approval from a Mono team member to build this pull request. A team member should reply with "approve" to approve a build of this pull request, "whitelist" to whitelist this and all future pull requests from this contributor, or "build" to explicitly request a build, even if one has already been done. Contributors can ignore this message.

@knocte
Copy link
Contributor Author

knocte commented Aug 5, 2014

This would prevent also some "head scratching" when people in the past inherited from IFoo and saw that their class was not working.

In the past, for the GInterface Foo we used to have:
a) FooAdapter: the helper class to use GInterface implementations.
b) Foo: the interface with the methods from native side.
c) FooImplementor: the interface to inherit from .NET classes.

Learning about these special suffixes "Adapter" and "Implementor" was
a bit cumbersome and not very API-consumer friendly.

Some months ago we modified gtk-sharp's generator to follow the "I"
prefix convention for .NET interfaces, which leaves us as:

a) FooAdapter.
b) IFoo.
c) IFooImplementor.

This is good because now the name "Foo" is free, so we can rename
"FooAdapter" to "Foo", and there's one less thing to learn (so this
is one of the things this commit achieves).

The next step is, then, getting rid of the "Implementor" suffix, how?
Simply switching the way this is exposed, as:

b) IFoo -> IFooBase
c) IFooImplementor -> IFoo

This way, if one wants to create a .NET class that implements the
GInterface Foo, she simply needs to inherit from "IFoo". And when
consuming, from .NET, native classes that implement the Foo GInterface,
the interface they will implement will be IFooBase instead of IFoo
(but there's no need to learn this because by using the API you will
simply be exposed to it).

Reviewed-by: Stephan Sundermann <[email protected]>
We also rename "Implementor" property of adapters to the
slightly more convoluted "VirtualImplementor", but the
reason is that Atk.Implementor.Implementor wouldn't be a
valid property because its name would match with the class
name.
@knocte knocte changed the title generator: Ginterface approach overhaul generator: Overhaul GInterface type naming approach Aug 5, 2014
@shana
Copy link
Member

shana commented Aug 6, 2014

Love this, having IFoo and Foo is great! Not sure about the IFooBase naming, though... what's it the Base of? Base is usually used to mark abstract or base classes that provide basic functionality, not really interfaces, and it's a really generic name. Maybe IFooNative?

@knocte
Copy link
Contributor Author

knocte commented Aug 6, 2014

Most of the time, IFooBase is a super-set of methods from IFoo, containing the virtual methods that you can override (via inheritting from IFoo in .NET, or by privately overriding in C in GObject), plus the methods that the GInterface provides with a "base" implementation (like a Mixin) which AFAIK cannot be overriden. That's why I used the word "Base", but certainly any other suggestions are welcome (Native suffix is interesting, maybe better indeed, although I have the feeling that someone will come here soon with an even better suggestion :) ).

@riccioclista
Copy link

I agree that it's not a very intuitive API and that it requires reading some docs and/or code samples before being able to use it. But I'm not sure, if this can be solved by changing the naming, because in my opinion inheriting from any IFoo (current naming) is comparatively complicated - and the current naming reflects that (it doesn't make it worse). I think, the easiest way to improve the current system is to improve documentation about it.
In my opinion the names FooAdapter and IFooImplementor are actually well chosen, because FooAdapter is something, you have to plug in, for you GInterface implementation to work, thus the name "Adapter". IFooImplementor represents the portion of IFoo that you "have to implement" to make it work. (The only names that may better fit, were "IFooImplementation" or "IFooImpl", because the class that inherits from that interface is not an "implementor" (that would be the programmer), but an implementation.)
IFoo is the main component of the three and it represents the thing that we actually are consuming in other classes of the library. For example, a Gtk.TreeView has an ITreeModel as its Model property. If you'd call the type ITreeModelBase or ITreeModelNative, it wouldn't fit as good as ITreeModel. If we'd look at Gtk as a domain model, renaming ITreeModel to e.g. ITreeModelNative would be like suffixing an entity name with a technical term, as if it were some technical implementation detail, not The actual thing. Furthermore, if someone consults C API docs or other language binding docs to learn about Gtk.TreeView.Model, he or she would be confused by the different type names.

About the name suggestions:
IFooBase is a misleading name in my opinion, because it suggests there is an inheritance relationship between IFoo and IFooBase, which actually isn't. If there were some kind of conceptual inheritance between those two, it would be the other way around, because IFooBase implements a super-set of methods from IFoo, which is indicative for a subclass not a base class.
IFooNative is better, because it doesn't sound like inheritance. However, it still suffers from the problems described in above.

@monoadmin
Copy link

Hello! I'm the build bot for the Mono project. I need approval from a Mono team member to build this pull request. A team member should reply with "approve" to approve a build of this pull request, "whitelist" to whitelist this and all future pull requests from this contributor, or "build" to explicitly request a build, even if one has already been done. Contributors can ignore this message.

@bl8
Copy link
Contributor

bl8 commented Sep 21, 2014

Thanks everyone for this proposal and for your comments.

I'd tend to be of the same opinion as Antonius, so in favor of keeping things as-is.

  • IFoo is the "naked", simple interface that you use just to work with the thing.
  • IFooImplementor represents the contract for implementors of IFoo

My guess would be that IFoo is currently used more often than IFooImplementor, so better keep the more frequent form as straightforward as possible.

Furthermore, after we apply this, I think it would be more confusing for people porting their GTK 2 code: if they were using TreeModel with GTK 2, they should now change it to ITreeModel, even if a TreeModel class still exists.

So I think the pain inflicted on existing code is not justified by the potential gains from this naming change.

I'm keeping this open for now, maybe I can change my mind.

@knocte
Copy link
Contributor Author

knocte commented Sep 22, 2014

Thanks for keeping it open, because I have some more ideas in my head that I would like to explain carefully in the coming days/weeks, which maybe are better suited.

@knocte
Copy link
Contributor Author

knocte commented May 25, 2015

After a bit less than a year, I've come back to this PR :)

I have two questions:

  1. Would a smaller PR, that just renames FooAdapter to Foo, be accepted? I think it has value on its own.

  2. I like Antonious' suggestion about renaming "Implementor" to "Implementation". Maybe the best approach here is let IFoo interface not actually exist, and have two different suffixes (so when discoverying the API via intellisense, it's easy to distinguish one from another). So how about IFooImplementor -> IFooImplementation, and IFoo -> IFooConsumable?

@riccioclista
Copy link

@knocte:
ad 1) In my opinion, this would be a great idea, if we also were to change the adapter class from a regular class to an abstract one. The reason is that it would force the programmer to create a cleaner API. The API used by users of the subclass would then only consist of the consumable interface IFoo and the subclass MyFoo (for instantiation). With the current model the subclass API user has to deal with 3 classes: IFoo, FooAdapter and MyFooImplementor, which is not that straight forward to understand because you have to know about the mechanics of GInterface implementation.
To illustrate the above, if the adapter class (Foo) were an abstract class, the subclass programmer would have to do this:

public class MyFoo : Foo
{
    public MyFoo ()
        : base (new MyFooImplementor ())
    {
    }
}

public class MyFooImplementor : IFooImplementor
{
}

And the subclass consumer would use it like this:

IFoo myFoo = new MyFoo ();

With the current model the subclass consumer writes code like this:

var myFoo = new FooAdapter (new MyFooImplementor ());

ad 2) As I said before, it certainly would be possible to use "IFooImplementation" instead of "IFooImplementor". I'm just not sure whether the gained value is enough to merit the change.
About "IFooConsumable": I don't think this is a good idea, because subclass users don't have to know about the consumable interface concept; they just want to use IFoo.

@riccioclista
Copy link

With all that I wrote in the previous comment, I still think its best to leave things naming-wise as they are, or to change the way GInterfaces are exposed entirely. If it is possible (from a code generation point of view), the easiest to use C# model to expose GInterfaces would be:

public interface IFoo
{
}

public abstract class Foo : IFoo
{
}

with Foo being the adapter and implementor base class in one.
There are 2 things to consider:

  • The abstract Foo class exposes the implementable interface portion as abstract members
  • Multiple inheritance is still supported via the IFoo interface

This, however, is totally outside the scope of this PR.

Base automatically changed from master to main March 9, 2021 14:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants