(Possible) Breaking change: local variables references inside nested function definitions

[DanH]

@christoph-hart I'm getting an error in my expansion installer callback on the line "local origin = hr;". I think this was largely copied from one of the example projects provided by @Christoph-Hart.

Just so I'm crystal clear, do I want to be using something other than 'local'?

I've had some users reporting that the expansion installer gets stuck trying to install the samples - I wonder if this is related...

inline function onINSTALLEXPControl(component, value)
    {
        
        if (value == 1)
        {
    
  local msg = "SELECT EXPANSION .hr1 FILE";

  Engine.showYesNoWindow("SELECT .HR1 FILE", msg, function(response)
  {
    if (response)
    {
      FileSystem.browse(FileSystem.Downloads, false, "*.hr1", function(hr)
      {
        if (hr.toString(2) == ".hr1")
        {
          local origin = hr; // Scoped copy
          msg = "SELECT A LOCATION FOR THE SAMPLE FILES";

          Engine.showYesNoWindow("SAMPLE LOCATION", msg, function(response)
          {
            if (response)
            {
              FileSystem.browseForDirectory(FileSystem.Samples, function(dir)
              {
                 expHandler.installExpansionFromPackage(origin, dir);
              });
            }
          });
        }
      });
    }
  });
}
    }

Content.getComponent("INSTALLEXP").setControlCallback(onINSTALLEXPControl);

[d.healey]

@danh You need to declare that variable as a reg outside of the function, put it at the top of your script/namespace

[DanH]

@d-healey ok so just:

reg origin = hr; // Scoped copy

inline function onINSTALLEXPControl(component, value)
    {
        
        if (value == 1)
        {
    
  local msg = "SELECT EXPANSION .hr1 FILE";

  Engine.showYesNoWindow("SELECT .HR1 FILE", msg, function(response)
  {
    if (response)
    {
      FileSystem.browse(FileSystem.Downloads, false, "*.hr1", function(hr)
      {
        if (hr.toString(2) == ".hr1")
        {
          
          msg = "SELECT A LOCATION FOR THE SAMPLE FILES";

          Engine.showYesNoWindow("SAMPLE LOCATION", msg, function(response)
          {
            if (response)
            {
              FileSystem.browseForDirectory(FileSystem.Samples, function(dir)
              {
                 expHandler.installExpansionFromPackage(origin, dir);
              });
            }
          });
        }
      });
    }
  });
}
    }

Content.getComponent("INSTALLEXP").setControlCallback(onINSTALLEXPControl);

[Christoph Hart]

@DanH yes this is example ticks all boxes for that issue, but I can't guarantee that there is nothing else wrong :)

But there is another code smell - you're declaring the local variable inside the nested function definition, which should not be possible (and I think the recent change will also prevent that, but I'll have to check).

In your case you might get away with just changing local to var for the origin definition.

[DanH]

@christoph-hart yes I changed 'local origin = hr;' to 'var origin = hr;' and the errors were gone.

EDIT: It's all going wrong when trying to install an expansion now though :face_with_tears_of_joy:

[d.healey]

@danh said in (Possible) Breaking change: local variables references inside nested function definitions:

if (hr.toString(2) == ".hr1")

Magic number alert!!! You should use hr.toString(hr.OnlyExtension)

[d.healey]

@danh said in (Possible) Breaking change: local variables references inside nested function definitions:

ok so just:

Yes, but it's no longer scoped now so you can change that comment ;)

Also hr does exist at init so you can't assign the value there, you have to do that within your function like you were before.

[DanH]

@d-healey said in (Possible) Breaking change: local variables references inside nested function definitions:

Also hr does exist at init so you can't assign the value there, you have to do that within your function like you were before.

with 'var'?

[d.healey]

@danh I'd say use a reg but if Christoph says a var is okay then var is ok. My point was you have reg origin = hr at the top of your script now, but hr doesn't exist until your FileSystem.browse call, so you can't assign it when you declare the origin variable at the top of your script you have to assign it after the call to browse just like you were with local origin = hr now you just put origin = hr. Or do what Christoph said.

[DanH]

This post is deleted!

[DanH]

@d-healey said in (Possible) Breaking change: local variables references inside nested function definitions:

Magic number alert!!! You should use hr.toString(hr.OnlyExtension)

Right, changing to the above makes the install process stop before the window appears allowing me to select the destination for the samples. So... select the .hr1 file and then nothing more...

[d.healey]

@danh Oo could be that the constant is bugged. I'm away from the computer for the next day so can't test, stick to your magic number for now :)

[ustk]

hr was already external, right? so why bother using another variable origin for the same thing in the first place?
It might be me, everyone around is telling me I need glasses... Arfff... 44... might be the time anyway...

[d.healey]

@ustk hr comes from the file browse call but the scope doesn't extend to the second browse call so it needs to be stored in a temporary variable

[ustk]

Just to say that I updated my custom branch, and all my var changed to local in the current project that is dependent on this very custom branch.
It's not a big deal to change back everything though as I remember this fix. But in the case it can be fixed it could help other people not to have this surprise, without knowing where it comes from :)

[ustk]

This post is deleted!

[orange]

@ustk Same here. I've used tons of local variables in all of my projects. Now this shit comes...

[d.healey]

@orange This problem was always there, it's just now you get a warning. It only applies to local variables in certain circumstances.

[orange]

@d-healey said in (Possible) Breaking change: local variables references inside nested function definitions:

@orange This problem was always there

My previous projects can't be opened with the latest develop branch.
But if I modify the local variables as var it works. So this issue is a new one.

[d.healey]

@orange

But if I modify the local variables as var it works. So this issue is a new one.

No it's an old issue, but now there is a new error so it might cause problems with opening old projects.

You don't need to change all locals to var - in fact that is a really bad idea. You only need to change the ones that are in nested functions. And I recommend not changing them to var because that pollutes the scriptwide namespace, I think it's better to create a reg object at the top of each namespace that can be used to hold the values - should I make a video about this?