SchematicsManager: Handling and caching list of known schematics, providing cli suggestions #2212

seijikun · 2022-11-08T00:18:02Z

I implemented a SchematicsManager that handles the listing / caching of known schematic files.
The SchematicsManager has multiple possible backends.
The preferred backend is:

FileWatcherSchematicsBackend:
This backend uses Java's WatchService to listen for changes in the schematics directory. This rids us of any file listing IO.

PollingSchematicsBackend:
This is the fallback backend for the previous mentioned that is only used when the previous failed to initialize (for whatever reason that might be). It caches the result and returns the previous result as long as it's not older than 10 seconds. This is basically an Eventually Consistent Cache.

I then went on to implement a SchematicConverter to parse Schematics from arguments and provide suggestions.

This fixes #2095

worldedit-core/src/main/java/com/sk89q/worldedit/util/schematic/Schematic.java

seijikun · 2022-11-08T22:48:41Z

This change in action:
(The suggestions for //schematics load ... and //schematics delete ...)

SchematicCompletion.mp4

me4502

I'm happy with this PR outside of the noted log points, but I'd also like someone else to have a go over it too :)

.../src/main/java/com/sk89q/worldedit/util/schematic/backends/FileWatcherSchematicsBackend.java

...core/src/main/java/com/sk89q/worldedit/util/schematic/backends/PollingSchematicsBackend.java

Joo200 · 2023-01-01T11:50:51Z

I noticed that the implementation doesn't handle symlinks correctly.
With the setting files.allow-symbolic-links the symlinks will be followed.

Another issue with symlinks: Using recursive symlinks doesn't work fine:

Create a folder in schematics (e.g. mkdir dummy)
Create a symlink that points to one directory upwards (e.g. cd dummy && ln -fs ../ test)
With this setup WorldEdit will follow the schematic path:

[12:45:30 INFO]: New Schematic found: /home/user/dev/server/paper/plugins/WorldEdit/schematics/dummy/test/dummy/test/dummy/test/dummy/test/dummy/test/dummy/test/dummy/test/dummy/test/dummy/test/dummy/test/dummy/test/dummy/test/dummy/test/dummy/test/dummy/test/dummy/test/dummy/test/dummy/test/dummy/test/dummy/test/dummy/test/dummy/test/dummy/test/dummy/test/dummy/test/dummy/test/dummy/test/dummy/test/dummy/test/dummy/test/dummy/test/dummy/test/dummy/test/dummy/test/dummy/test/dummy/test/dummy/test/dummy/test/dummy/test/dummy/test/dummy/test

octylFractal

I like this a lot, but there are some edgecases to handle and cleanup to do.

Can we also move the com.sk89q.worldedit.util.schematic stuff to com.sk89q.worldedit.internal.schematic? I don't think this should be exposed as API. The RecursiveDirectoryWatcher should probably also be moved to com.sk89q.worldedit.internal.util.

worldedit-core/src/main/java/com/sk89q/worldedit/util/schematic/SchematicPath.java

worldedit-core/src/main/java/com/sk89q/worldedit/util/io/file/RecursiveDirectoryWatcher.java

worldedit-core/src/main/java/com/sk89q/worldedit/util/schematic/backends/SchematicsBackend.java

worldedit-core/src/test/java/com/sk89q/worldedit/util/schematic/SchematicPathTest.java

...core/src/main/java/com/sk89q/worldedit/util/schematic/backends/PollingSchematicsBackend.java

wizjany · 2023-05-22T14:52:24Z

worldedit-core/src/main/java/com/sk89q/worldedit/extension/platform/Capability.java

@@ -21,6 +21,7 @@

 import com.sk89q.worldedit.WorldEdit;
 import com.sk89q.worldedit.internal.block.BlockStateIdAccess;
+import com.sk89q.worldedit.internal.util.LogManagerCompat;


extra import?

wizjany · 2023-05-22T14:58:03Z

worldedit-core/src/main/java/com/sk89q/worldedit/internal/schematic/SchematicsManager.java

