Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.5.0-beta1
    • Fix Version/s: 2.5.0-beta1
    • Component/s: Javascript
    • Labels:
      None

      Description

      The selection feature does not match the spec (addListenter vs. addSelectionListener)

        Activity

        Hide
        Matthew Hatem added a comment -

        Code review request here:
        https://reviews.apache.org/r/1615/

        Show
        Matthew Hatem added a comment - Code review request here: https://reviews.apache.org/r/1615/
        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1615/ ----------------------------------------------------------- Review request for shindig and Ryan Baxter. Summary ------- https://issues.apache.org/jira/browse/SHINDIG-1591 This addresses bug SHINDIG-1591 . https://issues.apache.org/jira/browse/SHINDIG-1591 Diffs http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/conservcontainer/sample-actions-runner.xml 1160436 http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/conservcontainer/sample-selection-listener.xml 1160436 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/feature.xml 1160436 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection.js 1160436 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection_container.js 1160436 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/taming.js 1160436 http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/selection/selection_test.js 1160436 Diff: https://reviews.apache.org/r/1615/diff Testing ------- Passes all unit tests. Thanks, Matthew
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1615/#review1604
        -----------------------------------------------------------

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection_container.js
        <https://reviews.apache.org/r/1615/#comment3626>

        How does this ever get called? Shouldn't this be called when the gadget calles removeListener? If so it should be called from your router function...right?

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/selection/selection_test.js
        <https://reviews.apache.org/r/1615/#comment3627>

        Shouldn't there be a test for removeListener as well?

        • Ryan

        On 2011-08-23 23:06:15, Matthew Hatem wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1615/

        -----------------------------------------------------------

        (Updated 2011-08-23 23:06:15)

        Review request for shindig and Ryan Baxter.

        Summary

        -------

        https://issues.apache.org/jira/browse/SHINDIG-1591

        This addresses bug SHINDIG-1591.

        https://issues.apache.org/jira/browse/SHINDIG-1591

        Diffs

        -----

        http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/conservcontainer/sample-actions-runner.xml 1160436

        http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/conservcontainer/sample-selection-listener.xml 1160436

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/feature.xml 1160436

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection.js 1160436

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection_container.js 1160436

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/taming.js 1160436

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/selection/selection_test.js 1160436

        Diff: https://reviews.apache.org/r/1615/diff

        Testing

        -------

        Passes all unit tests.

        Thanks,

        Matthew

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1615/#review1604 ----------------------------------------------------------- http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection_container.js < https://reviews.apache.org/r/1615/#comment3626 > How does this ever get called? Shouldn't this be called when the gadget calles removeListener? If so it should be called from your router function...right? http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/selection/selection_test.js < https://reviews.apache.org/r/1615/#comment3627 > Shouldn't there be a test for removeListener as well? Ryan On 2011-08-23 23:06:15, Matthew Hatem wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1615/ ----------------------------------------------------------- (Updated 2011-08-23 23:06:15) Review request for shindig and Ryan Baxter. Summary ------- https://issues.apache.org/jira/browse/SHINDIG-1591 This addresses bug SHINDIG-1591 . https://issues.apache.org/jira/browse/SHINDIG-1591 Diffs ----- http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/conservcontainer/sample-actions-runner.xml 1160436 http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/conservcontainer/sample-selection-listener.xml 1160436 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/feature.xml 1160436 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection.js 1160436 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection_container.js 1160436 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/taming.js 1160436 http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/selection/selection_test.js 1160436 Diff: https://reviews.apache.org/r/1615/diff Testing ------- Passes all unit tests. Thanks, Matthew
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-08-24 00:22:17, Ryan Baxter wrote:

        > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection_container.js, line 96

        > <https://reviews.apache.org/r/1615/diff/1/?file=34109#file34109line96>

        >

        > How does this ever get called? Shouldn't this be called when the gadget calles removeListener? If so it should be called from your router function...right?

        This allows the container to add and remove listeners, has nothing to do with gadget level API.

        On 2011-08-24 00:22:17, Ryan Baxter wrote:

        > http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/selection/selection_test.js, line 84

        > <https://reviews.apache.org/r/1615/diff/1/?file=34111#file34111line84>

        >

        > Shouldn't there be a test for removeListener as well?

        Yes I will add one.

        • Matthew

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1615/#review1604
        -----------------------------------------------------------

        On 2011-08-23 23:06:15, Matthew Hatem wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1615/

        -----------------------------------------------------------

        (Updated 2011-08-23 23:06:15)

        Review request for shindig and Ryan Baxter.

        Summary

        -------

        https://issues.apache.org/jira/browse/SHINDIG-1591

        This addresses bug SHINDIG-1591.

        https://issues.apache.org/jira/browse/SHINDIG-1591

        Diffs

        -----

        http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/conservcontainer/sample-actions-runner.xml 1160436

        http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/conservcontainer/sample-selection-listener.xml 1160436

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/feature.xml 1160436

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection.js 1160436

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection_container.js 1160436

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/taming.js 1160436

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/selection/selection_test.js 1160436

        Diff: https://reviews.apache.org/r/1615/diff

        Testing

        -------

        Passes all unit tests.

        Thanks,

        Matthew

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-08-24 00:22:17, Ryan Baxter wrote: > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection_container.js , line 96 > < https://reviews.apache.org/r/1615/diff/1/?file=34109#file34109line96 > > > How does this ever get called? Shouldn't this be called when the gadget calles removeListener? If so it should be called from your router function...right? This allows the container to add and remove listeners, has nothing to do with gadget level API. On 2011-08-24 00:22:17, Ryan Baxter wrote: > http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/selection/selection_test.js , line 84 > < https://reviews.apache.org/r/1615/diff/1/?file=34111#file34111line84 > > > Shouldn't there be a test for removeListener as well? Yes I will add one. Matthew ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1615/#review1604 ----------------------------------------------------------- On 2011-08-23 23:06:15, Matthew Hatem wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1615/ ----------------------------------------------------------- (Updated 2011-08-23 23:06:15) Review request for shindig and Ryan Baxter. Summary ------- https://issues.apache.org/jira/browse/SHINDIG-1591 This addresses bug SHINDIG-1591 . https://issues.apache.org/jira/browse/SHINDIG-1591 Diffs ----- http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/conservcontainer/sample-actions-runner.xml 1160436 http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/conservcontainer/sample-selection-listener.xml 1160436 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/feature.xml 1160436 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection.js 1160436 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection_container.js 1160436 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/taming.js 1160436 http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/selection/selection_test.js 1160436 Diff: https://reviews.apache.org/r/1615/diff Testing ------- Passes all unit tests. Thanks, Matthew
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-08-24 00:22:17, Ryan Baxter wrote:

        > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection_container.js, line 96

        > <https://reviews.apache.org/r/1615/diff/1/?file=34109#file34109line96>

        >

        > How does this ever get called? Shouldn't this be called when the gadget calles removeListener? If so it should be called from your router function...right?

        Matthew Hatem wrote:

        This allows the container to add and remove listeners, has nothing to do with gadget level API.

        So when the gadget adds a listener we add it to a list in the container, but when a gadget removes a listener we never remove it from the list in the container....why?

        • Ryan

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1615/#review1604
        -----------------------------------------------------------

        On 2011-08-23 23:06:15, Matthew Hatem wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1615/

        -----------------------------------------------------------

        (Updated 2011-08-23 23:06:15)

        Review request for shindig and Ryan Baxter.

        Summary

        -------

        https://issues.apache.org/jira/browse/SHINDIG-1591

        This addresses bug SHINDIG-1591.

        https://issues.apache.org/jira/browse/SHINDIG-1591

        Diffs

        -----

        http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/conservcontainer/sample-actions-runner.xml 1160436

        http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/conservcontainer/sample-selection-listener.xml 1160436

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/feature.xml 1160436

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection.js 1160436

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection_container.js 1160436

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/taming.js 1160436

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/selection/selection_test.js 1160436

        Diff: https://reviews.apache.org/r/1615/diff

        Testing

        -------

        Passes all unit tests.

        Thanks,

        Matthew

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-08-24 00:22:17, Ryan Baxter wrote: > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection_container.js , line 96 > < https://reviews.apache.org/r/1615/diff/1/?file=34109#file34109line96 > > > How does this ever get called? Shouldn't this be called when the gadget calles removeListener? If so it should be called from your router function...right? Matthew Hatem wrote: This allows the container to add and remove listeners, has nothing to do with gadget level API. So when the gadget adds a listener we add it to a list in the container, but when a gadget removes a listener we never remove it from the list in the container....why? Ryan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1615/#review1604 ----------------------------------------------------------- On 2011-08-23 23:06:15, Matthew Hatem wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1615/ ----------------------------------------------------------- (Updated 2011-08-23 23:06:15) Review request for shindig and Ryan Baxter. Summary ------- https://issues.apache.org/jira/browse/SHINDIG-1591 This addresses bug SHINDIG-1591 . https://issues.apache.org/jira/browse/SHINDIG-1591 Diffs ----- http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/conservcontainer/sample-actions-runner.xml 1160436 http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/conservcontainer/sample-selection-listener.xml 1160436 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/feature.xml 1160436 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection.js 1160436 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection_container.js 1160436 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/taming.js 1160436 http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/selection/selection_test.js 1160436 Diff: https://reviews.apache.org/r/1615/diff Testing ------- Passes all unit tests. Thanks, Matthew
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-08-24 00:22:17, Ryan Baxter wrote:

        > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection_container.js, line 96

        > <https://reviews.apache.org/r/1615/diff/1/?file=34109#file34109line96>

        >

        > How does this ever get called? Shouldn't this be called when the gadget calles removeListener? If so it should be called from your router function...right?

        Matthew Hatem wrote:

        This allows the container to add and remove listeners, has nothing to do with gadget level API.

        Ryan Baxter wrote:

        So when the gadget adds a listener we add it to a list in the container, but when a gadget removes a listener we never remove it from the list in the container....why?

        We've talked about this before. Each gadget adds only one listener (lazily) to the list in the container which proxies events to (potentially many) listeners registered with a local list at the gadget. This cuts down on the number of RPCs that are necessary to notify everyone.

        You might argue that I need a lifecycle listener to cleanup the proxy listeners. I don't want to hold up this patch for that though, I'll open a separate issue for that.

        On 2011-08-24 00:22:17, Ryan Baxter wrote:

        > http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/selection/selection_test.js, line 84

        > <https://reviews.apache.org/r/1615/diff/1/?file=34111#file34111line84>

        >

        > Shouldn't there be a test for removeListener as well?

        Matthew Hatem wrote:

        Yes I will add one.

        I take that back. It's currently not possible to test this without exposing things that should be private.

        Is there plan to upgrade/improve the unit testing in Shinding?

        • Matthew

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1615/#review1604
        -----------------------------------------------------------

        On 2011-08-23 23:06:15, Matthew Hatem wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1615/

        -----------------------------------------------------------

        (Updated 2011-08-23 23:06:15)

        Review request for shindig and Ryan Baxter.

        Summary

        -------

        https://issues.apache.org/jira/browse/SHINDIG-1591

        This addresses bug SHINDIG-1591.

        https://issues.apache.org/jira/browse/SHINDIG-1591

        Diffs

        -----

        http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/conservcontainer/sample-actions-runner.xml 1160436

        http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/conservcontainer/sample-selection-listener.xml 1160436

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/feature.xml 1160436

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection.js 1160436

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection_container.js 1160436

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/taming.js 1160436

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/selection/selection_test.js 1160436

        Diff: https://reviews.apache.org/r/1615/diff

        Testing

        -------

        Passes all unit tests.

        Thanks,

        Matthew

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-08-24 00:22:17, Ryan Baxter wrote: > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection_container.js , line 96 > < https://reviews.apache.org/r/1615/diff/1/?file=34109#file34109line96 > > > How does this ever get called? Shouldn't this be called when the gadget calles removeListener? If so it should be called from your router function...right? Matthew Hatem wrote: This allows the container to add and remove listeners, has nothing to do with gadget level API. Ryan Baxter wrote: So when the gadget adds a listener we add it to a list in the container, but when a gadget removes a listener we never remove it from the list in the container....why? We've talked about this before. Each gadget adds only one listener (lazily) to the list in the container which proxies events to (potentially many) listeners registered with a local list at the gadget. This cuts down on the number of RPCs that are necessary to notify everyone. You might argue that I need a lifecycle listener to cleanup the proxy listeners. I don't want to hold up this patch for that though, I'll open a separate issue for that. On 2011-08-24 00:22:17, Ryan Baxter wrote: > http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/selection/selection_test.js , line 84 > < https://reviews.apache.org/r/1615/diff/1/?file=34111#file34111line84 > > > Shouldn't there be a test for removeListener as well? Matthew Hatem wrote: Yes I will add one. I take that back. It's currently not possible to test this without exposing things that should be private. Is there plan to upgrade/improve the unit testing in Shinding? Matthew ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1615/#review1604 ----------------------------------------------------------- On 2011-08-23 23:06:15, Matthew Hatem wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1615/ ----------------------------------------------------------- (Updated 2011-08-23 23:06:15) Review request for shindig and Ryan Baxter. Summary ------- https://issues.apache.org/jira/browse/SHINDIG-1591 This addresses bug SHINDIG-1591 . https://issues.apache.org/jira/browse/SHINDIG-1591 Diffs ----- http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/conservcontainer/sample-actions-runner.xml 1160436 http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/conservcontainer/sample-selection-listener.xml 1160436 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/feature.xml 1160436 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection.js 1160436 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection_container.js 1160436 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/taming.js 1160436 http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/selection/selection_test.js 1160436 Diff: https://reviews.apache.org/r/1615/diff Testing ------- Passes all unit tests. Thanks, Matthew
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-08-24 00:22:17, Ryan Baxter wrote:

        > http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/selection/selection_test.js, line 84

        > <https://reviews.apache.org/r/1615/diff/1/?file=34111#file34111line84>

        >

        > Shouldn't there be a test for removeListener as well?

        Matthew Hatem wrote:

        Yes I will add one.

        Matthew Hatem wrote:

        I take that back. It's currently not possible to test this without exposing things that should be private.

        Is there plan to upgrade/improve the unit testing in Shinding?

        Ah ok.

        Not that I know of, but we are open to suggestions if you have ideas

        On 2011-08-24 00:22:17, Ryan Baxter wrote:

        > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection_container.js, line 96

        > <https://reviews.apache.org/r/1615/diff/1/?file=34109#file34109line96>

        >

        > How does this ever get called? Shouldn't this be called when the gadget calles removeListener? If so it should be called from your router function...right?

        Matthew Hatem wrote:

        This allows the container to add and remove listeners, has nothing to do with gadget level API.

        Ryan Baxter wrote:

        So when the gadget adds a listener we add it to a list in the container, but when a gadget removes a listener we never remove it from the list in the container....why?

        Matthew Hatem wrote:

        We've talked about this before. Each gadget adds only one listener (lazily) to the list in the container which proxies events to (potentially many) listeners registered with a local list at the gadget. This cuts down on the number of RPCs that are necessary to notify everyone.

        You might argue that I need a lifecycle listener to cleanup the proxy listeners. I don't want to hold up this patch for that though, I'll open a separate issue for that.

        Ah yes now I remember, thanks for the refresher.

        Yes it would be nice to clean this up.

        • Ryan

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1615/#review1604
        -----------------------------------------------------------

        On 2011-08-23 23:06:15, Matthew Hatem wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1615/

        -----------------------------------------------------------

        (Updated 2011-08-23 23:06:15)

        Review request for shindig and Ryan Baxter.

        Summary

        -------

        https://issues.apache.org/jira/browse/SHINDIG-1591

        This addresses bug SHINDIG-1591.

        https://issues.apache.org/jira/browse/SHINDIG-1591

        Diffs

        -----

        http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/conservcontainer/sample-actions-runner.xml 1160436

        http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/conservcontainer/sample-selection-listener.xml 1160436

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/feature.xml 1160436

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection.js 1160436

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection_container.js 1160436

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/taming.js 1160436

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/selection/selection_test.js 1160436

        Diff: https://reviews.apache.org/r/1615/diff

        Testing

        -------

        Passes all unit tests.

        Thanks,

        Matthew

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-08-24 00:22:17, Ryan Baxter wrote: > http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/selection/selection_test.js , line 84 > < https://reviews.apache.org/r/1615/diff/1/?file=34111#file34111line84 > > > Shouldn't there be a test for removeListener as well? Matthew Hatem wrote: Yes I will add one. Matthew Hatem wrote: I take that back. It's currently not possible to test this without exposing things that should be private. Is there plan to upgrade/improve the unit testing in Shinding? Ah ok. Not that I know of, but we are open to suggestions if you have ideas On 2011-08-24 00:22:17, Ryan Baxter wrote: > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection_container.js , line 96 > < https://reviews.apache.org/r/1615/diff/1/?file=34109#file34109line96 > > > How does this ever get called? Shouldn't this be called when the gadget calles removeListener? If so it should be called from your router function...right? Matthew Hatem wrote: This allows the container to add and remove listeners, has nothing to do with gadget level API. Ryan Baxter wrote: So when the gadget adds a listener we add it to a list in the container, but when a gadget removes a listener we never remove it from the list in the container....why? Matthew Hatem wrote: We've talked about this before. Each gadget adds only one listener (lazily) to the list in the container which proxies events to (potentially many) listeners registered with a local list at the gadget. This cuts down on the number of RPCs that are necessary to notify everyone. You might argue that I need a lifecycle listener to cleanup the proxy listeners. I don't want to hold up this patch for that though, I'll open a separate issue for that. Ah yes now I remember, thanks for the refresher. Yes it would be nice to clean this up. Ryan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1615/#review1604 ----------------------------------------------------------- On 2011-08-23 23:06:15, Matthew Hatem wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1615/ ----------------------------------------------------------- (Updated 2011-08-23 23:06:15) Review request for shindig and Ryan Baxter. Summary ------- https://issues.apache.org/jira/browse/SHINDIG-1591 This addresses bug SHINDIG-1591 . https://issues.apache.org/jira/browse/SHINDIG-1591 Diffs ----- http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/conservcontainer/sample-actions-runner.xml 1160436 http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/conservcontainer/sample-selection-listener.xml 1160436 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/feature.xml 1160436 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection.js 1160436 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection_container.js 1160436 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/taming.js 1160436 http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/selection/selection_test.js 1160436 Diff: https://reviews.apache.org/r/1615/diff Testing ------- Passes all unit tests. Thanks, Matthew
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1615/#review1618
        -----------------------------------------------------------

        Ship it!

        Committed revision 1161102.
        Please close the JIRA with this revision. Thanks Matt.

        • Ryan

        On 2011-08-23 23:06:15, Matthew Hatem wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1615/

        -----------------------------------------------------------

        (Updated 2011-08-23 23:06:15)

        Review request for shindig and Ryan Baxter.

        Summary

        -------

        https://issues.apache.org/jira/browse/SHINDIG-1591

        This addresses bug SHINDIG-1591.

        https://issues.apache.org/jira/browse/SHINDIG-1591

        Diffs

        -----

        http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/conservcontainer/sample-actions-runner.xml 1160436

        http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/conservcontainer/sample-selection-listener.xml 1160436

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/feature.xml 1160436

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection.js 1160436

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection_container.js 1160436

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/taming.js 1160436

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/selection/selection_test.js 1160436

        Diff: https://reviews.apache.org/r/1615/diff

        Testing

        -------

        Passes all unit tests.

        Thanks,

        Matthew

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1615/#review1618 ----------------------------------------------------------- Ship it! Committed revision 1161102. Please close the JIRA with this revision. Thanks Matt. Ryan On 2011-08-23 23:06:15, Matthew Hatem wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1615/ ----------------------------------------------------------- (Updated 2011-08-23 23:06:15) Review request for shindig and Ryan Baxter. Summary ------- https://issues.apache.org/jira/browse/SHINDIG-1591 This addresses bug SHINDIG-1591 . https://issues.apache.org/jira/browse/SHINDIG-1591 Diffs ----- http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/conservcontainer/sample-actions-runner.xml 1160436 http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/conservcontainer/sample-selection-listener.xml 1160436 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/feature.xml 1160436 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection.js 1160436 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection_container.js 1160436 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/taming.js 1160436 http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/selection/selection_test.js 1160436 Diff: https://reviews.apache.org/r/1615/diff Testing ------- Passes all unit tests. Thanks, Matthew
        Hide
        Matthew Hatem added a comment -

        Fixed in revision 1161102

        Show
        Matthew Hatem added a comment - Fixed in revision 1161102
        Hide
        Paul Lindner added a comment -

        part of 2.5.0-beta1 release.

        Show
        Paul Lindner added a comment - part of 2.5.0-beta1 release.

          People

          • Assignee:
            Unassigned
            Reporter:
            Matthew Hatem
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development