Skip to content

Conversation

@Alexees
Copy link
Contributor

@Alexees Alexees commented Aug 13, 2019

Overview

The MixedRealityToolkitFiles.cs TryUnregisterModuleFolder iterates a foreach loop and deletes entries within it, but instead of exiting it sets a bool to true which it returns at the end continuing to iterate the loop resulting in an Exception.

Changes

Now returns on entry find.

Verification

I recognized that when I moved the MRTK generated folders over to another one and then removed the old folder.

@Alexees Alexees changed the title Fixed iterating foreach after altering it TryUnregisterModuleFolder now returns on path deletion Aug 13, 2019
@wiwei
Copy link
Contributor

wiwei commented Aug 13, 2019

FYI, I think with the change here:

https://github.com/microsoft/MixedRealityToolkit-Unity/pull/5603/files

Going through all the mrtkFolders may actually be required - in practice, I don't think you'll actually see this really, but it might be good to keep this code as-is until the other PR goes in.

@wiwei
Copy link
Contributor

wiwei commented Aug 14, 2019

Okay, I think that with the new changes highlighted in that PR this is going to be necessary to handle NuGet consumption cases, even if it doesn't show up in non-nuget cases

@wiwei wiwei closed this Aug 14, 2019
@wiwei
Copy link
Contributor

wiwei commented Aug 14, 2019

If I hadn't made that other PR though, this would have been in a heartbeat

@Alexees Alexees deleted the mrtk_UnregisterModuleFolderFix branch August 28, 2019 06:42
@Alexees
Copy link
Contributor Author

Alexees commented Aug 28, 2019

@wiwei do you mean #5603, cause if so, this did not fix the problem. The loop still alters itself:

foreach (var modFolders in mrtkFolders)
{
if (modFolders.Value.Remove(normalizedFolder))
{
if (modFolders.Value.Count == 0)
{
mrtkFolders.Remove(modFolders.Key);
}
found = true;
}
}

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.

2 participants