Adding the ability to declare properties in interfaces. #287

BrassLion · 2016-11-07T20:10:17Z

This pull request allows interfaces to define properties with Djinni-supported data types that are implemented as C++ variables and accessed from Java, through auto-generated getter and setter methods, and Obj-C, through auto-generated @property methods.

Properties are defined in IDL files identically to Djinni record elements. Platform-specific getters and setters are auto-generated. Developers then must implement the generated C++ getters and setters and the backing variable. Properties may be preceded with a readonly keyword, in which case only the platform-specific getter is generated.

This pull request is mostly syntactic sugar and can be implemented by manually defining the getter and setter methods in the IDL file, but provides a more platform-native way of accessing member variables, particularly through the use of @property in Obj-C.

… only the getter in this instance.

smarx · 2016-11-07T20:10:22Z

Automated message from Dropbox CLA bot

@BrassLion, thanks for the pull request! It looks like you haven't yet signed the Dropbox CLA. Please sign it here.

smarx · 2016-11-07T20:42:33Z

Automated message from Dropbox CLA bot

@BrassLion, thanks for signing the CLA!

artwyman

Thanks for taking on an oft-requested feature. This looks great overall. I've made some suggestions inline. One which I made in the CppGenerator really deserves to be a top-level design comment:

I'd love to see more code reuse between method generation and property generation, to avoid duplicate code which might not receive the same bugfixes over time. In all generators other than ObjC, properties are generated in the same way as methods. I can see two potential approaches to that:

In the generator, one property at at a time, transform the Property into two Method objects, then invoke the normal method generation code.
At the top level, perform a transformation on the AST to produce an alternative AST without properties, then feed that AST into all generators except for ObjC (ideally with some sort of global "supportsProperties" query which can be extended later.

Option 1 seems most straightforward, but I thought Option 2 might be an interesting step in the direction of supporting other sorts of syntactic-sugar at the AST level.

artwyman · 2016-12-28T03:51:11Z

test-suite/djinni/common.djinni

@@ -14,4 +15,4 @@
 @import "single_language_interfaces.djinni"
 @import "extended_record.djinni"
 @import "varnames.djinni"
-@import "relative_paths.djinni"
+@import "relative_paths.djinni"


nit: newline at end of file

artwyman · 2016-12-28T03:51:23Z

test-suite/djinni/properties.djinni

+    readonly read_only_bool: bool;
+
+    static create_new(): properties_test_helper;
+}


nit: newline at end of file

artwyman · 2016-12-28T03:52:17Z

test-suite/djinni/properties.djinni

+    test_string: string;
+    test_list: list<i32>;
+    readonly read_only_bool: bool;
+


The test case should include some methods as well as properties, to test that they can be freely mixed. I'd suggest interspersing them in the file too, rather than grouping them together.

artwyman · 2016-12-28T03:53:25Z

test-suite/generated-src/cpp/properties_test_helper.hpp

+
+    static std::shared_ptr<PropertiesTestHelper> create_new();
+
+    virtual int32_t get_item() = 0;


Getters should be const methods in C++, unless you think there's an issue with that I'm not considering.

artwyman · 2016-12-28T03:54:00Z

test-suite/djinni/properties.djinni

@@ -0,0 +1,9 @@
+properties_test_helper = interface +c {


Can you include a +o +j test case as well?

artwyman · 2016-12-28T03:58:09Z

test-suite/handwritten-src/cpp/properties_test_helper_impl.hpp

+
+private:
+
+    int32_t m_item;


This does seem to just beg for there to be a way to auto-generate the members and getter/setter implementations too, at least optionally in the style of @synthesize in ObjC. That can potentially be tackled separately, though, not necessarily in this diff.

artwyman · 2016-12-28T04:00:59Z

src/source/CppGenerator.scala

@@ -297,6 +300,16 @@ class CppGenerator(spec: Spec) extends Generator(spec) {
            w.wl(s"virtual $ret ${idCpp.method(m.ident)}${params.mkString("(", ", ", ")")}$constFlag = 0;")
          }
        }
