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

Fixifaces #91

Closed
wants to merge 20 commits into from
Closed

Fixifaces #91

wants to merge 20 commits into from

Conversation

olabini
Copy link
Contributor

@olabini olabini commented Feb 11, 2016

This PR adds interfaces for all functionality and changes everything to work with interfaces. It also puts all constants into a separate package as well. The goal is to make it possible to create a completely interface based implementation so that you can then inject the GTK/GDK/GLib/Pango/Cairo implementation in one single place, making it possible to write well tested implementations of gui's.

This fixes #90.

Important - this PR will break clients. For most clients it shouldn't be very bad to update, and I firmly believe this will allow users to build better applications using gotk3.

Thoughts?

@andre-hub
Copy link
Member

can we move the interfaces to the same namespace?
iface as separate import space is too much now.

i hope the same namespace break not the client dependency.

@olabini
Copy link
Contributor Author

olabini commented Feb 11, 2016

Well, the whole idea of having them in separate interfaces is to be able to write tests without dragging in the whole GTK C libraries. Having it in the same package would kinda defeat the purpose.

@yamnikov-oleg
Copy link
Member

May be, iface packages could have more obvious names, something like igtk, igdk and so on?

Too, in terms of simplifying API, "real" types might not be needed now to be exported, since they have interface equivalents. Having two Widget types with same behaviour is confusing.

Therefore every package's logic would be divided into two actual packages: interfaced and real (igtk for class interfaces and gtk for their realization etc.), which in my opinion is more easy-to-learn scheme than just having separate interface package.

Just thinking.

PS. When this PR gets pulled, how about tagging last "oldschool" commit to let people reset their gotk3 version? And leaving noticable reset instruction in Readme? To ease people's transition.

@olabini
Copy link
Contributor Author

olabini commented Feb 11, 2016

Sure. Currently the organization is simply this:
/gtk - contains the real implementations of everything
/gtk/iface - contains the data and interfaces for everything.

Changing it to
/gtk
/igtk

would be easy.

Not sure what you mean with real types vs interface types - we need to have implementations of these things, and it's helpful to make it possible to use them from other packages.

Tagging is definitely a good idea.

@yamnikov-oleg
Copy link
Member

I mean, must gtk.Widget be exported, since one has iface.Widget + functions to gather it's real implementation? It's visibility is excessive, I think. How would someone ever obtain it without casting an interface?

Same refers to other types.

I know some of them must be visible between packages, like glib structs are necessary for embedding into gtk structs... But not all of them, right? So, API might be cleaned of some types, whose relation to package mechanics is not transparent.

@olabini
Copy link
Contributor Author

olabini commented Feb 11, 2016

Actually, maybe one alternative would be to not merge this at all, but create a new repository with this implementation. Then the old one will still exist, albeit with no updates. Thoughts?

@olabini
Copy link
Contributor Author

olabini commented Feb 11, 2016

It is definitely possible we can hide some of the real implementations inside of the packages. I don't think we need to do it in this PR though - we can do that as we go along.

@velovix
Copy link

velovix commented Feb 11, 2016

I agree with @yamnikov-oleg, I don't see any benefit to exporting both the real implementations and the interfaces. Why should anybody use the real implementation version if the interface version exists? Surely the only reason why anybody would do that is by mistake, so far as I can tell. I think it's okay that this would be a breaking change, so long as the old version of this library is tagged. Anybody that uses unversioned dependencies has to do so with the knowledge that their build might break.

@axel-freesp
Copy link

Hi,

my opinion to this topic is: the Go way to provide a library is through an interface. In general, I appreciate this approach to hide the wrappers behind an interface.

We need to take care, however. Gtk sometimes also comes up with its own interfaces (e.g. ListModel, TreeModel), and having interfaces behind interfaces makes not too much sense, IMHO.

WRT to name spaces, I would give it a try to leave the interface unmodified (namespaces gtk, gdk, glib and so on), but modify the implementation namespace (e.g. gtk_impl, gdk_impl and so on). That way, existing applications do have a chance to be able to link against the interfaces without (large) modifications). In addition, the implementations should be hidden (made opaque by renaming to lower case).

At the end, there should be a common way to generate new objects. This cannot be done in the interfaces, hence the implementations should be visible to some extend. Has anyone a good idea for that?

Cheers
Axel

@olabini
Copy link
Contributor Author

olabini commented Feb 13, 2016

Axel, we can't make all implementations hidden, since there are interdependencies between Glib, Gtk, Pango, Cairo and Gdk. However, we can hide most of them.

When it comes to the creation, this PR exposes an interface, for example gtk/iface/gtk.go called Gtk - this interface is implemented in gtk/real_gtk.go - thus making it so the only place that needs to refer the implementation is the place that uses the RealGtk implementation.

@axel-freesp
Copy link

Olabini,

would you give me an example of such a dependency that cannot be managed using the interfaces?

BTW: I had a brief look at your interface generator https://github.com/twstrike/gotk-gen which promises to be very useful in this context.

Cheers
Axel

@olabini
Copy link
Contributor Author

olabini commented Feb 15, 2016

As a few simple examples, most of the types in gtk, gdk, pango and cairo inherit from glib.Object. In some cases there are other inheritances - for example, GtkApplication inherits from GApplication (GApplication is in glib.)

@axel-freesp
Copy link

Sorry, but I can't get it why inheritance shall prevent from using an interface type. What is wrong with this view:
On the interface level:

  • glib.Object is an interface
  • gtk.Something is an interface and inherits glib.Object by interface composition
type Something interface {
    glib.Object
    ...
}

so all glib.Object methods can be applied to a gtk.Something instance

On the implementation level:

  • glib.ObjectImpl is an implementation of glib.Object
  • gtk.something is an implementation of gtk.Something and inherits glib.ObjectImpl by type composition