+            var fileWatcherBackend = FileWatcherSchematicsBackend.create(schematicsDir);
+            if (fileWatcherBackend.isPresent()) {
+                backend = fileWatcherBackend.get();
+            } else {
+                createFallbackBackend();
+            }
+        } catch (IOException e) {
+            createFallbackBackend();


we're generally not in the habit of using optionals in WE; it's being passed up from the recursivedirectory watcher, but isn't at all consistent with the fallback. additionally, it uses an empty optional to signify unsupportedexception, but on IOexception it just creates the fallback anyway. there is no difference in results.

wizjany · 2023-05-22T16:34:25Z

.../main/java/com/sk89q/worldedit/internal/schematic/backends/FileWatcherSchematicsBackend.java

+            if (event instanceof RecursiveDirectoryWatcher.FileCreatedEvent) {
+                LOGGER.debug("New Schematic found: " + event.path());
+            } else if (event instanceof RecursiveDirectoryWatcher.FileDeletedEvent) {
+                LOGGER.debug("Schematic deleted: " + event.path());
+            }


why can't this just be inside the try statement, the instanceof is already being checked once.

- Resolve first batch of review comments - Renamed Schematic to SchematicPath - Removed custom hashCode() and equals() implementations - Added unit-test to make sure it behaves as expected

Co-authored-by: Octavia Togami <octavia.togami@gmail.com>

UntouchedOdin0 · 2024-01-08T15:50:45Z

Has this unfortunately been abandoned now? It seemed like a really good idea.

me4502 reviewed Nov 8, 2022

View reviewed changes

seijikun force-pushed the master branch from 40c045e to abe82f4 Compare November 8, 2022 22:30

me4502 reviewed Nov 11, 2022

View reviewed changes

.../src/main/java/com/sk89q/worldedit/util/schematic/backends/FileWatcherSchematicsBackend.java Outdated Show resolved Hide resolved

...core/src/main/java/com/sk89q/worldedit/util/schematic/backends/PollingSchematicsBackend.java Outdated Show resolved Hide resolved

me4502 approved these changes Jan 1, 2023

View reviewed changes

octylFractal requested changes Feb 20, 2023

View reviewed changes

me4502 force-pushed the master branch from 5dc5334 to a9cca78 Compare May 21, 2023 07:13

me4502 requested a review from octylFractal May 21, 2023 09:52

wizjany requested changes May 22, 2023

View reviewed changes

seijikun and others added 9 commits June 24, 2023 16:41

Implement SchematicsManager to cache list of known schematics

0f34f74

Add command suggestions for schematic filenames

c009ffb

Rename Schematic to SchematicPath and cleanup

532e6ec

- Resolve first batch of review comments - Renamed Schematic to SchematicPath - Removed custom hashCode() and equals() implementations - Added unit-test to make sure it behaves as expected

Remove unncessary (un)init log points

9334c73

Apply suggestions from code review

6dda9a1

Co-authored-by: Octavia Togami <octavia.togami@gmail.com>

Resolve many of octy's review notes

e7890b1

Further review fixes

2283c75

Further review improvements

b7e095c

Fix symlink check

9b339f3

me4502 force-pushed the master branch from 5ec7913 to 9b339f3 Compare June 24, 2023 06:41

PR notes

441e25f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SchematicsManager: Handling and caching list of known schematics, providing cli suggestions #2212

SchematicsManager: Handling and caching list of known schematics, providing cli suggestions #2212

seijikun commented Nov 8, 2022

seijikun commented Nov 8, 2022

me4502 left a comment

Joo200 commented Jan 1, 2023

octylFractal left a comment

wizjany May 22, 2023

wizjany May 22, 2023

wizjany May 22, 2023

UntouchedOdin0 commented Jan 8, 2024

SchematicsManager: Handling and caching list of known schematics, providing cli suggestions #2212

Are you sure you want to change the base?

SchematicsManager: Handling and caching list of known schematics, providing cli suggestions #2212

Conversation

seijikun commented Nov 8, 2022

seijikun commented Nov 8, 2022

me4502 left a comment

Choose a reason for hiding this comment

Joo200 commented Jan 1, 2023

octylFractal left a comment

Choose a reason for hiding this comment

wizjany May 22, 2023

Choose a reason for hiding this comment

wizjany May 22, 2023

Choose a reason for hiding this comment

wizjany May 22, 2023

Choose a reason for hiding this comment

UntouchedOdin0 commented Jan 8, 2024