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

Fix error when unloading a script with multiple variables sections #6047

Merged
merged 8 commits into from
Sep 22, 2023
48 changes: 31 additions & 17 deletions src/main/java/ch/njol/skript/structures/StructVariables.java
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,7 @@
*/
package ch.njol.skript.structures;

import java.util.ArrayDeque;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Deque;
import java.util.HashMap;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.*;
Copy link
Member

Choose a reason for hiding this comment

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

star import should be reverted

Copy link
Member Author

Choose a reason for hiding this comment

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

oh my bad I didn't notice


import org.bukkit.event.Event;
import org.jetbrains.annotations.Nullable;
Expand Down Expand Up @@ -87,14 +80,14 @@ public static class DefaultVariables implements ScriptData {

private final Deque<Map<String, Class<?>[]>> hints = new ArrayDeque<>();
private final List<NonNullPair<String, Object>> variables;
private boolean loaded;

public DefaultVariables(Collection<NonNullPair<String, Object>> variables) {
this.variables = ImmutableList.copyOf(variables);
}

@SuppressWarnings("unchecked")
public void add(String variable, Class<?>... hints) {
if (hints == null || hints.length <= 0)
if (hints == null || hints.length == 0)
return;
if (CollectionUtils.containsAll(hints, Object.class)) // Ignore useless type hint.
return;
Expand All @@ -115,7 +108,7 @@ public void exitScope() {
/**
* Returns the type hints of a variable.
* Can be null if no type hint was saved.
*
*
* @param variable The variable string of a variable.
* @return type hints of a variable if found otherwise null.
*/
Expand All @@ -140,14 +133,24 @@ public boolean hasDefaultVariables() {
public List<NonNullPair<String, Object>> getVariables() {
return variables;
}

private boolean isLoaded() {
return loaded;
}
}

@Override
public boolean init(Literal<?>[] args, int matchedPattern, ParseResult parseResult, EntryContainer entryContainer) {
SectionNode node = entryContainer.getSource();
node.convertToEntries(0, "=");

List<NonNullPair<String, Object>> variables = new ArrayList<>();
List<NonNullPair<String, Object>> variables;
Script script = getParser().getCurrentScript();
Copy link
Member

Choose a reason for hiding this comment

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

What happens if a user just writes down variables: in effect commands? does it throw a NPE?

Copy link
Member

Choose a reason for hiding this comment

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

Nothing should happen since effect commands are parsed as an effect :)

DefaultVariables existing = script.getData(DefaultVariables.class); // if the user has TWO variables: sections
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
DefaultVariables existing = script.getData(DefaultVariables.class); // if the user has TWO variables: sections
DefaultVariables existing = script.getData(DefaultVariables.class); // if the user has TWO+ variables: sections

if (existing != null && existing.hasDefaultVariables()) {
variables = new ArrayList<>(existing.variables);
} else {
variables = new ArrayList<>();
}
for (Node n : node) {
if (!(n instanceof EntryNode)) {
Skript.error("Invalid line in variables structure");
Expand Down Expand Up @@ -227,29 +230,40 @@ public boolean init(Literal<?>[] args, int matchedPattern, ParseResult parseResu
}
variables.add(new NonNullPair<>(name, o));
}
getParser().getCurrentScript().addData(new DefaultVariables(variables));
script.addData(new DefaultVariables(variables)); // we replace the previous entry
return true;
}

@Override
public boolean load() {
DefaultVariables data = getParser().getCurrentScript().getData(DefaultVariables.class);
if (data == null) { // this shouldn't happen
Skript.error("Default variables data missing");
return false;
} else if (data.isLoaded()) {
return true;
}
for (NonNullPair<String, Object> pair : data.getVariables()) {
APickledWalrus marked this conversation as resolved.
Show resolved Hide resolved
String name = pair.getKey();
if (Variables.getVariable(name, null, false) != null)
continue;

Variables.setVariable(name, pair.getValue(), null, false);
}
data.loaded = true;
return true;
}

@Override
public void postUnload() {
Script script = getParser().getCurrentScript();
DefaultVariables data = script.getData(DefaultVariables.class);
for (NonNullPair<String, Object> pair : data.getVariables())
Variables.setVariable(pair.getKey(), null, null, false);
if (data == null) // band-aid fix for this section's behaviour being handled by a previous section
return; // see https://github.com/SkriptLang/Skript/issues/6013
for (NonNullPair<String, Object> pair : data.getVariables()) {
String name = pair.getKey();
if (name.contains("<") && name.contains(">")) // probably a template made by us
Variables.setVariable(pair.getKey(), null, null, false);
}
script.removeData(DefaultVariables.class);
}

Expand Down