From 0f05960eeebe0b68595c23c2f14557647fe5d982 Mon Sep 17 00:00:00 2001 From: soloturn Date: Sat, 22 Aug 2026 12:25:32 +0200 Subject: [PATCH] fix(reporter): sort log tabs alphabetically instead of by creation time sortLogFiles() sorted by file creation time, reversed (newest first). With two log files from one session (Terasology-init.log, Terasology-menu.log) that reads as arbitrary - neither the order they were written in nor the order their names suggest - rather than a deliberate choice. Sorting by filename instead is deterministic and, for Terasology's own log naming, happens to match session order too. Also carries the GlobalProperties NPE guard from #56 (needed to construct GlobalProperties() at all in cr-core's own test classpath, same as there - see that PR for the full explanation). Will collapse to a no-op merge once #56 lands first. First item from #53 (1 of 5); items 2 and 3 are #56 and #58. Co-Authored-By: Claude Sonnet 5 --- .../crashreporter/GlobalProperties.java | 22 +++++---- .../pages/ErrorMessagePanel.java | 35 ++++++++++----- .../pages/ErrorMessagePanelTabOrderTest.java | 45 +++++++++++++++++++ 3 files changed, 81 insertions(+), 21 deletions(-) create mode 100644 cr-core/src/test/java/org/terasology/crashreporter/pages/ErrorMessagePanelTabOrderTest.java diff --git a/cr-core/src/main/java/org/terasology/crashreporter/GlobalProperties.java b/cr-core/src/main/java/org/terasology/crashreporter/GlobalProperties.java index 7c7cade..94a062b 100644 --- a/cr-core/src/main/java/org/terasology/crashreporter/GlobalProperties.java +++ b/cr-core/src/main/java/org/terasology/crashreporter/GlobalProperties.java @@ -39,16 +39,20 @@ public enum KEY { public GlobalProperties() { String propsUrl = "/crashreporter.properties"; String defaultPropsUrl = "/crashreporter_defaults.properties"; - try (InputStream stream = CrashReporter.class.getResourceAsStream(defaultPropsUrl)) { - properties.load(stream); - } catch (IOException e) { - // this should never go wrong - System.err.println("Unable to load default properties"); - } - try (InputStream stream = CrashReporter.class.getResourceAsStream(propsUrl)) { - properties.load(stream); + loadIfPresent(defaultPropsUrl); + // Only cr-core's downstream consumers (cr-terasology, cr-destsol, ...) ship this file - + // it's absent when cr-core is used standalone, which getResourceAsStream signals with + // null rather than an IOException, so that has to be checked explicitly. + loadIfPresent(propsUrl); + } + + private void loadIfPresent(String resourceUrl) { + try (InputStream stream = CrashReporter.class.getResourceAsStream(resourceUrl)) { + if (stream != null) { + properties.load(stream); + } } catch (IOException e) { - System.err.println("Unable to load " + propsUrl); + System.err.println("Unable to load " + resourceUrl); } } diff --git a/cr-core/src/main/java/org/terasology/crashreporter/pages/ErrorMessagePanel.java b/cr-core/src/main/java/org/terasology/crashreporter/pages/ErrorMessagePanel.java index 2b7518f..9bdbbf6 100644 --- a/cr-core/src/main/java/org/terasology/crashreporter/pages/ErrorMessagePanel.java +++ b/cr-core/src/main/java/org/terasology/crashreporter/pages/ErrorMessagePanel.java @@ -35,7 +35,6 @@ import java.nio.file.Path; import java.nio.file.SimpleFileVisitor; import java.nio.file.attribute.BasicFileAttributes; -import java.nio.file.attribute.FileTime; import java.util.Comparator; import java.util.EnumSet; import java.util.List; @@ -163,24 +162,24 @@ else if (evt.getPropertyName() == LogUpdateWorker.MODIFIED) { logUpdateWorker.execute(); } + /** + * Alphabetical by filename, not creation time (see #53 item 1): creation-time order looked + * arbitrary to users - two log files from one session sort as "newest first", which is + * neither the order they were written in nor the order their names suggest. Alphabetical is + * deterministic and, for Terasology's own naming (e.g. {@code Terasology-init.log} before + * {@code Terasology-menu.log}), happens to match session order too. + */ private static void sortLogFiles(List files) { files.sort(new Comparator() { @Override public int compare(Path p0, Path p1) { - try { - BasicFileAttributes attr0 = Files.readAttributes(p0, BasicFileAttributes.class); - BasicFileAttributes attr1 = Files.readAttributes(p1, BasicFileAttributes.class); - FileTime time0 = attr0.creationTime(); - FileTime time1 = attr1.creationTime(); - return time0.compareTo(time1); - } catch (Exception e) { - // ignore silently - return 0; - } + String name0 = p0.getFileName().toString(); + String name1 = p1.getFileName().toString(); + return name0.compareToIgnoreCase(name1); } - }.reversed()); // invert sort order + }); } @Override @@ -246,6 +245,18 @@ public Path getLogFile() { return idx >= 0 ? logFiles.get(idx) : null; } + /** + * @return the tab titles (filenames relative to the log folder) in the order they're + * displayed - see {@link #sortLogFiles}. + */ + public List getTabTitles() { + List titles = Lists.newArrayListWithCapacity(tabPane.getTabCount()); + for (int i = 0; i < tabPane.getTabCount(); i++) { + titles.add(tabPane.getTitleAt(i)); + } + return titles; + } + private static String readLogFileContent(Path logFile) { StringBuilder builder = new StringBuilder(); diff --git a/cr-core/src/test/java/org/terasology/crashreporter/pages/ErrorMessagePanelTabOrderTest.java b/cr-core/src/test/java/org/terasology/crashreporter/pages/ErrorMessagePanelTabOrderTest.java new file mode 100644 index 0000000..f418822 --- /dev/null +++ b/cr-core/src/test/java/org/terasology/crashreporter/pages/ErrorMessagePanelTabOrderTest.java @@ -0,0 +1,45 @@ +// Copyright 2026 The Terasology Foundation +// SPDX-License-Identifier: Apache-2.0 + +package org.terasology.crashreporter.pages; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import org.terasology.crashreporter.CrashReporter; +import org.terasology.crashreporter.GlobalProperties; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Arrays; +import java.util.List; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +/** + * Regression test for #53 item 1: log tabs sorted by file creation time (newest first) instead of + * alphabetically, which read as arbitrary/broken with more than one log file present. + */ +class ErrorMessagePanelTabOrderTest { + + @Test + void tabsAreOrderedAlphabeticallyRegardlessOfCreationOrder(@TempDir Path logFolder) throws IOException, InterruptedException { + // Written out of alphabetical order, with a real gap between creation times so a + // creation-time-based sort (the old behavior) would disagree with alphabetical order. + writeLog(logFolder, "Terasology-menu.log", "MENU"); + Thread.sleep(10); + writeLog(logFolder, "Terasology-init.log", "INIT"); + + ErrorMessagePanel panel = new ErrorMessagePanel(new GlobalProperties(), new RuntimeException("boom"), + logFolder, CrashReporter.MODE.CRASH_REPORTER); + + List titles = panel.getTabTitles(); + assertEquals(Arrays.asList("Terasology-init.log", "Terasology-menu.log"), titles, + "Expected tabs in alphabetical order regardless of which file was created first, got: " + titles); + } + + private static void writeLog(Path folder, String name, String content) throws IOException { + Files.write(folder.resolve(name), content.getBytes(StandardCharsets.UTF_8)); + } +}