Bug 203553 - "role" annotation in @TopComponent.Registration should allow multiple roles
"role" annotation in @TopComponent.Registration should allow multiple roles
Status: RESOLVED FIXED
Product: platform
Classification: Unclassified
Component: Window System
7.1
All All
: P1 (vote)
: 7.1
Assigned To: Stanislav Aubrecht
issues@platform
: API, API_REVIEW_FAST
Depends on:
Blocks: 199452
  Show dependency treegraph
 
Reported: 2011-10-11 22:03 UTC by Geertjan Wielenga
Modified: 2011-10-20 14:28 UTC (History)
2 users (show)

See Also:
Issue Type: ENHANCEMENT
:


Attachments
Patch for multiple roles (3.67 KB, patch)
2011-10-12 12:51 UTC, Geertjan Wielenga
Details | Diff
A new patch, incoporating Jesse's comments (4.35 KB, patch)
2011-10-12 13:27 UTC, Geertjan Wielenga
Details | Diff
Duplicate role check done (5.59 KB, patch)
2011-10-12 13:54 UTC, Geertjan Wielenga
Details | Diff
Removing a test that I added previously erroneously (5.32 KB, patch)
2011-10-12 14:11 UTC, Geertjan Wielenga
Details | Diff
Mainly fixing various validations based on Jesse's comments (7.46 KB, patch)
2011-10-12 21:55 UTC, Geertjan Wielenga
Details | Diff
Fixing role.isEmpty and javadoc about empty string (7.42 KB, patch)
2011-10-17 07:01 UTC, Geertjan Wielenga
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Geertjan Wielenga 2011-10-11 22:03:20 UTC
Right now, only one role (i.e., a String, instead of Array of Strings) is accepted for the "role" attribute on @TopComponent.Registration.
Comment 1 Stanislav Aubrecht 2011-10-12 08:21:01 UTC
patch is welcome
Comment 2 Geertjan Wielenga 2011-10-12 12:51:29 UTC
Created attachment 111935 [details]
Patch for multiple roles
Comment 3 Jesse Glick 2011-10-12 12:59:45 UTC
This was intended as an API review.
Comment 4 Jesse Glick 2011-10-12 13:09:02 UTC
[JG01] Default value of {} is wrong here. It means that if no role list is specified, the component is not registered at all! I think you meant to leave it as "" (or equivalently {""}).


[JG02] Javadoc needs to be clarified about permitted values, and LayerGenerationException should be thrown if these constraints are not met:

a. A zero-length array should probably be forbidden (for then no registrations would be made, which seems meaningless).

b. "" is permitted to mean that the window should be shown in all roles, but this must be the sole entry (i.e. roles={"", "admin"} makes little sense).

c. Duplicate entries should be forbidden.


[JG03] As an alternative to JG01 + JG02a + JG02b, treat {} as meaning registration in the default layout, like current role="", use that as the default value, and forbid empty strings in the array. I am not sure which is more intuitive.


[JG04] testTCRegisteredInRoleFine ought to check that there is a registration in UnitTestRole2 (no need to check its details, just assertNotNull ought to suffice.)
Comment 5 Geertjan Wielenga 2011-10-12 13:27:54 UTC
Created attachment 111939 [details]
A new patch, incoporating Jesse's comments
Comment 6 Geertjan Wielenga 2011-10-12 13:31:02 UTC
[GW01] Took the [JG03] approach, i.e., the same as original, if no roles are provided, i.e., roles is empty, then the default layout is used.

[GW02] Incorporated [JG04], i.e., changed the test.

