You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@netbeans.apache.org by GitBox <gi...@apache.org> on 2020/07/12 19:44:53 UTC
[GitHub] [netbeans] JaroslavTulach commented on a change in pull request #2232: Make it easy to create dynamically updated ProxyLookup instances without subclassing
JaroslavTulach commented on a change in pull request #2232:
URL: https://github.com/apache/netbeans/pull/2232#discussion_r453354820
##########
File path: platform/openide.util.lookup/apichanges.xml
##########
@@ -25,6 +25,33 @@
<apidef name="lookup">Lookup API</apidef>
</apidefs>
<changes>
+ <change id="ProxyLookupController">
+ <api name="lookup"/>
+ <summary>Add ProxyLookup.Controller to set lookups dynamically without
+ subclassing</summary>
+ <version major="8" minor="43"/>
+ <date year="2020" month="7" day="4"/>
+ <author login="tboudreau"/>
+ <compatibility addition="yes" source="compatible" semantic="compatible" binary="compatible"/>
+ <description>
+ <p>
+ One of the most common uses of ProxyLookup is to dynamically
+ change the set of lookups being delegated to. The objection
+ to exposing the <code>setLookups()</code> method is straightforward -
+ it would be inviting <i>clients</i> of such a Lookup to call it.
+ </p><p>
+ A cursory grep of NetBeans' own sources finds <b>53 such subclasses</b>
Review comment:
Rather than describing usage of `grep`, I suggest to make the changes in the overall NetBeans sources, once we agree on the API.
##########
File path: platform/openide.util.lookup/test/unit/src/org/openide/util/lookup/ProxyLookupFactoryMethodsTest.java
##########
@@ -0,0 +1,195 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.openide.util.lookup;
+
+import java.util.Arrays;
+import static java.util.Arrays.asList;
+import java.util.HashSet;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.Executor;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ThreadFactory;
+import java.util.concurrent.TimeUnit;
+import java.util.function.BiConsumer;
+import java.util.function.Consumer;
+import org.junit.Test;
+import static org.junit.Assert.*;
+import org.openide.util.Lookup;
+import org.openide.util.LookupEvent;
+import org.openide.util.LookupListener;
+import org.openide.util.lookup.ProxyLookupFactoryMethodsTest.TThreadFactory.TThread;
+
+/**
+ *
+ * @author Tim Boudreau
+ */
+public class ProxyLookupFactoryMethodsTest {
+
+ private Consumer<Lookup[]> lookupConsumer;
+ private BiConsumer<Executor, Lookup[]> lookupBiConsumer;
+
+ private Lookup createWithSingleConsumer(Lookup... lookups) {
+ ProxyLookup.Controller controller = new ProxyLookup.Controller(lookups);
+ lookupConsumer = controller::setLookups;
+ return new ProxyLookup(controller);
Review comment:
This would be replaced with:
```java
ProxyLookup.Controller controller = new ProxyLookup.Controller();
ProxyLookup l = new ProxyLookup(controller);
l.setLookups(lookups);
return l;
```
I suggest to remove `BiConsumer` altogether as the test should mimic the API usage - e.g. directly use the `Controller`.
##########
File path: platform/openide.util.lookup/src/org/openide/util/lookup/ProxyLookup.java
##########
@@ -57,6 +58,26 @@
public ProxyLookup(Lookup... lookups) {
data = ImmutableInternalData.EMPTY.setLookupsNoFire(lookups, true);
}
+ /**
+ * Create a ProxyLookup whose contents can be set dynamically without
+ * exposing the setter to consumers of the lookup; the passed
Review comment:
The passed in `Consumer` is no longer called back. It can:
"be used later to change the set of lookups the `ProxyLookup` delegates to via `Controller.setLookups` method"
##########
File path: platform/openide.util.lookup/src/org/openide/util/lookup/ProxyLookup.java
##########
@@ -89,7 +110,96 @@ public synchronized String toString() {
}
return map.keySet();
}
-
+
+ /**
+ * A controller which allows the set of lookups being proxied to be
+ * set dynamically.
+ *
+ * @since 8.43
+ */
+ public static final class Controller {
+
+ private BiConsumer<? super Executor, ? super Lookup[]> consumer;
Review comment:
I really want this change to not rely on JDK8 APIs.
I suggest to change this field to `ProxyLookup`.
##########
File path: platform/openide.util.lookup/src/org/openide/util/lookup/ProxyLookup.java
##########
@@ -89,7 +110,96 @@ public synchronized String toString() {
}
return map.keySet();
}
-
+
+ /**
+ * A controller which allows the set of lookups being proxied to be
+ * set dynamically.
+ *
+ * @since 8.43
+ */
+ public static final class Controller {
+
+ private BiConsumer<? super Executor, ? super Lookup[]> consumer;
+
+ /**
+ * Create a controller with an initial set of lookups that will be
+ * proxied by a {@link ProxyLookup} this controller is passed to the
+ * constructor of.
+ * @param lookups An array of lookups
+ */
+ public Controller(Lookup... lookups) {
+ consumer = new InitialConsumer(lookups);
+ }
+
+ /**
+ * Create a controller with an initially empty set of lookups to
+ * proxy to.
+ */
+ public Controller() {
+ consumer = new InitialConsumer();
+ }
+
+ /**
+ * Set the lookups on the {@link ProxyLookup} this controller controls;
+ * if called before that ProxyLookup has been created, the lookup
+ * contents will be set during that ProxyLookup's constructor as if you
+ * had passed them to the constructor directly, but the executor
+ * parameter will be ignored (nothing will be listening to it while it's
+ * in its constructor anyway).
+ *
+ * @param exe An executor to notify in
+ * @param lookups An array of Lookups to be proxied
+ */
+ public synchronized void setLookups(Executor exe, Lookup... lookups) {
+ consumer.accept(exe, lookups);
+ }
+
+ /**
+ * Set the lookups on the {@link ProxyLookup} this controller controls;
+ * if called before that ProxyLookup has been created, the lookup
+ * contents will be set during that ProxyLookup's constructor as if you
+ * had passed them to the constructor directly.
+ *
+ * @param exe An executor to notify in
+ * @param lookups An array of Lookups to be proxied
+ */
+ public void setLookups(Lookup... lookups) {
+ setLookups(null, lookups);
+ }
+
+ synchronized Lookup[] setProxyLookup(ProxyLookup lkp) {
+ if (!(this.consumer instanceof InitialConsumer)) {
+ throw new IllegalStateException("Attempting to use "
+ + "ProxyLookup.Controller for more than one "
+ + "ProxyLookup is illegal");
+ }
+ Lookup[] result = ((InitialConsumer) consumer).lookups();
+ this.consumer = lkp::setLookups;
Review comment:
```
if (this.consumer != null) {
throw ISE();
}
this.consumer = lkp;
```
is all you need, in my opinion.
##########
File path: platform/openide.util.lookup/src/org/openide/util/lookup/ProxyLookup.java
##########
@@ -89,7 +110,96 @@ public synchronized String toString() {
}
return map.keySet();
}
-
+
+ /**
+ * A controller which allows the set of lookups being proxied to be
+ * set dynamically.
+ *
+ * @since 8.43
+ */
+ public static final class Controller {
+
+ private BiConsumer<? super Executor, ? super Lookup[]> consumer;
+
+ /**
+ * Create a controller with an initial set of lookups that will be
+ * proxied by a {@link ProxyLookup} this controller is passed to the
+ * constructor of.
+ * @param lookups An array of lookups
+ */
+ public Controller(Lookup... lookups) {
Review comment:
I don't see a value in this constructor. I suggest to remove it.
##########
File path: platform/openide.util.lookup/src/org/openide/util/lookup/ProxyLookup.java
##########
@@ -89,7 +110,96 @@ public synchronized String toString() {
}
return map.keySet();
}
-
+
+ /**
+ * A controller which allows the set of lookups being proxied to be
+ * set dynamically.
+ *
+ * @since 8.43
+ */
+ public static final class Controller {
+
+ private BiConsumer<? super Executor, ? super Lookup[]> consumer;
+
+ /**
+ * Create a controller with an initial set of lookups that will be
+ * proxied by a {@link ProxyLookup} this controller is passed to the
+ * constructor of.
+ * @param lookups An array of lookups
+ */
+ public Controller(Lookup... lookups) {
+ consumer = new InitialConsumer(lookups);
+ }
+
+ /**
+ * Create a controller with an initially empty set of lookups to
+ * proxy to.
+ */
+ public Controller() {
+ consumer = new InitialConsumer();
+ }
+
+ /**
+ * Set the lookups on the {@link ProxyLookup} this controller controls;
+ * if called before that ProxyLookup has been created, the lookup
+ * contents will be set during that ProxyLookup's constructor as if you
+ * had passed them to the constructor directly, but the executor
+ * parameter will be ignored (nothing will be listening to it while it's
+ * in its constructor anyway).
+ *
+ * @param exe An executor to notify in
+ * @param lookups An array of Lookups to be proxied
+ */
+ public synchronized void setLookups(Executor exe, Lookup... lookups) {
+ consumer.accept(exe, lookups);
Review comment:
Throw `NullPointerException` if the `consumer` field isn't yet set and document that as expected behavior. That's what I'd suggest.
##########
File path: platform/openide.util.lookup/src/org/openide/util/lookup/ProxyLookup.java
##########
@@ -89,7 +110,96 @@ public synchronized String toString() {
}
return map.keySet();
}
-
+
+ /**
+ * A controller which allows the set of lookups being proxied to be
+ * set dynamically.
+ *
+ * @since 8.43
+ */
+ public static final class Controller {
+
+ private BiConsumer<? super Executor, ? super Lookup[]> consumer;
+
+ /**
+ * Create a controller with an initial set of lookups that will be
+ * proxied by a {@link ProxyLookup} this controller is passed to the
+ * constructor of.
+ * @param lookups An array of lookups
+ */
+ public Controller(Lookup... lookups) {
+ consumer = new InitialConsumer(lookups);
+ }
+
+ /**
+ * Create a controller with an initially empty set of lookups to
+ * proxy to.
+ */
+ public Controller() {
+ consumer = new InitialConsumer();
+ }
+
+ /**
+ * Set the lookups on the {@link ProxyLookup} this controller controls;
+ * if called before that ProxyLookup has been created, the lookup
+ * contents will be set during that ProxyLookup's constructor as if you
+ * had passed them to the constructor directly, but the executor
+ * parameter will be ignored (nothing will be listening to it while it's
+ * in its constructor anyway).
+ *
+ * @param exe An executor to notify in
+ * @param lookups An array of Lookups to be proxied
+ */
+ public synchronized void setLookups(Executor exe, Lookup... lookups) {
+ consumer.accept(exe, lookups);
+ }
+
+ /**
+ * Set the lookups on the {@link ProxyLookup} this controller controls;
+ * if called before that ProxyLookup has been created, the lookup
+ * contents will be set during that ProxyLookup's constructor as if you
+ * had passed them to the constructor directly.
+ *
+ * @param exe An executor to notify in
+ * @param lookups An array of Lookups to be proxied
+ */
+ public void setLookups(Lookup... lookups) {
+ setLookups(null, lookups);
+ }
+
+ synchronized Lookup[] setProxyLookup(ProxyLookup lkp) {
+ if (!(this.consumer instanceof InitialConsumer)) {
+ throw new IllegalStateException("Attempting to use "
+ + "ProxyLookup.Controller for more than one "
+ + "ProxyLookup is illegal");
+ }
+ Lookup[] result = ((InitialConsumer) consumer).lookups();
+ this.consumer = lkp::setLookups;
+ return result;
+ }
+
+ private static class InitialConsumer implements BiConsumer<Executor, Lookup[]> {
Review comment:
Remove this class.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists