Skip to content
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
package com.ultikits.plugins.sidebar.config;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;

import com.ultikits.ultitools.abstracts.AbstractConfigEntity;
import com.ultikits.ultitools.annotations.ConfigEntity;
Expand Down Expand Up @@ -44,13 +47,13 @@ public class SideBarConfig extends AbstractConfigEntity {
"&7欢迎, &f%player_name%",
"",
"&e在线人数: &f%server_online%/%server_max_players%",
"&e世界: &f%world_name%",
"&e世界: &f%player_world%",
"",
"&e金币: &f%vault_eco_balance_formatted%",
"&ePing: &f%player_ping%ms",
"",
"&7服务器时间",
"&f%server_time_hh:mm:ss%",
"&f%server_time_HH:mm:ss%",

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Migrate the persisted 12-hour time line

On upgrades where sidebar.yml already contains the old shipped &f%server_time_hh:mm:ss% entry, AbstractConfigEntity.init() preserves the persisted lines list, while the new migration rewrites only the world-name entry. Consequently, this HH correction reaches fresh installations only, and existing servers continue displaying an ambiguous 12-hour time without an AM/PM marker; include the byte-identical legacy time entry in the targeted migration as well.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Real finding, fixed in commits 1694c8f (RED test) and e6792ed (fix).

Confirmed by reading git log -p on this file: the pre-6.3-equivalent shipped default was the
byte-identical line "&f%server_time_hh:mm:ss%", and AbstractConfigEntity.init() never
overwrites an existing lines value on disk -- exactly the same persistence gap migrateLegacyWorldNameDefaultLine()
was written for, just not extended to this second entry.

Two new RED tests proved the defect before the fix: a persisted list containing only the legacy
time line was not rewritten, and a real-upgrade scenario (both legacy lines + an operator's
custom line) left the time line stale while migrating the world-name line and preserving the
custom line correctly. Both failed against the unmodified method.

Fix: renamed migrateLegacyWorldNameDefaultLine() to migrateLegacyDefaultLines() and
generalised it to an exact-match lookup table (LEGACY_LINE_REPLACEMENTS) mapping every tracked
legacy default (the %world_name% world line and the hh:mm:ss time line) to its corrected
replacement, so a future shipped-default correction only needs a map entry, not a new loop.
SideBarService.init()'s call site and comment updated to match. Operator-customised lines,
including a line that only mentions a legacy token inside other text, are still left untouched --
covered by the pre-existing exact-match test.

mvn -B verify: Tests run: 124, Failures: 0, Errors: 0, Skipped: 0; BUILD SUCCESS; jacoco "All
coverage checks have been met."

"",
"&6play.example.com"
);
Expand All @@ -64,4 +67,87 @@ public class SideBarConfig extends AbstractConfigEntity {
public SideBarConfig() {
super("config/sidebar.yml");
}

/**
* The pre-6.3.0 shipped default world-name line, which used the invalid PlaceholderAPI
* syntax {@code %world_name%} (UltiKits/UltiSideBar#13 -- the real "World" expansion
* placeholder, {@code %world_name_<world>%}, requires an explicit world argument).
* {@code AbstractConfigEntity.init()} never overwrites a key that already exists on disk,
* so any server that has ever started this plugin keeps this exact string in its persisted
* {@code sidebar.yml} forever unless it is rewritten explicitly.
*/
private static final String LEGACY_WORLD_NAME_LINE = "&e世界: &f%world_name%";

/**
* The corrected default that replaces {@link #LEGACY_WORLD_NAME_LINE}, kept in sync by hand
* with the "lines" default above.
*/
private static final String CURRENT_WORLD_NAME_LINE = "&e世界: &f%player_world%";

/**
* The pre-6.3.0 shipped default server-time line, which used the ambiguous 12-hour pattern
* {@code hh:mm:ss} with no AM/PM marker (PR #15 round-3 review). Same persistence problem as
* {@link #LEGACY_WORLD_NAME_LINE}: {@code AbstractConfigEntity.init()} preserves this exact
* string in {@code sidebar.yml} on every server that has ever started an older version of
* this plugin, unless it is rewritten explicitly.
*/
private static final String LEGACY_SERVER_TIME_LINE = "&f%server_time_hh:mm:ss%";

/**
* The corrected default that replaces {@link #LEGACY_SERVER_TIME_LINE} with the unambiguous
* 24-hour pattern, kept in sync by hand with the "lines" default above.
*/
private static final String CURRENT_SERVER_TIME_LINE = "&f%server_time_HH:mm:ss%";

/**
* Every byte-identical legacy default line this plugin has ever shipped, mapped to its
* corrected replacement. Extend this map -- not the loop in
* {@link #migrateLegacyDefaultLines()} -- when a future shipped default needs the same
* exact-match migration treatment.
*/
private static final Map<String, String> LEGACY_LINE_REPLACEMENTS;

static {
Map<String, String> replacements = new LinkedHashMap<>();
replacements.put(LEGACY_WORLD_NAME_LINE, CURRENT_WORLD_NAME_LINE);
replacements.put(LEGACY_SERVER_TIME_LINE, CURRENT_SERVER_TIME_LINE);
LEGACY_LINE_REPLACEMENTS = Collections.unmodifiableMap(replacements);
}

/**
* One-time migration for a persisted {@code sidebar.yml} whose {@code lines} list still
* carries one or more old, invalid shipped defaults tracked in
* {@link #LEGACY_LINE_REPLACEMENTS} (issue #13; PR #15 round-3 review extended this from the
* world-name line alone to also cover the 12-hour server-time line). Rewrites only a list
* entry that is byte-identical to a tracked legacy default -- any operator customisation,
* including a line that merely mentions a legacy token alongside other text, is left
* untouched. Idempotent: once migrated, no entry matches a tracked legacy default any more,
* so a second call is a no-op.
* <p>
* Must be called after {@code init(UltiToolsPlugin)} has populated {@link #lines} from
* disk. The caller is responsible for persisting the result with {@code save()} when this
* method returns {@code true} -- this method only updates the in-memory value.
*
* @return {@code true} if at least one line was rewritten, {@code false} otherwise
*/
public boolean migrateLegacyDefaultLines() {
if (lines == null) {
return false;
}
boolean changed = false;
List<String> migrated = new ArrayList<>(lines.size());
for (String line : lines) {
String replacement = LEGACY_LINE_REPLACEMENTS.get(line);
if (replacement != null) {
migrated.add(replacement);
changed = true;
} else {
migrated.add(line);
}
}
if (changed) {
lines = migrated;
}
return changed;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import org.bukkit.scoreboard.*;
import org.bukkit.scheduler.BukkitTask;

import java.io.IOException;
import java.util.*;
import java.util.concurrent.ConcurrentHashMap;

Expand Down Expand Up @@ -59,6 +60,24 @@ public void init() {
dataOperator = plugin.getDataOperator(SideBarPreference.class);
bukkitPlugin = Bukkit.getPluginManager().getPlugin("UltiTools");

// One-time migration (issue #13, CR-01; extended by PR #15 round-3 review to also cover
// the legacy 12-hour server-time line): AbstractConfigEntity.init() -- which has already
// run by this point, via UltiToolsPlugin's constructor -- only fills keys that are
// MISSING from the persisted file and never overwrites an existing "lines" value, so a
// server that has ever started an older version of this plugin keeps every stale shipped
// default (the invalid %world_name% line, the ambiguous %server_time_hh:mm:ss% line)
// forever without this explicit, exact-match rewrite. Runs again on every reload() (this
// method is also called from reload()), which is harmless: once migrated, the exact-match
// check finds nothing left to rewrite.
if (config.migrateLegacyDefaultLines()) {
try {
config.save();
} catch (IOException e) {
plugin.getLogger().warn("Failed to persist the sidebar.yml legacy default line "
+ "migration: " + e.getMessage());
}
}

// Check PlaceholderAPI
placeholderApiAvailable = Bukkit.getPluginManager().getPlugin("PlaceholderAPI") != null;
if (!placeholderApiAvailable) {
Expand Down
Loading