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

GVRKit plugin metadata generation issue #877

Closed
NathanaelA opened this issue Feb 9, 2018 · 3 comments
Closed

GVRKit plugin metadata generation issue #877

NathanaelA opened this issue Feb 9, 2018 · 3 comments
Assignees
Milestone

Comments

@NathanaelA
Copy link
Contributor

I have a GVRKit plugin that I have been developing for a project, and the runtime metadata is incorrect, and I need to find a solution to get around this in NativeScript.

In some ways slightly related to #826 (Like him I've got one that is working).
Partially related to this issue #710...

Error is:
Error: Actual arguments count: "0". Expected: "1".

      - Name:            handleTrigger
        JsName:          handleTrigger
        Filename:        GVRRenderer.h
        Module:          
          FullName:        GVRKit.GVRRenderer
          IsPartOfFramework: true
          IsSystemModule:  false
          Libraries:       
        Flags:           [ IsIosAppExtensionAvailable ]
        Type:            Method
        Signature:       
          - Type:            Bool

      - Name:            'handleTrigger:'
        JsName:          handleTrigger
        Filename:        GVRRenderer.h
        Module:          
          FullName:        GVRKit.GVRRenderer
          IsPartOfFramework: true
          IsSystemModule:  false
          Libraries:       
        Flags:           [ IsIosAppExtensionAvailable ]
        Type:            Method
        Signature:       
          - Type:            Bool
          - Type:            Interface
            JsName:          GVRHeadPose
            WithProtocols:   

Do you see that the metadata JS name is the SAME, but the core name is different. One has a prototype of:
bool handleTrigger()
and the other is
bool handleTrigger(GVRHeadPose *headpose)

I really want to call the first option. I can't seem to because NativeScript has decided the only valid one is the second one. ;-(

Solutions?

@ulvesked
Copy link

ulvesked commented Oct 9, 2018

Hi, I have the same issues with StarIO-framework.

  - Name:            'appendLineFeed:'
    JsName:          appendLineFeed
    Filename:        ~/nativescript-star-io/platforms/ios/StarIO_Extension.framework/Headers/ISCBBuilder.h
    Module:          
      FullName:        StarIO_Extension.ISCBBuilder
      IsPartOfFramework: true
      IsSystemModule:  false
      Libraries:       
    Flags:           [ IsIosAppExtensionAvailable ]
    Type:            Method
    Signature:       
      - Type:            Void
      - Type:            Int
  - Name:            appendLineFeed
    JsName:          appendLineFeed
    Filename:        ~/nativescript-star-io/platforms/ios/StarIO_Extension.framework/Headers/ISCBBuilder.h
    Module:          
      FullName:        StarIO_Extension.ISCBBuilder
      IsPartOfFramework: true
      IsSystemModule:  false
      Libraries:       
    Flags:           [ IsIosAppExtensionAvailable ]
    Type:            Method
    Signature:       
      - Type:            Void

This is not the only place I have discovered it. It is the same with appendMultiple:height: vs appendMultipleHeight: that both is recognised as appendMultipleHeight in Nativescript.

Is there a plan to fix this?

@ulvesked
Copy link

ulvesked commented Oct 9, 2018

The issue is in MetaFactory::populateIdentificationFields which converts the objectiveC signature to a javascript compatible name. It splits by : and joins with uppercase first letters.

https://github.com/NativeScript/ios-metadata-generator/blob/aa5c18df4e9d73d186864530420d8db1e5ef1834/src/Meta/MetaFactory.cpp#L504

    const clang::ObjCMethodDecl* method = clang::dyn_cast<clang::ObjCMethodDecl>(&decl);
    std::string selector = method->getSelector().getAsString();
    vector<string> tokens;
    StringUtils::split(selector, ':', std::back_inserter(tokens));
    for (vector<string>::size_type i = 1; i < tokens.size(); ++i) {
        tokens[i][0] = toupper(tokens[i][0]);
        tokens[0] += tokens[i];
    }
    meta.jsName = tokens[0];

The problem is when there are two similar methods. I see three ways to solve this:

  1. Add a separator rather than just concatenating the name. HOWEVER this will break all existing apps and plugins.

  2. Detect naming conflicts and add some suffix to the conflicting method. This should have no effect on existing apps or plugins since they only got one of the methods before. There is a risk that the "wrong" method gets the suffix which may break some apps.

  3. Make "overloaded" javascript functions that detect number of arguments and call the correct ObjC-method. See below. This will be the most unobtrusive for existing apps and plugins but will probably require more work in the runtime.

An example for alt. 3 above:
SDK contains two ObjectiveC methods:

- (void)appendLineFeed;
- (void)appendLineFeed:(NSInteger)line;

The ObjC signatures are

'appendLineFeed'
'appendLineFeed:'

They only differ by a colon (since the first argument is not included in the signature name)

This could generate "overloaded" typescript definitions like

function appendLineFeed(): void;
function appendLineFeed(line: number): void;

and the implementation something like

function appendLineFeed() {
    if (arguments.length == 0) {
        // Call objc 'appendLineFeed'
    }
    else {
        // Call objc 'appendLineFeed:'
    }
}

I have not checked how the actual implementation of js->objc functions work but I believe something like this could be doable and not very intrusive/breaking.

A similar example with appendMultiple:

ObjectiveC methods:

- (void)appendMultiple:(NSInteger)width height:(NSInteger)height;
- (void)appendMultipleHeight:(NSInteger)height;

The ObjC signatures are

'appendMultiple:height:'
'appendMultipleHeight:'

typescript overloads

function appendMultipleHeight(width: number, height: number): void;
function appendMultipleHeight(height: number): void;

and the implementation something like

function appendMultipleHeight() {
    if (arguments.length == 2) {
        // Call objc  'appendMultiple:height:'
    }
    else {
        // Call objc 'appendMultipleHeight:'
    }
}

@mbektchiev
Copy link
Contributor

Hi @ulvesked. Thank you for the ideas presented. I like approach number 3 the most, but it has a small drawback - there could still be cases where the number of arguments will be the same. Consider the following hypothetical example:

- (void)appendMultiple:(NSInteger)width height:(NSInteger)height;
- (void)append:(NSInteger)height multipleHeight: (NSInteger)multipleHeight;

Both methods would end up with the same name (appendMultipleHeight) and the same number of arguments (two). That's why I think that besides the logic that you proposed, we could make the metadata generator fallback to appending a number suffix to the JS name of the method whenever it detects such a collision, which I suppose will be an extremely rare situation.

We will try to fix the issue for the upcoming 5.0 release of {N}.

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

No branches or pull requests

6 participants