type something struct {
    glib.ObjectImpl
    ...
}

I want to emphasize that the gtk implementation may know about (include) glib implementation, but the gtk interface needs not. The Go language rules do not permit to write glib.object for the implementation since the type is needed outside its module. But glib.ObjectImpl needs not be visible in any interface.
Are there other opinions?

Cheers
Axel

@olabini
Copy link
Contributor Author

olabini commented Feb 16, 2016

Axel, I'm not sure what you're arguing against right now. You asked earlier about cases where the implementation structs needed to be exposed. I give you an example, then you give a counter example that requires glib.Object to be exposed, just like I said. I'm very confused - what you're describing is exactly what the PR already does.

@axel-freesp
Copy link

What I want to say is that no implementation structs need to be exposed to users of the interfaces.

Maybe I was misunderstanding your statements - if so: sorry for the noise.

Cheers
Axel

@olabini
Copy link
Contributor Author

olabini commented Feb 16, 2016

But in your own example you have this:
type something struct {
glib.ObjectImpl
...
}

where glib.ObjectImpl have to be an exposed implementation struct. I'm still confused about what you're trying to say.

@axel-freesp
Copy link

type something struct { glib.ObjectImpl ... }
is written in the implementation of gtk, not in the interfaces.

I mean that we can design the interfaces in such a way that no implementation types get exposed through them. If we need to expose an implementation type to a depending module implementation, this should be done through a different interface. E.g. glib.Object is defined as an interface type in glib interface, while glib.ObjectImpl is defined as a struct in the glib implementation. The interface of gtk needs not know about glib.ObjectImpl, only the implementation of gtk does. Hence, we need something like a private interface between the implementations that are not visible by the (public) interfaces.

Clear now?
Axel

@axel-freesp
Copy link

@olabini , I made a figure for better discussing: https://github.com/axel-freesp/gotk3-examples/blob/master/gtk-interfaces.png

Cheers
Axel

@axel-freesp
Copy link

Hi Ola,

I'm sorry that our discussion has been so confusing. On the other hand, the picture becomes clear while talking about these questions. Let me describe my view from a different perspective.

IMHO, in principle, the existing implementation of gtk and its depending modules need no modifications if you want to install an interface for users only. There is no need that the implementation of gtk uses the user interfaces of gdk or cairo to satisfy the dependencies. That means, adding user interfaces would simply add some files to the repository. (Renaming of types to manage visibility of their details can be done later.) The only need for modifications would be with the gtk-examples, to reflect using the new interfaces instead of the implementation directly.

Instead, your PR makes massive changes to existing code. The changes are so many that the Files Changed view of this page fails. I doubt that this is all needed, but I'm not against being convinced, that's why I started this discussion. In addition, travis testing breaks, which needs some more discussion (do we maintain deprecated code, or is it time to remove it?)

Can you try to simply add the interface files, without other modifications?

Cheers
Axel

@olabini
Copy link
Contributor Author

olabini commented Feb 17, 2016

Hi,

No, we can not add the interface files without modifications - that would be useless. The reason is that the existing files need their signatures changed in order to match the interfaces. For example, if something takes a *Window as an argument, we need to change that to take an iface.Window instead - otherwise there is no real point in doing this.

I'm still trying to figure out what you mean with the private implementations. You know that Go only has one type of visibility, based on the initial character? It's not possible to create something like ObjectImpl in your diagram that is only visible from some places but not all places.

@olabini
Copy link
Contributor Author

olabini commented Feb 17, 2016

When it comes to fixing the build, I will do that once I have sorted out a few other issues, including making things private etc.

@andre-hub
Copy link
Member

So currently I do not want to merge that way.
In addition, I understand axel-freeSP arguments as well as of olabini.
First, I think we should agree on a proper procedure before something merged.

As in the meantime the changes of this PR already claimed many commits, I would appreciate that the PR is shrinking to only commit :-).

The diagram from alex is nice, but really brings us wider?
The last changes from olabini is better, but a package with the name impl?

the last problem, the changes don't break the travis checks :-).

@axel-freesp
Copy link

the existing files need their signatures changed in order to match the interfaces

Ok. I was wrong with this point.

It's not possible to create something like ObjectImpl in your diagram that is only visible from some places but not all places.

I see it different. The visibility (using upper or lower case first symbol letter) of Go symbols is per-namespace. Of course, nobody can prevent you from using a namespace and its exported symbols. But it makes a difference if (1) a symbol is exported/available using a given interface or (2) that symbol is only visible if you import an otherwise not needed namespace. (And I guess that's the reason why you want to have interface and implementation in different namesaces.) That way, the user is prevented from the unintended use of impl types.

Cheers
Axel

@axel-freesp
Copy link

Ola,

another point: do you want to add compile-time checks that the implementation types actually realize the intended interfaces? I find it very useful to add a line, e.g.
var _ cairo.Context = (*Context)(nil)
to have a compiler proof that *Context implements cairo.Context.

See also https://golang.org/doc/faq#guarantee_satisfies_interface

Cheers
Axel

@olabini
Copy link
Contributor Author

olabini commented Feb 17, 2016

Axel, ok - I understand what you mean. But the pull request already does that.
Also, when it comes to compile time checks, the pull request already has that as well.

@olabini
Copy link
Contributor Author

olabini commented Feb 17, 2016

Andre, I'll fix the build, and once I've done that I'll sqush everything and submit a new PR.

Do you have a better alternative than impl for the package name?

@olabini
Copy link
Contributor Author

olabini commented Feb 17, 2016

I've sent a new PR with some of the commits squashed.

@andre-hub andre-hub closed this Feb 22, 2016
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

Successfully merging this pull request may close these issues.

Make everything interface based to facilitate testing
5 participants