Merge lp://qastaging/~abreu-alexandre/unity-js-scopes/expand-filter-support into lp://qastaging/unity-js-scopes

Proposed by Alexandre Abreu
Status: Needs review
Proposed branch: lp://qastaging/~abreu-alexandre/unity-js-scopes/expand-filter-support
Merge into: lp://qastaging/unity-js-scopes
Diff against target: 1667 lines (+1451/-31)
12 files modified
src/bindings/index.js (+30/-0)
src/bindings/src/addon.cc (+69/-2)
src/bindings/src/filter-group.cc (+39/-0)
src/bindings/src/filter-group.h (+82/-0)
src/bindings/src/option-selector-filter.cc (+17/-1)
src/bindings/src/option-selector-filter.h (+68/-28)
src/bindings/src/range-input-filter.cc (+259/-0)
src/bindings/src/range-input-filter.h (+312/-0)
src/bindings/src/value-slider-filter.cc (+141/-0)
src/bindings/src/value-slider-filter.h (+234/-0)
src/bindings/src/value-slider-labels.cc (+91/-0)
src/bindings/src/value-slider-labels.h (+109/-0)
To merge this branch: bzr merge lp://qastaging/~abreu-alexandre/unity-js-scopes/expand-filter-support
Reviewer Review Type Date Requested Status
Marcus Tomlinson (community) Needs Fixing
unity-api-1-bot continuous-integration Approve
PS Jenkins bot continuous-integration Pending
Review via email: mp+296067@code.qastaging.launchpad.net

Commit message

Add support for FilterGroup, ValueSliderLabels, ValueSliderFilter, RangeInputFilter; improve Filter handling

Description of the change

Add support for FilterGroup, ValueSliderLabels, ValueSliderFilter, RangeInputFilter; improve Filter handling

To post a comment you must log in.
Revision history for this message
unity-api-1-bot (unity-api-1-bot) wrote :
review: Needs Fixing (continuous-integration)
144. By Alexandre Abreu

Ooops forgot part of the commits, object constructors

