Fix Smart Save dialog potentially using the wrong codec

[AlexTMjugador]

Codecs[codec] in the Smart Save dialog callback may not point to the same codec as the export_codec variable for custom formats that reuse the codecs of other formats, but proxy them with the JS Proxy class to implement the decorator pattern. To support this use case and increase the consistency of the code, which uses export_codec in the nearby lines, let's use that instead.

[oxkitsune]

Any update on this?

[JannisX11]

I was not looking to change this. There is no intended support for codecs where the value of their ID property does not match their ID in Codecs. There are probably a lot of other cases where it is required to match. So this change would not increase but decrease the consistency of the code. Also, this change would break the option to save as a project file.

[AlexTMjugador]

I don't want support for codecs whose ID property does not match their ID in the Codecs object.

I want to reuse a codec for a different format via proxying, which is a good software engineering practice according to DRY and code reuse principles.

More specifically, I'm interested in defining a new custom format whose relevant options are:

const formatOptions = {
  id: "format-id",
  // Other options...
  codec: new Proxy(Formats["java_block"].codec, {
    get: (target, property) => {
      console.info("Codec export hooks here (for applying pre-export transforms, etc.)");
      return target[property];
    },
  }),
};

This way the codec logic for a given format (in this case, java_block) can be reused for formats where it is desired to do so, but some extra behavior is wanted before and/or after exporting. I found an important use case for this for downscaling models before export, and then undoing such scaling.

Also, this change would break the option to save as a project file.

I worked around the current Blockbench behavior in my plugin by temporarily swapping the codec in the Codecs object with the value of export_codec and, after significant testing by both me and other teammates, we noticed no behavior breakage due to this change, including saving as a project file. My workaround looks like this (in Typescript):

Edit: after giving this a further look I agree this change does not have the expected behavior when selecting the "Blockbench Project" option in the smart save dialog, as codec has a value of project in the affected else if, which should be taken as a signal that export_codec should not be used. Still, this can be better handled by adding another else if (codec == "project") { Codecs.project.export(); } case before this one:

diff --git a/js/io/io.js b/js/io/io.js
index 919760ab..d5ff91a7 100644
--- a/js/io/io.js
+++ b/js/io/io.js
@@ -723,9 +723,10 @@ BARS.defineActions(function() {
                                                                if (codec == 'both') {
                                                                        Codecs.project.export();
                                                                        export_codec.export();
-       
-                                                               } else if (codec) {
-                                                                       Codecs[codec].export();
+                                                               } else if (codec == 'project') {
+                                                                       Codecs.project.export();
+                                                               } else {
+                                                                       export_codec.export();
                                                                }
                                                                if (checkboxes.dont_show_again) {
                                                                        settings.dialog_save_codec.set(false);

So this change would not increase but decrease the consistency of the code.

I don't find hard evidence to support this claim given the fact that several lines immediately above this one (see lines 700, 708 and 718) use the export_codec variable instead.

I respect if you really decide that this change is not appropriate for Blockbench, but I don't believe it was properly reviewed, and its purpose well understood. Would you mind giving it another look? I don't mind answering any questions you might have about it :slightly_smiling_face: