yardo, do you need to listen on changes of the debugger lookup in Ergonomics IDE?
Issue #156272 occurs when the lookup content is changed, therefore I guess that yes. Currently it is possible to listen
on the list returned from org.netbeans.spi.debugger.ContextProvider.lookup() via cast to Customizer, but it's not nice API.

Therefore I'm thinking about creating LookupContextProvider extending ContextProvider with o.o.u.Lookup getLookup() method.
That lookup would be org.openide.loaders.FolderLookup + DebuggerMetaInfServicesLookup.
I guess it's O.K. to use deprecated FolderLookup, since I need to combine it with my own DebuggerMetaInfServicesLookup
instead of MetaInfServicesLookup and therefore can not use Lookups.forPath(). Right?

The one thing that I know of that is missing from MISL is support for factory methods, which we could easily add. Much
better than copying MISL into api.debugger; there is some subtle code (e.g. checking for potential CCEs from parallel
module loads) that would be best done in one place only.

MISL does support masks, contrary to your assertion in issue #153093:

your.Impl
#-some.unwanted.Impl

and @ServiceProvider even supports this syntax to some extent. MISL also supports ordering.

I am not sure what "lazy instantiation" means in this context. MISL.P.getInstance instantiates the class on demand.

That would be definitely more comfortable and would prevent from code duplication.
But in order to keep compatibility I'd also need to have

some.unwanted.Impl-hidden
behaving the same as:
#-some.unwanted.Impl

"lazy instantiation" assures that the instances are not created when one calls e.g. ContextProvider.lookup.size(). Only
after one iterates through the list and need to actually retrieve the instance object, then we create the instance. But
skimming through MISL it looks like it does the same.

Then I'd have to copy only RecognizeInstanceObjects to combine:
FolderLookup with Lookups.metaInfServices("META-INF/debugger/" + path)
instead of:
FolderLookup with Lookups.metaInfServices("META-INF/namedservices/" + path)

"In order to keep compatibility" you would anyway need to retain the current code which scans META-INF/debugger intact.
You would additionally delegate to Lookups.forPath(folder), which scans META-INF/namedservices/${path} as well as
${path} in the SFS. The two contributors are disjoint. Old entries in M-I/d would have the exact same syntax and
capabilities as now, whereas those in M-I/ns and SFS would have the capabilities specified by MISL and FolderLookup. We
need only ensure that L.fP provides a superset of the current abilities (e.g. add support for factory methods in MISL),
and then you can change known modules to use L.fP-friendly registration (probably via annotations), and perhaps print a
warning to the log for modules still using M-I/d registration.

> "In order to keep compatibility" you would anyway need to retain the current code which scans META-INF/debugger intact.

Well, that would create a problem with "-hidden" lines. Some debuggers use this to remove some default data models.
If known modules registers services into META-INF/namedservices/${path} and remove them from META-INF/debugger, the
"-hidden" line used by an unknown debugger under META-INF/debugger will not remove the service.
This is why I was trying to have all logic at one place and keep it under META-INF/debugger.
Now I've recalled that the hidden items are remembered in o.n.a.d.Lookup.LookupList across several sources when lookups
are merged in CompoundLookupList. The logic seems to be too complicated to put into MetaInfServicesLookup. It would be
analogous to assuring that MetaInfServicesLookup shares "removeClasses" when under ProxyLookup.