Revision history for this message
unity-api-1-bot (unity-api-1-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Marcus Tomlinson (marcustomlinson) wrote :

Yeah, looks mostly done. A few gaps here and there though.

You've only bound one constructor for the RangeInputFilter, ValueSliderFilter and ValueSliderLabels classes. We need to do the "v8::FunctionCallbackInfo<v8::Value> const& args" thing for each of these in order to cover the available overrides.

Also, could you please test against the example scope I wrote in lp:~marcustomlinson/unity-js-scopes/filters_example. I suspect there will be other errors, but already I'm seeing:

/home/marcustomlinson/Projects/work/unity-js-scopes/filters_example/examples/filters-build/src/filters.marcustomlinson_filters.js:471
                  var price_filter = new scopes.lib.RangeInputFilter("price", "Min", "", "to", "Max", "", group);
                                     ^

Error: argument count does not match the corresponding C++ function definition
    at Error (native)
    at null.<anonymous> (/home/marcustomlinson/Projects/work/unity-js-scopes/filters_example/examples/filters-build/src/filters.marcustomlinson_filters.js:471:38)

review: Needs Fixing
Revision history for this message
Alexandre Abreu (abreu-alexandre) wrote :

> Yeah, looks mostly done. A few gaps here and there though.
>
> You've only bound one constructor for the RangeInputFilter, ValueSliderFilter
> and ValueSliderLabels classes. We need to do the
> "v8::FunctionCallbackInfo<v8::Value> const& args" thing for each of these in
> order to cover the available overrides.
>
> Also, could you please test against the example scope I wrote in
> lp:~marcustomlinson/unity-js-scopes/filters_example. I suspect there will be
> other errors, but already I'm seeing:
>
> /home/marcustomlinson/Projects/work/unity-js-scopes/filters_example/examples
> /filters-build/src/filters.marcustomlinson_filters.js:471
> var price_filter = new scopes.lib.RangeInputFilter("price",
> "Min", "", "to", "Max", "", group);
> ^
>
> Error: argument count does not match the corresponding C++ function definition
> at Error (native)
> at null.<anonymous> (/home/marcustomlinson/Projects/work/unity-js-
> scopes/filters_example/examples/filters-
> build/src/filters.marcustomlinson_filters.js:471:38)

yes I remember chose to bind the most "complete" constructor ...
I used your example with null & null for the extra values,

Agree that it should be updated with varargs support though,

145. By Alexandre Abreu

Add varargs support for ValueSliderFilter & RangeInputFilter constructors

Revision history for this message
Alexandre Abreu (abreu-alexandre) wrote :

updated

Revision history for this message
unity-api-1-bot (unity-api-1-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Marcus Tomlinson (marcustomlinson) wrote :
Download full text (4.5 KiB)

This is what I get now, running the filters example scope:

#
# Fatal error in ../deps/v8/src/api.h, line 412
# Check failed: that == __null || (*reinterpret_cast<v8::internal::Object* const*>(that))->IsJSObject().
#

==== C stack trace ===============================

 1: V8_Fatal
 2: v8::Utils::OpenHandle(v8::Object const*, bool)
 3: v8::Object::Has(v8::Local<v8::Value>)
 4: ValueSliderLabels::ValueSliderLabels(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, v8::Local<v8::Value>)
 5: ValueSliderLabels* v8cpp::internal::ObjectFactory<ValueSliderLabels>::new_object<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, v8::Local<v8::Value> >(v8::Isolate*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, v8::Local<v8::Value>)
 6: v8cpp::internal::FunctionTraits<ValueSliderLabels* (*)(v8::Isolate*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, v8::Local<v8::Value>)>::ReturnType v8cpp::internal::call_from_v8_impl<ValueSliderLabels* (*)(v8::Isolate*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, v8::Local<v8::Value>), 0ul, 1ul, 2ul>(ValueSliderLabels* (*&&)(v8::Isolate*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, v8::Local<v8::Value>), v8::FunctionCallbackInfo<v8::Value> const&, v8cpp::internal::CallFromV8Traits<ValueSliderLabels* (*)(v8::Isolate*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, v8::Local<v8::Value>), 1ul>, v8cpp::internal::IntSequence<unsigned long, 0ul, 1ul, 2ul>)
 7: v8cpp::internal::FunctionTraits<ValueSliderLabels* (*)(v8::Isolate*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, v8::Local<v8::Value>)>::ReturnType v8cpp::internal::call_from_v8<ValueSliderLabels* (*)(v8::Isolate*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, v8::Local<v8::Value>)>(ValueSliderLabels* (*&&)(v8::Isolate*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, v8::Local<v8::Value>), v8::FunctionCallbackInfo<v8::Value> const&)
 8: void v8cpp::internal::Class<ValueSliderLabels>::set_constructor<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_tra...

Read more...

review: Needs Fixing
Revision history for this message
unity-api-1-bot (unity-api-1-bot) wrote :

FAILED: Continuous integration, rev:145
https://jenkins.canonical.com/unity-api-1/job/lp-unity-js-scopes-ci/12/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build/105/console
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-0-fetch/112
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-1-sourcepkg/release=vivid+overlay/69
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-1-sourcepkg/release=xenial/69
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-1-sourcepkg/release=yakkety/69
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=vivid+overlay/45/console
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=xenial/45
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=xenial/45/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=yakkety/45
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=yakkety/45/artifact/output/*zip*/output.zip
    ABORTED: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=vivid+overlay/45/console
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=xenial/45
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=xenial/45/artifact/output/*zip*/output.zip
    ABORTED: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=yakkety/45/console
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=vivid+overlay/45/console
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=xenial/45
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=xenial/45/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=yakkety/45
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=yakkety/45/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-unity-js-scopes-ci/12/rebuild

review: Needs Fixing (continuous-integration)
146. By Alexandre Abreu

Update ValueSliderLabels' constructor to fit the Scopes API

Revision history for this message
Alexandre Abreu (abreu-alexandre) wrote :

I updated the code to fit the js example

Revision history for this message
unity-api-1-bot (unity-api-1-bot) wrote :

PASSED: Continuous integration, rev:146
https://jenkins.canonical.com/unity-api-1/job/lp-unity-js-scopes-ci/14/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build/396
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-0-fetch/402
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-1-sourcepkg/release=vivid+overlay/317
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-1-sourcepkg/release=xenial+overlay/317
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-1-sourcepkg/release=yakkety/317
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=vivid+overlay/247
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=vivid+overlay/247/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=xenial+overlay/247
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=xenial+overlay/247/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=yakkety/247
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=yakkety/247/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=vivid+overlay/247
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=vivid+overlay/247/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=xenial+overlay/247
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=xenial+overlay/247/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=yakkety/247
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=yakkety/247/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=vivid+overlay/247
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=vivid+overlay/247/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=xenial+overlay/247
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=xenial+overlay/247/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=yakkety/247
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=yakkety/247/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-unity-js-scopes-ci/14/rebuild

review: Approve (continuous-integration)
Revision history for this message
Marcus Tomlinson (marcustomlinson) wrote :

While there are no longer any visible error messages when running the example scope from lp:~marcustomlinson/unity-js-scopes/filters_example, there are no results showing up in the scope at all.

I'm not sure where things are going wrong now. Running the example scope against lp:~marcustomlinson/unity-js-scopes/filters works fine, but obviously there are some changes here (especially documentation) that I don't have.

I wonder if it'd be wise to somewhat merge the 2?

review: Needs Fixing

Unmerged revisions

146. By Alexandre Abreu

Update ValueSliderLabels' constructor to fit the Scopes API

145. By Alexandre Abreu

Add varargs support for ValueSliderFilter & RangeInputFilter constructors

144. By Alexandre Abreu

Ooops forgot part of the commits, object constructors

143. By Alexandre Abreu

Add support for FilterGroup, ValueSliderLabels, ValueSliderFilter, RangeInputFilter; improve Filter handling

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: