Add plugin support #140

SamCarlberg · 2017-08-11T16:34:17Z

Moves builtin widgets and data types to :plugins:base

Moves network table sources and widgets to :plugins:networktables

Autogenerates source trees for each registered source type

Features

Known issues

Startup takes about 5 seconds to load the UI due to LiveWindow tab autopopulation

Screenshots

Plugin preferences dialog

Moved a bunch of code into :api Not all tests succeed

…ting

SamCarlberg · 2017-08-11T18:54:47Z

Dialog for loading/unloading plugins

Still need to make unloading plugins remove its exported widgets and replace the sources of its type with dummies

JLLeitschuh · 2017-08-11T21:03:24Z

api/api.gradle.kts

+    fun testFx(name: String, version: String = "4.0.+") =
+            create(group = "org.testfx", name = name, version = version)
+    testCompile(testFx(name = "testfx-core"))
+    testCompile(testFx(name = "testfx-junit5"))


Since this testCompile dependency is shared now in both api and app you should put it there so you don't have duplicate version numbers.

"There" being...?

build.gradle.kts in the root project.

JLLeitschuh · 2017-08-11T21:12:48Z

api/src/main/java/edu/wpi/first/shuffleboard/api/data/DataType.java

+    return name;
+  }
+
+  public final Class<T> getJavaClass() {


Vs getting the Groovy class or the Kotlin class? What is this supposed to mean? A javadoc would be helpful.

JLLeitschuh · 2017-08-11T21:14:38Z

api/src/main/java/edu/wpi/first/shuffleboard/api/data/DataType.java

-    return DataTypes.forName(name).orElse(DataTypes.Unknown);
+  @Override
+  public int hashCode() {
+    return getClass().hashCode();


Why aren't you also hashing on the name?

JLLeitschuh

Quite a few things.

JLLeitschuh · 2017-08-11T21:15:57Z

api/src/main/java/edu/wpi/first/shuffleboard/api/data/DataType.java

+  @Override
+  public boolean equals(Object obj) {
+    if (obj == null) {
+      return false;
    }


Usually, you have a check for this immediately after the null check as an optimization:

if (this == obj) return true;

I should really stop using IntelliJ's autogenerated equals/hashcode methods, they miss stuff like this

JLLeitschuh · 2017-08-11T21:20:32Z

api/src/main/java/edu/wpi/first/shuffleboard/api/data/DataTypes.java

+    });
+  }
+
+  private static Comparator<Class<?>> closestTo(Class<?> target) {


Can you explain why this is needed and what it is? Also, this is a complex bit of logic, are you sure that Guava doesn't have some sort of reflection utility to do this?
https://github.com/google/guava/wiki/ReflectionExplained

Also, if you do need it, I really hope that this is unit tested.

JLLeitschuh · 2017-08-11T21:23:27Z

api/src/main/java/edu/wpi/first/shuffleboard/api/data/DataTypes.java

+        .collect(Collectors.toSet());
+  }
+
+  private static Class<?> boxedType(Class<?> primitiveType) {


This could be replaced with Primitives.wrap:

http://google.github.io/guava/releases/21.0/api/docs/com/google/common/primitives/Primitives.html#wrap-java.lang.Class-

JLLeitschuh · 2017-08-11T21:27:57Z

app/src/main/java/edu/wpi/first/shuffleboard/app/MainWindowController.java

+                System.out.println("GENERATING FOR " + plugin.getName());
+                setup(plugin);
+              } else {
+                System.out.println("REMOVING ITEMS FOR " + plugin.getName());


You probably want to move to using a logger like slf4j

I left those in while I was debugging. We use Java's builtin Logger class for logging; this was a quick and dirty (and easy to read) way to debug

JLLeitschuh · 2017-08-11T21:28:15Z

app/src/main/java/edu/wpi/first/shuffleboard/app/MainWindowController.java

@@ -213,7 +271,7 @@ public void save() {
  private void saveAs() {
    FileChooser chooser = new FileChooser();
    chooser.getExtensionFilters().setAll(
-            new FileChooser.ExtensionFilter("SmartDashboard Save File (.json)", "*.json"));
+        new FileChooser.ExtensionFilter("SmartDashboard Save File (.json)", "*.json"));


Formatter changes?

JLLeitschuh · 2017-08-11T21:33:06Z

app/src/main/java/edu/wpi/first/shuffleboard/app/plugin/PluginLoader.java

+        return false;
+      }
+    }
+    return false;


