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

Legacy ND2 removal #4054

Merged
merged 9 commits into from
Jul 20, 2023
Merged

Legacy ND2 removal #4054

merged 9 commits into from
Jul 20, 2023

Conversation

dgault
Copy link
Member

@dgault dgault commented Jul 17, 2023

Opening this PR as a fresh version of #4049, this PR should keep the history of NativeND2Reader when using git log --follow -p ND2Reader.java

This the follow up to #4030

  • LegacyND2Reader has been removed
  • ND2Reader has been removed
  • NativeND2Reader has been moved to ND2Reader and deprecation flags removed
  • Options to use the LegacyReader in Bio-Formats Plugins have been removed
  • lib/LegacyND2Reader.dll has been removed

Remaining decision on renaming nativend2.chunkmap to nd2.chunkmap

@dgault dgault added this to the 7.0.0 milestone Jul 17, 2023
@dgault
Copy link
Member Author

dgault commented Jul 17, 2023

The remaining issue is the handling of nativend2.chunkmap. Renaming this to nd2.chunkmap absolutely makes sense to me.

The only users who I can see be impacted by the renaming will be those using the command line tools, were you will be supplying the raw String value rather than using ND2Reader.USE_CHUNKMAP_KEY. Do we want to retain functionality for the old String or is renaming suitable for a major version?

@melissalinkert
Copy link
Member

Looks good to me, and confirmed that git log -p --follow ND2Reader.java now shows the history as expected. I think I'm fine with renaming nativend2.chunkmap to nd2.chunkmap.

@sbesson
Copy link
Member

sbesson commented Jul 19, 2023

On the ND2 chunkmap option, my preference would be to rename the key as nd2.chunkmap with a slight preference for adding some backwards compatibility for the existing option as we did not notify of the breaking change in Bio-Formats 6.x.

Locally, the following changes on top of the current branch were sufficient to allow existing code using the nativend2.chunkmap string explicitly to keep working with a warning.

diff --git a/components/formats-gpl/src/loci/formats/in/ND2Reader.java b/components/formats-gpl/src/loci/formats/in/ND2Reader.java
index 4cd74eceec..01e7ce6ccd 100644
--- a/components/formats-gpl/src/loci/formats/in/ND2Reader.java
+++ b/components/formats-gpl/src/loci/formats/in/ND2Reader.java
@@ -80,7 +80,10 @@ public class ND2Reader extends SubResolutionFormatReader {
   public static final long ND2_MAGIC_BYTES_2 = 0x6a502020L;
   private static final int BUFFER_SIZE = 32 * 1024;
 
-  public static final String USE_CHUNKMAP_KEY = "nativend2.chunkmap";
+  /** Legacy chunkmap option key will be removed in Bio-Formats 8.0.0. */
+  @Deprecated
+  public static final String USE_CHUNKMAP_LEGACY_KEY = "nativend2.chunkmap";
+  public static final String USE_CHUNKMAP_KEY = "nd2.chunkmap";
   public static final boolean USE_CHUNKMAP_DEFAULT = true;
 
   // -- Fields --
@@ -150,8 +153,14 @@ public class ND2Reader extends SubResolutionFormatReader {
   public boolean useChunkMap() {
     MetadataOptions options = getMetadataOptions();
     if (options instanceof DynamicMetadataOptions) {
+      if (((DynamicMetadataOptions) options).get(USE_CHUNKMAP_LEGACY_KEY) != null) {
+        LOGGER.warn("Legacy chunkmap option detected use {} instead",
+          USE_CHUNKMAP_KEY);
+      }
+      Boolean defaultValue = ((DynamicMetadataOptions) options).getBoolean(
+        USE_CHUNKMAP_LEGACY_KEY, USE_CHUNKMAP_DEFAULT);
       return ((DynamicMetadataOptions) options).getBoolean(
-        USE_CHUNKMAP_KEY, USE_CHUNKMAP_DEFAULT);
+        USE_CHUNKMAP_KEY, defaultValue);
     }
     return USE_CHUNKMAP_DEFAULT;
   }

Copy link
Member

@sbesson sbesson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quickly retested the chunkmap options (renamed & legacy) using a public ND2 sample:

build@42fe18ce620d:/uod/idr/repos/curated/nd2/public/maxime$ showinf -version
Version: 7.0.0-SNAPSHOT
Build date: 20 July 2023
VCS revision: f68d57bfe9a1a38e0b3e74bb2fac5077589de3aa
build@42fe18ce620d:/uod/idr/repos/curated/nd2/public/maxime$ showinf -nopix BF007.nd2 -debug | grep chunk
Attempting to use chunk map = true
build@42fe18ce620d:/uod/idr/repos/curated/nd2/public/maxime$ showinf -nopix BF007.nd2 -debug -option nd2.chunkmap true | grep chunk
Attempting to use chunk map = true
build@42fe18ce620d:/uod/idr/repos/curated/nd2/public/maxime$ showinf -nopix BF007.nd2 -debug -option nd2.chunkmap false | grep chunk
Attempting to use chunk map = false
Adding non-chunkmap offset 73744, nameLength = 4072, dataLength = 51145
build@42fe18ce620d:/uod/idr/repos/curated/nd2/public/maxime$ showinf -nopix BF007.nd2 -debug -option nativend2.chunkmap false | grep chunk
Legacy chunkmap option detected use nd2.chunkmap instead
Attempting to use chunk map = false
Adding non-chunkmap offset 73744, nameLength = 4072, dataLength = 51145
build@42fe18ce620d:/uod/idr/repos/curated/nd2/public/maxime$ showinf -nopix BF007.nd2 -debug -option nativend2.chunkmap true | grep chunk
Legacy chunkmap option detected use nd2.chunkmap instead
Attempting to use chunk map = true
build@42fe18ce620d:/uod/idr/repos/curated/nd2/public/maxime$ showinf -nopix BF007.nd2 -debug -option nativend2.chunkmap true -option nd2.chunkmap false | grep chunk
Legacy chunkmap option detected use nd2.chunkmap instead
Attempting to use chunk map = false
Adding non-chunkmap offset 73744, nameLength = 4072, dataLength = 51145

I think the nightly CI repository failures are unrelated to this change. If so, this looks good to merge from my perspective

@dgault
Copy link
Member Author

dgault commented Jul 20, 2023

Yeah, repo test failure is related to a new operetta sample file I added

@dgault dgault merged commit 3d4f0ca into ome:develop Jul 20, 2023
17 checks passed
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.

3 participants