Uff, it looks like debugger lookup is too specialized. :-(

Other approach can be to only convert o.n.a.d.Lookup.MetaInf to AbstractLookup (ala MetaInfServicesLookup) and add
Lookups.forPath() just because of SFS.
Or making MetaInfServicesLookup public with some protected methods to handle the reading process from the files.

"If known module registers services into M-I/ns and remove them from M-I/d, the *-hidden line used by an unknown
debugger under M-I/d will not remove the service" - true, but is this a serious enough compatibility concern to merit a
rather more complicated solution? I.e. are there likely to be many infrequently-maintained third-party modules masking
debugger services from nb.org modules?

BTW mentlicher points out correctly that NamedServicesProvider.find should be delegating to all NamedServicesProvider's
that can be found, not just the first one. Fixing this would make it possible for M-I/d registrations to be part of
L.fP, which would be appealing (no need for a special API to look up debugger services at all), though it would not
address the cross-provider masking issue.

Per discussion with Yarda, I'll improve the current org.netbeans.api.debugger.Lookup.MetaInf to include also
Lookups.forPath(). I'll keep the current logic of o.n.a.d.Lookup.MetaInf, because it
- Creates instances of specified classes either via a default constructor or constructor taking ContextProvider as an
argument, passing the session lookup in.
- Creates instances via methods taking either nothing or ContextProvider as an argument.
- Manages "hidden" services across compound lookup.

However, the inclusion of Lookups.forPath() is possible by registering a special proxy implementation of the desired
interface, which will delegate to the old String used to create the instance in the way how
o.n.a.d.Lookup.MetaInf.createInstance() does it.

Created attachment 76040 [details]
A patch adding Lookups.forPath() to Lookup.MetaInf. Test included.

I've created a patch that adds Lookups.forPath() to debugger's Lookup.MetaInf as suggested.
It seems to work fine, test is passing.

Thinking about debugger's Lookup, IMHO it would deserve standard o.o.u.Lookup interface. I mean add something like:

interface LookupContextProvider extends org.netbeans.spi.debugger.ContextProvider {
   o.o.u.Lookup getLookup();
}

The current "List lookup(String, Class)" method does not provide a nice way to listen on the lookup result. One have to
cast the List to Customizer to attach a PropertyChangeListener.
IMHO this deserves API improvement, since in Ergonomics IDE it's a necessity to listen on the result.

Thoughts?

I'll prepare the LookupContextProvider with the o.o.u.Lookup implementation based on o.n.a.d.Lookup if we agree on this.

I'm not sure what the purpose of the latest patch is. It seems that it would not serve to really move toward a normal
Lookup registration pattern, since the debugger SPI interfaces cannot be constructed as normal singletons anyway, so you
would be stuck always using ContextAwareServiceHandler.createService for everything. Maybe Yarda sees what the goal here is.

I'm not really excited about this solution either. But this minimizes the impact on current debugger architecture while
allows to use Lookups.forPath() and therefore to register the services more effectively from performance point of view
(since reads from META-INF are slow according to Yarda, layers are cached).

ContextAwareServiceHandler.createService automatically creates "factories" for individual services. Otherwise I would
have to create a bunch of factory classes. In fact I'd like this solution more, it'd me more clean - to have something like:
abstract class DebuggerServiceProvider {
  protected abstract DebuggerService createSessionService(ContextProvider context);
}
I'd register implementations of DebuggerServiceProvider into the lookup and debugger would call
createSessionService(context) on it to get the live instance.

But there are several problems associated with that - we'd have to pollute debugger with a bunch of new classes and the
"hidden" concept would likely be reworked also.
Current usage are:
grep -r hidden main/*/src/META-INF
As soon as we register these services via standard lookup, this concept stops working. Using
ContextAwareServiceHandler.createService is compatibe with the current "hidden" concept.

If it is true that "reads from META-INF are slow" then we have a problem much bigger than the debugger, since
META-INF/services/* is used quite heavily.

Regular global lookup supports hiding implementations, using both layer registration and M-I/s (incl. @ServiceProvider),
so I'm not sure what the issue there is.

Re: "reads from META-INF are slow". Reads from META-INF/services are fast. But reads from META-INF/debugger/ are slow, 
as they are not cached.

Re. "where is it heading?" Martin will attach working patch soon, then we'll be able to tell. But I was looking over 
his shoulder and as far as I can tell, end users and performance shall be pleased with what Martin managed to achieve.

Created attachment 76189 [details]
Patch of debugger lookup changes with annotation processors

In addition to the first patch, I've improved the debugger lookup logic more towards the factory pattern. With the
annotations generating the declarative registration, it should be more flexible to register and handle debugger services.

The main registration interface org.netbeans.debugger.registry.ContextAwareSupport is improved to be typed and serve
more as a factory. It has the suggested "T forContext(ContextProvider context)" method.

All debugger services that are implementations of interfaces (e.g. all models from spi.viewmodel) should be registered
via org.netbeans.spi.debugger.DebuggerServiceRegistration. That will generate registration via
org.netbeans.debugger.registry.ContextAwareServiceHandler.createService(). That proxy handler creates the instances as
necessary.

For abstract classes we have to create individual registration mechanism via <Service_name>ContextAware classes (see
e.g. ActionsProviderContextAware, others are similar).
AttachTypeContextAware is special, it's display name can be registered declaratively and therefore
AttachType.getTypeDisplayName() was changed to return null. That is important in Ergonomics IDE not to create the actual
instances of AttachType implementation classes when we just want to see which attach types are registered. The instance
is created as soon as someone asks for getCustomizer().

Because instance creation is now distributed through all these "factory" classes, they all delegate to
ContextAwareSupport.createInstance() so that we have the logic at one place. I did not put it into public API, since it
should be of no interest for others, it's to be used only by factories. I've put an implementation dependency of
spi.debugger.ui module on api.debugger to be able to access the ContextAwareSupport.
However, I've realized that JPDA debugger also registers it's own services, e.g.
org.netbeans.modules.debugger.jpda.ui.SourcePath or org.netbeans.spi.debugger.jpda.SmartSteppingCallback.
Therefore we'll have to probably make some extra registration module with friend dependency or put it into public API of
spi.debugger module.

SourcePath should probably directly implement ContextAwareService, since it registers itself under it's class.
So we should also create an annotation that will generate registration of ContextAwareService. And we'll also need to
have ContextAwareService public.

Well, now I've realized, that maybe we do not need the *ContextAware classes at all. The service classes can directly
implement ContextAwareService interface and call the appropriate constructor in forContext() method. That way we would
not need the ugly ContextAwareSupport.createInstance() method. I'll try to change that if I'll not run into some problem...

So why can't we start caching META-INF/debugger/* too? Seems easy enough.

I am just wondering whether we actually need to change the underlying registration mechanism, so long as we can create
easy-to-use annotations for the SPI side. Would be easier, less risky, and more compatible to make annotations use the
existing mechanism. The question is whether interoperability with Lookup buys you some advantage from the API side, or
enables some performance optimization which is impossible otherwise.

Please do not introduce new impl deps. (Probably we should be pushing for all existing impl deps to be removed.) Factor
out any necessary interfaces into a separate module with friends.

BTW you probably want to configure

---%<--- ~/.hgrc or %USER%\mercurial.ini etc.
[diff]
git=1
---%<---

Thanks for your comments Jesse.
caching META-INF/debugger/* will certainly help, but AFAIK it'd not be enough. Yarda can you comment?
As I got it, we need to have a mechanism of "lazy" lookup registration, which is achieved through *ContextAware classes.

I've tried to change ActionsProvider to directly implement ContextAwareService. That would allow a cleaner
implementation, though implementation classes would be created sooner - from the Lookup. Yarda explained that for
ergonomics IDE we need at least semi-declarative registration of AttachType that will allow to read the display name
without actually instantiating the appropriate class. With AttachType directly implementing ContextAwareService that
would not be possible. That's why the intermediate *ContextAware proxy classes were suggested. But I agree that this
whole change is sort of risky and not really clean. We'll pollute our code base with a buch of proxy classes and a
support for creation of the context-aware instances.

In any case, I'll remove the impl. dependency and put it either into public API or a new friend module.

BTW, I've removed git=1 from ~/.hgrc, since it made diff much slower. I'll add it at least for the API rewiew.

Created attachment 76193 [details]
Prototype of ActionsProvider implementing ContextAwareService.

The advantage of this protopype of ActionsProvider would be that it can be registered by the standard ServiceProvider
into the default Lookup and therefore we'd not need the special ActionsProvider.Registration and annotation processor.
However, every impl. of ActionsProvider would have to declare a default constructor and one instance would be created
just to call the forContext() method. Not really nice. :-( Can be an unexpected behavior for implementors.

Which is why true factories would be better - you have one stateless singleton, and 0+ instances of a different class
with the session context.

While I was writing some tests, I've found out that Lookups.forPath() finds registered classes from all subforders. If
that the correct behavior? It seems to be that way in FolderLookup.

Since this behavior is different from what o.n.a.d.Lookup does, I'm afraid I can NOT use Lookups.forPath() at all.

Well, it looks like I can check Lookup.Item.getId(). That contains the path. Though it's a sort of hack IMHO, it will
rely on how getId() for objects registered on SFS looks like. I'm not really excited abut this, but it looks like
restriction of the lookup by getId() is the only way how we can use Lookups.forPath() in debugger.

An other, better solution would be to restrict Lookup.forPath() to the given path. But inspecting subfolders is
intentional, right? Anyway, this would be an incompatible change.

Created attachment 76208 [details]
The proposed change of debugger lookup, including annotations and tests.

Created attachment 76209 [details]
Just the API part of the change, that is associated with using Lookups.forPath() in searching for debugger services.

Please review this change.
Due to reasons explained at http://www.netbeans.org/issues/show_bug.cgi?id=153093#desc7 debugger's own lookup system
(that reads services from META-INF/debugger is enhanced to also read services from Lookups.forPath("Debugger/...")
ContextAwareService and ContextAwareSupport classes are introduced as well as a bunch of support classes in the
implementation.

Y01 I'd rather see total laziness - e.g. "/* If total laziness is necessary" being on by default. I do not expect one 
additional method call will be significant and with laziness we can do more and optimization by moving more and more 
things into the @annotation. Imagine ActionsProvider.Registration to list array of always enabled actions... but this 
can wait after the integration - we'll remeasure the impact on loaded classes and then we can reevaluate.

Y02 I'd rather use "Debugger/" + path + "/Lookup" for Lookups.forPath, as the code would be simplified. But it is up 
to you, I guess. Btw. please specify in arch.xml what is the stability of current registrations format in the layer.

Y03 There is missing javadoc in many of the new @annotations

Y01 There is already laziness implemented for some abstract classes - ActionsProvider.ContextAware,
AttachType.ContextAware (with the display name taking from the annotation), BreakpointType.ContextAware (also taking
display name from the annotation) and several others. JPDA classes do not behave lazy, since it's not necessary IMHO.
We can turn on the laziness for interfaces in ContextAwareServiceHandler easily if necessary, the code is prepared and
just commented there. I'd leave it for later performance tuning.

Y02 "Debugger/" + path + "/Lookup" can be used for Lookups.forPath to get rid of the recursive behavior. But if there's
a chance that we'll have something like "Lookups.forPath(String path, boolean recursive)" some day, I'd rather keep the
current PathLookup translation, that'll be replaced by that new method.
If we know that we do not want to add anything like that it'll probably be better to use ".../Lookup" paths so that we
do not hurt performance.
The stability of current registrations format in the layer should be the same as in META-INF/debugger, I'll add it into
arch.xml.

Y03 Missing javadoc will be added before commit to trunk. If I have it sooner, I'll attach it here... Javadoc of all
annotations will be mostly the same.

Thanks for the review. Jesse, do you have some comments to the final change?
I plan to commit this on Sunday, with complete Javadoc and at least some JPDA services rewritten to this new concept.

I'm sorry I did not find time to look at this (and issue #153093) in the past few days. I will try to do so today.

[JG01] apichanges.xml#//class[@name] should mention only top-level classes.

To JG01 - in fact this should be reported to you if you try building Javadoc for the module, which you should do
whenever making API changes. Your current code would cause nbms-and-javadoc builds to fail. Check that links work,
Javadoc comments are formatted readably, etc.

Use {@code ...} for code sequences. Entering e.g. "META-INF/debugger/<DebuggerEngine-id>/LazyActionsManagerListener"
will not only use a proportional font, it will not escape &lt;.

"registerred" and "preferrably" are misspelled.


BTW you can use 'hg diff' to generate patches like this, even if you have done some merges from trunk since you started.
You need only identify the base revision. (Especially using --git is a good idea as it correctly handles binary files
and file renames.)


[JG02] Can you be assured that the "unsupported" PathLookup methods will never be called? Is this all about the
recursive nature of FolderLookup? If so, I would rather introduce a Lookups.forPath(path,recursive) now.

(I also find the folder-recursive behavior to be counterintuitive and generally unwanted. The lookup result is a
flattened list anyway - you cannot use the subfolder conversion facility that FolderInstance supports - so using
subfolders is not very useful. I think it is only done that way because of our brain-damaged historical Services folder,
which loads all subfolders into one big lookup and then relied on InstanceCookie.Of to filter out unwanted entries.)


[JG03] MirroredTypesException (as opposed to MirroredTypeException) is never thrown by current impls of JSR 269 - this
is a filed JRE bug. So for now you do need to use Element.getAnnotationMirrors().

I'm not sure you can rely on elementValues.get(ee).toString() having a particular format. This should probably be
rewritten to get the actual List<AnnotationValue> or whatever it is.


[JG04] Consider using instanceAttribute wherever possible. It may not apply in your cases, if you are not registering
true singletons but rather require constructors or methods accepting ContextProvider.


[JG05] isClassOf does not look right. Are you considering interfaces as well as superclasses? Did you try
TypeUtilities.isAssignable?


[JG06] commaSeparated, whatever it does, appears to be unused.


[JG07] instantiableClassOrMethod does not appear to handle methods!


[JG08] Although I'm not sure exactly how you're using it, it may be profitable for ContextAwareService to have a method

Class<T> type();

This trick allows you to find the service interface at runtime from a heterogeneous Collection<ContextAwareService<?>>.
That can be used in type-safe code (no warnings, no @SuppressWarnings("unchecked")).


Still looking, sorry for being so late...

[JG09] Try

private static String[] splitClasses(String classes) {
  return classes.split("[, ]+");
}


[JG10] "return cnt == annotations.size();" is probably wrong. I think you just want to unconditionally return true,
since you have examined all the potentially annotated classes.


[JG11] DebuggerServiceRegistration.types() says "interfaces that this class implements" but the processor does not seem
to verify that.


[JG12] Do not use intvalue("position", position); use .position(position), which correctly handles the Integer.MAX_VALUE
default value (by not doing anything).


[JG13] Second isClassOf impl does not actually check anything for METHODs. Should check return value.

Seems like we may need some new convenience APIs in LayerBuilder for producing class/method names as strings, checking
return values assignable to various types, looking for constructors/static methods with particular param patterns, etc.
I had to do some extra work for @ProjectServiceProvider. This kind of code tends to be tricky to write correctly and
probably needs to be centralized. At the least, you should try to share common impls among the various
DebuggerProcessor's you are writing now.


[JG14] Please don't create yet another clone of org.netbeans.api.project.TestUtil. If there is some generic
functionality, consider whether it should not be in some shared test utility class, e.g. in openide.util or
openide.filesystems.

In this case, DEFAULT_LOOKUP can probably be replaced with the standard MockLookup. Also note that unit tests now load
from XML layers referenced from manifests in the test runtime CP by default, which among other things means that you can
just annotate a test service in test/unit/src and the processed version should be available with no additional setup -
looks like you are already making use of this.


[JG15] Don't forget to adjust apichanges.xml#//date to reflect the actual date of integration.


[JG16] @SupportedAnnotationTypes on org.netbeans.modules.debugger.jpda.apiregistry.DebuggerProcessor looks wrong.


[JG17] The use of new UnsupportedOperationException("Not supported yet.") in impls like
SmartSteppingCallback.ContextAware looks wrong. From what I understand this to be doing, it will _never_ be supported,
and should never be called. That means an 'assert false;' would be more appropriate.


That's all for now. Since I don't fully understand the basic semantics of what you are trying to do, I will leave it to
Yarda to comment on that, and confine myself to superficial comments about the patch.

Jesse, thanks for your detailed comments.

[JG01] Javadoc generated and errors corrected. Thanks for the spelling corrections.

BTW: I've integrated this API change into my local HG repository in a several commits. I did not find a way how to get
one diff of all changes. Export only prints individual diffs of every change separately. With CVS branches all was much
easier. I guess I have to learn ho to work with HG patches. The looong list of q... commands discouraged me from that so
far.

[JG02] PathLookup methods are called by o.n.a.d.Lookup only and I need just the Result for now. I plan to implement the
remaining methods in the future when I'll extend ContextProvider with o.o.u.Lookup.Provider.
And yes, this is only about the recursive nature of FolderLookup. PathLookup will be removed as soon as
Lookups.forPath(path,recursive) is introduced.

[JG03] I've found only http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6519115
where they suggest to use Element.getAnnotationMirrors() It's not fixed yet.
Anyway, I did not want to generate the exception, this should work O.K.

I've added getValue() call, which should make it more safe.

[JG04] I do not think I can use instanceAttribute. I need to call a static method to get the instance.

[JG05] isClassOf() is checking only abstract classes. I need to write a different check for interfaces... That's TBD.

[JG06] Yes, copied, but unused. I wasn't sure I will not need it. I'll remove it.

[JG07] Fixed.

[JG08] I do not think "Class<T> type();" is needed. ContextAwareService always implements the interface or extends the
abstract class we search for. Therefore I do not get a heterogeneous collection anywhere. The only usage of
ContextAwareService is in Lookup. ContextAwareService can implement more than one interface, then the type() method
would have to be replaced with Class[] types(). But that would complicate the interface for other implementors.

[JG09] I do not think it's more efficient. But O.K., it should be more robust and readable.

[JG10] It was suggested to make that check, I've also copied that from the original processor. I agree that it's
reasonable to return true.

[JG11] Yes, this is TBD, already in [JG05].

[JG12] done.

[JG13] Right. I'll check the return type.

Well the DebuggerProcessors are in different modules, I'd have to put some support into public API, which is not nice.
Putting that directly into LayerGeneratingProcessor might be better. If I get some time, I'll try to suggest something...

[JG14] I just copied that code from options module. Thought it's necessary, I did not read that code at all. I'll delete
it if it's not needed.

[JG15] Will do.

[JG16] Corrected.

[JG17] Does not make much difference to me, but O.K.

I've tried to describe the basic semantics at
http://www.netbeans.org/issues/show_bug.cgi?id=156687#desc16
ContextAwareService is the factory, which is registered into Lookups.forPath().
o.n.a.d.Lookup uses that factory to create the context-aware instances.
Unfortunately that means that we need one implementation of ContextAwareService for every registered type. I've created
a Proxy for interfaces, but we need extra implementations for abstract classes.

Thanks for the review, going to integrate on Monday...

Integrated in following changesets:
http://hg.netbeans.org/main/rev/416a2e21be06
http://hg.netbeans.org/main/rev/85fdf0755e33
http://hg.netbeans.org/main/rev/96d7b0b3803c
http://hg.netbeans.org/main/rev/41b35a7d9090
http://hg.netbeans.org/main/rev/9b6cba672130
http://hg.netbeans.org/main/rev/5f3aa6b76a52
http://hg.netbeans.org/main/rev/afa36905aafa
http://hg.netbeans.org/main/rev/ae4164f2f3af
http://hg.netbeans.org/main/rev/d570356d215b

Regarding diffs: you do not need to use MQ just to produce a final diff of changes; fine if you have committed piecemeal
changesets to a local branch. (MQ's potential advantages: can work on various patches at various times without making
separate clones or using named branches; can "hide" intermediate work so that only a single changeset is kept in
permanent history.)

You just need to use 'hg di -r $base' where $base is the starting revision (where you cloned the branch) if you made a
linear series of changes with no intervening merges. If you did merge from trunk, then $base would just be the last
trunk rev which you merged.

Integrated into 'main-golden', will be available in build *200902030229* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/416a2e21be06
User: mentlicher@netbeans.org
Log: #156687 - Include Lookups.forPath() to debugger's Lookup.MetaInf. Test added.

Integrated into 'main-golden', will be available in build *200902110201* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/115c1d49f6dc
User: Pavel Flaska <pflaska@netbeans.org>
Log: #156687: Use new debugger attach type lookup in debuggers

Verified ... and Closing all issues resolved into NetBeans 6.7 and earlier.