This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.

Bug 201893 - Ordering does not report warning for Project's LookupProviders
Summary: Ordering does not report warning for Project's LookupProviders
Status: RESOLVED FIXED
Alias: None
Product: projects
Classification: Unclassified
Component: Generic Infrastructure (show other bugs)
Version: 7.1
Hardware: All All
: P3 normal (vote)
Assignee: apireviews
URL:
Keywords: API, API_REVIEW_FAST
Depends on: 201991
Blocks:
  Show dependency tree
 
Reported: 2011-09-09 14:17 UTC by Tomas Zezula
Modified: 2011-09-20 12:02 UTC (History)
1 user (show)

See Also:
Issue Type: TASK
Exception Reporter:


Attachments
Diff file (1.95 KB, patch)
2011-09-09 14:20 UTC, Tomas Zezula
Details | Diff
Fixed patch file (4.89 KB, patch)
2011-09-09 14:44 UTC, Tomas Zezula
Details | Diff
Patch file (5.38 KB, patch)
2011-09-09 14:53 UTC, Tomas Zezula
Details | Diff
JFX ActionProvider patch (7.80 KB, patch)
2011-09-09 16:43 UTC, Tomas Zezula
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tomas Zezula 2011-09-09 14:17:27 UTC

    
Comment 1 Tomas Zezula 2011-09-09 14:20:01 UTC
Created attachment 110571 [details]
Diff file
Comment 2 Tomas Zezula 2011-09-09 14:20:31 UTC
Changed the default value of the LookupProvider.Registration.ProjectType.position to allow clients to order provided project Lookups.
Behavioral change is that recompiled services with no explicit position will now be at 0 rather than MAX_VALUE, meaning they will be ahead of rather than behind services with an explicit positive value.
Comment 3 Tomas Zezula 2011-09-09 14:44:15 UTC
Created attachment 110572 [details]
Fixed patch file
Comment 4 Tomas Zezula 2011-09-09 14:53:48 UTC
Created attachment 110573 [details]
Patch file

Improved apichanges
Comment 5 Jesse Glick 2011-09-09 16:02:52 UTC
<code>warnings from org.openide.filesystems.Ordering</code> should be <code>org.openide.filesystems.Ordering</code> I guess.


Ordering should also be fixed, so that no warning is issued in case newly compiled modules with position=0 are combined with old binary modules with undefined position. (There should still be a warning in case some services are specified with nonzero position and some old binary modules have undefined position, since then it is unclear whether an ordering was expected or not.) Specifically:

diff --git a/openide.filesystems/test/unit/src/org/openide/filesystems/OrderingTest.java b/openide.filesystems/test/unit/src/org/openide/filesystems/OrderingTest.java
--- a/openide.filesystems/test/unit/src/org/openide/filesystems/OrderingTest.java
+++ b/openide.filesystems/test/unit/src/org/openide/filesystems/OrderingTest.java
@@ -233,6 +233,22 @@
         assertEmptyLog();
     }
 
