Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removed depth when searching extension jars #1461

Merged
merged 3 commits into from Feb 22, 2024

Conversation

Rylern
Copy link
Contributor

@Rylern Rylern commented Feb 8, 2024

I removed the depth parameter when searching for extension jars (it was set at 1). This corrects the bug but I don't know if it's very safe to not have a depth at all. Maybe I should just set a higher depth (like 5)?

I also implemented a try-with-resources block because Files.walk() needs to be closed.

@Rylern Rylern linked an issue Feb 8, 2024 that may be closed by this pull request
@Rylern Rylern changed the title Removed depth Removed depth when searching extension jars Feb 8, 2024
@petebankhead
Copy link
Member

Thanks! I agree unbounded makes me nervous. I guess that I might have been intending depth=1 to mean one subdirectory, but I guess it really is 0 subdirectories.

Perhaps increase to 3, and check that allows nested subdirectories?

If the depth is stored as a private static field - rather than local to the method - then it would be at least possible to override it in a startup script, if anyone really needs a higher value.

@petebankhead
Copy link
Member

Thanks @Rylern this looks good & brings up a couple of minor questions for you, @alanocallaghan and @finglis

  • How 'deep' should we search for extensions (i.e. how many sub-directories)?
  • Should there be any way to override the depth?

Currently in this PR I think it will search 4 subdirectories deep, and there is no option to override this because the field is final.

If it wasn't final then the following script should work:

// Prints, even though it's private
println qupath.lib.gui.ExtensionClassLoader.MAX_EXTENSION_JARS_DEPTH

// Try to set
qupath.lib.gui.ExtensionClassLoader.MAX_EXTENSION_JARS_DEPTH = 5

// Print again
println qupath.lib.gui.ExtensionClassLoader.MAX_EXTENSION_JARS_DEPTH

and that would allow the user to sneakily adjust the depth in a startup script.

I've sometimes found such sneaky scripts to be useful - mostly when a user wants to do something I hadn't thought to make in the public API - but I'm not sure if we want to permit or block it. Either way, the script only works because of Groovy's relaxed attitude to private variables - so it's risky for the user to do such things.

If no one has strong opinions, I can just merge the PR as it currently is. But it's worth knowing that the choice of final/non-final has this implication, since Groovy is our scripting language.

@alanocallaghan
Copy link
Contributor

Unrelated and possibly bikeshedding, but do you have an IDE setting to remove trailing whitespace on save? The intellij default (only on modified lines) seems a bit better, because otherwise it adds noise to the diffs.

I think it's unlikely to come up because anybody with 4 directory deep extension directories is up to something interesting, and beyond that we'd potentially be walking a lot of files, but I'd be open to leaving the field non-final in case somebody has a particular setup they want to use

@Rylern
Copy link
Contributor Author

Rylern commented Feb 8, 2024

I agree, we can leave the field non-final as it doesn't have any bad consequence

@petebankhead petebankhead merged commit cae379e into qupath:main Feb 22, 2024
3 checks passed
@Rylern Rylern deleted the extension-subdirectories-fix branch February 23, 2024 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

QuPath extensions folder does not allow for subdirectories
3 participants