Paper icon indicating copy to clipboard operation
Paper copied to clipboard

Add /paper dumplisteners command

Open Warriorrrr opened this issue 3 years ago • 10 comments
trafficstars

Succeeds #6359

Added in a hover message with the listener's priority and added some better error messaging.

Warriorrrr avatar Oct 25 '22 19:10 Warriorrrr

Sorry for the click, GitHub mobile is a questionable interface.

lynxplay avatar Oct 25 '22 19:10 lynxplay

maybe can be good make "dump" a subcommand and have like /paper dump item and /paper dump listeners? or maybe make a simple /paper listeners

Doc94 avatar Oct 30 '22 16:10 Doc94

Can we somehow have tab completion for all the events?

dont know how without including a lib like classgraph to scan the classpath for all available subclasses of event (like I did here with the legacy org.reflections lib https://gist.github.com/MiniDigger/f369210813bd65d43a62468b8dafcaeb#file-globaleventlistener-java-L36) if we dont care about non bukkit events, an annotation processor that does the scanning at compile time and writes the result somewhere might work? could be a cool project I guess.

MiniDigger avatar Nov 15 '22 07:11 MiniDigger

Could do something hacky like this for suggestions:

Index: src/main/java/org/bukkit/event/HandlerList.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/main/java/org/bukkit/event/HandlerList.java b/src/main/java/org/bukkit/event/HandlerList.java
--- a/src/main/java/org/bukkit/event/HandlerList.java	(revision a2042f22ffeca004f57cb53666569212fd1d2045)
+++ b/src/main/java/org/bukkit/event/HandlerList.java	(date 1668547189704)
@@ -33,6 +33,13 @@
      */
     private static ArrayList<HandlerList> allLists = new ArrayList<HandlerList>();
 
+    // Paper start
+    /**
+     * Event types which have instantiated a {@link HandlerList}.
+     */
+    public static final java.util.Set<String> EVENT_TYPES = java.util.concurrent.ConcurrentHashMap.newKeySet();
+    // Paper end
+
     /**
      * Bake all handler lists. Best used just after all normal event
      * registration is complete, ie just after all plugins are loaded if
@@ -94,6 +101,13 @@
      * The HandlerList is then added to meta-list for use in bakeAll()
      */
     public HandlerList() {
+        // Paper start
+        StackWalker.getInstance(StackWalker.Option.RETAIN_CLASS_REFERENCE)
+            .walk(s -> s.filter(f -> Event.class.isAssignableFrom(f.getDeclaringClass())))
+            .findFirst()
+            .map(f -> f.getDeclaringClass().getName())
+            .ifPresent(EVENT_TYPES::add);
+        // Paper end
         handlerslots = new EnumMap<EventPriority, ArrayList<RegisteredListener>>(EventPriority.class);
         for (EventPriority o : EventPriority.values()) {
             handlerslots.put(o, new ArrayList<RegisteredListener>());

But we probably don't want to expose that field to api (could also use an instance field and the getAllHandlerLists method, but it uses an expensive clone which we avoid with CHM here, also wouldn't solve the problem of public field)

jpenilla avatar Nov 15 '22 21:11 jpenilla

I think it would be useful to have a build step that gets a list of all events. I think there should be some processor that does that already to check and make sure all event classes have a HandlerList and the right method name and what not. Could kill 2 birds with one stone by also having that collect and inject a list of event types somewhere into the jar

Machine-Maker avatar Nov 15 '22 21:11 Machine-Maker

I don't think tab completion here is really still in the scope of the PR concerning how tedious it is without shipping a relfections like library.

As general feedback, it might be nice to include a notice in the class not found response that it expects a fully qualified name ?

lynxplay avatar Nov 17 '22 15:11 lynxplay

It's been brought up in discord that guava has a classpath scanner utility. Not entirely sure how it will behave with plugin class loaders. If we go with a classpath scanner we will probably want to cache it and bust the cache whenever a plugin is loaded. I think the solution I proposed earlier is simpler, could just do a hack and make the field private + access it with reflection. But if the PR author isn't comfortable implementing that I guess it can be left for someone else to add. Another thing with the classpath scanner is not every event extending class has a handlerlist, so it would require extra checks there that are built into my approach. The only real downside with my solution is unused (no listeners, class hasn't been initialized) events will not be in tab complete.

And I agree that the class not found response should include that information.

jpenilla avatar Nov 17 '22 17:11 jpenilla

Updated the PR to mention the fully qualified name being required and removed the redundant sort.

I don't mind adding tab completions since it's pretty cool to have, will try out guava's classpath scanner first. I did some testing and it seems like the Class.forName doesn't find any plugin classes, probably due to the different class loaders but I'm not sure how to fix it.

Warriorrrr avatar Nov 19 '22 13:11 Warriorrrr

Probably need to iterate plugin classloaders if it can't find the class on the api's.

Also the completion cache needs to get nuked when new plugins are loaded. I don't think there's an easy callback for that in server, which is part of why I think the handlerlist patch is simpler. Did guava's scanner discover plugin classes or did it have the same behavior as Class.forName?

jpenilla avatar Nov 19 '22 16:11 jpenilla

The guava scanner has the same behaviour as the Class.forName, the handlerlist patch does seem a lot better in that regard, will replace it with that.

Warriorrrr avatar Nov 19 '22 17:11 Warriorrrr

Plugin classloaders will now have all exceptions caught. As plugin classloaders can throw a bunch of exceptions depending on if they are unloaded or not. Additionally, each plugin is now wrapped in a try-catch.

Owen1212055 avatar Nov 23 '22 22:11 Owen1212055