+        // Properties


It would be nice if this shared more code with the method-generating codepaths, since that will help to keep the code simple and make it likely to all be updated in sync with future changes. You should be able to create method definitions from the properties, then use the normal method-generation code.

One option to consider (though it might be too aggressive) would be to implement property generation in most languages as a transformation on the AST, by generating a new AST with methods in place of properties, then running the normal generator on them. That could be a suitable approach for all generators which don't have special support for properties (i.e. ObjC with its @Property declaration).

BrassLion · 2017-01-12T20:00:20Z

I'll have a look into making these changes when I next get a chance. In regards to your suggestion of transforming the AST to another without properties, what way of achieving this in Scala would you be envisioning? My Scala knowledge is fairly basic, so any pointers would be appreciated!

artwyman · 2017-01-12T22:12:46Z

I don't have specifics in mind, really. It's more of a theoretical suggestion. By AST I think I just mean the list of TypeDecl objects which is the allIdl variable in Main.

choiip · 2017-10-02T16:07:34Z

This is a remarkable feature. The generated API for objective-c looks better. @BrassLion, any update on the changes? It is good the hear that this nice feature could be included in Djinni.

BrassLion · 2017-10-10T14:39:35Z

I'm afraid I haven't had any time to return to these changes and I don't see myself getting back to this in the near future. Hopefully someone will be able to continue this feature or use it as inspiration. Apologies!

BrassLion added 7 commits November 7, 2016 20:02

Implemented properties for ObjC and Cpp.

02ec580

Fixed property include mapping for C++ and ObjC.

c9f6049

Removed unused development methods.

9291cb1

Added list type to property test.

05dac06

Implemented properties for Java and JNI.

be2f9d9

Added ability to declare property readonly in ObjC and C++. Generates…

626d55d

… only the getter in this instance.

Added ability to declare property readonly in Java and JNI.

20be293

artwyman suggested changes Dec 28, 2016

View reviewed changes

artwyman assigned BrassLion Dec 28, 2016

artwyman added the enhancement label Dec 28, 2016

ghost mentioned this pull request Mar 22, 2018

Property declaration support in interfaces #360

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding the ability to declare properties in interfaces. #287

Adding the ability to declare properties in interfaces. #287

BrassLion commented Nov 7, 2016

smarx commented Nov 7, 2016

smarx commented Nov 7, 2016

artwyman left a comment

artwyman Dec 28, 2016

artwyman Dec 28, 2016

artwyman Dec 28, 2016

artwyman Dec 28, 2016

artwyman Dec 28, 2016

artwyman Dec 28, 2016

artwyman Dec 28, 2016

BrassLion commented Jan 12, 2017

artwyman commented Jan 12, 2017

choiip commented Oct 2, 2017

BrassLion commented Oct 10, 2017


		static std::shared_ptr<PropertiesTestHelper> create_new();

		virtual int32_t get_item() = 0;

Adding the ability to declare properties in interfaces. #287

Are you sure you want to change the base?

Adding the ability to declare properties in interfaces. #287

Conversation

BrassLion commented Nov 7, 2016

smarx commented Nov 7, 2016

smarx commented Nov 7, 2016

artwyman left a comment

Choose a reason for hiding this comment

artwyman Dec 28, 2016

Choose a reason for hiding this comment

artwyman Dec 28, 2016

Choose a reason for hiding this comment

artwyman Dec 28, 2016

Choose a reason for hiding this comment

artwyman Dec 28, 2016

Choose a reason for hiding this comment

artwyman Dec 28, 2016

Choose a reason for hiding this comment

artwyman Dec 28, 2016

Choose a reason for hiding this comment

artwyman Dec 28, 2016

Choose a reason for hiding this comment

BrassLion commented Jan 12, 2017

artwyman commented Jan 12, 2017

choiip commented Oct 2, 2017

BrassLion commented Oct 10, 2017