+    public void testGetOrderZeroPositionsMixedWithUnordered() throws Exception { // #201893
+        apex.setAttribute("position", 0);
+        cone.setAttribute("position", 0);
+        assertOrder(true, apex, cone, ball, dent);
+        assertEmptyLog();
+    }
+
+    public void testGetOrderZeroPositionsMixedWithUnorderedAndOrdered() throws Exception { // #201893
+        apex.setAttribute("position", 0);
+        cone.setAttribute("position", 0);
+        dent.setAttribute("position", -1);
+        assertOrder(true, dent, apex, cone, ball);
+        assertLog("ball");
+        assertLog("dent");
+    }
+
     public void testSetOrderConservativeOneJump() throws Exception {
         apex.setAttribute("position", 17);
         ball.setAttribute("position", 9);

Note that both tests currently fail in assertOrder - it seems that Ordering does not necessarily place unordered files at the end of the list, despite <http://wiki.netbeans.org/FolderOrdering103187#Ordering_semantics>. Not sure if this is easily fixable, since introducing new edges to the topological sort might clash with old relative orders (a/b=true, see testGetOrderMixed). Also not clear that anyone would care.


But a completely different approach to the problem would be to leave the registrations using Integer.MAX_VALUE, and introduce a new folder attribute (name TBD) that would instruct Ordering to suppress the "Not all children..." warning (though not other warnings), by skipping the block controlled by 'logWarnings && !childrenByPosition.isEmpty()...'. LookupProviderAnnotationProcessor would then generate this attribute on the .../Lookup folder. The effect would be similar to your patch (if it included a change to Ordering), but there would be no concern over mixing old binary modules with newly compiled modules.

That would perhaps better match our intuition that partial orderings in Lookup are normal and usually harmless. LookupMerger positions should be irrelevant, and while LookupProvider's may be meaningfully ordered, @PSP registrations for unrelated services are all mixed together in the same folder for historical reasons - so it would be normal to define an order for all impls of ActionProvider but not define an order for any impls of ProjectOpenedHook.


Of course it would have been better if LookupProviderSupport.createCompositeLookup had been defined from the start to read these three kinds of registrations from distinct locations - Projects/$type/Lookup/Providers/*.instance, Projects/$type/Lookup/Mergers/$xfacename.instance, Projects/$type/Lookup/Services/$xfacename/*.instance - which would make DelegatingLookupImpl much simpler, make LazyLookupProviders obsolete, and behave naturally with the existing Ordering (since for any given folder you would in fact either want everything ordered or nothing). Unfortunately I cannot think of a simple way to migrate to this state while retaining compatibility with modules compiled using the older processor (or using manual registration).


Perhaps the best approach for the short term is simply to suppress the warning in Ordering in the special case of Projects/**/Lookup (note that the infix may include '/' in the case of Maven). Then no API review is needed now, and we retain the longer-term option of improving LPS's behavior.
Comment 6 Tomas Zezula 2011-09-09 16:43:21 UTC
Created attachment 110579 [details]
JFX ActionProvider patch
Comment 7 Tomas Zezula 2011-09-09 16:56:06 UTC
Jesse, you suggest not to change the projectapi annotations and just to change the Ordering not to issue the warning. Right?
In this case there is not need for API review. I will apply the jfx.diff patch and do the change in Ordering with someone help.
Comment 8 Jesse Glick 2011-09-09 17:16:45 UTC
(In reply to comment #6)
> Created an attachment (id=110579) [details]
> JFX ActionProvider patch

Tips:

- @ProjectServiceProvider can be given on a constructor with appropriate arguments, which may be more natural or convenient for a class which is only used in this way

- no need to specify an empty <folder name="Lookup"/> in (either) XML layer (whole Projects folder could apparently be removed in javafx2/project/layer.xml)

(In reply to comment #7)
> you suggest not to change the projectapi annotations and just to change
> the Ordering not to issue the warning. Right?

Right. Something like:

diff --git a/openide.filesystems/src/org/openide/filesystems/Ordering.java b/openide.filesystems/src/org/openide/filesystems/Ordering.java
--- a/openide.filesystems/src/org/openide/filesystems/Ordering.java
+++ b/openide.filesystems/src/org/openide/filesystems/Ordering.java
@@ -170,7 +170,7 @@
                 previousChild = subsequentChild;
             }
         }
-        if (logWarnings && !childrenByPosition.isEmpty() && childrenByPosition.size() < children.size()) {
+        if (logWarnings && /* #201893 */!parent.getPath().matches("Projects/.+/Lookup") && !childrenByPosition.isEmpty() && childrenByPosition.size() < children.size()) {
             List<FileObject> missingPositions = new ArrayList<FileObject>(children);
             for (ChildAndPosition cap : childrenByPosition) {
                 missingPositions.remove(cap.child);
Comment 9 Tomas Zezula 2011-09-09 18:38:07 UTC
OK, thanks.
I will change the Ordering in this way.
Comment 10 Tomas Zezula 2011-09-12 09:02:28 UTC
Fixed jet-main 3f0476cfb4e2.
Fixed as Jesse proposed.
In future the project lookup provider registration should change to allow only partial ordering, I've created a new enhancement #201991.
Comment 11 Quality Engineering 2011-09-13 16:10:03 UTC
Integrated into 'main-golden'
Changeset: http://hg.netbeans.org/main-golden/rev/3f0476cfb4e2
User: Tomas Zezula <tzezula@netbeans.org>
Log: #201893:Ordering does not report warning for Project's LookupProviders
Comment 12 Jesse Glick 2011-09-20 12:02:37 UTC
(In reply to comment #10)
> Fixed jet-main 3f0476cfb4e2.

BTW <folder name="Lookup"/> is unnecessary in j2seproject's XML layer since generated-layer.xml will define this folder anyway.

Should also check whether @ProjectServiceProvider can be used on J2SEActionProvider; means the update helper should be looked up on demand, and startFSListener should be inlined into constructor (I suppose).