[GW03] Also incremented the version number and changed the javadoc.
Comment 7 Geertjan Wielenga 2011-10-12 13:35:08 UTC
Ah, forgot to check for duplicate values.
Comment 8 Geertjan Wielenga 2011-10-12 13:54:08 UTC
Created attachment 111941 [details]
Duplicate role check done
Comment 9 Geertjan Wielenga 2011-10-12 13:54:38 UTC
[GW04] Duplicate role check done.
Comment 10 Geertjan Wielenga 2011-10-12 14:11:44 UTC
Created attachment 111942 [details]
Removing a test that I added previously erroneously
Comment 11 Jesse Glick 2011-10-12 17:36:49 UTC
(In reply to comment #6)
> [GW01] Took the [JG03] approach, i.e., the same as original, if no roles are
> provided, i.e., roles is empty, then the default layout is used.

I am confused by that statement. According to the latest patch, you _did_ accept JG01 + JG02a + JG02b (treat {""} as default layout) and did _not_ accept JG03 (treat {} as default layout).

You also did not implement JG02a or the validation part of JG02b, i.e. rejecting roles={"", "something"} or roles={}. Whatever you pick, be explicit in the Javadoc about what you are expecting.


[JG05] 6.47 spec version needs to be used in both apichanges.xml and @since. Update the date of change in apichanges.xml, too - effectively replacing the original API change with this one.


[JG06] List<String> uniqueRolesList should be a set, not a list. Also note that Collection.add returns a boolean. Therefore the compact idiom is:

Set<String> uniqueRoles = new HashSet<String>();
for (String role : roles) {
  if (!uniqueRoles.add(role)) {
    throw new LayerGenerationException(...);
  }
  // continue...
}

With JG02a and JG02b, it might be:

if (roles.length == 0) {
  throw new LayerGenerationException(...);
}
Set<String> uniqueRoles = new HashSet<String>();
for (String role : roles) {
  if (!uniqueRoles.add(role)) {
    throw new LayerGenerationException(...);
  }
  // as now...
}
if (roles.contains("") && roles.size() > 1) {
  throw new LayerGenerationException(...);
}

With JG03 it would be something like:

if (roles.length == 0) {
  generate("Windows2", ...);
} else {
  Set<String> uniqueRoles = new HashSet<String>();
  for (String role : roles) {
    if (!uniqueRoles.add(role)) {
      throw new LayerGenerationException(...);
    }
    if (role.isEmpty()) {
      throw new LayerGenerationException(...);
    }
    generate("Windows2/Roles/" + role, ...);
  }
}
...
private void generate(String rootFolder, ...extract method params...) {
  ...extract method body...
}


[JG07] By the way, mechanics of patch creation: in TopComponentProcessorTest.java you have

---%<---
-<<EOL>>
+    <<EOL>>
---%<---

which is an undesirable formatting change, and also means you have neglected to configure the IDE's editor correctly; set Editor > General > Remove Trailing Whitespace to From Modified Lines Only. Not important in this case (just one line) but good to practice for the future.
Comment 12 Geertjan Wielenga 2011-10-12 21:55:43 UTC
Created attachment 111955 [details]
Mainly fixing various validations based on Jesse's comments
Comment 13 Geertjan Wielenga 2011-10-12 21:58:21 UTC
[GW05] Fixed all items listed by Jesse above and treating {} as the default layout.
Comment 14 Jesse Glick 2011-10-12 22:41:45 UTC
By the way it is convenient to set <date> to seven days from the day the API review was opened, since that is when you expect to commit it.


"an empty string for the default layout" in Javadoc is no longer correct.


if (uniqueRoles.contains("") && uniqueRoles.size() > 1) is not correct, since now "" is illegal at all times. That is why I suggested

    if (role.isEmpty()) {
      throw new LayerGenerationException(...);
    }
Comment 15 Geertjan Wielenga 2011-10-17 07:01:13 UTC
Created attachment 112110 [details]
Fixing role.isEmpty and javadoc about empty string
Comment 16 Geertjan Wielenga 2011-10-19 10:12:44 UTC
http://hg.netbeans.org/core-main/rev/ad5a622cc05f
Comment 17 Geertjan Wielenga 2011-10-19 10:13:45 UTC
Committed the enhancement, after verifying with Jesse that he didn't have any other comments, and because it is a week after API review began.
Comment 18 Jesse Glick 2011-10-19 14:18:03 UTC
In that case it is FIXED.
Comment 19 Jesse Glick 2011-10-19 14:18:54 UTC
Linking to original issue.
Comment 20 Quality Engineering 2011-10-20 14:28:36 UTC
Integrated into 'main-golden'
Changeset: http://hg.netbeans.org/main-golden/rev/ad5a622cc05f
User: geertjan@netbeans.org
Log: #203553: Multiple roles need to be supported in new window system role annotation


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2012, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo