From a6ed65c3d87beb71c0f8c1eaf7c98def0a78473e Mon Sep 17 00:00:00 2001 From: Henry Loenwind Date: Mon, 1 Jul 2024 15:39:10 +0200 Subject: [PATCH] Redid undo system And prevent invalid lists from being committed --- .../client/gui/ConfigurationScreen.java | 297 ++++++++++++------ 1 file changed, 207 insertions(+), 90 deletions(-) diff --git a/src/main/java/net/neoforged/neoforge/client/gui/ConfigurationScreen.java b/src/main/java/net/neoforged/neoforge/client/gui/ConfigurationScreen.java index 92290bad14..170ad1f10e 100644 --- a/src/main/java/net/neoforged/neoforge/client/gui/ConfigurationScreen.java +++ b/src/main/java/net/neoforged/neoforge/client/gui/ConfigurationScreen.java @@ -260,9 +260,10 @@ public Object any() { protected final Context context; protected boolean changed = false; protected RestartType needsRestart = RestartType.NONE; - protected final Map undoMap = new HashMap<>(); protected final Map sectionCache = new HashMap<>(); protected Button undoButton, resetButton; // must not be changed after creation unless the reference inside the layout also is replaced + protected final Button doneButton = Button.builder(CommonComponents.GUI_DONE, button -> onClose()).width(Button.SMALL_WIDTH).build(); + protected final UndoManager undoManager = new UndoManager(); public ConfigurationSectionScreen(final ModContainer mod, final Minecraft mc, final Screen parent, final ModConfig.Type type, final ModConfig modConfig) { this(Context.top(mod, mc, parent, type, modConfig), Component.translatable(translationChecker.check(mod.getModId() + ".configuration." + type.name().toLowerCase() + ".title"))); @@ -322,6 +323,17 @@ protected void onChanged(final String key) { } } + protected boolean isAnyNondefault() { + for (final Entry entry : context.entries) { + if (entry.getRawValue() instanceof final ModConfigSpec.ConfigValue cv) { + if (!(getValueSpec(entry.getKey()) instanceof ListValueSpec) && isNonDefault(cv)) { + return true; + } + } + } + return false; + } + @Override protected void addOptions() { rebuild(); @@ -332,7 +344,6 @@ protected ConfigurationSectionScreen rebuild() { if (list != null) { // this may be called early, skip and wait for init() then list.children().clear(); boolean hasUndoableElements = false; - boolean hasNonDefaultValues = false; final List elements = new ArrayList<>(); for (final Entry entry : context.entries) { final String key = entry.getKey(); @@ -358,7 +369,6 @@ protected ConfigurationSectionScreen rebuild() { elements.add(createOtherValue(key, cv)); } hasUndoableElements |= elements.getLast() != null; - hasNonDefaultValues |= isNonDefault(cv); } } else if (rawValue instanceof final UnmodifiableConfig subsection) { final Object object = context.valueSpecs.get(key); @@ -385,7 +395,6 @@ protected ConfigurationSectionScreen rebuild() { if (hasUndoableElements && undoButton == null) { createUndoButton(); createResetButton(); - setResetButtonstate(hasNonDefaultValues); } } return this; @@ -404,9 +413,13 @@ protected Element createStringValue(final String key, final Predicate te box.setResponder(newValue -> { if (newValue != null && tester.test(newValue)) { if (!newValue.equals(source.get())) { - undoMap.putIfAbsent(key, source.get()); - target.accept(newValue); - onChanged(key); + undoManager.add(v -> { + target.accept(v); + onChanged(key); + }, newValue, v -> { + target.accept(v); + onChanged(key); + }, source.get()); } box.setTextColor(EditBox.DEFAULT_TEXT_COLOR); return; @@ -475,11 +488,15 @@ public Codec codec() { protected Element createBooleanValue(final String key, final ValueSpec spec, final Supplier source, final Consumer target) { return new Element(getTranslationComponent(key), getTooltipComponent(key), - new OptionInstance<>(getTranslationKey(key), getTooltip(key), OptionInstance.BOOLEAN_TO_STRING, BOOLEAN_VALUES_NO_PREFIX, source.get(), newVal -> { + new OptionInstance<>(getTranslationKey(key), getTooltip(key), OptionInstance.BOOLEAN_TO_STRING, BOOLEAN_VALUES_NO_PREFIX, source.get(), newValue -> { // regarding change detection: new value always is different (cycle button) - undoMap.putIfAbsent(key, source.get()); - target.accept(newVal); - onChanged(key); + undoManager.add(v -> { + target.accept(v); + onChanged(key); + }, newValue, v -> { + target.accept(v); + onChanged(key); + }, source.get()); })); } @@ -492,9 +509,13 @@ protected > Element createEnumValue(final String key, final Va new OptionInstance<>(getTranslationKey(key), getTooltip(key), (caption, displayvalue) -> Component.literal(displayvalue.name()), new Custom<>(list), source.get(), newValue -> { // regarding change detection: new value always is different (cycle button) - undoMap.putIfAbsent(key, source.get()); - target.accept(newValue); - onChanged(key); + undoManager.add(v -> { + target.accept(v); + onChanged(key); + }, newValue, v -> { + target.accept(v); + onChanged(key); + }, source.get()); })); } @@ -509,9 +530,13 @@ protected Element createIntegerValue(final String key, final ValueSpec spec, fin (caption, displayvalue) -> Options.genericValueLabel(caption, Component.literal("" + displayvalue)), new OptionInstance.IntRange(min, max), null /* codec */, source.get(), newValue -> { if (!newValue.equals(source.get())) { - undoMap.putIfAbsent(key, source.get()); - target.accept(newValue); - onChanged(key); + undoManager.add(v -> { + target.accept(v); + onChanged(key); + }, newValue, v -> { + target.accept(v); + onChanged(key); + }, source.get()); } })); } else { @@ -530,24 +555,28 @@ protected > Element createNumberBox(fin final EditBox box = new EditBox(font, Button.DEFAULT_WIDTH, Button.DEFAULT_HEIGHT, getTranslationComponent(key)); box.setEditable(true); - box.setFilter(newValue -> { + box.setFilter(newValueString -> { try { - parser.apply(newValue); + parser.apply(newValueString); return true; } catch (final NumberFormatException e) { - return newValue.isEmpty() || ((range == null || range.getMin().compareTo(zero) < 0) && newValue.equals("-")); + return newValueString.isEmpty() || ((range == null || range.getMin().compareTo(zero) < 0) && newValueString.equals("-")); } }); box.setTooltip(Tooltip.create(getTooltipComponent(key))); box.setValue(source.get() + ""); - box.setResponder(newValue -> { + box.setResponder(newValueString -> { try { - final T val = parser.apply(newValue); - if (tester != null ? tester.test(val) : (val != null && (range == null || range.test(val)) && spec.test(val))) { - if (!val.equals(source.get())) { - undoMap.putIfAbsent(key, source.get()); - target.accept(val); - onChanged(key); + final T newValue = parser.apply(newValueString); + if (tester != null ? tester.test(newValue) : (newValue != null && (range == null || range.test(newValue)) && spec.test(newValue))) { + if (!newValue.equals(source.get())) { + undoManager.add(v -> { + target.accept(v); + onChanged(key); + }, newValue, v -> { + target.accept(v); + onChanged(key); + }, source.get()); } box.setTextColor(EditBox.DEFAULT_TEXT_COLOR); return; @@ -584,9 +613,9 @@ protected Element createList(final String key, final ListValueSpec spec, fin @Override public void render(GuiGraphics p_281549_, int p_281550_, int p_282878_, float p_282465_) { + setUndoButtonstate(undoManager.canUndo()); // in render()? Really? --- Yes! This is how vanilla does it. + setResetButtonstate(isAnyNondefault()); super.render(p_281549_, p_281550_, p_282878_, p_282465_); - // need to check both as a screen may opt to store its initial value in the map on creation. The list screen does this. - setUndoButtonstate(changed && !undoMap.isEmpty()); // in render()? Really? --- Yes! This is how vanilla does it. } @Override @@ -599,7 +628,7 @@ protected void addFooter() { if (resetButton != null) { linearlayout.addChild(resetButton); } - linearlayout.addChild(Button.builder(CommonComponents.GUI_DONE, button -> onClose()).build()); + linearlayout.addChild(doneButton); } else { super.addFooter(); } @@ -607,14 +636,9 @@ protected void addFooter() { protected void createUndoButton() { undoButton = Button.builder(UNDO, button -> { - for (final Entry entry : context.entries) { - if (entry.getRawValue() instanceof final ModConfigSpec.ConfigValue cv) { - undo(entry.getKey(), cv); - } - } - setResetButtonstate(true); + undoManager.undo(); rebuild(); - }).tooltip(Tooltip.create(UNDO_TOOLTIP)).build(); + }).tooltip(Tooltip.create(UNDO_TOOLTIP)).width(Button.SMALL_WIDTH).build(); undoButton.active = false; } @@ -624,16 +648,27 @@ protected void setUndoButtonstate(boolean state) { } } + @SuppressWarnings("unchecked") protected void createResetButton() { resetButton = Button.builder(RESET, button -> { + List> list = new ArrayList<>(); for (final Entry entry : context.entries) { if (entry.getRawValue() instanceof final ModConfigSpec.ConfigValue cv) { - reset(entry.getKey(), cv); + if (!(getValueSpec(entry.getKey()) instanceof ListValueSpec) && isNonDefault(cv)) { + final String key = entry.getKey(); + list.add(undoManager.step(v -> { + cv.set(v); + onChanged(key); + }, getValueSpec(key).correct(null), v -> { + cv.set(v); + onChanged(key); + }, cv.get())); + } } } - setResetButtonstate(false); + undoManager.add(list); rebuild(); - }).tooltip(Tooltip.create(RESET_TOOLTIP)).build(); + }).tooltip(Tooltip.create(RESET_TOOLTIP)).width(Button.SMALL_WIDTH).build(); } protected void setResetButtonstate(boolean state) { @@ -642,23 +677,6 @@ protected void setResetButtonstate(boolean state) { } } - @SuppressWarnings("unchecked") - protected void undo(String key, @SuppressWarnings("rawtypes") ModConfigSpec.ConfigValue configValue) { - if (undoMap.containsKey(key)) { - configValue.set(undoMap.remove(key)); - onChanged(key); - } - } - - @SuppressWarnings("unchecked") - protected void reset(String key, @SuppressWarnings("rawtypes") ModConfigSpec.ConfigValue configValue) { - if (isNonDefault(configValue)) { - undoMap.put(key, configValue.get()); - configValue.set(getValueSpec(key).correct(null)); - onChanged(key); - } - } - @Override public void onClose() { if (changed) { @@ -725,7 +743,15 @@ protected ConfigurationSectionScreen rebuild() { idx = 0; for (final Element element : elements) { if (element != null) { - list.addSmall(createListLabel(idx), element.getWidget(options)); + final AbstractWidget widget = element.getWidget(options); + if (widget instanceof EditBox box) { + // Force our responder to check content and set text colour. + // This is only needed on lists, as section cannot have new UI elements added with bad data. + // Here, this can happen when a new element is added to the list. + // As the new value is the old value, no undo record will be created. + box.setValue(box.getValue()); + } + list.addSmall(createListLabel(idx), widget); } idx++; } @@ -734,12 +760,15 @@ protected ConfigurationSectionScreen rebuild() { if (undoButton == null) { createUndoButton(); createResetButton(); - setResetButtonstate(isNonDefault(valueList)); } } return this; } + protected boolean isAnyNondefault() { + return !cfgList.equals(valueList.getDefault()); + } + /** * Creates a button to add a new element to the end of the list and adds it to the UI.

* @@ -752,10 +781,16 @@ protected void createAddElementButton() { if (newElement != null && (sizeRange == null || sizeRange.test(cfgList.size() + 1))) { list.addSmall(Button.builder(NEW_LIST_ELEMENT, button -> { - cfgList.add((T) newElement.get()); + List newValue = new ArrayList<>(cfgList); + newValue.add((T) newElement.get()); + undoManager.add(v -> { + cfgList = v; + onChanged(key); + }, newValue, v -> { + cfgList = v; + onChanged(key); + }, cfgList); rebuild(); - onChanged(key); - undoMap.putIfAbsent(key, 0); }).build(), null); } } @@ -820,10 +855,14 @@ protected boolean swap(final int idx, final boolean simulate) { values.add(idx, values.remove(idx + 1)); final boolean valid = spec.test(values); if (!simulate && valid) { - cfgList = values; + undoManager.add(v -> { + cfgList = v; + onChanged(key); + }, values, v -> { + cfgList = v; + onChanged(key); + }, cfgList); rebuild(); - onChanged(key); - undoMap.putIfAbsent(key, 0); } return valid; } @@ -833,10 +872,14 @@ protected boolean del(final int idx, final boolean simulate) { values.remove(idx); final boolean valid = spec.test(values); if (!simulate && valid) { - cfgList = values; + undoManager.add(v -> { + cfgList = v; + onChanged(key); + }, values, v -> { + cfgList = v; + onChanged(key); + }, cfgList); rebuild(); - onChanged(key); - undoMap.putIfAbsent(key, 0); } return valid; } @@ -852,37 +895,38 @@ public void onClose() { super.onClose(); } + @Override + public void render(GuiGraphics p_281549_, int p_281550_, int p_282878_, float p_282465_) { + doneButton.active = spec.test(cfgList); + super.render(p_281549_, p_281550_, p_282878_, p_282465_); + } + protected void onChanged(final String key) { changed = true; // parent's onChanged() will be fired when we actually assign the changed list } @SuppressWarnings("unchecked") - protected void undo(String key, @SuppressWarnings("rawtypes") ModConfigSpec.ConfigValue configValue) { - if (key.equals(this.key)) { - // we can't trust this value, it is most likely the undo value for one of the edit fields. - // But if we get a List, it must have been set by reset(), as there is no edit field that - // can produce Lists. Then we can (and must) use that instead of falling back to the original - // value, because in that case we're undoing a reset. - final Object undoValue = undoMap.remove(key); - cfgList = new ArrayList<>(undoValue instanceof List ? (List) undoValue : valueList.get()); - } - changed = false; + protected void createResetButton() { + resetButton = Button.builder(RESET, button -> { + undoManager.add( + v -> { + cfgList = v; + onChanged(key); + }, new ArrayList<>((List) getValueSpec(key).correct(null)), + v -> { + cfgList = v; + onChanged(key); + }, cfgList); + rebuild(); + }).tooltip(Tooltip.create(RESET_TOOLTIP)).width(Button.SMALL_WIDTH).build(); } - @SuppressWarnings("unchecked") - protected void reset(String key, @SuppressWarnings("rawtypes") ConfigValue configValue) { - if (key.equals(this.key)) { - undoMap.put(key, cfgList); - configValue.set(getValueSpec(key).correct(null)); - onChanged(key); - // actually changes the value here, so notify the parent - if (context.parent instanceof ConfigurationSectionScreen parent) { - parent.onChanged(key); - } - cfgList = new ArrayList<>(valueList.get()); + protected void setResetButtonstate(boolean state) { + if (resetButton != null) { + resetButton.active = state; } - }; + } public class ListLabelWidget extends AbstractContainerWidget { protected final Button upButton = Button.builder(MOVE_LIST_ELEMENT_UP, this::up).build(); @@ -1006,4 +1050,77 @@ protected void updateWidgetNarration(final NarrationElementOutput pNarrationElem } } } + + /** + * A class representing an undo/redo buffer.

+ * + * Every undo step is represented as 2 actions, one to initially execute when the step is added and + * to redo after an undo, and one to execute to undo the step. + */ + public static final class UndoManager { + public static record Step(Consumer run, T newValue, Consumer undo, T oldValue) { + private void runUndo() { + undo.accept(oldValue); + } + + private void runRedo() { + run.accept(newValue); + } + }; + + private final List> undos = new ArrayList<>(); + private final List> redos = new ArrayList<>(); + + public void undo() { + if (canUndo()) { + Step step = undos.removeLast(); + step.runUndo(); + redos.add(step); + } + } + + public void redo() { + if (canRedo()) { + Step step = redos.removeLast(); + step.runRedo(); + undos.add(step); + } + } + + private void add(Step step, boolean execute) { + undos.add(step); + redos.clear(); + if (execute) { + step.runRedo(); + } + } + + public Step step(Consumer run, T newValue, Consumer undo, T oldValue) { + return new Step<>(run, newValue, undo, oldValue); + } + + public void add(Consumer run, T newValue, Consumer undo, T oldValue) { + add(step(run, newValue, undo, oldValue), true); + } + + public void addNoExecute(Consumer run, T newValue, Consumer undo, T oldValue) { + add(step(run, newValue, undo, oldValue), false); + } + + public void add(Step... steps) { + add(ImmutableList.copyOf(steps)); + } + + public void add(final List> steps) { + add(new Step<>(n -> steps.forEach(Step::runRedo), null, n -> steps.forEach(Step::runUndo), null), true); + } + + public boolean canUndo() { + return !undos.isEmpty(); + } + + public boolean canRedo() { + return !redos.isEmpty(); + } + } }