Log in this case too.

That'll be logged a lot; one statement per non-plugin class. For example, the base plugin jar would log ~20 times here

JLLeitschuh · 2017-08-11T21:33:47Z

app/src/test/java/edu/wpi/first/shuffleboard/app/json/DashboardTabPaneSaverTest.java

@@ -19,11 +19,12 @@

 import static org.junit.jupiter.api.Assertions.assertEquals;

+@Disabled


Why is this whole test disabled?

Dependent on network tables

JLLeitschuh · 2017-08-11T21:36:20Z

plugins/networktables/networktables.gradle.kts

@@ -0,0 +1,3 @@
+dependencies {


Above this line add a description = "[SOME HELPFUL DESCRIPTION HERE]"

This has been resolved.

JLLeitschuh · 2017-08-11T21:36:49Z

plugins/base/base.gradle.kts

+""".trimMargin()
+
+dependencies {
+    foo


Oops, was trying to see why it wasn't evaluating declarations like compile and testRuntime

JLLeitschuh · 2017-08-11T21:38:33Z

plugins/plugins.gradle.kts

+                create(group = "org.testfx", name = name, version = version)
+        testCompile(testFx(name = "testfx-core"))
+        testCompile(testFx(name = "testfx-junit5"))
+        testRuntime(testFx(name = "openjfx-monocle", version = "8u76-b04"))


This code is again duplicated here.

A widget is disabled when: - the plugin defining that widget is unloaded - the plugin defining the source for that widget is unloaded - the plugin defining the data type of that widget is unloaded

JLLeitschuh · 2017-08-15T20:28:51Z

api/src/main/java/edu/wpi/first/shuffleboard/api/data/DataTypes.java

+            .filter(t -> t != All)
+            .sorted((t1, t2) -> classComparator.reversed().compare(t1.getJavaClass(), t2.getJavaClass()))
+            .collect(Collectors.toList());
+        return Optional.of(sorted.get(0));


Can't you just call first() on the set and get an optional?
Also, should this be Optional.ofNullable?

Ah, that may work. ofNullable is pointless since elements should never be null

JLLeitschuh · 2017-08-15T20:37:52Z

api/src/main/java/edu/wpi/first/shuffleboard/api/util/Registry.java

+   */
+  public final void registerAll(T... items) {
+    Objects.requireNonNull(items, "items");
+    for (T item : items) {


The array must not be null but you could pass registerAll(item1, null, item2, null) what would happen in that case? Should you check each of the args for null too?

register does a null check....

Okay! Cool!

JLLeitschuh · 2017-08-15T20:40:25Z

api/src/main/java/edu/wpi/first/shuffleboard/api/widget/Widgets.java

   * @throws InvalidWidgetException if the widget is invalid
   */
-  private static void validate(Description description) throws InvalidWidgetException {
+  private void validate(Description description) throws InvalidWidgetException {


~~Why is this no longer static? It doesn't use any member variables.~~
Never mind

JLLeitschuh · 2017-08-15T20:41:53Z

api/src/main/java/edu/wpi/first/shuffleboard/api/widget/Widgets.java

+   */
+  public List<Widget> getActiveWidgets() {
+    // Use a copy; don't want elements in the list to be removed by GC while someone's using it
+    return new ArrayList<>(activeWidgets.keySet());


Why not use an ImmutableList.copyOf?

JLLeitschuh · 2017-08-15T20:43:59Z

api/src/test/java/edu/wpi/first/shuffleboard/api/sources/recording/SerializationTest.java

-
-    expected = new String[]{"foo", "bar"};
-    assertArrayEquals(expected, Serialization.readStringArray(fooBarBytes, 0));
-  }


Why were all these tests removed? Are they now irrelevant?

They were dependent on serializers that were moved to the base plugin

JLLeitschuh · 2017-08-15T20:49:22Z

app/src/main/java/edu/wpi/first/shuffleboard/app/plugin/PluginLoader.java

+  public void loadPluginJar(URI jarUri) throws IOException {
+    URL url = jarUri.toURL();
+    PrivilegedAction<URLClassLoader> getClassLoader = () -> {
+      return new URLClassLoader(new URL[]{url}, ClassLoader.getSystemClassLoader());


Is this using the class loader that the rest of the code is using?

I know there are some wacky edge cases you need to be aware of when dealing with class loaders and the garbage collector.

I'd have to look into that. This is basically the same code that the original dashboard uses for loading jars at runtime and that has worked for the past 6+ years

JLLeitschuh · 2017-08-15T20:52:31Z

app/src/main/java/edu/wpi/first/shuffleboard/app/plugin/PluginLoader.java

+    plugin.getThemes().forEach(Themes.getDefault()::register);
+
+    plugin.onLoad();
+    plugin.setLoaded(true);


Why is this necessary?

JLLeitschuh · 2017-08-15T20:53:25Z

app/src/main/java/edu/wpi/first/shuffleboard/app/sources/DestroyedSource.java

+
+/**
+ * A type of data source that represents the state of another source that has been destroyed or removed as a result
+ * of its defining plugin being unloaded. The data source may


Incomplete javadoc.

JLLeitschuh · 2017-08-15T20:54:54Z

app/src/test/java/edu/wpi/first/shuffleboard/app/json/DashboardTabPaneSaverTest.java

@@ -19,11 +19,12 @@

 import static org.junit.jupiter.api.Assertions.assertEquals;

+@Disabled("Depends on network tables")


Do you have a plan to re-enable this?

Yeah. I'll need to change the save file to use mock widgets

JLLeitschuh · 2017-08-15T20:57:37Z

app/src/test/java/edu/wpi/first/shuffleboard/app/plugin/PluginLoaderTest.java

+  @Test
+  public void testLoadClass() {
+    PluginLoader loader = new PluginLoader();
+    boolean loaded = loader.loadPluginClass(MockPlugin.class);


As soon as you try doing this with more complicated classes that actually have plugin logic you are going to end up adding stuff to the Widgets singleton and other singletons which will affect other tests.

JLLeitschuh · 2017-08-15T21:01:27Z

...ins/base/src/test/java/edu/wpi/first/shuffleboard/plugin/base/widget/AbstractWidgetTest.java

+  @AfterAll
+  public static void resetRegistries() {
+    Widgets.setDefault(new Widgets());
+    DataTypes.setDefault(new DataTypes());


This is where static singletons bite you in the butt. You really shouldn't need to do this.

If you have a failure in one of your tests here it can cause a ripple throughout the rest of your testing framework which can be really hard to debug. I would highly recommend you refactor your code so that you can test it without using static singletons.

codecov-io · 2017-08-22T15:36:22Z

Codecov Report

Merging #140 into master will decrease coverage by 29.37%.
The diff coverage is 21.84%.

@@              Coverage Diff              @@
##             master     #140       +/-   ##
=============================================
- Coverage      42.6%   13.22%   -29.38%     
+ Complexity      475      186      -289     
=============================================
  Files           125      136       +11     
  Lines          3070     3523      +453     
  Branches        236      263       +27     
=============================================
- Hits           1308      466      -842     
- Misses         1667     3026     +1359     
+ Partials         95       31       -64

SamCarlberg added 9 commits August 10, 2017 14:49

Split code into "base" and a "networktables" plugins

5d5b1f9

Moved a bunch of code into :api Not all tests succeed

Stub out dashboard API, move dependents back to :app

aa49798

Merge branch 'master' into plugins

3f94f0a

Dynamically create and update tree table views for source types

7a9680d

Fix NetworkTable plugin tests, move MapType to API

8142edc

Fix SourceTreeTable root issues, start adding tests

f50620c

Make all branches expanded by default, fix composite sources not upda…

d46788b

…ting

Remove missed merge artifact

3637dd6

Auto-expand the accordion to the most recent sruce type

fabde38

SamCarlberg added api Related to the API module core work in progress A pull request that is still a work-in-progress and should not be merged labels Aug 11, 2017

SamCarlberg added 2 commits August 11, 2017 14:47

Add initial UI for loading/unloading plugins, fix loading JARs

58145ae

Remove stub dashboard class

b1cfcc5

SamCarlberg force-pushed the plugins branch from 389a93f to b1cfcc5 Compare August 11, 2017 18:52

JLLeitschuh reviewed Aug 11, 2017

View reviewed changes

JLLeitschuh suggested changes Aug 11, 2017

View reviewed changes

SamCarlberg added 10 commits August 13, 2017 17:19

Disable widgets when their plugin is unloaded

2e620bb

A widget is disabled when: - the plugin defining that widget is unloaded - the plugin defining the source for that widget is unloaded - the plugin defining the data type of that widget is unloaded

Fix occassional stack overflow when reloading plugins

51b8eb5

Cleanup

343ae8e

Add docs

f005f91

Move testfx dependencies to root build script

d97a0f4

Tests for DataTypes.closestTo, simplify that method

18fe725

Primitives.wrap()

f2b727a

Log pluginloader errors when instnatiating plugin classes

6df17af

Minor cleanup

b630568

Merge branch 'master' into 'plugins'

bdd9fb2

JLLeitschuh reviewed Aug 15, 2017

View reviewed changes

SamCarlberg added 7 commits August 16, 2017 10:24

Unparameterize SourceEntry, refactor Debouncer, improve docs

f62b228

Let PluginLoader load all jars in a directory

46b54b7

Docs

561dc8d

Use findFirst in forJavaType()

eea6b13

Implementation dependency on ntcore-jni

ac032dc

Minor improvements, CTRL+L to open plugins dialog

5a35fab

Merge branch 'master' into 'plugins'

e0a1379

SamCarlberg force-pushed the plugins branch from f1be45c to e0a1379 Compare August 17, 2017 23:11

SamCarlberg added 3 commits August 22, 2017 11:20

Fix WidgetSaverTest, remove NOPMD for logs, minor other fixes

f02e15d

Merge branch 'master' into plugins

36aac92

Remove stub for loading plugin .class files

de7dc1c

SamCarlberg added 2 commits August 22, 2017 12:00

Remove dependency on Sources registry from NetworkTable plugin

e5bb681

Remove NOPMD for log statements

19018b2

nightpool approved these changes Aug 22, 2017

View reviewed changes

Checkstyle

8082519

SamCarlberg merged commit 1197c65 into wpilibsuite:master Aug 22, 2017

nightpool mentioned this pull request Sep 6, 2017

Should have support for custom plugins #102

Closed

SamCarlberg deleted the plugins branch September 17, 2017 13:40

		@@ -19,11 +19,12 @@

		import static org.junit.jupiter.api.Assertions.assertEquals;

		@Disabled

		@@ -19,11 +19,12 @@

		import static org.junit.jupiter.api.Assertions.assertEquals;

		@Disabled("Depends on network tables")

Add plugin support #140

Add plugin support #140

Conversation

SamCarlberg commented Aug 11, 2017 • edited

Features

Known issues

Screenshots

Plugin preferences dialog

SamCarlberg commented Aug 11, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JLLeitschuh Aug 11, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JLLeitschuh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JLLeitschuh Aug 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JLLeitschuh Aug 15, 2017 • edited

Choose a reason for hiding this comment

codecov-io commented Aug 22, 2017

Codecov Report

SamCarlberg commented Aug 11, 2017 •

edited

SamCarlberg commented Aug 11, 2017 •

edited

JLLeitschuh Aug 11, 2017 •

edited

JLLeitschuh Aug 15, 2017 •

edited

JLLeitschuh Aug 15, 2017 •

edited