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

Classes can implement multiple interfaces #501

Merged
merged 9 commits into from
Jul 21, 2016

Conversation

petekanev
Copy link
Contributor

@petekanev petekanev commented Jun 27, 2016

Description

With the following changes, and those in the android-static-binding-generator repo, full support to implementing interfaces will be enabled. The supported syntax is as follows:

Suppose we have the following interfaces and classes:

com.a.b.C.Interface1 has methods interface1Method1(Object, int), interface1Method2(int, Object), interface1Method3(int)
com.c.d.D.C.Interface2 has methods interface2Method1(), interface2Method2(boolean)
com.a.b.ClassToExtend is the class for extension with methods 'ClassToExtendMethod1()', and 'ClassToExtendMethod2()'

Javascript

com.a.b.ClassToExtend.extend({
  interfaces: [com.a.b.C.Interface1, com.c.d.D.C.Interface2],

  ClassToExtendMethod1: function() { // overrides method of class ClassToExtend
  },

  interface1Method1: function(p1, p2) { ... }, // implement methods from Interface1
  interface1Method2: function(p1, p2) { ... },
  interface1Method3: function(p1) { ... },

  interface2Method1: function() { ... }, // implement methods from Interface2
  interface2Method2: function(p1) { ... }
}

Typescript

@Interfaces([com.a.b.C.Interface1, com.c.d.D.C.Interface2])
class MyExtendedClass extends com.a.b.ClassToExtend {
  ClassToExtendMethod1() { // overrides method of class ClassToExtend
  }

  interface1Method1(p1, p2) { ... } // implement methods from Interface1
  interface1Method2(p1, p2) { ... }
  interface1Method3(p1) { ... }

  interface2Method1() { ... } // implement methods from Interface2
  interface2Method2(p1) { ... }
}

The resulting extended class will be implementing both interfaces declared in the interfaces array.

Notable changes in files

Does your commit message include the wording below to reference a specific issue in this repo?

Addresses issue #442

Related Pull Requests

repo: android-static-binding-generator, PR: 10 - contains the SBG logic, while the changes to the runtime binding generator in this repository mimic it.

Does your pull request have [unit tests]?

No unit tests as the functionality is difficult to unit test with the current test structure. A sample repo demonstrates the new functionality by extending object and implementing multiple interfaces. To further verify that the correct class is generated you can fetch the .dex from the application folder, d2j it and explore its contents.

ping: @NativeScript/android-runtime

@ns-bot
Copy link

ns-bot commented Jun 27, 2016

💚

@Plamen5kov Plamen5kov changed the title Classes can implemented multiple interfaces Classes can implement multiple interfaces Jun 28, 2016
@Plamen5kov
Copy link
Contributor

👍

@blagoev
Copy link
Contributor

blagoev commented Jul 1, 2016

@Pip3r4o can we rename "interfaces" to "implements"

com.a.b.ClassToExtend.extend({
implements: [com.a.b.C.Interface1, com.c.d.D.C.Interface2],

ClassToExtendMethod1: function() { // overrides method of class ClassToExtend
},

interface1Method1: function(p1, p2) { ... }, // implement methods from Interface1
interface1Method2: function(p1, p2) { ... },
interface1Method3: function(p1) { ... },

interface2Method1: function() { ... }, // implement methods from Interface2
interface2Method2: function(p1) { ... }
}

or better we can move this to the extend function arguments as such
com.a.b.ClassToExtend.extend({
ClassToExtendMethod1: function() { // overrides method of class ClassToExtend
},

interface1Method1: function(p1, p2) { ... }, // implement methods from Interface1
interface1Method2: function(p1, p2) { ... },
interface1Method3: function(p1) { ... },

interface2Method1: function() { ... }, // implement methods from Interface2
interface2Method2: function(p1) { ... }
}, [com.a.b.C.Interface1, com.c.d.D.C.Interface2])

Since this is leaked to runtime functions like getOwnProperties etc

@NativeScript/android-runtime Share you comments on this

@ns-bot
Copy link

ns-bot commented Jul 1, 2016

💚

@vchimev
Copy link

vchimev commented Jul 1, 2016

run ci

@ns-bot
Copy link

ns-bot commented Jul 1, 2016

💔

@slavchev
Copy link

slavchev commented Jul 1, 2016

This makes sense as Java doesn't support explicit interface implementations (e.g. resolve method collision).

@blagoev
Copy link
Contributor

blagoev commented Jul 1, 2016

@slavchev which one ?

@petekanev
Copy link
Contributor Author

@blagoev, I am up for using 'implements' so long as we are okay having 2
distinct words for the same thing between the 2 runtimes as iOS currently
uses 'interfaces'

On Friday, 1 July 2016, blagoev notifications@github.com wrote:

@slavchev https://github.com/slavchev which one ?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#501 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AJ-u2r-FnOfTVjPl9ESZAZINCzvziEouks5qRMZ2gaJpZM4I_YRy
.

Sincerely,
Pete.

@atanasovg
Copy link
Contributor

My vote goes for consistency with the iOS Runtime. The closer API-wise both Runtimes are, the better.

@blagoev
Copy link
Contributor

blagoev commented Jul 1, 2016

ok thanks

@Pip3r4o ignore this suggestion

@slavchev
Copy link

slavchev commented Jul 1, 2016

I was referring to avoid interface method collision. As for the syntax, I cannot state some preference or opinion as long we avoid interface method collision.

@atanasovg
Copy link
Contributor

Btw, @Pip3r4o, it is [protocols] in iOS - in other words what's natural for the language. With that in mind - what @blagoev suggests seems more natural for Java - implements

@petekanev
Copy link
Contributor Author

Must have remembered it wrong. Then yes, @blagoev's suggestion makes sense
too

On Friday, 1 July 2016, Georgi Atanasov notifications@github.com wrote:

Btw, @Pip3r4o https://github.com/Pip3r4o, it is [protocols] in iOS
http://docs.nativescript.org/runtimes/ios/how-to/ObjC-Subclassing#calling-base-methods-Protocol


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#501 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AJ-u2rxsGyZdZAc3VeRL2TJfzMHVF8_Eks5qRMxKgaJpZM4I_YRy
.

Sincerely,
Pete.

@slavchev
Copy link

slavchev commented Jul 1, 2016

In past we had some concerns about extends which is a keyword in JavaScript. I didn't fully understand them but I would like to point that implements is also a (reserved) keyword.

@blagoev
Copy link
Contributor

blagoev commented Jul 1, 2016

One note on the suggestion:
How do we support this in typescript.
Our current approach of analyzing javascript code and __extend patching is working since it redirects the call the to appropriate A.B.C.extend() call which is the {N} JavaScript syntax.

If we keep the interfaces member as part of the implementation object it means one should write in TypeScript something like this

class MyActivity extends Activity implements CallbackHandler
{
interfaces: []
.... or this
static interfaces = [CallbackHandler];
public myMethod()
}

which results in JavaScript
MyActivity.interfaces = [CallbackHandler]; when static
or
this.interfaces = [CallbackHandler] when instance member

In either case our current approach will not work

If we move this as extends argument it will be possible to support decorators for the interfaces in Typescript like so

@interface(CallbackHandler)
class MyActivity implements CallbackHandler
{
}

or there is some other idea to support the proposed syntax from TypeScript

@blagoev
Copy link
Contributor

blagoev commented Jul 1, 2016

Ultimately my thinking is to start analyzing TypeScript code for extends (especially since there is public TS compiler API available) then we will not have this problem of supporting TypeScript extends and implements through our JavaScript syntax and will remove any redundant declarations in the form of decorators and other hacks we are currently doing

@blagoev
Copy link
Contributor

blagoev commented Jul 1, 2016

@slavchev My suggestion was to be on par with the A.B.C.extend so "implement" is the correct word. If we decide to rename "interfaces"

@blagoev
Copy link
Contributor

blagoev commented Jul 1, 2016

Ok the other idea is to handle this using decorators that will inject the interfaces member on the implementation object before calling extend so this will work out of the box

@interface(CallbackHandler)
class MyActivity implements CallbackHandler
{
}

@petekanev
Copy link
Contributor Author

Using a decorator sounds like a good idea and should be easy enough to implement in the runtime binding generator, and shouldn't take a lot of effort to write in the SBG either.

@Plamen5kov
Copy link
Contributor

run CI

@dtopuzov
Copy link
Contributor

dtopuzov commented Jul 4, 2016

run ci

@ns-bot
Copy link

ns-bot commented Jul 4, 2016

💔

@petekanev
Copy link
Contributor Author

run ci

@ns-bot
Copy link

ns-bot commented Jul 4, 2016

💚

@petekanev petekanev force-pushed the pete/feature-implement-interfaces branch from 87e579e to 68a7155 Compare July 7, 2016 07:46
@ns-bot
Copy link

ns-bot commented Jul 7, 2016

💚

@petekanev
Copy link
Contributor Author

I have since updated the static binding generator to support @Interfaces([a.b.C]) decorator syntax in typescript, this is being tested for the runtime binding generator and will soon be pushed. I stuck with interfaces, because implements is a keyword. The property/decorator name can be discussed further, and changed, if deemed necessary.

@slavchev
Copy link

slavchev commented Jul 8, 2016

👍

1 similar comment
@Plamen5kov
Copy link
Contributor

👍

@ns-bot
Copy link

ns-bot commented Jul 8, 2016

💔

@ns-bot
Copy link

ns-bot commented Jul 8, 2016

💚

@petekanev petekanev force-pushed the pete/feature-implement-interfaces branch from e5519f0 to 0f8bc97 Compare July 11, 2016 06:58
@ns-bot
Copy link

ns-bot commented Jul 11, 2016

💚

@petekanev petekanev force-pushed the pete/feature-implement-interfaces branch from 0f8bc97 to ead0e8b Compare July 21, 2016 09:08
@ns-bot
Copy link

ns-bot commented Jul 21, 2016

💔

@ns-bot
Copy link

ns-bot commented Jul 21, 2016

💚

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants