Created attachment 108482 [details]
proposed patch & api changes

By the way there is no clear reason why bug #198856 is separate from this. You do not need a separate issue just to file an API review.


[JG01] Using an empty file to mask an item with the same name is nonstandard; normally you would use *_hidden files. I would expect the behavior of a Roles/* folder to be like MultiFileSystem.

In fact I have already written a more generic mechanism that lets you dynamically merge in customizations to _any_ part of the system file system; http://blogs.oracle.com/geertjan/entry/jdesktoppane_on_the_netbeans_platform alludes to this.

I understand however that the window system is a little "special" in how it uses the SFS; at least, storing customizations in $userdir/config/Windows2Local/ rather than simply in $userdir/config/Windows2/ is inconsistent with all other kinds of SFS-based configuration in the platform. I am not sure about the historical reasons for this choice, nor whether the window system even listens to changes in the Windows2 folder.


[JG02] WindowSystemListener is using a nonstandard variant of the event listener pattern. All the methods, not just beforeLoad, should take WindowSystemConfiguration. And this should be renamed WindowSystemEvent. Anyway calling a setter on an event object is very unusual.

Or, if the only use case for WindowSystemListener is setting the role, then do this entirely differently:

class WindowManager {
  /** if during startup, will take effect before the window system loads */
  public void setRole(String roleName);
}

