Wave
  1. Wave
  2. WAVE-317

Allow adding (copy/pasting) multiple participants into a wave

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: Web Client
    • Labels:

      Description

      I think it would be nice to allow to paste/enter multiple participant ids separated by comma into the "add participant" dialog box. The list will be parsed/validated by the client and interpreted like it the user added these participants one by one. This way it would be easier to create waves with multiple participants.
      The code is handled in ParticipantController.

        Activity

        Hide
        Michael MacFadden added a comment -

        Committed in revision 1293628.

        Show
        Michael MacFadden added a comment - Committed in revision 1293628.
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        Ship it!

        I'll get this committed later today.

        • Michael

        On 2012-02-23 16:41:26, rocklund wrote:

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

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

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

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

        (Updated 2012-02-23 16:41:26)

        Review request for wave.

        Summary

        -------

        Implemented as suggested in the jira-issue as a client side implementation in ParticipantController.java. I selected the separator for the multiple participant list to be comma (,) as stated in the jira issue.

        This addresses bug wave-317.

        https://issues.apache.org/jira/browse/wave-317

        Diffs

        -----

        src/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantController.java ec9e25a

        test/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantControllerTest.java PRE-CREATION

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

        Testing

        -------

        Compiled and run.

        * Tested to add single as well as multiple participants

        * Tested to add participants both with and without @localhost

        * Tested to add invalid participants

        Thanks,

        rocklund

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3882/#review5309 ----------------------------------------------------------- Ship it! I'll get this committed later today. Michael On 2012-02-23 16:41:26, rocklund wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3882/ ----------------------------------------------------------- (Updated 2012-02-23 16:41:26) Review request for wave. Summary ------- Implemented as suggested in the jira-issue as a client side implementation in ParticipantController.java. I selected the separator for the multiple participant list to be comma (,) as stated in the jira issue. This addresses bug wave-317. https://issues.apache.org/jira/browse/wave-317 Diffs ----- src/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantController.java ec9e25a test/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantControllerTest.java PRE-CREATION Diff: https://reviews.apache.org/r/3882/diff Testing ------- Compiled and run. * Tested to add single as well as multiple participants * Tested to add participants both with and without @localhost * Tested to add invalid participants Thanks, rocklund
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        (Updated 2012-02-23 16:41:26.330316)

        Review request for wave.

        Changes
        -------

        Updated according to the comments from Yuri

        Summary (updated)
        -------

        Implemented as suggested in the jira-issue as a client side implementation in ParticipantController.java. I selected the separator for the multiple participant list to be comma (,) as stated in the jira issue.

        This addresses bug wave-317.
        https://issues.apache.org/jira/browse/wave-317

        Diffs (updated)


        src/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantController.java ec9e25a
        test/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantControllerTest.java PRE-CREATION

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

        Testing
        -------

        Compiled and run.

        • Tested to add single as well as multiple participants
        • Tested to add participants both with and without @localhost
        • Tested to add invalid participants

        Thanks,

        rocklund

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3882/ ----------------------------------------------------------- (Updated 2012-02-23 16:41:26.330316) Review request for wave. Changes ------- Updated according to the comments from Yuri Summary (updated) ------- Implemented as suggested in the jira-issue as a client side implementation in ParticipantController.java. I selected the separator for the multiple participant list to be comma (,) as stated in the jira issue. This addresses bug wave-317. https://issues.apache.org/jira/browse/wave-317 Diffs (updated) src/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantController.java ec9e25a test/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantControllerTest.java PRE-CREATION Diff: https://reviews.apache.org/r/3882/diff Testing ------- Compiled and run. Tested to add single as well as multiple participants Tested to add participants both with and without @localhost Tested to add invalid participants Thanks, rocklund
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        I agree, if we can quickly update based on Yuri's comments, we can get this committed. I would be happy to commit the patch onces it's updated.

        • Michael

        On 2012-02-18 12:54:05, rocklund wrote:

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

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

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

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

        (Updated 2012-02-18 12:54:05)

        Review request for wave.

        Summary

        -------

        Implemented as suggested in the jira-issue as a client side implementation in ParticipantController.java. I selected the separator for the multiple participant list to be semi-colon(.

        This addresses bug wave-317.

        https://issues.apache.org/jira/browse/wave-317

        Diffs

        -----

        src/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantController.java ec9e25a

        test/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantControllerTest.java PRE-CREATION

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

        Testing

        -------

        Compiled and run.

        * Tested to add single as well as multiple participants

        * Tested to add participants both with and without @localhost

        * Tested to add invalid participants

        Thanks,

        rocklund

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3882/#review5283 ----------------------------------------------------------- I agree, if we can quickly update based on Yuri's comments, we can get this committed. I would be happy to commit the patch onces it's updated. Michael On 2012-02-18 12:54:05, rocklund wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3882/ ----------------------------------------------------------- (Updated 2012-02-18 12:54:05) Review request for wave. Summary ------- Implemented as suggested in the jira-issue as a client side implementation in ParticipantController.java. I selected the separator for the multiple participant list to be semi-colon( . This addresses bug wave-317. https://issues.apache.org/jira/browse/wave-317 Diffs ----- src/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantController.java ec9e25a test/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantControllerTest.java PRE-CREATION Diff: https://reviews.apache.org/r/3882/diff Testing ------- Compiled and run. * Tested to add single as well as multiple participants * Tested to add participants both with and without @localhost * Tested to add invalid participants Thanks, rocklund
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        Ship it!

        Just left a bunch of minor comments.

        src/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantController.java
        <https://reviews.apache.org/r/3882/#comment11517>

        Please rename participantList to "participants" as it's not a java.util.List

        src/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantController.java
        <https://reviews.apache.org/r/3882/#comment11518>

        Why not use here enhanced (java 5) for loop?

        test/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantControllerTest.java
        <https://reviews.apache.org/r/3882/#comment11516>

        Copyright 2010 Google Inc.
        ->
        Copyright 2012 Apache Wave

        test/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantControllerTest.java
        <https://reviews.apache.org/r/3882/#comment11515>

        Please remove empty line

        • Yuri

        On 2012-02-18 12:54:05, rocklund wrote:

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

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

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

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

        (Updated 2012-02-18 12:54:05)

        Review request for wave.

        Summary

        -------

        Implemented as suggested in the jira-issue as a client side implementation in ParticipantController.java. I selected the separator for the multiple participant list to be semi-colon(.

        This addresses bug wave-317.

        https://issues.apache.org/jira/browse/wave-317

        Diffs

        -----

        src/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantController.java ec9e25a

        test/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantControllerTest.java PRE-CREATION

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

        Testing

        -------

        Compiled and run.

        * Tested to add single as well as multiple participants

        * Tested to add participants both with and without @localhost

        * Tested to add invalid participants

        Thanks,

        rocklund

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3882/#review5271 ----------------------------------------------------------- Ship it! Just left a bunch of minor comments. src/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantController.java < https://reviews.apache.org/r/3882/#comment11517 > Please rename participantList to "participants" as it's not a java.util.List src/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantController.java < https://reviews.apache.org/r/3882/#comment11518 > Why not use here enhanced (java 5) for loop? test/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantControllerTest.java < https://reviews.apache.org/r/3882/#comment11516 > Copyright 2010 Google Inc. -> Copyright 2012 Apache Wave test/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantControllerTest.java < https://reviews.apache.org/r/3882/#comment11515 > Please remove empty line Yuri On 2012-02-18 12:54:05, rocklund wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3882/ ----------------------------------------------------------- (Updated 2012-02-18 12:54:05) Review request for wave. Summary ------- Implemented as suggested in the jira-issue as a client side implementation in ParticipantController.java. I selected the separator for the multiple participant list to be semi-colon( . This addresses bug wave-317. https://issues.apache.org/jira/browse/wave-317 Diffs ----- src/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantController.java ec9e25a test/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantControllerTest.java PRE-CREATION Diff: https://reviews.apache.org/r/3882/diff Testing ------- Compiled and run. * Tested to add single as well as multiple participants * Tested to add participants both with and without @localhost * Tested to add invalid participants Thanks, rocklund
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        (Updated 2012-02-18 12:54:05.737137)

        Review request for wave.

        Changes
        -------

        Updated the unit tests to use assertEquals instead of assertTrue

        Summary
        -------

        Implemented as suggested in the jira-issue as a client side implementation in ParticipantController.java. I selected the separator for the multiple participant list to be semi-colon(.

        This addresses bug wave-317.
        https://issues.apache.org/jira/browse/wave-317

        Diffs (updated)


        src/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantController.java ec9e25a
        test/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantControllerTest.java PRE-CREATION

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

        Testing
        -------

        Compiled and run.

        • Tested to add single as well as multiple participants
        • Tested to add participants both with and without @localhost
        • Tested to add invalid participants

        Thanks,

        rocklund

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3882/ ----------------------------------------------------------- (Updated 2012-02-18 12:54:05.737137) Review request for wave. Changes ------- Updated the unit tests to use assertEquals instead of assertTrue Summary ------- Implemented as suggested in the jira-issue as a client side implementation in ParticipantController.java. I selected the separator for the multiple participant list to be semi-colon( . This addresses bug wave-317. https://issues.apache.org/jira/browse/wave-317 Diffs (updated) src/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantController.java ec9e25a test/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantControllerTest.java PRE-CREATION Diff: https://reviews.apache.org/r/3882/diff Testing ------- Compiled and run. Tested to add single as well as multiple participants Tested to add participants both with and without @localhost Tested to add invalid participants Thanks, rocklund
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        test/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantControllerTest.java
        <https://reviews.apache.org/r/3882/#comment11382>

        Why not use assertEquals rather than assert true. For example:

        assertEqual(String message, int expected, int actual)

        This will provide better output during test execution.

        • Michael

        On 2012-02-16 19:19:37, rocklund wrote:

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

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

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

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

        (Updated 2012-02-16 19:19:37)

        Review request for wave.

        Summary

        -------

        Implemented as suggested in the jira-issue as a client side implementation in ParticipantController.java. I selected the separator for the multiple participant list to be semi-colon(.

        This addresses bug wave-317.

        https://issues.apache.org/jira/browse/wave-317

        Diffs

        -----

        src/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantController.java ec9e25a

        test/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantControllerTest.java PRE-CREATION

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

        Testing

        -------

        Compiled and run.

        * Tested to add single as well as multiple participants

        * Tested to add participants both with and without @localhost

        * Tested to add invalid participants

        Thanks,

        rocklund

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3882/#review5197 ----------------------------------------------------------- test/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantControllerTest.java < https://reviews.apache.org/r/3882/#comment11382 > Why not use assertEquals rather than assert true. For example: assertEqual(String message, int expected, int actual) This will provide better output during test execution. Michael On 2012-02-16 19:19:37, rocklund wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3882/ ----------------------------------------------------------- (Updated 2012-02-16 19:19:37) Review request for wave. Summary ------- Implemented as suggested in the jira-issue as a client side implementation in ParticipantController.java. I selected the separator for the multiple participant list to be semi-colon( . This addresses bug wave-317. https://issues.apache.org/jira/browse/wave-317 Diffs ----- src/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantController.java ec9e25a test/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantControllerTest.java PRE-CREATION Diff: https://reviews.apache.org/r/3882/diff Testing ------- Compiled and run. * Tested to add single as well as multiple participants * Tested to add participants both with and without @localhost * Tested to add invalid participants Thanks, rocklund
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        (Updated 2012-02-16 19:19:37.700527)

        Review request for wave.

        Changes
        -------

        Thanks for the reviews. Updated according to the comments:

        • Fixed indentation
        • Added spaces in between arithmetical operations
        • Updated to use equals instead of compareTo in unit test
        • Moved the buildParticipantList method to the ParticipantController
        • Moved the unit tests to the new class ParticipantControllerTest.java
        • Added @Nullable annotation to localDomain

        @Yuri: I don't understand how you mean that I can move the localDomain null-check outside of the for-loop? The current address processed in the for-loop is handled differently depending on the result of that null-check.

        Summary
        -------

        Implemented as suggested in the jira-issue as a client side implementation in ParticipantController.java. I selected the separator for the multiple participant list to be semi-colon(.

        This addresses bug wave-317.
        https://issues.apache.org/jira/browse/wave-317

        Diffs (updated)


        src/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantController.java ec9e25a
        test/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantControllerTest.java PRE-CREATION

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

        Testing
        -------

        Compiled and run.

        • Tested to add single as well as multiple participants
        • Tested to add participants both with and without @localhost
        • Tested to add invalid participants

        Thanks,

        rocklund

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3882/ ----------------------------------------------------------- (Updated 2012-02-16 19:19:37.700527) Review request for wave. Changes ------- Thanks for the reviews. Updated according to the comments: Fixed indentation Added spaces in between arithmetical operations Updated to use equals instead of compareTo in unit test Moved the buildParticipantList method to the ParticipantController Moved the unit tests to the new class ParticipantControllerTest.java Added @Nullable annotation to localDomain @Yuri: I don't understand how you mean that I can move the localDomain null-check outside of the for-loop? The current address processed in the for-loop is handled differently depending on the result of that null-check. Summary ------- Implemented as suggested in the jira-issue as a client side implementation in ParticipantController.java. I selected the separator for the multiple participant list to be semi-colon( . This addresses bug wave-317. https://issues.apache.org/jira/browse/wave-317 Diffs (updated) src/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantController.java ec9e25a test/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantControllerTest.java PRE-CREATION Diff: https://reviews.apache.org/r/3882/diff Testing ------- Compiled and run. Tested to add single as well as multiple participants Tested to add participants both with and without @localhost Tested to add invalid participants Thanks, rocklund
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        src/org/waveprotocol/wave/model/wave/ParticipantId.java
        <https://reviews.apache.org/r/3882/#comment11265>

        Construct -> Constructs

        src/org/waveprotocol/wave/model/wave/ParticipantId.java
        <https://reviews.apache.org/r/3882/#comment11266>

        I think it would be better to add @Nullable annotation on the parameter

        src/org/waveprotocol/wave/model/wave/ParticipantId.java
        <https://reviews.apache.org/r/3882/#comment11267>

        an vector -> the array

        src/org/waveprotocol/wave/model/wave/ParticipantId.java
        <https://reviews.apache.org/r/3882/#comment11268>

        @throws InvalidParticipantAddress if at least one of the addresses failed validation.

        src/org/waveprotocol/wave/model/wave/ParticipantId.java
        <https://reviews.apache.org/r/3882/#comment11264>

        I think it would be better to put this logic into a static util class, or even just a static method in ParticipantController. The logic of building a list of participants from a string with optional default domain is relevant mostly for client side, and not generic enough to be put inside ParticipantId class.

        src/org/waveprotocol/wave/model/wave/ParticipantId.java
        <https://reviews.apache.org/r/3882/#comment11261>

        Please add spaces
        int i=0;i<addressList.length;i++
        ->
        int i = 0; i < addressList.length; i++

        src/org/waveprotocol/wave/model/wave/ParticipantId.java
        <https://reviews.apache.org/r/3882/#comment11262>

        Seems like this check can be done outside of the loop.

        src/org/waveprotocol/wave/model/wave/ParticipantId.java
        <https://reviews.apache.org/r/3882/#comment11263>

        Please remove empty line

        test/org/waveprotocol/wave/model/wave/ParticipantIdTest.java
        <https://reviews.apache.org/r/3882/#comment11272>

        Please makes sure we follow consistent indentation

        test/org/waveprotocol/wave/model/wave/ParticipantIdTest.java
        <https://reviews.apache.org/r/3882/#comment11269>

        Please add spaces before and after all arithmetical operators.

        test/org/waveprotocol/wave/model/wave/ParticipantIdTest.java
        <https://reviews.apache.org/r/3882/#comment11271>

        Why using compareTo instead of equals?

        test/org/waveprotocol/wave/model/wave/ParticipantIdTest.java
        <https://reviews.apache.org/r/3882/#comment11270>

        Please fix indentation.

        • Yuri

        On 2012-02-15 22:54:41, rocklund wrote:

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

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

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

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

        (Updated 2012-02-15 22:54:41)

        Review request for wave.

        Summary

        -------

        Implemented as suggested in the jira-issue as a client side implementation in ParticipantController.java. I selected the separator for the multiple participant list to be semi-colon(.

        This addresses bug wave-317.

        https://issues.apache.org/jira/browse/wave-317

        Diffs

        -----

        src/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantController.java ec9e25a

        src/org/waveprotocol/wave/model/wave/ParticipantId.java a5dbdf6

        test/org/waveprotocol/wave/model/wave/ParticipantIdTest.java 58a2772

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

        Testing

        -------

        Compiled and run.

        * Tested to add single as well as multiple participants

        * Tested to add participants both with and without @localhost

        * Tested to add invalid participants

        Thanks,

        rocklund

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3882/#review5157 ----------------------------------------------------------- src/org/waveprotocol/wave/model/wave/ParticipantId.java < https://reviews.apache.org/r/3882/#comment11265 > Construct -> Constructs src/org/waveprotocol/wave/model/wave/ParticipantId.java < https://reviews.apache.org/r/3882/#comment11266 > I think it would be better to add @Nullable annotation on the parameter src/org/waveprotocol/wave/model/wave/ParticipantId.java < https://reviews.apache.org/r/3882/#comment11267 > an vector -> the array src/org/waveprotocol/wave/model/wave/ParticipantId.java < https://reviews.apache.org/r/3882/#comment11268 > @throws InvalidParticipantAddress if at least one of the addresses failed validation. src/org/waveprotocol/wave/model/wave/ParticipantId.java < https://reviews.apache.org/r/3882/#comment11264 > I think it would be better to put this logic into a static util class, or even just a static method in ParticipantController. The logic of building a list of participants from a string with optional default domain is relevant mostly for client side, and not generic enough to be put inside ParticipantId class. src/org/waveprotocol/wave/model/wave/ParticipantId.java < https://reviews.apache.org/r/3882/#comment11261 > Please add spaces int i=0;i<addressList.length;i++ -> int i = 0; i < addressList.length; i++ src/org/waveprotocol/wave/model/wave/ParticipantId.java < https://reviews.apache.org/r/3882/#comment11262 > Seems like this check can be done outside of the loop. src/org/waveprotocol/wave/model/wave/ParticipantId.java < https://reviews.apache.org/r/3882/#comment11263 > Please remove empty line test/org/waveprotocol/wave/model/wave/ParticipantIdTest.java < https://reviews.apache.org/r/3882/#comment11272 > Please makes sure we follow consistent indentation test/org/waveprotocol/wave/model/wave/ParticipantIdTest.java < https://reviews.apache.org/r/3882/#comment11269 > Please add spaces before and after all arithmetical operators. test/org/waveprotocol/wave/model/wave/ParticipantIdTest.java < https://reviews.apache.org/r/3882/#comment11271 > Why using compareTo instead of equals? test/org/waveprotocol/wave/model/wave/ParticipantIdTest.java < https://reviews.apache.org/r/3882/#comment11270 > Please fix indentation. Yuri On 2012-02-15 22:54:41, rocklund wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3882/ ----------------------------------------------------------- (Updated 2012-02-15 22:54:41) Review request for wave. Summary ------- Implemented as suggested in the jira-issue as a client side implementation in ParticipantController.java. I selected the separator for the multiple participant list to be semi-colon( . This addresses bug wave-317. https://issues.apache.org/jira/browse/wave-317 Diffs ----- src/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantController.java ec9e25a src/org/waveprotocol/wave/model/wave/ParticipantId.java a5dbdf6 test/org/waveprotocol/wave/model/wave/ParticipantIdTest.java 58a2772 Diff: https://reviews.apache.org/r/3882/diff Testing ------- Compiled and run. * Tested to add single as well as multiple participants * Tested to add participants both with and without @localhost * Tested to add invalid participants Thanks, rocklund
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        (Updated 2012-02-15 22:54:41.751527)

        Review request for wave.

        Changes
        -------

        • Broke out creation of participant list to new method in ParticipantIdUtil
        • Created unit test for the new method
        • Changed the separator from semi-colon to comma (@Michael: I actually had failed to see that it said comma in the bug even though I looked for it )
        • Updated the dialog text to mention the separator

        Summary
        -------

        Implemented as suggested in the jira-issue as a client side implementation in ParticipantController.java. I selected the separator for the multiple participant list to be semi-colon(.

        This addresses bug wave-317.
        https://issues.apache.org/jira/browse/wave-317

        Diffs (updated)


        src/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantController.java ec9e25a
        src/org/waveprotocol/wave/model/wave/ParticipantId.java a5dbdf6
        test/org/waveprotocol/wave/model/wave/ParticipantIdTest.java 58a2772

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

        Testing
        -------

        Compiled and run.

        • Tested to add single as well as multiple participants
        • Tested to add participants both with and without @localhost
        • Tested to add invalid participants

        Thanks,

        rocklund

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3882/ ----------------------------------------------------------- (Updated 2012-02-15 22:54:41.751527) Review request for wave. Changes ------- Broke out creation of participant list to new method in ParticipantIdUtil Created unit test for the new method Changed the separator from semi-colon to comma (@Michael: I actually had failed to see that it said comma in the bug even though I looked for it ) Updated the dialog text to mention the separator Summary ------- Implemented as suggested in the jira-issue as a client side implementation in ParticipantController.java. I selected the separator for the multiple participant list to be semi-colon( . This addresses bug wave-317. https://issues.apache.org/jira/browse/wave-317 Diffs (updated) src/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantController.java ec9e25a src/org/waveprotocol/wave/model/wave/ParticipantId.java a5dbdf6 test/org/waveprotocol/wave/model/wave/ParticipantIdTest.java 58a2772 Diff: https://reviews.apache.org/r/3882/diff Testing ------- Compiled and run. Tested to add single as well as multiple participants Tested to add participants both with and without @localhost Tested to add invalid participants Thanks, rocklund
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        "a/C://wintmp//Par1B9.tmp//ParticipantController-HEAD-left.java"
        <https://reviews.apache.org/r/3882/#comment11230>

        Can we extract this code that handles building of participants into separate method?
        Also it would be great to add a unit tests for this new method.

        "a/C://wintmp//Par1B9.tmp//ParticipantController-HEAD-left.java"
        <https://reviews.apache.org/r/3882/#comment11231>

        I think it would be better first to validate all participants and add them all only in case all addresses are valid - kind of "all or nothing" transaction.

        • Yuri

        On 2012-02-13 22:56:00, rocklund wrote:

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

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

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

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

        (Updated 2012-02-13 22:56:00)

        Review request for wave.

        Summary

        -------

        Implemented as suggested in the jira-issue as a client side implementation in ParticipantController.java. I selected the separator for the multiple participant list to be semi-colon(.

        This addresses bug wave-317.

        https://issues.apache.org/jira/browse/wave-317

        Diffs

        -----

        "a/C://wintmp//Par1B9.tmp//ParticipantController-HEAD-left.java" ec9e25a

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

        Testing

        -------

        Compiled and run.

        * Tested to add single as well as multiple participants

        * Tested to add participants both with and without @localhost

        * Tested to add invalid participants

        Thanks,

        rocklund

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3882/#review5128 ----------------------------------------------------------- "a/C://wintmp//Par1B9.tmp//ParticipantController-HEAD-left.java" < https://reviews.apache.org/r/3882/#comment11230 > Can we extract this code that handles building of participants into separate method? Also it would be great to add a unit tests for this new method. "a/C://wintmp//Par1B9.tmp//ParticipantController-HEAD-left.java" < https://reviews.apache.org/r/3882/#comment11231 > I think it would be better first to validate all participants and add them all only in case all addresses are valid - kind of "all or nothing" transaction. Yuri On 2012-02-13 22:56:00, rocklund wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3882/ ----------------------------------------------------------- (Updated 2012-02-13 22:56:00) Review request for wave. Summary ------- Implemented as suggested in the jira-issue as a client side implementation in ParticipantController.java. I selected the separator for the multiple participant list to be semi-colon( . This addresses bug wave-317. https://issues.apache.org/jira/browse/wave-317 Diffs ----- "a/C://wintmp//Par1B9.tmp//ParticipantController-HEAD-left.java" ec9e25a Diff: https://reviews.apache.org/r/3882/diff Testing ------- Compiled and run. * Tested to add single as well as multiple participants * Tested to add participants both with and without @localhost * Tested to add invalid participants Thanks, rocklund
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        Is there a reason you are using a semi colon instead of a comma like the bug suggestion. Is the standard for email to use commas? If so maybe we should use that instead. I suppose different programs might do it differently. In which case I suppose either is fine. In either case we should probably put a hint as to what the delimiter is somewhere in the add participants dialog.

        "a/C://wintmp//Par1B9.tmp//ParticipantController-HEAD-left.java"
        <https://reviews.apache.org/r/3882/#comment11229>

        Should we change the prompt to be something like "Add participant(s): " an / or let them know that they can add multiple participants using a delimiter?

        "a/C://wintmp//Par1B9.tmp//ParticipantController-HEAD-left.java"
        <https://reviews.apache.org/r/3882/#comment11228>

        Two comments. 1) perhaps we should separate the addresses with a comma instead of a space. 2) Is there any information we can extract from the exception here in relation to the address that was not valid to add to the error message?

        • Michael

        On 2012-02-13 22:56:00, rocklund wrote:

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

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

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

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

        (Updated 2012-02-13 22:56:00)

        Review request for wave.

        Summary

        -------

        Implemented as suggested in the jira-issue as a client side implementation in ParticipantController.java. I selected the separator for the multiple participant list to be semi-colon(.

        This addresses bug wave-317.

        https://issues.apache.org/jira/browse/wave-317

        Diffs

        -----

        "a/C://wintmp//Par1B9.tmp//ParticipantController-HEAD-left.java" ec9e25a

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

        Testing

        -------

        Compiled and run.

        * Tested to add single as well as multiple participants

        * Tested to add participants both with and without @localhost

        * Tested to add invalid participants

        Thanks,

        rocklund

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3882/#review5127 ----------------------------------------------------------- Is there a reason you are using a semi colon instead of a comma like the bug suggestion. Is the standard for email to use commas? If so maybe we should use that instead. I suppose different programs might do it differently. In which case I suppose either is fine. In either case we should probably put a hint as to what the delimiter is somewhere in the add participants dialog. "a/C://wintmp//Par1B9.tmp//ParticipantController-HEAD-left.java" < https://reviews.apache.org/r/3882/#comment11229 > Should we change the prompt to be something like "Add participant(s): " an / or let them know that they can add multiple participants using a delimiter? "a/C://wintmp//Par1B9.tmp//ParticipantController-HEAD-left.java" < https://reviews.apache.org/r/3882/#comment11228 > Two comments. 1) perhaps we should separate the addresses with a comma instead of a space. 2) Is there any information we can extract from the exception here in relation to the address that was not valid to add to the error message? Michael On 2012-02-13 22:56:00, rocklund wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3882/ ----------------------------------------------------------- (Updated 2012-02-13 22:56:00) Review request for wave. Summary ------- Implemented as suggested in the jira-issue as a client side implementation in ParticipantController.java. I selected the separator for the multiple participant list to be semi-colon( . This addresses bug wave-317. https://issues.apache.org/jira/browse/wave-317 Diffs ----- "a/C://wintmp//Par1B9.tmp//ParticipantController-HEAD-left.java" ec9e25a Diff: https://reviews.apache.org/r/3882/diff Testing ------- Compiled and run. * Tested to add single as well as multiple participants * Tested to add participants both with and without @localhost * Tested to add invalid participants Thanks, rocklund
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        Review request for wave.

        Summary
        -------

        Implemented as suggested in the jira-issue as a client side implementation in ParticipantController.java. I selected the separator for the multiple participant list to be semi-colon(.

        This addresses bug wave-317.
        https://issues.apache.org/jira/browse/wave-317

        Diffs


        "a/C://wintmp//Par1B9.tmp//ParticipantController-HEAD-left.java" ec9e25a

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

        Testing
        -------

        Compiled and run.

        • Tested to add single as well as multiple participants
        • Tested to add participants both with and without @localhost
        • Tested to add invalid participants

        Thanks,

        rocklund

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3882/ ----------------------------------------------------------- Review request for wave. Summary ------- Implemented as suggested in the jira-issue as a client side implementation in ParticipantController.java. I selected the separator for the multiple participant list to be semi-colon( . This addresses bug wave-317. https://issues.apache.org/jira/browse/wave-317 Diffs "a/C://wintmp//Par1B9.tmp//ParticipantController-HEAD-left.java" ec9e25a Diff: https://reviews.apache.org/r/3882/diff Testing ------- Compiled and run. Tested to add single as well as multiple participants Tested to add participants both with and without @localhost Tested to add invalid participants Thanks, rocklund

          People

          • Assignee:
            Olof
            Reporter:
            Yuri Zelikov
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development