Merge lp:~daniellim0611/unity-js-scopes/unity-js-scopes-bug1523595 into lp:unity-js-scopes

[Daniel Lim]

[Alexandre Abreu (abreu-alexandre)]

Thank you for working on this!

This code doesn't build though, could you fix it?

also:

- could you remove the "no-op" changes (prob tab/space updates)?
- unity::scopes::ScopeMetadata::results_ttl_type returns an enum which is basically in integral type, but you declare it as strings in JS. It should be defined as integers matching the C++ values,

review: Needs Fixing

[Daniel Lim (daniellim0611)]

Sorry I don't know what tabs/space are you referring to.

[Daniel Lim (daniellim0611)]

And where can I get the integral values?

[Alexandre Abreu (abreu-alexandre)]

> Sorry I don't know what tabs/space are you referring to.

If you look at the diff below, there are a few diffs that dont seem to change anything

[Alexandre Abreu (abreu-alexandre)]

> And where can I get the integral values?

see https://developer.ubuntu.com/api/devel/ubuntu-14.10/cplusplus/unity-scopes/classunity_1_1scopes_1_1_scope_metadata.html

[lp:~daniellim0611/unity-js-scopes/unity-js-scopes-bug1523595]

121. By Marcus Tomlinson on 2016-01-07

Stop the scopes runtime and exit gracefully upon SIGTERM

[Daniel Lim (daniellim0611)]

I've changed the enum. But I don't know how to remove the tabs/spaces

[Daniel Lim (daniellim0611)]

> I've changed the enum. But I don't know how to remove the unwanted diffs

[Daniel Lim (daniellim0611)]

> I've changed the enum. But I don't know how to remove the unwanted diffs

[Daniel Lim (daniellim0611)]

Oops this is awkward. Sorry

[Daniel Lim (daniellim0611)]

Can I change the commits? Is that what I am supposed to do to remove the unwanted changes? If yes, may I get some help on how to do so?

[Alexandre Abreu (abreu-alexandre)]

> Can I change the commits? Is that what I am supposed to do to remove the
> unwanted changes? If yes, may I get some help on how to do so?

you can do this:

bzr branch lp:unity-js-scopes trunk
cd trunk
bzr merge lp:~daniellim0611/unity-js-scopes/unity-js-scopes-bug1523595

then with

bzr diff

you can see the diffs, and update your code until the diffs are clean.
You can then

bzr commit -m "your message"

and

bzr push lp:~daniellim0611/unity-js-scopes/unity-js-scopes-bug1523595

it'll mess up the history a bit but it is fine,

[lp:~daniellim0611/unity-js-scopes/unity-js-scopes-bug1523595]

122. By daniellim <email address hidden> on 2016-01-08

Removed no-op changes for index.js

123. By daniellim <email address hidden> on 2016-01-08

Removed no-op changed for simple.js & index.js

[Daniel Lim (daniellim0611)]

I've removed some of the unwanted changes. The others are quite hard to see the difference.
Apparently the editor I used, atom, auto-removed all the spaces for me. So I used gedit instead.

But regardless of that, is the enumeration correct now?

[Alexandre Abreu (abreu-alexandre)]

Could you make the name of the enum match the c++ one: ResultsTtlType ?

review: Needs Fixing

[lp:~daniellim0611/unity-js-scopes/unity-js-scopes-bug1523595]

124. By daniellim <email address hidden> on 2016-01-09

Modified index.js

[Daniel Lim (daniellim0611)]

Ok changed it. Hope that there are no more silly mistakes.

[lp:~daniellim0611/unity-js-scopes/unity-js-scopes-bug1523595]

125. By daniellim <email address hidden> on 2016-01-11

Removed no-op changes for index.js

126. By daniellim <email address hidden> on 2016-01-11

Removed no-op changes for simple.js

[Alexandre Abreu (abreu-alexandre)]

Thank you,

It still doesn't build though. I looked into it and it caused by a limitation of v8cpp (the lib that we use to handle the details of the v8 -> c++ bindings) that doesn't properly handle enum class.