It is not even clear to me why you would do this from a ModuleInstall anyway; there could only be one module doing so! The typical use case would be switching to a different role in response to some user action, right? (For example, Eclipse's Debug perspective is opened when you debug a project.)


[JG03] What is mode-properties2_4.dtd for? There is no mention of this elsewhere in the issue or diff.

(It would be easier to see any changes from mode-properties2_3.dtd if you had used 'hg cp', by the way - then it would appear as a copy-plus-edit, rather than as a fresh file with no history. You also need to define diff.git=1 in your .hgrc.)


[JG04] Documentation about an API feature belongs in certain places - actual Javadoc comments where tied to a particular class or method, */src/**/doc-files/api.html otherwise. apichanges.xml should be used only to _mention_ that a change was made, not as the primary reference.

JG01: window system does listen to changes in Windows2 folder. so when a module is enabled/disabled new windows may open/close at the right locations.

a role must be allowed to hide some files from the default window system layout. if we can use some file system trick to merge Windows2 folder with Windows2/Roles/SomeRole folder then it would be great. otherwise the window system would have to merge those folders 'manually' but it wouldn't see *_hidden files. hence the workaround with empty files.

JG02: the main purpose of WindowSystemListener is to enable better integration of NetBeans winsys into the next version of JDeveloper.

to switch to a new role the window system must first store current user settings under the current role, set new role name and then load window layout from the new role. so the best time to switch a role is just before window system loads.
if somebody wants to implement window layout/perspectives switcher, they need to register WindowSystemListener, show a dialog window listing available roles and then invoke 'Reload Windows' action. during the reload, the window system will ask for a new role using the registered listener.

JG03: the changes in mode DTD are actually unrelated to this API change. they're needed to implement some other window system features (minimize/undock/drag window group)

JG04: i'll update the documentation

(In reply to comment #3)
> if we can use some file system trick to merge Windows2 folder with
> Windows2/Roles/SomeRole folder then it would be great.

Well, try using MultiFileSystem as this would handle merging automatically, including standard *_hidden masks.

> if somebody wants to implement window layout/perspectives switcher, they need
> to register WindowSystemListener, show a dialog window listing available roles
> and then invoke 'Reload Windows' action. during the reload, the window system
> will ask for a new role using the registered listener.

This seems very roundabout, and just confirms that the use cases do not imply the need for WindowSystemListener - a simple WindowManager.setRole(String) call would suffice, where the WM impl would know to do things in a certain order. You could call setRole during ModuleInstall.restored and the winsys would be loaded with that role active, though this seems like something no module should do - the winsys should rather simply persist the active role from the last shutdown; or a switcher dialog could call setRole at any time.

(If the _only_ use case for switching roles is from a switcher dialog, and not - say - in response to a domain-specific user gesture as in Eclipse, then it would be better not to make any API to switch roles at all; put the switcher dialog directly into core.windows.)

> JG03: the changes in mode DTD are actually unrelated to this API change.
> they're needed to implement some other window system features
> (minimize/undock/drag window group)

OK, so remove them from this diff.

(In reply to comment #4)
> Well, try using MultiFileSystem as this would handle merging automatically,
> including standard *_hidden masks.

We were thinking about this with Standa and there does not seem any way to reuse _hidden files for this purpose.

> This seems very roundabout, and just confirms that the use cases do not imply
> the need for WindowSystemListener - a simple WindowManager.setRole(String) call
> would suffice, 

The problem is the scope. setRole method is available all the time and people might believe they may call it (which is not what we want). The window system is still in the charge and only asks some other modules to advice it at a specific time what role to use. The callback on an "event" object of a listener exactly reflects this "call this method only now" requirement. Sure we might have:

public abstract class RoleNameProvider {
  protected abstract String selectedRoleName();
}

and find it through Lookup.getDefault()...

(In reply to comment #4)
> (In reply to comment #3)
> > if we can use some file system trick to merge Windows2 folder with
> > Windows2/Roles/SomeRole folder then it would be great.
> 
> Well, try using MultiFileSystem as this would handle merging automatically,
> including standard *_hidden masks.

can you pls provide a code snippet illustrating how to merge Windows2 folder with Windows2/Roles/SomeRole. the solution must also fire correct events when an enabled modules adds/removes some content in the Roles folder.

> 
> > if somebody wants to implement window layout/perspectives switcher, they need
> > to register WindowSystemListener, show a dialog window listing available roles
> > and then invoke 'Reload Windows' action. during the reload, the window system
> > will ask for a new role using the registered listener.
> 
> This seems very roundabout, and just confirms that the use cases do not imply
> the need for WindowSystemListener - a simple WindowManager.setRole(String) call
> would suffice, where the WM impl would know to do things in a certain order.
> You could call setRole during ModuleInstall.restored and the winsys would be
> loaded with that role active, though this seems like something no module should
> do - the winsys should rather simply persist the active role from the last
> shutdown; or a switcher dialog could call setRole at any time.
> 
> (If the _only_ use case for switching roles is from a switcher dialog, and not
> - say - in response to a domain-specific user gesture as in Eclipse, then it
> would be better not to make any API to switch roles at all; put the switcher
> dialog directly into core.windows.)

i don't plan to add any UI for role switching to NetBeans IDE - unless our HIE experts say it's a good idea.
but i want allow platform app developers to implement role switching and i can't find any strong argument against having method WindowManager.setRole(String)...

Created attachment 108516 [details]
new patch

(In reply to comment #5)
> setRole method is available all the time and people
> might believe they may call it (which is not what we want).

Apparently you wanted _someone_ to call it. So back to the question - what exactly are the use cases here?

> The window system
> is still in the charge and only asks some other modules to [advise] it at a
> specific time what role to use.

What is that specific time? The only code sample given so far is from ModuleInstall.restored, which seems clearly inappropriate.

(In reply to comment #6)
> can you pls provide a code snippet illustrating how to merge Windows2 folder
> with Windows2/Roles/SomeRole. the solution must also fire correct events when
> an enabled modules adds/removes some content in the Roles folder.

Code similar to that in the blog entry mentioned in JG01 should do this. That example used separate layer.xml fragments as the only layer provider; it is also possible to use a subdir of the SFS.

(I did not include that example since there is a bug in DataShadow - if shadow.primaryFile.fileSystem != Repository.default.defaultFileSystem then the interpretation of the originalFile attr is broken, which prevents @ActionReference etc. from working. But the winsys does not use shadows anyway.)

So use something along these lines (did not test this exact code yet):

final String role = ...;
FileSystem sfs = FileUtil.getConfigRoot().getFileSystem();
FileSystem roled = new MultiFileSystem(new FileSystem[] {sfs}) {
  public @Override FileObject findResource(String name) {
    return super.findResource(convert(name));
  }
  protected @Override FileObject findResourceOn(FileSystem fs, String res) {
    return super.findResourceOn(fs, convert(res));
  }
  String convert(String path) {
    if (path.startsWith("Windows2/")) {
      return "Windows2/Roles/" + role + path.substring("Windows2".length());
    } else {
      return path;
    }
  }
  public @Override void addNotify() {}
  public @Override void removeNotify() {}
};
FileSystem merged = new MultiFileSystem(new FileSystem[] {roled, sfs}) {
  public @Override void addNotify() {}
  public @Override void removeNotify() {}
};

> i don't plan to add any UI for role switching to NetBeans IDE [...]
> but i want allow platform app developers to implement role switching

If that is the only use case, then you need no API - just create the UI in core.windows, register an action to display it with @ActionRegistration but no @ActionReference, and leave it to an app to choose to display this action in e.g. the Windows menu.

> i can't find any strong argument against having method
> WindowManager.setRole(String)...

Then you probably do not need a listener; you just need to document in setRole that the visual effects of the role switch may be delayed a couple of seconds while the old user-customized layout is saved, new role defaults are loaded, and any new user-customized layout is loaded.

Stando, if you Cc-ed me regarding the question about Role switching UI in NetBeans, then my answer is that we don't plan to have it in NetBeans IDE. Also my understanding is that JDev wants to permit role switching only during startup (first, or every time?), not anytime when JDev is running. I hope I'm not confusing roles with task/editor based layouts.

(In reply to comment #9)
> permit role switching only during startup

This use case would be better served by a CLI option interpreted by core.windows:

  --role db-admin

(In reply to comment #8)
> > i don't plan to add any UI for role switching to NetBeans IDE [...]
> > but i want allow platform app developers to implement role switching
> 
> If that is the only use case, then you need no API - just create the UI in
> core.windows, register an action to display it with @ActionRegistration but no
> @ActionReference, and leave it to an app to choose to display this action in
> e.g. the Windows menu.

there are many ways to implement role switching UI - modal dialog box, dropdown menu, combo box in a toolbar or even automatic switching when some other action is triggered in the platform app. i think it's best to provide API to platform developers and let them implement the UI themselves.

> 
> > i can't find any strong argument against having method
> > WindowManager.setRole(String)...
> 
> Then you probably do not need a listener; you just need to document in setRole
> that the visual effects of the role switch may be delayed a couple of seconds
> while the old user-customized layout is saved, new role defaults are loaded,
> and any new user-customized layout is loaded.

i still need the listener to integrate the NB window system into JDev. i could do it without a new public API using implementation dependency on core.window. but i think platform developers would benefit from the listener API as well.

unless there are no more objections, i'll start integrating the new patch later on this week.

setRole/getRole do not have @since tag.

(In reply to comment #13)
> setRole/getRole do not have @since tag.

i'll fix that

Details of the semantics of roles (folder locations, masking, etc.) should be in WindowManager.setRole, not in apichanges.xml.

apichanges.xml should use <class .../> on WindowManager, WindowSystemListener, and WindowSystemEvent.

Also, <issue number="198856"/> should be <issue number="198859"/>.

And do not forget to update <date>.

ModuleInstall.restore - should be "restored".

BTW do not use the IDE's built-in diff for creating patches for review. Configure diff.git=1 and use 'hg di' (or MQ, etc.).

API changes are in 28e12812f968

the actual implementation will follow later

WindowSystemListener impl: 51065bf0a248

[JG05] Consider following http://wiki.netbeans.org/CompatibilityPolicy#Technical_procedure_for_making_an_incompatible_phased_change to remove org.openide.windows.Workspace (and related methods), which is effectively superseded by this new API.

There might be other deprecated stuff in o.o.windows that is truly unused and could be removed at the same time (but TopComponent.requestFocusInWindow probably needs to be undeprecated).

Integrated into 'main-golden'
Changeset: http://hg.netbeans.org/main-golden/rev/28e12812f968
User: S. Aubrecht <saubrecht@netbeans.org>
Log: #198859 - API changes to support multiple window layouts

I am missing a "getRoles" method, i.e., this method will provide a list of all the roles defined in the system, not just the current role, which is what "getRole" does. I think "getRoles" is essential (e.g., to display in a GUI component so that the user can choose a role). Hence, reopening.

getRoles() == FileUtil.getConfigFile("Windows2/Roles").getChildren()

also note you can switch to a role which does not have a subfolder under Windows2/Roles. in that case the initial window layout will be the same as the default one but user's changes will be persisted to a different location.