Bug 203553

patch is welcome

Created attachment 111935 [details]
Patch for multiple roles

This was intended as an API review.

[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.)

Created attachment 111939 [details]
A new patch, incoporating Jesse's comments

[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.

Ah, forgot to check for duplicate values.

Created attachment 111941 [details]
Duplicate role check done

[GW04] Duplicate role check done.

Created attachment 111942 [details]
Removing a test that I added previously erroneously

(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.

Created attachment 111955 [details]
Mainly fixing various validations based on Jesse's comments

[GW05] Fixed all items listed by Jesse above and treating {} as the default layout.

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(...);
    }

Created attachment 112110 [details]
Fixing role.isEmpty and javadoc about empty string

http://hg.netbeans.org/core-main/rev/ad5a622cc05f

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.

In that case it is FIXED.

Linking to original issue.

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