I filed a bug

https://bugs.launchpad.net/bugs/1532879

and it should be commited & released tomorrow. I'll check the code as soon as it is.

[Daniel Lim (daniellim0611)]

Actually I want to apologize. I am in boarding school and the school wifi blocks stuff from ubuntu, so I couldnt upgrade to Ubuntu 15 and Ubuntu 14 doesn't support unity scopes in javascript, therefore I wasn't able to build and test the code. I know that this isn't the right thing to do when coding but I thought it is just as simple as declaring a variable so I did not bother much.

Sorry for all inconvenience caused.

[Alexandre Abreu (abreu-alexandre)]

> Actually I want to apologize. I am in boarding school and the school wifi
> blocks stuff from ubuntu, so I couldnt upgrade to Ubuntu 15 and Ubuntu 14
> doesn't support unity scopes in javascript, therefore I wasn't able to build
> and test the code. I know that this isn't the right thing to do when coding
> but I thought it is just as simple as declaring a variable so I did not bother
> much.
>
> Sorry for all inconvenience caused.

no worries, I am here to help you land this branch :)

[Daniel Lim (daniellim0611)]

How's it?
Sorry I hope I can proceed to the next gci task ASAP. I have spent more than 10 days on this already.
I want to get the t-shirt :p

[Alexandre Abreu (abreu-alexandre)]

> How's it?
> Sorry I hope I can proceed to the next gci task ASAP. I have spent more than
> 10 days on this already.
> I want to get the t-shirt :p

The bug fix landed yesterday and now your branch builds and works fine,

+1

review: Approve

=== modified file 'examples/simple/simple.js'
--- examples/simple/simple.js	2015-11-08 15:49:12 +0000
+++ examples/simple/simple.js	2016-01-11 14:26:09 +0000
@@ -32,6 +32,7 @@
     console.log('  ' + metadata.hot_key());
     console.log('  ' + metadata.invisible());
     console.log('  ' + metadata.scope_directory());
+    console.log('  ' + metadata.results_ttl_type());
     console.log('  ' + metadata.location_data_needed());
 }
 
@@ -164,4 +165,3 @@
         preview: function(result, action_metadata) {}
     }
 );
-

=== modified file 'src/bindings/index.js'
--- src/bindings/index.js	2015-12-18 14:27:50 +0000
+++ src/bindings/index.js	2016-01-11 14:26:09 +0000
@@ -166,6 +166,14 @@
     },
 };
 
+// results_ttl_type enumeration type
+var ResultsTtlType = {
+    None: 0,
+    Small: 1,
+    Medium: 2,
+    Large: 3
+}
+
 ConnectivityStatus = {
     Unknown: "Unknown",
     Connected: "Connected",

=== modified file 'src/bindings/src/addon.cc'
--- src/bindings/src/addon.cc	2015-12-18 14:27:50 +0000
+++ src/bindings/src/addon.cc	2016-01-11 14:26:09 +0000
@@ -379,6 +379,7 @@
       .add_method("set_scope_state_callback", &Registry::set_scope_state_callback)
       .add_method("set_list_update_callback", &Registry::set_list_update_callback);
 
+
     v8cpp::Class<unity::scopes::ScopeMetadata> scope_metadata(isolate);
     scope_metadata
       // unity::scopes::ScopeMetadata
@@ -395,7 +396,7 @@
       // .add_method("appearance_attributes", &unity::scopes::ScopeMetadata::appearance_attributes)
       .add_method("scope_directory", &unity::scopes::ScopeMetadata::scope_directory)
       // .add_method("serialize", &unity::scopes::ScopeMetadata::serialize)
-      // .add_method("results_ttl_type", &unity::scopes::ScopeMetadata::results_ttl_type)
+      .add_method("results_ttl_type", &unity::scopes::ScopeMetadata::results_ttl_type)
       // .add_method("settings_definitions", &unity::scopes::ScopeMetadata::settings_definitions)
       .add_method("location_data_needed", &unity::scopes::ScopeMetadata::location_data_needed);