diff --git a/AGENTS.md b/AGENTS.md index 3302da4ce6..612aae08d9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -7,5 +7,10 @@ metadata. Preserve copyright headers, upstream notices, license provenance, `@author` tags, and historical attribution unless the user explicitly asks for a specific removal and the legal/provenance impact has been considered. +For code that remains live in the repository, authorship and copyright +information must stay attached to that code. Removing or moving dead code may +remove the corresponding attribution only when the attributed code is removed as +well. + Comment cleanup may update stale technical claims, but it must not erase who wrote or contributed the original code. diff --git a/BUGS.md b/BUGS.md index c9c17d6dab..458be4c095 100644 --- a/BUGS.md +++ b/BUGS.md @@ -47,7 +47,7 @@ What grew: ## 1. Real bugs -### 1.1 HIGH — Zip-slip / TAR-slip on extraction +### 1.1 ~~HIGH — Zip-slip / TAR-slip on extraction~~ **FIXED** **`barebones-format-zip/.../ZipArchiveFile.java:112-147`**, **`barebones-format-tar/.../TarEntryIterator.java:97-116`** @@ -57,6 +57,10 @@ normalisation, no leading-`/` rejection, no backslash rejection. A crafted archive with `../../etc/passwd` writes outside the extraction root. +Fixed before this pass: archive iterators validate entry names through +`SafePath`, skip and log unsafe entries, and tests cover leading slash, +drive-prefix, backslash, NUL, and parent-escape rejection. + ### 1.2 ~~HIGH — SFTP host-key verification not configured~~ **FIXED** Fixed in Phase 14: new `HostKeyPolicy` enum (YES / ASK / NO, default ASK) wired through `session.setConfig("StrictHostKeyChecking", …)`. @@ -89,7 +93,7 @@ Per-receive `setSoTimeout` was already wired upstream. Still missing: a global "fail RPC after N seconds" budget that bounds the retry loop in `Rpc.rpc_call`. -### 1.5 HIGH — TransferFileJob has no network timeouts +### 1.5 ~~HIGH — TransferFileJob has no network timeouts~~ **FIXED** **`barebones-core/.../job/impl/TransferFileJob.java`**, plus the underlying SFTP connect: **`barebones-protocol-sftp/.../SFTPConnectionHandler.java:91`** @@ -98,6 +102,11 @@ Connect timeout is 5 s (probably too short on slow VPN), but the ongoing transfer has no socket-read timeout — a half-open TCP connection wedges the copy-job thread indefinitely. +Fixed before this pass: `SftpTimeouts` now exposes configurable +connect, read, and keepalive bounds, and `SFTPConnectionHandler` applies +them to the JSch session/channel so wedged SFTP transfers fail in bounded +time instead of hanging the job thread indefinitely. + ### 1.6 ~~HIGH — macOS Keychain item-ref leaked~~ **FIXED** Fixed in Phase 14: new `CFRelease` JNA binding in `SecurityFramework`; `deleteByLookup()` now calls it on the @@ -124,12 +133,16 @@ existing `equals()` contract. `CredentialsMappingTest` round-trips through `HashSet` to prove the fix. SpotBugs baseline shrunk by 4 entries. -### 1.10 MED — `ZipInputStream` leaked on iteration error +### 1.10 ~~MED — `ZipInputStream` leaked on iteration error~~ **FIXED** **`barebones-format-zip/.../ZipArchiveFile.java:250-258`** If an exception is thrown mid-iteration the stream is never closed in a finally. Each failed lookup leaks an FD. +Fixed before this pass: the non-random-access zip-entry lookup now +closes the `ZipInputStream` on every failure path and attaches close +failures as suppressed exceptions. + ### 1.11 ~~MED — S3 connection-cache key contains plaintext access+secret keys~~ **FIXED** Fixed in Phase 14: cache key now uses `SHA-256(accessKey + 0x00 + secretKey)` (hex-encoded) in the @@ -146,33 +159,50 @@ serialised, or appears in a heap dump, the secret is exposed. Fix: hash credentials (SHA-256) into the cache key. --> -### 1.12 MED — `S3TransferManager.completionFuture().join()` blocks EDT +### 1.12 ~~MED — `S3TransferManager.completionFuture().join()` blocks EDT~~ **RESOLVED** **`barebones-protocol-s3/.../S3Object.java:329-330`** `SpillingPutOutputStream.close()` is reachable from a Swing copy-job on the EDT. A 40 MiB upload then freezes the UI for the full upload duration. Needs a SwingWorker shim. -### 1.13 MED — Decompression-bomb / per-entry size limits absent +Resolved before this pass: file transfers run in `FileJob.start()` on a +dedicated job thread, not on the EDT, and S3 multipart uploads publish +status-bar progress while the job thread waits for transfer completion. +The synchronous `OutputStream.close()` contract is preserved so callers +do not observe a successful close before the upload is durable. + +### 1.13 ~~MED — Decompression-bomb / per-entry size limits absent~~ **FIXED** **`barebones-format-zip/`**, **`barebones-format-tar/`**, **`barebones-archiver/`** No per-entry or cumulative size cap during extraction. A 1 MB zip that expands to 50 GB will exhaust memory or disk. Same for entry count: a million-entry zip parses its central directory unbounded. -### 1.14 MED — Text viewer loads entire file into memory +Fixed before this pass: archive listing wraps iterators with +`BoundedExtraction`, enforcing per-entry, cumulative declared-size, and +entry-count caps. `BoundedExtractionTest` covers the cap behavior. + +### 1.14 ~~MED — Text viewer loads entire file into memory~~ **FIXED** **`barebones-viewer-text/.../TextViewer.java`** No size check before handing the bytes to `RSyntaxTextArea`. Opening a multi-GB log file crashes the JVM. -### 1.15 MED — `AbstractArchiveFile.createEntriesTree()` is not thread-safe (TODO admits) +Fixed before this pass: `TextViewer` prompts before loading files above +the large-file threshold and aborts the open if the user declines. + +### 1.15 ~~MED — `AbstractArchiveFile.createEntriesTree()` is not thread-safe (TODO admits)~~ **FIXED** **`barebones-commons-file/.../AbstractArchiveFile.java:122`** Multiple threads calling `ls()` simultaneously can race on the shared tree-build state. Latent because the file table mostly serialises calls, but parallel directory listings hit it. +Fixed before this pass: `createEntriesTree()` and `checkEntriesTree()` +are synchronized, so concurrent listings cannot race on +`entryTreeRoot`, `entryTreeDate`, or `archiveEntryFiles`. + ### 1.16 ~~MED — `AppleScript.outputBuffer` is unbounded~~ **FIXED** `ScriptOutputListener` now caps at 1 MiB (`MAX_OUTPUT_CHARS = 1 << 20`). Once exceeded, a visible @@ -200,12 +230,17 @@ and `writesAfterTruncationAreDropped`. shared daemon thread, not an EDT freeze. Conversion to `wait/notify` would touch every call site for negligible benefit. -### 1.18 MED — S3 `isDirectory()` / `exists()` swallow IOException → false-negative +### 1.18 ~~MED — S3 `isDirectory()` / `exists()` swallow IOException → false-negative~~ **FIXED** **`barebones-protocol-s3/.../S3Object.java:96-116`** `HeadObject` returning a transient 5xx makes the file look like it doesn't exist; the user sees their files vanish until refresh. +Phase 32 now treats `NoSuchKey` as the only normal missing-object path. +Other metadata lookup failures are logged and `exists()` reports the +last known state, or true for unknown state, instead of turning a +transient lookup failure into a false absence. + ### 1.19 ~~MED — S3 connection cache grows unbounded, never closed~~ **FIXED (shutdown)** `S3ProtocolProvider` now implements `AutoCloseable`; the bootstrap shutdown hook reflectively invokes `Activator.shutdown()` which @@ -278,10 +313,16 @@ The `main` is a CLI utility (`java EncodingDetector ` → prints the detected encoding). The println is the CLI's only output, not stray debug. Kept. -### 1.28 LOW — `ZipArchiveFile.java:154` hard-codes UTF-8 for symlink targets +### 1.28 ~~LOW — `ZipArchiveFile.java:154` hard-codes UTF-8 for symlink targets~~ **RESOLVED** Zip spec allows non-UTF-8; an EFS-flagged entry is fine but legacy encoded ones (CP932 etc) round-trip wrong. +Resolved before this pass: the supported platform scope is macOS/Linux, +where symlink targets are treated as UTF-8 path text for this Java UI. +The legacy non-EFS zip filename encoding path remains handled by the zip +provider; symlink payload bytes do not carry an EFS bit or a reliable zip +metadata encoding to apply instead. + ### 1.29 ~~MED — Green checks could hide no-op test / analysis runs~~ **FIXED** Phase 31 found three ways CI could look green while proving too little: `./gradlew test` could reuse up-to-date outputs instead of re-executing tests, @@ -351,6 +392,356 @@ updates, combo-box refreshes, and loading-state changes run from `done()` on the EDT. Look-and-feel install failures are logged and the failed class is not added to the custom list. +### 1.35 ~~MED — SFTP stream-open cleanup swallows close failures~~ **FIXED** +**`barebones-protocol-sftp/.../SFTPFile.java:191,582`** + +When `getOutputStream()` or `getInputStream(long)` fails after acquiring an +`SFTPConnectionHandler`, the error path tries to close the handler but swallows +any close failure in an empty catch. That hides leaked/dirty connection state +from both logs and callers, exactly in the path where the original operation +already failed. + +Phase 32 preserves the original stream-open failure and logs cleanup-close +failures with the SFTP URL and thrown close exception. + +### 1.36 ~~MED — S3 metadata probes still hide transient failures~~ **FIXED** +**`barebones-protocol-s3/.../S3Object.java:96-135`** + +`isDirectory()`, `exists()`, `getDate()`, and `getSize()` still catch +`IOException` from `ensureMetadata()` and return `false` or `0` without a log or +other signal. A transient 5xx, timeout, or auth failure can make an object look +absent/empty/stale until refresh, and callers cannot distinguish "missing key" +from "metadata lookup failed". + +Phase 32 keeps `NoSuchKey` as the normal "missing" path, records the last +metadata failure, and logs every non-missing metadata lookup failure from +`isDirectory()`, `exists()`, `getDate()`, and `getSize()` with URL context. +Unknown metadata is not marked as a successful absence. + +### 1.37 ~~LOW — Runtime OS family still carries unsupported legacy platforms~~ **FIXED** +**`barebones-commons-runtime/.../OsFamily.java`**, +**`barebones-commons-file/.../FileURL.java:902`** + +The active product scope is macOS + Linux only, but `OsFamily` still recognizes +Solaris, OS/2, FreeBSD, AIX, HP-UX, OpenVMS, and Haiku. That stale surface keeps +dead branches such as OS/2 case-insensitive path comparison alive and makes +`UNKNOWN_OS_FAMILY` report as Unix-based. After Windows and other platform +support were removed, unknown/non-target OSes should not masquerade as supported +Unix behavior. + +Phase 32 reduced runtime OS family handling to macOS, Linux, and unknown. +Mac/Linux stay Unix-based; unknown does not. The OS/2-only case-insensitive +`FileURL.pathEquals()` branch was removed with the OS/2 family. + +### 1.38 ~~MED — `OpenWithMenu` mutates Swing menus off the EDT~~ **FIXED** +**`barebones-core/.../OpenWithMenu.java:121-142`** + +The "Open With" menu correctly pushes native application discovery off the +event-dispatch thread, but the worker thread then adds separators/actions, +stops the spinner, removes the loading item, changes enabled state, and repacks +the popup directly. Those are Swing mutations and can race with menu refresh, +painting, and popup lifecycle. + +Phase 32 keeps native application discovery in the background, then applies the +menu changes from `SwingWorker.done()` on the EDT. Stale worker results are +discarded if the selected file changed before discovery finished, and discovery +failures are logged instead of leaving a spinning loading item behind. + +### 1.39 ~~MED — `FolderPanel` builds Swing components on a raw background thread~~ **FIXED** +**`barebones-core/.../FolderPanel.java:144-176`** + +`FolderPanel` starts an anonymous thread to create and install the drive button, +location field, breadcrumb bar, drop targets, and focus listener. Those are +Swing/AWT component operations and must happen on the event-dispatch thread. +The anonymous worker also has no lifecycle owner or error reporting. + +Phase 32 removes the raw thread and constructs the location controls +synchronously with the rest of the panel initialization. The constructor is +already called through the existing `MainFrame` panel-building path, so this +keeps UI state deterministic without adding another background lifecycle. + +### 1.40 ~~MED — Status-bar volume updater writes Swing state off the EDT~~ **FIXED** +**`barebones-core/.../StatusBar.java:455-475`** + +The status-bar disk-space updater runs on a daemon thread, which is correct for +potentially slow filesystem probes, but it calls +`volumeSpaceLabel.setVolumeSpace(...)` directly from that worker. That mutates +Swing label state outside the event-dispatch thread once per minute while the +main frame is active. + +Phase 32 keeps the filesystem probes on the daemon updater and marshals the +label update back through `SwingUtilities.invokeLater(...)`. + +### 1.41 ~~MED — delayed file-table edit action runs off the EDT~~ **FIXED** +**`barebones-core/.../FileTable.java:1312-1335`** + +A single click on the current row starts an anonymous thread, sleeps 800 ms, and +then may open filename/date/permissions editing actions directly from that +worker thread. The delay is UI event timing, not background I/O, and the action +path mutates Swing state. + +Phase 32 replaces the custom sleep thread with a non-repeating Swing `Timer`, so +the double-click delay stays event-driven and the edit/action path runs on the +EDT. + +### 1.42 ~~MED — quick-list icon loading races Swing state and spawns duplicate workers~~ **FIXED** +**`barebones-core/.../QuickListWithIcons.java:123-138`** + +While a quick-list icon is loading, every repaint that sees the waiting icon +starts another anonymous worker for the same item. Those workers update the +shared `HashMap`, stop the spinning icon, and repaint the Swing popup directly +from background threads, racing both popup reopen/clear and rendering. + +Phase 32 switches the icon cache to a concurrent map, starts only the first +loader per item, and marshals spinner/repaint changes back onto the EDT. + +### 1.43 ~~MED — queued trash swallows interrupts while waiting~~ **FIXED** +**`barebones-os-api/.../QueuedTrash.java:105-107,144-146`** + +`waitForPendingOperations()` drops `InterruptedException` and continues as if +the caller had waited successfully. The trash batching thread also drops +interrupts during its debounce sleep, so shutdown/cancel paths cannot observe +that the wait was interrupted. + +Phase 32 restores the interrupt flag in both paths. Callers waiting for pending +trash work return with the interrupt preserved, and the batching thread stops +debouncing and moves the currently queued files instead of hiding the signal. + +### 1.44 ~~LOW — S3 spilled-upload temp-file cleanup failure is invisible~~ **FIXED** +**`barebones-protocol-s3/.../S3Object.java:323-329`** + +`SpillingPutOutputStream.close()` deletes the temporary upload spill file in a +best-effort cleanup block, but an `IOException` from `Files.deleteIfExists(...)` +is silently ignored. The OS may eventually sweep temp storage, but a failed +delete is still useful diagnostic context for long-running sessions and disk +pressure reports. + +Phase 32 logs a warning with the spill path and exception while preserving the +primary upload/close result. + +### 1.45 ~~MED — update-check dialog builds and shows Swing UI from a worker thread~~ **FIXED** +**`barebones-core/.../CheckVersionDialog.java:104-205`** + +`CheckVersionDialog` starts a raw background thread to avoid blocking on the +version lookup, but the thread then calls `setTitle`, `init`, `addComponent`, +`getActionValue`, `dispose`, and error-dialog code directly. The network lookup +belongs off the EDT; dialog construction and interaction do not. + +Phase 32 replaces the raw `Thread`/`Runnable` path with `SwingWorker`: version +lookup and browser-support probing run in `doInBackground()`, while result UI, +modal interaction, preference persistence, and fallback error dialogs run from +`done()` on the EDT. + +### 1.46 ~~LOW — text-editor miss beep creates unbounded anonymous threads~~ **FIXED** +**`barebones-viewer-text/.../TextEditorImpl.java:281-288`** + +When search finds no match, the editor starts a new anonymous thread for each +beep because `Toolkit.beep()` can block. Holding the shortcut at the end of a +file can create repeated short-lived threads for a non-critical UI signal. + +Phase 32 routes beeps through a daemon single-thread executor and coalesces +requests while a beep is already running. + +### 1.47 ~~MED — server connect panels silently ignore invalid port commits~~ **FIXED** +**`barebones-protocol-sftp/.../SFTPPanel.java:173-178`**, +**`barebones-protocol-nfs/.../NFSPanel.java:129-134`**, +**`barebones-protocol-s3/.../S3Panel.java:144-150`** + +When the user edits a port spinner and confirms the connect dialog with Enter, +each kept remote panel calls `commitEdit()` and swallows `ParseException`. +Invalid text can therefore fall back to the previous spinner value with no +visible error, making the dialog connect somewhere other than what the field +appears to contain. + +Phase 32 converts the parse failure into an `IllegalArgumentException` with the +offending value. `ServerConnectDialog` catches it, logs the validation failure, +and leaves the dialog open with an error message instead of proceeding. + +### 1.48 ~~MED — SFTP random-access seek hides close failures~~ **FIXED** +**`barebones-protocol-sftp/.../SFTPFile.java:804-810`** + +`SFTPRandomAccessInputStream.seek(long)` closes the current stream before +opening a new one at the requested offset, but it silently ignores +`IOException` from the close. A failed close can hide connection cleanup trouble +and then report the seek as successful if the replacement stream opens. + +Phase 32 lets the close failure propagate from `seek(...)`, which already +declares `IOException`, instead of fabricating a successful reposition. + +### 1.49 ~~LOW — About dialog hides homepage browse failures~~ **FIXED** +**`barebones-core/.../AboutDialog.java:430-436`** + +Clicking the homepage button catches and ignores `IOException` from +`DesktopManager.browse(...)`. On systems without a working browser/open handler, +the button appears to do nothing and the failure is not logged. + +Phase 32 logs the failure and shows the same style of error dialog used by +other browser-open paths. + +### 1.50 ~~LOW — S3 provider shutdown hides connection-close failures~~ **FIXED** +**`barebones-protocol-s3/.../S3ProtocolProvider.java:51-57`** + +`S3ProtocolProvider.close()` catches and ignores `RuntimeException` from cached +connection close. The method can run during normal app shutdown or explicit +provider teardown, so close failures should not disappear entirely. + +Phase 32 adds provider logging and records a warning for each failed cached +connection close while still continuing to close the rest of the cache. + +### 1.51 ~~LOW — shutdown-hook removal state is silently ignored~~ **FIXED** +**`barebones-core/.../Activator.java:122-127`** + +`Activator.stopAll()` ignores `IllegalStateException` from +`Runtime.removeShutdownHook(...)`. The VM-already-shutting-down case is benign, +but it should still be visible when debug logging is enabled so shutdown +ordering problems can be diagnosed. + +Phase 32 logs the already-shutting-down state at DEBUG and continues with the +normal quit path. + +### 1.52 ~~MED — S3 object metadata cache survives delete on same instance~~ **FIXED** +**`barebones-protocol-s3/.../S3Object.java:225-235`** + +The MinIO Testcontainers integration added in Phase 32 exposed that +`S3Object.delete()` removes the remote key but leaves the same Java object with +`metadataKnown=true` from a prior upload/head call. A following `exists()` on +that instance can report `true` without re-checking the backend. + +Phase 32 updates local metadata state after a successful delete so the same +object immediately reports absent, and rename source objects inherit that state +through the existing copy-then-delete path. + +### 1.53 ~~MED — notification popup close timer mutates Swing off the EDT~~ **FIXED** +**`barebones-core/.../NotificationPopup.java:57-58,191-203`** + +The notification popup schedules close events with a default `java.util.Timer`, +which creates a non-daemon timer thread and invokes `popup.hidePopup()` directly +from that background thread. Popup visibility is Swing state, so the hide must +run on the event-dispatch thread; the non-daemon timer also gives a singleton UI +helper an avoidable JVM-lifetime side effect. + +Phase 32 replaces the utility timer/task pair with a non-repeating Swing +`Timer`. Closing now runs on the EDT, replacing a pending close cancels the old +Swing timer, and no extra timer thread is kept alive by the popup singleton. + +### 1.54 ~~MED — async file-frame loader mutates frame UI off the EDT~~ **FIXED** +**`barebones-core/.../FileFrame.java:88-107`** + +`FileFrame` uses `AsyncPanel` so viewer/editor file opening can run away from +the event-dispatch thread, but the worker path also calls `setJMenuBar(...)`, +`showGenericErrorDialog()`, and `dispose()` directly. Those are frame/dialog UI +mutations and can race the viewer window lifecycle. + +Phase 32 leaves file-presenter opening on the existing background path, but +moves menu-bar installation into the `AsyncPanel.updateLayout()` callback that +already runs on the EDT. Error-dialog display and disposal are also marshalled +through `SwingUtilities.invokeLater(...)`. + +### 1.55 ~~MED — async panel load failures leave a permanent loading spinner~~ **FIXED** +**`barebones-core/.../AsyncPanel.java:106-115`** + +`AsyncPanel.loadTargetComponent()` starts a worker and calls +`getTargetComponent()` with no error boundary. If a subclass throws a runtime +exception while creating the target component, the worker dies, the wait +component remains visible forever, and the failure is not logged. + +Phase 32 names the loader thread, logs runtime failures, and replaces the wait +component with a small error label on the EDT instead of leaving a fake loading +state behind. + +### 1.56 ~~MED — Claude review found follow-up GUI/S3 edge cases~~ **FIXED** +**`barebones-core/.../FileFrame.java`**, +**`barebones-core/.../OpenWithMenu.java`**, +**`barebones-core/.../NotificationPopup.java`**, +**`barebones-core/.../AsyncPanel.java`**, +**`barebones-protocol-s3/.../S3Object.java`**, +**`barebones-protocol-s3/.../S3MinIOIntegrationTest.java`** + +The authenticated noninteractive Claude Code review of PR #40 found several +actionable follow-ups in the sweep changes: `FileFrame.updateLayout()` could +still run after an async presenter-open failure and dereference a failed +presenter path; `OpenWithMenu` could insert a leading separator because the +loading item was still counted; `NotificationPopup`'s new Swing timer assumed +all callers were already on the EDT; `AsyncPanel` still built its fallback error +label on the worker thread; S3 metadata failure logging compared exception +identity outside the synchronized metadata section; and the MinIO integration +test relied on the repo-wide JUnit PER_CLASS lifecycle setting instead of +declaring its own lifecycle. + +Phase 32 fixed those review findings by short-circuiting the failed +`FileFrame` layout path, restoring the "more than just loading item" separator +condition, marshaling notification display to the EDT, constructing the async +fallback label on the EDT, simplifying S3 metadata failure logging, and adding +an explicit `@TestInstance(PER_CLASS)` annotation to the MinIO test. + +### 1.57 ~~LOW — Claude review found remaining async cleanup issues~~ **FIXED** +**`barebones-viewer-text/.../TextEditorImpl.java`**, +**`barebones-core/.../OpenWithMenu.java`**, +**`barebones-core/.../QuickListWithIcons.java`** + +The follow-up Claude review also found lower-risk async cleanup issues that +were not part of the first patch: the editor's coalesced beep helper used a +static executor with no application shutdown ownership, `OpenWithMenu` treated +only the exact same `AbstractFile` instance as the same async request, and +quick-list icon loading still created a raw thread for each uncached item. + +Phase 32 replaces the static beep executor with a bounded one-shot daemon task, +checks stale Open With results by requested file URL, and loads quick-list icons +through `SwingWorker` so icon lookups stay off the EDT while cache updates and +repaints return through Swing's lifecycle. + +### 1.58 ~~MED — Main frame startup still constructs Swing state off the EDT~~ **FIXED** +**`barebones-core/.../Application.java`**, +**`barebones-core/.../MainFrame.java`**, +**`barebones-core/.../FolderPanel.java`** + +The same sweep found that the application still creates the initial main frames +from a `MainFrameInit` thread and `MainFrame` builds both `FolderPanel` +instances through a worker executor. That means substantial Swing state is +constructed outside the EDT. This is broader than the focused quick-list and +notification fixes: safely moving it requires changing the startup lifecycle, +preload behavior, frame visibility timing, and the async work split between UI +construction and filesystem/model initialization. + +Phase 32 documents this as the next GUI-threading refactor candidate instead of +making a narrow partial change that would leave the startup invariant unclear. + +Phase 32 now routes non-EDT `WindowManager.createNewMainFrame(...)` +callers through `SwingUtilities.invokeAndWait(...)` and removes the +`MainFrame` worker executor that previously built folder panels, toolbar, +menu bar, status bar, and command bar off the EDT. The delayed startup +version-check dialog construction is also marshalled back to the EDT. + +### 1.59 ~~MED — Second Claude review found more async/lifecycle edges~~ **FIXED** +**`barebones-core/.../AsyncPanel.java`**, +**`barebones-core/.../FileFrame.java`**, +**`barebones-core/.../CheckVersionDialog.java`**, +**`barebones-core/.../NotificationPopup.java`**, +**`barebones-core/.../QuickListWithIcons.java`**, +**`barebones-os-api/.../QueuedTrash.java`**, +**`barebones-protocol-s3/.../S3Object.java`**, +**`barebones-protocol-sftp/.../SFTPFile.java`** + +A second authenticated noninteractive Claude review of the updated PR diff +found additional lifecycle issues: failed `FileFrame` presenter opens could +still return a half-initialized presenter to `AsyncPanel`; `AsyncPanel` only +caught `RuntimeException`, so linkage and other loader errors could still leave +the spinner up forever; S3 metadata cache fields were still unsynchronized +across lookups, upload metadata refresh, and delete invalidation; the version +check dialog opened a modal dialog directly from `SwingWorker.done()`; the +notification popup singleton could be constructed off the EDT; quick lists used +a shared static spinner across instances; SFTP random-access seek left the old +stream reference in place if close failed; and `QueuedTrash` interrupt handling +was easier to misread than necessary. + +Phase 32 fixes those by treating failed presenter opens as async-panel loader +failures, skipping async-panel replacement after a disposed failure path, +catching and logging loader `Throwable`, synchronizing S3 metadata mutation and +reads, scheduling version-result dialogs after the worker completion event, +constructing the notification popup singleton on the EDT, making quick-list +spinners instance-owned, clearing SFTP random-access streams before close, and +returning immediately when trash waiting is interrupted. + --- ## 2. UX gaps @@ -550,9 +941,9 @@ hanging forever. ### 4.2 ~~No retry / backoff on transient mount failures~~ **OBSOLETE** The mount-helper module was removed in PR #24. -### 4.3 S3 connection cache never closes connections (see 1.19) +### 4.3 ~~S3 connection cache never closes connections (see 1.19)~~ **FIXED** -### 4.4 AES-GCM key never zeroed on close (see 1.8) +### 4.4 ~~AES-GCM key never zeroed on close (see 1.8)~~ **FIXED** ### 4.5 ~~`WeakHashMap`-keyed listeners GC'd silently~~ **FIXED** `ThemeManager`, `ThemeData`, and `ThemeCache` now hold listeners @@ -564,22 +955,27 @@ model — listeners just GC'd themselves out of existence). Other WeakHashMap usages in the codebase are real key→value caches, not listener pseudo-sets, and are unaffected. -### 4.6 PARTIALLY FIXED — Shutdown hook registered for `SecretStore` +### 4.6 ~~PARTIALLY FIXED — Shutdown hook registered for `SecretStore`~~ **FIXED** Phase 14 wires `Bootstrap.shutdown()` as a JVM shutdown hook that closes the active `SecretStoreService.store()` (frees libsecret schema, zeroes AES-GCM key). Cached `S3Connection`s are NOT yet cleaned up — to land in Phase 16 alongside the rest of the shutdown / lifecycle work. -### 4.7 S3 `SpillingPutOutputStream` temp file: deletion-error masks upload error +Fixed before this pass: the shutdown path now closes cached S3 +connections through the provider shutdown hook as described in 1.19. + +### 4.7 ~~S3 `SpillingPutOutputStream` temp file: deletion-error masks upload error~~ **FIXED** **`barebones-protocol-s3/.../S3Object.java:319-338`** — if the finally's `Files.deleteIfExists` throws, it shadows the original upload exception. Catch + log the deletion failure, never let it escape from the finally. -### 4.8 NFS code → see 1.4 +### 4.8 ~~NFS code → see 1.4~~ **PARTIALLY FIXED** -### 4.9 SFTP fixed 5s connect timeout (see 1.5) — make configurable +### 4.9 ~~SFTP fixed 5s connect timeout (see 1.5) — make configurable~~ **FIXED** +`SftpTimeouts` now exposes configurable connect, read, and keepalive +settings. --- diff --git a/PLAN.md b/PLAN.md index 42450af9aa..ccf903ac2d 100644 --- a/PLAN.md +++ b/PLAN.md @@ -50,7 +50,8 @@ Source: forked from https://github.com/mucommander/mucommander to https://github | **28** | done | **Remove dead FreeBSD mount shell-out** — deleted the unsupported `/sbin/mount -p` path and kept Linux mount discovery on `/proc/mounts`. | this PR | | **29** | done | **JUnit 5 + protocol scope cleanup** — migrate legacy tests to JUnit 5, improve S3 endpoint URL parsing, remove retired-protocol future scope, and evaluate NFSv4 replacement options. | landed in #37 | | **30** | done | **Architecture refactor batch** — archive format `ServiceLoader`, remove vendored `apache-bzip2`, centralize runtime tunables, and make javac unchecked/deprecation warnings fail the build. | landed in #38 | -| **31** | in progress | **Repo skill + architecture/docs/check sweep** — add repo-local Java GUI slop cleanup skill, document current architecture, align stale docs/comments with the implementation, and harden CI against fake-green checks. | this PR | +| **31** | done | **Repo skill + architecture/docs/check sweep** — add repo-local Java GUI slop cleanup skill, document current architecture, align stale docs/comments with the implementation, and harden CI against fake-green checks. | landed in #39 | +| **32** | in progress | **Repo-wide Java GUI slop sweep** — run the repo-local slop-cleaning skill against current `origin/main`, record each finding in `BUGS.md`, fix actionable issues, and keep continuity docs current. | this PR | **Hard rule**: only one branch / one PR is in flight at a time. The user — not the LLM — decides when a PR is ready and when the next one starts. The LLM does not autonomously open new PRs to fan out work in parallel. @@ -1241,13 +1242,140 @@ has passed `./gradlew cleanTest test --stacktrace`, `./gradlew check fatJar :cyclonedxDirectBom --stacktrace` plus the CI artifact assertions), after the Claude second-opinion fixes were applied. The requested noninteractive `claude` review was approved by the user and completed; actionable findings were -applied. Before handing the PR back, push the branch and verify GitHub Actions on -PR #39. +applied. PR #39 was merged by the user. **Exit criteria**: repo-local skill and architecture docs are present; stale current-facing docs/comments are aligned without stripping authorship; no-op checks fail loudly; full checks and CI are green. +### Phase 32 — Repo-wide Java GUI slop sweep (this PR) + +Phase 32 starts from `origin/main` after PR #39 was merged. The active branch is +`phase-32/slop-sweep`; no PR exists yet. Scope is a single-PR sweep using the +repo-local `clean-java-gui-slop` skill: search for fake-green checks, Swing/EDT +mistakes, background work lifecycle leaks, swallowed failures, stale comments, +and accidental Windows support. Every new finding must be recorded in +`BUGS.md` before its fix is made, and this section must be updated after each +substantial chunk so work survives compaction or a fresh context. + +Current state: local `main` was fast-forwarded to `origin/main`, this branch was +created from `origin/main`, and the initial skill sweep recorded and fixed three +findings in `BUGS.md`: SFTP stream-open cleanup now logs handler-close failures, +S3 metadata probes now log non-missing lookup failures instead of silently +returning false/zero, and `OsFamily` has been narrowed to macOS/Linux/unknown +with focused tests. The OS/2-only `FileURL.pathEquals()` branch was removed. +The next GUI pass recorded and fixed two EDT findings: native "Open With" +application discovery now applies menu mutations from `SwingWorker.done()` and +discards stale refresh results, while `FolderPanel` no longer creates Swing/AWT +location controls from an unmanaged background thread. `AGENTS.md` now also +states the live-code rule that authorship/copyright stays attached to code that +remains in the repository. The follow-up GUI/threading pass recorded and fixed +three more findings: `StatusBar` now marshals disk-space label updates onto the +EDT, `FileTable` uses a Swing `Timer` for delayed single-click edit actions, +and `QuickListWithIcons` starts only one icon loader per item while keeping +spinner/repaint mutations on the EDT. The cleanup/interrupt pass then fixed +queued-trash interrupt handling and made S3 spilled-upload temp-file cleanup +failures visible in logs. The startup/text-viewer pass moved +`CheckVersionDialog` to a `SwingWorker` split so network lookup stays in the +background while dialog UI runs on the EDT, and coalesced text-editor miss beeps +through a daemon single-thread executor. The connect-dialog pass made SFTP, NFS, +and S3 invalid port commits fail visibly through `ServerConnectDialog` instead +of silently reusing the previous spinner value. The ignored-catch pass then +made SFTP random-access seek propagate close failures, surfaced About-dialog +homepage browse failures, and logged S3 provider connection-close failures. +It also made the benign shutdown-hook removal race visible at DEBUG instead of +silently swallowing the state. Local validation passed with `git diff --check`, +`./gradlew cleanTest test --stacktrace`, and `./gradlew check --stacktrace`. +User added one more required deliverable for this same PR: add a MinIO-backed +Testcontainers S3 integration path, alongside the existing LocalStack coverage, +so S3-compatible endpoint behavior is tested against a real MinIO server as +well. The MinIO image tag confirmed from Docker Hub metadata is +`minio/minio:RELEASE.2025-09-07T16-13-09Z`; the implementation should keep the +same Docker-unavailable skip behavior as the LocalStack tests. The new MinIO +test exposed and fixed one additional S3 correctness issue: successful +`S3Object.delete()` now invalidates local metadata so the same object instance +does not keep reporting `exists() == true`. Next work is to rerun local +validation, then commit, push, open the single Phase 32 PR, and monitor CI. +Final local validation after the MinIO addition passed with +`./gradlew :barebones-protocol-s3:test --tests ...S3MinIOIntegrationTest +--stacktrace`, `./gradlew cleanTest test --stacktrace`, and `./gradlew check +--stacktrace`. +After PR #40 opened, the user requested one more sweep and a Claude Code CLI +review attempt in the same PR. The extra sweep has so far recorded and fixed +two more EDT issues: `NotificationPopup` now uses a non-repeating Swing timer +instead of a non-daemon `java.util.Timer` that hid popups off the EDT, and +`FileFrame` now installs viewer menu bars and shows/disposes error UI from the +EDT path after async loading. It also made `AsyncPanel` loader failures logged +and visible instead of leaving a permanent loading spinner. Claude Code CLI is installed locally +(`claude --version` reports 2.1.143 and `claude --help` confirms `-p/--print` +noninteractive mode), but the smoke invocation currently fails immediately with +`Not logged in · Please run /login`; `claude doctor` then hung without output +and the started process was stopped. `claude auth status` confirms +`loggedIn: false`, and the dedicated noninteractive `claude ultrareview 40 +--timeout 1` path fails immediately with "Ultrareview is currently +unavailable." Local validation after the extra sweep passed with `git diff +--check`, `./gradlew cleanTest test --stacktrace`, and `./gradlew check +--stacktrace`; next work is to commit/push these extra fixes onto PR #40 and +watch CI again. +The user confirmed Claude login, and rerunning outside the sandbox showed +`claude auth status` logged in. A tool-using `claude -p` review launched but +hit `Reached max turns (20)` without findings, so the PR diff was piped into a +one-turn `claude -p` review. Claude reported actionable follow-ups that were +recorded in `BUGS.md` and fixed: explicit MinIO `@TestInstance(PER_CLASS)`, +failed `FileFrame` async layout short-circuit, `OpenWithMenu` loading-item +separator guard, EDT marshaling for `NotificationPopup.displayNotification`, +EDT construction of `AsyncPanel` fallback label, and simplified S3 metadata +failure logging without unsynchronized identity comparison. Next work is to +rerun validation, commit/push, and watch PR #40 CI again. +The continued Claude-guided sweep recorded and fixed three lower-risk async +cleanup items: `TextEditorImpl` no longer owns an unclosed static beep +executor, `OpenWithMenu` now accepts same-URL async Open With results instead +of requiring the same `AbstractFile` instance, and `QuickListWithIcons` now +uses `SwingWorker` for icon loading instead of raw per-item threads. The same +sweep also recorded a larger remaining GUI-threading issue: initial +`MainFrame`/`FolderPanel` construction still happens from `MainFrameInit` and a +worker executor instead of consistently on the EDT. That requires a dedicated +startup lifecycle refactor because a partial constructor-only edit would leave +the frame visibility and preload invariants ambiguous. Next work is to rerun +validation, commit/push, and watch PR #40 CI again. +A second current-diff Claude review eventually completed. Its concrete findings +were recorded as `BUGS.md` 1.59 and patched: failed file-presenter opens now +throw through `AsyncPanel` instead of returning half-initialized UI; async panel +loader failures include `Throwable` and skip replacement after the panel has +been disposed; `QueuedTrash.waitForPendingOperations()` returns immediately +after preserving interruption; `CheckVersionDialog` schedules modal result UI +after `SwingWorker.done()` returns; `NotificationPopup` is constructed on the +EDT; quick-list spinners are instance-owned; S3 metadata state is synchronized +across reads and mutations; and SFTP random-access streams clear stale handles +when seek/close closes the old stream. Next work is to rerun local validation, +commit/push, and watch PR #40 CI again. +Local validation after the second-review fixes passed with `git diff --check`, +focused compile plus `:barebones-protocol-s3:test --tests +dev.barebones.commander.commons.file.protocol.s3.S3MinIOIntegrationTest`, +`./gradlew cleanTest test --stacktrace`, and `./gradlew check --stacktrace`. +Next work is to commit/push the Phase 32 follow-up patch and watch PR #40 CI. +The user then requested fixing all remaining bugs in the same open PR. The pass +confirmed the old open real-bug entries were either already fixed by earlier +Phase 32 code or still concrete. The concrete changes in this pass route +non-EDT `WindowManager.createNewMainFrame(...)` calls through the EDT, remove +the `MainFrame` worker executor that built Swing components off the EDT, +marshal the delayed startup update-check dialog back to the EDT, and make S3 +`exists()` treat non-missing metadata lookup failures as unknown/last-known +state instead of false absence. `BUGS.md` now marks the stale/fixed real-bug +entries as fixed/resolved and leaves only architecture/refactor review notes as +open headings. Next work is to rerun validation, commit/push to PR #40, and +watch CI. +Validation for this all-bugs pass passed with `git diff --check`, focused core +compile plus MinIO S3 integration test, `./gradlew cleanTest test +--stacktrace`, and `./gradlew check --stacktrace`. Next work is to commit/push +to PR #40 and watch CI. + +**Exit criteria**: all actionable findings discovered in this sweep are either +fixed or explicitly documented as deferred; local validation includes at least +`./gradlew cleanTest test --stacktrace`, `./gradlew check --stacktrace`, +package-smoke artifact checks if CI/build wiring changes, and `git diff +--check`; a single PR is opened and GitHub Actions are green. + ## 7. Compatibility with upstream We may want to **pull bug fixes from upstream muCommander** for at least 1 year. To keep this cheap: diff --git a/barebones-commons-file/src/main/java/dev/barebones/commander/commons/file/FileURL.java b/barebones-commons-file/src/main/java/dev/barebones/commander/commons/file/FileURL.java index 65e1c96cb8..99207b93dd 100644 --- a/barebones-commons-file/src/main/java/dev/barebones/commander/commons/file/FileURL.java +++ b/barebones-commons-file/src/main/java/dev/barebones/commander/commons/file/FileURL.java @@ -34,7 +34,6 @@ import dev.barebones.commander.commons.file.protocol.search.SearchFile; import dev.barebones.commander.commons.file.protocol.search.SearchSchemeParser; import dev.barebones.commander.commons.file.util.PathUtils; -import dev.barebones.commander.commons.runtime.OsFamily; import dev.barebones.commander.commons.util.StringUtils; /** @@ -899,10 +898,8 @@ public boolean portEquals(FileURL url) { * @return true if the path of this URL and the given URL are equal */ public boolean pathEquals(FileURL url) { - boolean isCaseSensitiveOS = !OsFamily.getCurrent().equals(OsFamily.OS_2); - - String path1 = isCaseSensitiveOS ? this.getPath() : this.getPath().toLowerCase(); - String path2 = isCaseSensitiveOS ? url.getPath() : url.getPath().toLowerCase(); + String path1 = this.getPath(); + String path2 = url.getPath(); if(path1.equals(path2)) return true; diff --git a/barebones-commons-runtime/src/main/java/dev/barebones/commander/commons/runtime/OsFamily.java b/barebones-commons-runtime/src/main/java/dev/barebones/commander/commons/runtime/OsFamily.java index 1c1301d6ea..3dd98f3838 100644 --- a/barebones-commons-runtime/src/main/java/dev/barebones/commander/commons/runtime/OsFamily.java +++ b/barebones-commons-runtime/src/main/java/dev/barebones/commander/commons/runtime/OsFamily.java @@ -32,20 +32,6 @@ public enum OsFamily { MAC_OS("macOS"), /** Linux */ LINUX("Linux"), - /** Solaris */ - SOLARIS("Solaris"), - /** OS/2 */ - OS_2("OS/2"), - /** FreeBSD */ - FREEBSD("FreeBSD"), - /** AIX */ - AIX("AIX"), - /** HP-UX */ - HP_UX("HP-UX"), - /** OpenVMS */ - OPENVMS("OpenVMS"), - /** Haiku */ - HAIKU("Haiku"), /** Other OS */ UNKNOWN_OS_FAMILY("Unknown"); @@ -93,25 +79,13 @@ public static OsFamily getCurrent() { * * * @return true if the current OS is UNIX-based */ public boolean isUnixBased() { return this==MAC_OS - || this==LINUX - || this==SOLARIS - || this==FREEBSD - || this==AIX - || this==HP_UX - || this== UNKNOWN_OS_FAMILY; - - // Not UNIX-based: OS/2 and OpenVMS + || this==LINUX; } /** @@ -137,33 +111,10 @@ static OsFamily parseSystemProperty(String osNameProp) { if (osNameProp.startsWith("Mac OS X")) { return MAC_OS; } - // OS/2 family - if (osNameProp.startsWith("OS/2")) { - return OS_2; - } // Linux family if (osNameProp.startsWith("Linux")) { return LINUX; } - // Solaris family - if (osNameProp.startsWith("Solaris") || osNameProp.startsWith("SunOS")) { - return SOLARIS; - } - if (osNameProp.startsWith("FreeBSD")) { - return FREEBSD; - } - if (osNameProp.startsWith("AIX")) { - return AIX; - } - if (osNameProp.startsWith("HP-UX")) { - return HP_UX; - } - if (osNameProp.startsWith("OpenVMS")) { - return OPENVMS; - } - if (osNameProp.startsWith("Haiku")) { - return HAIKU; - } // Any other OS return UNKNOWN_OS_FAMILY; diff --git a/barebones-commons-runtime/src/test/java/dev/barebones/commander/commons/runtime/OsFamilyTest.java b/barebones-commons-runtime/src/test/java/dev/barebones/commander/commons/runtime/OsFamilyTest.java new file mode 100644 index 0000000000..0f6af8a7e9 --- /dev/null +++ b/barebones-commons-runtime/src/test/java/dev/barebones/commander/commons/runtime/OsFamilyTest.java @@ -0,0 +1,51 @@ +/* + * Copyright (C) 2026 barebones-commander contributors + * + * This file is free software; you can redistribute it and/or modify + * it under the terms of the GNU Lesser General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This file is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program. If not, see . + */ + +package dev.barebones.commander.commons.runtime; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import org.junit.jupiter.api.Test; + +/** + * A JUnit test case for {@link OsFamily}. + */ +public class OsFamilyTest { + + @Test + public void parsesSupportedFamilies() { + assertEquals(OsFamily.MAC_OS, OsFamily.parseSystemProperty("Mac OS X")); + assertEquals(OsFamily.LINUX, OsFamily.parseSystemProperty("Linux")); + } + + @Test + public void unsupportedFamiliesAreUnknown() { + assertEquals(OsFamily.UNKNOWN_OS_FAMILY, OsFamily.parseSystemProperty("Windows 11")); + assertEquals(OsFamily.UNKNOWN_OS_FAMILY, OsFamily.parseSystemProperty("SunOS")); + assertEquals(OsFamily.UNKNOWN_OS_FAMILY, OsFamily.parseSystemProperty("OS/2")); + assertEquals(OsFamily.UNKNOWN_OS_FAMILY, OsFamily.parseSystemProperty("OpenVMS")); + } + + @Test + public void onlySupportedFamiliesAreUnixBased() { + assertTrue(OsFamily.MAC_OS.isUnixBased()); + assertTrue(OsFamily.LINUX.isUnixBased()); + assertFalse(OsFamily.UNKNOWN_OS_FAMILY.isUnixBased()); + } +} diff --git a/barebones-core/src/main/java/dev/barebones/commander/Activator.java b/barebones-core/src/main/java/dev/barebones/commander/Activator.java index 7607324137..95c08e45de 100644 --- a/barebones-core/src/main/java/dev/barebones/commander/Activator.java +++ b/barebones-core/src/main/java/dev/barebones/commander/Activator.java @@ -122,8 +122,8 @@ public void stopAll() { if (ShutdownHook.performShutdownTasks() && shutdownHook != null) { try { Runtime.getRuntime().removeShutdownHook(shutdownHook); - } catch (IllegalStateException ignored) { - // VM is already shutting down. + } catch (IllegalStateException e) { + LOGGER.debug("Shutdown hook could not be removed because the VM is already shutting down", e); } } System.exit(0); diff --git a/barebones-core/src/main/java/dev/barebones/commander/Application.java b/barebones-core/src/main/java/dev/barebones/commander/Application.java index d4667d353c..e7f90db98c 100644 --- a/barebones-core/src/main/java/dev/barebones/commander/Application.java +++ b/barebones-core/src/main/java/dev/barebones/commander/Application.java @@ -542,7 +542,8 @@ private void run() { if (MuConfigurations.getPreferences() .getVariable(MuPreference.CHECK_FOR_UPDATE, MuPreferences.DEFAULT_CHECK_FOR_UPDATE)) { CompletableFuture.runAsync(() -> { - new CheckVersionDialog(WindowManager.getCurrentMainFrame(), false); + SwingUtilities.invokeLater(() -> + new CheckVersionDialog(WindowManager.getCurrentMainFrame(), false)); }, CompletableFuture.delayedExecutor(10L, TimeUnit.SECONDS)); } diff --git a/barebones-core/src/main/java/dev/barebones/commander/ui/dialog/about/AboutDialog.java b/barebones-core/src/main/java/dev/barebones/commander/ui/dialog/about/AboutDialog.java index e279143203..2893c2b686 100644 --- a/barebones-core/src/main/java/dev/barebones/commander/ui/dialog/about/AboutDialog.java +++ b/barebones-core/src/main/java/dev/barebones/commander/ui/dialog/about/AboutDialog.java @@ -19,12 +19,15 @@ import dev.barebones.commander.Activator; import dev.barebones.commander.RuntimeConstants; +import dev.barebones.commander.commons.logging.Logger; +import dev.barebones.commander.commons.logging.LoggerFactory; import dev.barebones.commander.commons.util.ui.dialog.FocusDialog; import dev.barebones.commander.commons.util.ui.layout.FluentPanel; import dev.barebones.commander.core.desktop.DesktopManager; import dev.barebones.commander.desktop.ActionType; import dev.barebones.commander.text.Translator; import dev.barebones.commander.ui.action.ActionProperties; +import dev.barebones.commander.ui.dialog.InformationDialog; import dev.barebones.commander.ui.icon.IconManager; import dev.barebones.commander.ui.main.MainFrame; import dev.barebones.commander.ui.theme.Theme; @@ -62,6 +65,8 @@ * @author Maxence Bernard, Nicolas Rinaudo */ public class AboutDialog extends FocusDialog implements ActionListener { + private static final Logger LOGGER = LoggerFactory.getLogger(AboutDialog.class); + // - Styles ----------------------------------------------------------------- // -------------------------------------------------------------------------- /** Style for normal text. */ @@ -431,8 +436,13 @@ else if (e.getSource() == homeButton) { try { DesktopManager.browse(URI.create(RuntimeConstants.HOMEPAGE_URL).toURL()); } - // Ignores errors here as there really isn't anything we can do. - catch (IOException ignored) { + catch (IOException ex) { + LOGGER.warn("Failed to open homepage URL: {}", RuntimeConstants.HOMEPAGE_URL, ex); + InformationDialog.showErrorDialog(this, + Translator.get("error"), + Translator.get("cannot_open_url", RuntimeConstants.HOMEPAGE_URL), + ex.getMessage(), + ex); } } else if (e.getSource() == licenseButton) new LicenseDialog(this).showDialog(); diff --git a/barebones-core/src/main/java/dev/barebones/commander/ui/dialog/server/ServerConnectDialog.java b/barebones-core/src/main/java/dev/barebones/commander/ui/dialog/server/ServerConnectDialog.java index bcf5aede35..b99a48b897 100644 --- a/barebones-core/src/main/java/dev/barebones/commander/ui/dialog/server/ServerConnectDialog.java +++ b/barebones-core/src/main/java/dev/barebones/commander/ui/dialog/server/ServerConnectDialog.java @@ -280,6 +280,10 @@ public void actionPerformed(ActionEvent e) { catch(IOException ex) { InformationDialog.showErrorDialog(this, Translator.get("table.folder_access_error_title"), Translator.get("folder_does_not_exist")); } + catch(IllegalArgumentException ex) { + LOGGER.warn("Invalid server connection input", ex); + InformationDialog.showErrorDialog(this, Translator.get("error"), ex.getMessage()); + } } diff --git a/barebones-core/src/main/java/dev/barebones/commander/ui/dialog/startup/CheckVersionDialog.java b/barebones-core/src/main/java/dev/barebones/commander/ui/dialog/startup/CheckVersionDialog.java index edefe59591..592ec36fa7 100644 --- a/barebones-core/src/main/java/dev/barebones/commander/ui/dialog/startup/CheckVersionDialog.java +++ b/barebones-core/src/main/java/dev/barebones/commander/ui/dialog/startup/CheckVersionDialog.java @@ -34,6 +34,8 @@ import dev.barebones.commander.commons.logging.LoggerFactory; import javax.swing.JCheckBox; +import javax.swing.SwingUtilities; +import javax.swing.SwingWorker; import java.awt.BorderLayout; import java.awt.Container; import java.awt.Dimension; @@ -42,6 +44,7 @@ import java.net.URL; import java.util.ArrayList; import java.util.List; +import java.util.concurrent.ExecutionException; /** * This class takes care of retrieving the information about the latest barebones-commander version from a remote server and @@ -49,7 +52,7 @@ * * @author Maxence Bernard */ -public class CheckVersionDialog extends QuestionDialog implements Runnable { +public class CheckVersionDialog extends QuestionDialog { private static final Logger LOGGER = LoggerFactory.getLogger(CheckVersionDialog.class); /** @@ -68,6 +71,9 @@ public class CheckVersionDialog extends QuestionDialog implements Runnable { */ private final static Dimension MINIMUM_DIALOG_DIMENSION = new Dimension(320, 0); + private record VersionCheckResult(boolean showDialog, String title, String message, URL downloadURL, boolean downloadOption) { + } + public enum CheckVersionAction implements DialogAction { OK(Translator.get("ok")), @@ -101,17 +107,42 @@ public CheckVersionDialog(MainFrame mainFrame, boolean userInitiated) { this.userInitiated = userInitiated; // Do all the hard work in a separate thread - new Thread(this, "CheckVersionDialog").start(); + new SwingWorker() { + @Override + protected VersionCheckResult doInBackground() { + return checkVersion(); + } + + @Override + protected void done() { + try { + VersionCheckResult result = get(); + SwingUtilities.invokeLater(() -> showVersionCheckResult(result)); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + dispose(); + } catch (ExecutionException e) { + LOGGER.warn("Failed to complete version check", e.getCause()); + if (userInitiated) { + VersionCheckResult result = new VersionCheckResult(true, + Translator.get("version_dialog.not_available_title"), + Translator.get("version_dialog.not_available"), + null, + false); + SwingUtilities.invokeLater(() -> showVersionCheckResult(result)); + } else { + dispose(); + } + } + } + }.execute(); } /** * Checks for updates and notifies the user of the outcome. */ - public void run() { - Container contentPane = getContentPane(); - contentPane.setLayout(new BorderLayout()); - + private VersionCheckResult checkVersion() { String message; String title; VersionChecker version; @@ -147,8 +178,7 @@ public void run() { // If the version check was not iniated by the user (i.e. was automatic), // we do not need to inform the user that he already has the latest version if (!userInitiated) { - dispose(); - return; + return new VersionCheckResult(false, null, null, null, false); } title = Translator.get("version_dialog.no_new_version_title"); @@ -160,26 +190,39 @@ public void run() { // If the version check was not initiated by the user (i.e. was automatic), // we do not need to inform the user that the check failed if (!userInitiated) { - dispose(); - return; + LOGGER.debug("Automatic version check failed", e); + return new VersionCheckResult(false, null, null, null, false); } + LOGGER.warn("User-initiated version check failed", e); title = Translator.get("version_dialog.not_available_title"); message = Translator.get("version_dialog.not_available"); } + return new VersionCheckResult(true, title, message, downloadURL, downloadOption); + } + + private void showVersionCheckResult(VersionCheckResult result) { + if (!result.showDialog()) { + dispose(); + return; + } + + Container contentPane = getContentPane(); + contentPane.setLayout(new BorderLayout()); + // Set title - setTitle(title); + setTitle(result.title()); List actions = new ArrayList<>(); actions.add(CheckVersionAction.OK); // 'Go to website' choice (if available) - if (downloadOption) { + if (result.downloadOption()) { actions.add(CheckVersionAction.GO_TO_WEBSITE); } - init(new InformationPane(message, null, Font.PLAIN, InformationPane.INFORMATION_ICON), + init(new InformationPane(result.message(), null, Font.PLAIN, InformationPane.INFORMATION_ICON), actions, 0); @@ -195,7 +238,7 @@ public void run() { if (action == CheckVersionAction.GO_TO_WEBSITE) { try { - DesktopManager.executeOperation(DesktopManager.BROWSE, new Object[]{downloadURL}); + DesktopManager.executeOperation(DesktopManager.BROWSE, new Object[]{result.downloadURL()}); } catch (Exception e) { InformationDialog.showErrorDialog(this); } diff --git a/barebones-core/src/main/java/dev/barebones/commander/ui/layout/AsyncPanel.java b/barebones-core/src/main/java/dev/barebones/commander/ui/layout/AsyncPanel.java index 0f1f4a9f07..d680c4106d 100644 --- a/barebones-core/src/main/java/dev/barebones/commander/ui/layout/AsyncPanel.java +++ b/barebones-core/src/main/java/dev/barebones/commander/ui/layout/AsyncPanel.java @@ -31,6 +31,8 @@ import javax.swing.event.AncestorEvent; import javax.swing.event.AncestorListener; +import dev.barebones.commander.commons.logging.Logger; +import dev.barebones.commander.commons.logging.LoggerFactory; import dev.barebones.commander.text.Translator; import dev.barebones.commander.ui.icon.SpinningDial; @@ -54,6 +56,7 @@ * @author Maxence Bernard */ public abstract class AsyncPanel extends JPanel { + private static final Logger LOGGER = LoggerFactory.getLogger(AsyncPanel.class); /** The component displayed while the target component is being loaded */ private JComponent waitComponent; @@ -105,14 +108,33 @@ public void ancestorMoved(AncestorEvent event) {} */ private void loadTargetComponent() { new Thread(() -> { - JComponent targetComponent = getTargetComponent(); + JComponent targetComponent; + boolean failed = false; + try { + targetComponent = getTargetComponent(); + } catch (Throwable e) { + LOGGER.error("Failed to load async panel target component", e); + targetComponent = null; + failed = true; + } + JComponent finalTargetComponent = targetComponent; + boolean loadFailed = failed; SwingUtilities.invokeLater(() -> { + if (loadFailed && !isDisplayable()) { + return; + } remove(waitComponent); setBorder(new EmptyBorder(0, 0, 0, 0)); - add(targetComponent, BorderLayout.CENTER); - updateLayout(); + if (loadFailed || finalTargetComponent == null) { + add(new JLabel(Translator.get("error")), BorderLayout.CENTER); + revalidate(); + repaint(); + } else { + add(finalTargetComponent, BorderLayout.CENTER); + updateLayout(); + } }); - }).start(); + }, "AsyncPanelLoader").start(); } /** diff --git a/barebones-core/src/main/java/dev/barebones/commander/ui/main/FolderPanel.java b/barebones-core/src/main/java/dev/barebones/commander/ui/main/FolderPanel.java index 19a66a2349..7f22c9ee84 100644 --- a/barebones-core/src/main/java/dev/barebones/commander/ui/main/FolderPanel.java +++ b/barebones-core/src/main/java/dev/barebones/commander/ui/main/FolderPanel.java @@ -141,37 +141,35 @@ it calls actionPerformed() each time an item is highlighted with the arrow (UP/D panel.add(locationPanel, BorderLayout.NORTH); - new Thread(() -> { - GridBagConstraints c = new GridBagConstraints(); - c.fill = GridBagConstraints.HORIZONTAL; - c.gridy = 0; - - // Create and add drive button - this.driveButton = new DrivePopupButton(this); - c.weightx = 0; - c.gridx = 0; - locationPanel.add(driveButton, c); - - // Create location text field and wrap it in a LocationBar that can - // alternate between the text field and a breadcrumb view (Ctrl key). - this.locationTextField = new LocationTextField(this); - LocationBar locationBar = new LocationBar(this, locationTextField); - - // Give location field all the remaining space until the PoupupsButton - c.weightx = 1; - c.gridx = 1; - // Add some space between drive button and location combo box (none by default) - c.insets = new Insets(0, 4, 0, 0); - locationPanel.add(locationBar, c); - disableCtrlFocusTraversalKeys(locationTextField); - registerCycleThruFolderPanelAction(locationTextField); - - // Allow the location field to change the current directory when a file/folder is dropped on it - FileDropTargetListener dropTargetListener = new FileDropTargetListener(this, true); - new DropTarget(locationTextField, dropTargetListener); - new DropTarget(driveButton, dropTargetListener); - locationTextField.addFocusListener(this); - }).start(); + GridBagConstraints c = new GridBagConstraints(); + c.fill = GridBagConstraints.HORIZONTAL; + c.gridy = 0; + + // Create and add drive button + this.driveButton = new DrivePopupButton(this); + c.weightx = 0; + c.gridx = 0; + locationPanel.add(driveButton, c); + + // Create location text field and wrap it in a LocationBar that can + // alternate between the text field and a breadcrumb view (Ctrl key). + this.locationTextField = new LocationTextField(this); + LocationBar locationBar = new LocationBar(this, locationTextField); + + // Give location field all the remaining space until the PoupupsButton + c.weightx = 1; + c.gridx = 1; + // Add some space between drive button and location combo box (none by default) + c.insets = new Insets(0, 4, 0, 0); + locationPanel.add(locationBar, c); + disableCtrlFocusTraversalKeys(locationTextField); + registerCycleThruFolderPanelAction(locationTextField); + + // Allow the location field to change the current directory when a file/folder is dropped on it + FileDropTargetListener dropTargetListener = new FileDropTargetListener(this, true); + new DropTarget(locationTextField, dropTargetListener); + new DropTarget(driveButton, dropTargetListener); + locationTextField.addFocusListener(this); // Initialize quick lists in background fileTablePopups = CompletableFuture.supplyAsync(() -> { diff --git a/barebones-core/src/main/java/dev/barebones/commander/ui/main/MainFrame.java b/barebones-core/src/main/java/dev/barebones/commander/ui/main/MainFrame.java index 1534c88d1c..061356bec6 100644 --- a/barebones-core/src/main/java/dev/barebones/commander/ui/main/MainFrame.java +++ b/barebones-core/src/main/java/dev/barebones/commander/ui/main/MainFrame.java @@ -32,11 +32,6 @@ import java.util.List; import java.util.Map; import java.util.WeakHashMap; -import java.util.concurrent.CompletableFuture; -import java.util.concurrent.ExecutionException; -import java.util.concurrent.ExecutorService; -import java.util.concurrent.Executors; -import java.util.concurrent.Future; import javax.swing.InputMap; import javax.swing.JComponent; @@ -44,7 +39,6 @@ import javax.swing.JLayeredPane; import javax.swing.JPanel; import javax.swing.JSplitPane; -import javax.swing.SwingUtilities; import javax.swing.WindowConstants; import javax.swing.table.TableColumnModel; @@ -157,7 +151,7 @@ private void setWindowIcon() { } } - private void init(Future leftFolderPanel, Future rightFolderPanel, ExecutorService executor) throws ExecutionException, InterruptedException { + private void init(FolderPanel leftFolderPanel, FolderPanel rightFolderPanel) { // Set the window icon setWindowIcon(); // Register jobs listeners for UI notification purposes @@ -183,8 +177,8 @@ public Insets getInsets() { contentPane.add(insetsPane, BorderLayout.CENTER); // Initializes the folder panels and file tables. - this.leftFolderPanel = leftFolderPanel.get(); - this.rightFolderPanel = rightFolderPanel.get(); + this.leftFolderPanel = leftFolderPanel; + this.rightFolderPanel = rightFolderPanel; leftTable = this.leftFolderPanel.getFileTable(); rightTable = this.rightFolderPanel.getFileTable(); activeTable = leftTable; @@ -197,26 +191,20 @@ public Insets getInsets() { // preferences. // Note: Toolbar.setVisible() has to be called no matter if Toolbar is visible or not, in order for it to be // properly initialized - executor.execute(() -> { - this.toolbar = new ToolBar(this); - this.toolbarPanel = ToolbarMoreButton.wrapToolBar(toolbar); - this.toolbarPanel.setVisible(MuConfigurations.getPreferences().getVariable(MuPreference.TOOLBAR_VISIBLE, MuPreferences.DEFAULT_TOOLBAR_VISIBLE)); - contentPane.add(toolbarPanel, BorderLayout.NORTH); - }); - - executor.execute(() -> { - // Create menu bar (has to be created after toolbar) - ok, but why? - // PSko - I guess it is related to loading Actions and that icons - // for toolbar action should have icons, but in menu they should not. - // However, still I don't get how setting the icon to null here in MenuToolkit#addMenuItem - // impacts menu icons....... if nullify is commented-out there, then icons all of sudden - // show in the menu causing this: https://github.com/mucommander/mucommander/issues/1178 - MainMenuBar menuBar = new MainMenuBar(this); - SwingUtilities.invokeLater(() -> { - getJFrame().setJMenuBar(menuBar); - getJFrame().revalidate(); - }); - }); + this.toolbar = new ToolBar(this); + this.toolbarPanel = ToolbarMoreButton.wrapToolBar(toolbar); + this.toolbarPanel.setVisible(MuConfigurations.getPreferences().getVariable(MuPreference.TOOLBAR_VISIBLE, MuPreferences.DEFAULT_TOOLBAR_VISIBLE)); + contentPane.add(toolbarPanel, BorderLayout.NORTH); + + // Create menu bar (has to be created after toolbar) - ok, but why? + // PSko - I guess it is related to loading Actions and that icons + // for toolbar action should have icons, but in menu they should not. + // However, still I don't get how setting the icon to null here in MenuToolkit#addMenuItem + // impacts menu icons....... if nullify is commented-out there, then icons all of sudden + // show in the menu causing this: https://github.com/mucommander/mucommander/issues/1178 + MainMenuBar menuBar = new MainMenuBar(this); + getJFrame().setJMenuBar(menuBar); + getJFrame().revalidate(); // Create the split pane that separates folder panels and allows to resize how much space is allocated to the // both of them. The split orientation is loaded from and saved to the preferences. @@ -274,18 +262,16 @@ public Insets getInsets() { YBoxPanel southPanel = new YBoxPanel(); southPanel.addSpace(2); - executor.execute(() -> { - // Add status bar - this.statusBar = new StatusBar(this); - southPanel.add(statusBar); - - // Show command bar only if it hasn't been disabled in the preferences - this.commandBar = new CommandBar(this); - // Note: CommandBar.setVisible() has to be called no matter if CommandBar is visible or not, in order for it to be properly initialized - this.commandBar.setVisible(MuConfigurations.getPreferences().getVariable(MuPreference.COMMAND_BAR_VISIBLE, MuPreferences.DEFAULT_COMMAND_BAR_VISIBLE)); - southPanel.add(commandBar); - insetsPane.add(southPanel, BorderLayout.SOUTH); - }); + // Add status bar + this.statusBar = new StatusBar(this); + southPanel.add(statusBar); + + // Show command bar only if it hasn't been disabled in the preferences + this.commandBar = new CommandBar(this); + // Note: CommandBar.setVisible() has to be called no matter if CommandBar is visible or not, in order for it to be properly initialized + this.commandBar.setVisible(MuConfigurations.getPreferences().getVariable(MuPreference.COMMAND_BAR_VISIBLE, MuPreferences.DEFAULT_COMMAND_BAR_VISIBLE)); + southPanel.add(commandBar); + insetsPane.add(southPanel, BorderLayout.SOUTH); // Perform CloseAction when the user asked the window to close getJFrame().setDefaultCloseOperation(WindowConstants.DO_NOTHING_ON_CLOSE); @@ -317,16 +303,9 @@ public MainFrame(ConfFileTableTab[] leftTabs, int indexOfLeftSelectedTab, FileTa ConfFileTableTab[] rightTabs, int indexOfRightSelectedTab, FileTableConfiguration rightTableConf) { super(); // left to easily debug the performance frameInstance = PreloadedJFrame.getJFrame(this); - ExecutorService executor = Executors.newFixedThreadPool(4); - try { - var leftFolderPanel = executor.submit(() -> new FolderPanel(this, leftTabs, indexOfLeftSelectedTab, leftTableConf)); - var rightFolderPanel = executor.submit(() -> new FolderPanel(this, rightTabs, indexOfRightSelectedTab, rightTableConf)); - init(leftFolderPanel, rightFolderPanel, executor); - } catch (ExecutionException | InterruptedException e) { - throw new RuntimeException(e); - } finally { - executor.shutdown(); - } + FolderPanel leftFolderPanel = new FolderPanel(this, leftTabs, indexOfLeftSelectedTab, leftTableConf); + FolderPanel rightFolderPanel = new FolderPanel(this, rightTabs, indexOfRightSelectedTab, rightTableConf); + init(leftFolderPanel, rightFolderPanel); for (boolean isLeft = true; ; isLeft=false) { FileTable fileTable = isLeft ? leftTable : rightTable; @@ -354,26 +333,17 @@ public MainFrame(MainFrame mainFrame) { FileTable leftFileTable = leftFolderPanel.getFileTable(); FileTable rightFileTable = rightFolderPanel.getFileTable(); - ExecutorService executor = Executors.newFixedThreadPool(4); - try { - init(CompletableFuture.completedFuture( // non-async - new FolderPanel(this, new ConfFileTableTab[] { - new ConfFileTableTab(leftFolderPanel.getCurrentFolder().getURL())}, - 0, leftFileTable.getConfiguration())), - CompletableFuture.completedFuture( - new FolderPanel(this, new ConfFileTableTab[] { - new ConfFileTableTab(rightFolderPanel.getCurrentFolder().getURL())}, - 0, rightFileTable.getConfiguration())), - executor); - - // TODO: Sorting should be part of the FileTable configuration - this.leftTable.sortBy(leftFileTable.getSortInfo()); - this.rightTable.sortBy(rightFileTable.getSortInfo()); - } catch (ExecutionException | InterruptedException e) { - throw new RuntimeException(e); - } finally { - executor.shutdown(); - } + init( + new FolderPanel(this, new ConfFileTableTab[] { + new ConfFileTableTab(leftFolderPanel.getCurrentFolder().getURL())}, + 0, leftFileTable.getConfiguration()), + new FolderPanel(this, new ConfFileTableTab[] { + new ConfFileTableTab(rightFolderPanel.getCurrentFolder().getURL())}, + 0, rightFileTable.getConfiguration())); + + // TODO: Sorting should be part of the FileTable configuration + this.leftTable.sortBy(leftFileTable.getSortInfo()); + this.rightTable.sortBy(rightFileTable.getSortInfo()); } public JFrame getJFrame() { diff --git a/barebones-core/src/main/java/dev/barebones/commander/ui/main/StatusBar.java b/barebones-core/src/main/java/dev/barebones/commander/ui/main/StatusBar.java index fa803a8a98..6acbce415b 100644 --- a/barebones-core/src/main/java/dev/barebones/commander/ui/main/StatusBar.java +++ b/barebones-core/src/main/java/dev/barebones/commander/ui/main/StatusBar.java @@ -38,6 +38,7 @@ import javax.swing.JPanel; import javax.swing.JPopupMenu; import javax.swing.SwingConstants; +import javax.swing.SwingUtilities; import dev.barebones.commander.commons.logging.Logger; import dev.barebones.commander.commons.logging.LoggerFactory; @@ -467,7 +468,11 @@ private synchronized void startAutoUpdate() { long volumeFree = getFreeSpace(currentFolder); long volumeTotal = getTotalSpace(currentFolder); - volumeSpaceLabel.setVolumeSpace(volumeTotal, volumeFree); + SwingUtilities.invokeLater(() -> { + if (!mainFrameDisposed && isVisible()) { + volumeSpaceLabel.setVolumeSpace(volumeTotal, volumeFree); + } + }); } // Sleep for a while diff --git a/barebones-core/src/main/java/dev/barebones/commander/ui/main/WindowManager.java b/barebones-core/src/main/java/dev/barebones/commander/ui/main/WindowManager.java index 9d779d2b0a..ea6bb5a9c8 100644 --- a/barebones-core/src/main/java/dev/barebones/commander/ui/main/WindowManager.java +++ b/barebones-core/src/main/java/dev/barebones/commander/ui/main/WindowManager.java @@ -40,6 +40,7 @@ import java.awt.Frame; import java.awt.event.WindowEvent; import java.awt.event.WindowListener; +import java.lang.reflect.InvocationTargetException; import java.util.Collection; import java.util.List; import java.util.Timer; @@ -202,7 +203,22 @@ public static void tryRefreshCurrentFolders() { * @param mainFrameBuilder the mainFrame builder * @return the newly created MainFrame. */ - public static synchronized void createNewMainFrame(MainFrameBuilder mainFrameBuilder) { + public static void createNewMainFrame(MainFrameBuilder mainFrameBuilder) { + if (!SwingUtilities.isEventDispatchThread()) { + try { + SwingUtilities.invokeAndWait(() -> createNewMainFrameOnEdt(mainFrameBuilder)); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new IllegalStateException("Interrupted while creating main frame", e); + } catch (InvocationTargetException e) { + throw new IllegalStateException("Failed to create main frame", e.getCause()); + } + return; + } + createNewMainFrameOnEdt(mainFrameBuilder); + } + + private static synchronized void createNewMainFrameOnEdt(MainFrameBuilder mainFrameBuilder) { LOGGER.debug("creating a new main frame..."); Collection newMainFrames = mainFrameBuilder.build(); diff --git a/barebones-core/src/main/java/dev/barebones/commander/ui/main/menu/OpenWithMenu.java b/barebones-core/src/main/java/dev/barebones/commander/ui/main/menu/OpenWithMenu.java index 2a8f26fb76..4fdb640e9e 100644 --- a/barebones-core/src/main/java/dev/barebones/commander/ui/main/menu/OpenWithMenu.java +++ b/barebones-core/src/main/java/dev/barebones/commander/ui/main/menu/OpenWithMenu.java @@ -21,11 +21,14 @@ import javax.swing.JMenu; import javax.swing.JMenuItem; import javax.swing.JSeparator; +import javax.swing.SwingWorker; import dev.barebones.commander.command.Command; import dev.barebones.commander.command.CommandManager; import dev.barebones.commander.command.CommandType; import dev.barebones.commander.commons.file.AbstractFile; +import dev.barebones.commander.commons.logging.Logger; +import dev.barebones.commander.commons.logging.LoggerFactory; import dev.barebones.commander.commons.util.ui.helper.MenuToolkit; import dev.barebones.commander.core.desktop.DesktopManager; import dev.barebones.commander.process.ProcessRunner; @@ -39,6 +42,8 @@ import java.io.IOException; import java.util.Collections; +import java.util.List; +import java.util.concurrent.ExecutionException; /** @@ -53,6 +58,8 @@ * @author Nicolas Rinaudo */ public class OpenWithMenu extends JMenu { + private static final Logger LOGGER = LoggerFactory.getLogger(OpenWithMenu.class); + private final MainFrame mainFrame; private AbstractFile selectedFile; @@ -139,31 +146,31 @@ private void populateNativeApplications() { loadingItem.setIcon(spinningIcon); // need to set both disabled and normal, otherwise it doesn't appear loadingItem.setEnabled(false); spinningIcon.setAnimated(true); - // going to run getCommandsForOpenWith in background as it may take some time to complete - // especially if a given file has a lot of apps that can be opened with... - new Thread(() -> { - - var commands = DesktopManager.getAppsForOpenWith(selectedFile); - if (!commands.isEmpty() && getItemCount() > 1) { - add(new JSeparator()); + AbstractFile requestedFile = selectedFile; + var requestedFileURL = requestedFile.getURL(); + new SwingWorker, Void>() { + @Override + protected List doInBackground() { + return DesktopManager.getAppsForOpenWith(requestedFile); } - var separateDefault = commands.size() > 1; - for (Command cmd : commands) { - MuAction action = createMuAction(cmd); - action.setLabel(cmd.getDisplayName()); - add(action).setIcon(cmd.getIcon()); - if (separateDefault) { - add(new JSeparator()); - separateDefault = false; + + @Override + protected void done() { + try { + List commands = get(); + if (selectedFile != null && requestedFileURL.equals(selectedFile.getURL())) { + populateNativeApplications(commands); + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + LOGGER.warn("Interrupted while loading native Open With applications for {}", requestedFile, e); + } catch (ExecutionException e) { + LOGGER.warn("Failed to load native Open With applications for {}", requestedFile, e.getCause()); + } finally { + removeLoadingItem(loadingItem, spinningIcon); } } - spinningIcon.setAnimated(false); - super.remove(loadingItem); - if (getItemCount() == 0) { - setEnabled(false); - } - super.getPopupMenu().pack(); - }, "OpenWithAppThread").start(); + }.execute(); } else { if (DesktopManager.canEnableOpenWithApps()) { if (getItemCount() > 0) { @@ -178,4 +185,26 @@ private void populateNativeApplications() { } } + private void populateNativeApplications(List commands) { + if (!commands.isEmpty() && getItemCount() > 1) { + add(new JSeparator()); + } + var separateDefault = commands.size() > 1; + for (Command cmd : commands) { + MuAction action = createMuAction(cmd); + action.setLabel(cmd.getDisplayName()); + add(action).setIcon(cmd.getIcon()); + if (separateDefault) { + add(new JSeparator()); + separateDefault = false; + } + } + } + + private void removeLoadingItem(JMenuItem loadingItem, SpinningDial spinningIcon) { + spinningIcon.setAnimated(false); + super.remove(loadingItem); + setEnabled(getItemCount() > 0); + super.getPopupMenu().pack(); + } } diff --git a/barebones-core/src/main/java/dev/barebones/commander/ui/main/table/FileTable.java b/barebones-core/src/main/java/dev/barebones/commander/ui/main/table/FileTable.java index 1aeb651102..87879fc1de 100644 --- a/barebones-core/src/main/java/dev/barebones/commander/ui/main/table/FileTable.java +++ b/barebones-core/src/main/java/dev/barebones/commander/ui/main/table/FileTable.java @@ -44,6 +44,7 @@ import javax.swing.ListSelectionModel; import javax.swing.SwingConstants; import javax.swing.SwingUtilities; +import javax.swing.Timer; import javax.swing.UIManager; import javax.swing.table.JTableHeader; import javax.swing.table.TableCellRenderer; @@ -1308,35 +1309,27 @@ public void mouseClicked(MouseEvent e) { // Not checking for this would cause a single click on the inactive table's current row to trigger // the filename/date/permission editor if (hasFocus() && System.currentTimeMillis() - focusGainedTime > 100) { - // Create a new thread and sleep long enough to ensure that this click was not the first of a double click - new Thread() { - @Override - public void run() { - try { sleep(800); } - catch (InterruptedException e) { - Thread.currentThread().interrupt(); - return; + Timer editDelayTimer = new Timer(800, event -> { + // Do not execute this block (cancel editing) if: + // - a double click was made in the last second + // - current row changed + // - isEditing() is true which could happen if multiple clicks were made + if ((System.currentTimeMillis() - lastDoubleClickTimestamp) > 1000 && row == currentRow) { + if (column == Column.NAME) { + if(!isEditing()) + editCurrentFilename(); } - - // Do not execute this block (cancel editing) if: - // - a double click was made in the last second - // - current row changed - // - isEditing() is true which could happen if multiple clicks were made - if ((System.currentTimeMillis() - lastDoubleClickTimestamp) > 1000 && row == currentRow) { - if (column == Column.NAME) { - if(!isEditing()) - editCurrentFilename(); - } - else if(column == Column.DATE) { - ActionManager.performAction(ActionType.ChangeDate, mainFrame); - } - else if(column == Column.PERMISSIONS) { - if(getSelectedFile().getChangeablePermissions().getIntValue()!=0) - ActionManager.performAction(ActionType.ChangePermissions, mainFrame); - } + else if(column == Column.DATE) { + ActionManager.performAction(ActionType.ChangeDate, mainFrame); + } + else if(column == Column.PERMISSIONS) { + if(getSelectedFile().getChangeablePermissions().getIntValue()!=0) + ActionManager.performAction(ActionType.ChangePermissions, mainFrame); } } - }.start(); + }); + editDelayTimer.setRepeats(false); + editDelayTimer.start(); } } } diff --git a/barebones-core/src/main/java/dev/barebones/commander/ui/notifier/NotificationPopup.java b/barebones-core/src/main/java/dev/barebones/commander/ui/notifier/NotificationPopup.java index 38e6e81019..3a2f73998a 100644 --- a/barebones-core/src/main/java/dev/barebones/commander/ui/notifier/NotificationPopup.java +++ b/barebones-core/src/main/java/dev/barebones/commander/ui/notifier/NotificationPopup.java @@ -25,6 +25,7 @@ import javax.swing.JPanel; import javax.swing.JPopupMenu; import javax.swing.SwingUtilities; +import javax.swing.Timer; import javax.swing.event.PopupMenuEvent; import javax.swing.event.PopupMenuListener; import java.awt.BorderLayout; @@ -34,8 +35,7 @@ import java.awt.Point; import java.awt.event.MouseAdapter; import java.awt.event.MouseEvent; -import java.util.Timer; -import java.util.TimerTask; +import java.lang.reflect.InvocationTargetException; /** * A singleton class that shows notification popup in the provided frame (for example main frame). @@ -54,8 +54,7 @@ final class NotificationPopup { */ private static final float OPACITY = 0.8f; - private final Timer closingTimer; - private TimerTask closingTask; + private Timer closingTimer; private final CustomPopupMenu popup; private final JPanel panel; @@ -119,7 +118,6 @@ private void hidePopup() { } private NotificationPopup() { - closingTimer = new Timer(); popupListener = new CustomPopupMenuListener(); popup = new CustomPopupMenu(); @@ -145,12 +143,34 @@ public Insets getInsets() { popup.add(panel, BorderLayout.CENTER); } - private static class NotificationPopupHolder { - private static final NotificationPopup INSTANCE = new NotificationPopup(); - } + private static NotificationPopup instance; public static NotificationPopup getInstance() { - return NotificationPopupHolder.INSTANCE; + synchronized (NotificationPopup.class) { + if (instance != null) { + return instance; + } + } + if (SwingUtilities.isEventDispatchThread()) { + return createInstance(); + } + final NotificationPopup[] result = new NotificationPopup[1]; + try { + SwingUtilities.invokeAndWait(() -> result[0] = createInstance()); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new IllegalStateException("Interrupted while creating notification popup", e); + } catch (InvocationTargetException e) { + throw new IllegalStateException("Failed to create notification popup", e.getCause()); + } + return result[0]; + } + + private static synchronized NotificationPopup createInstance() { + if (instance == null) { + instance = new NotificationPopup(); + } + return instance; } /** @@ -165,6 +185,11 @@ public static NotificationPopup getInstance() { */ public void displayNotification(JFrame mainFrame, String notificationText, Color bgColor, Color fgColor, long timeout) { + if (!SwingUtilities.isEventDispatchThread()) { + SwingUtilities.invokeLater(() -> + displayNotification(mainFrame, notificationText, bgColor, fgColor, timeout)); + return; + } if (notificationText == null || notificationText.isBlank()) { return; // noop } @@ -188,19 +213,16 @@ private void setOpacity(Component comp, float opacity) { } private void scheduleClosing(long timeout) { - TimerTask task = new TimerTask() { - @Override - public void run() { - closingTask = null; - popup.hidePopup(); - } - }; - TimerTask oldTask = closingTask; - closingTask = task; - if (oldTask != null) { - oldTask.cancel(); - }; - closingTimer.schedule(task, timeout); + if (closingTimer != null) { + closingTimer.stop(); + } + int delay = (int) Math.min(timeout, Integer.MAX_VALUE); + closingTimer = new Timer(delay, event -> { + closingTimer = null; + popup.hidePopup(); + }); + closingTimer.setRepeats(false); + closingTimer.start(); } private Point getPosition(JFrame mainFrame, int width) { diff --git a/barebones-core/src/main/java/dev/barebones/commander/ui/quicklist/QuickListWithIcons.java b/barebones-core/src/main/java/dev/barebones/commander/ui/quicklist/QuickListWithIcons.java index 590f1a735b..cc401e764b 100644 --- a/barebones-core/src/main/java/dev/barebones/commander/ui/quicklist/QuickListWithIcons.java +++ b/barebones-core/src/main/java/dev/barebones/commander/ui/quicklist/QuickListWithIcons.java @@ -17,19 +17,24 @@ package dev.barebones.commander.ui.quicklist; -import java.awt.Dimension; -import java.awt.Image; -import java.util.HashMap; - -import javax.swing.Icon; -import javax.swing.ImageIcon; -import javax.swing.event.PopupMenuEvent; -import javax.swing.event.PopupMenuListener; - -import dev.barebones.commander.commons.file.AbstractFile; -import dev.barebones.commander.ui.icon.CustomFileIconProvider; -import dev.barebones.commander.ui.icon.FileIcons; -import dev.barebones.commander.ui.icon.IconManager; +import java.awt.Dimension; +import java.awt.Image; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ExecutionException; + +import javax.swing.Icon; +import javax.swing.ImageIcon; +import javax.swing.SwingWorker; +import javax.swing.SwingUtilities; +import javax.swing.event.PopupMenuEvent; +import javax.swing.event.PopupMenuListener; + +import dev.barebones.commander.commons.file.AbstractFile; +import dev.barebones.commander.commons.logging.Logger; +import dev.barebones.commander.commons.logging.LoggerFactory; +import dev.barebones.commander.ui.icon.CustomFileIconProvider; +import dev.barebones.commander.ui.icon.FileIcons; +import dev.barebones.commander.ui.icon.IconManager; import dev.barebones.commander.ui.icon.SpinningDial; import dev.barebones.commander.ui.quicklist.item.QuickListDataList; import dev.barebones.commander.ui.quicklist.item.QuickListDataListWithIcons; @@ -40,12 +45,13 @@ * * @author Arik Hadas */ - -public abstract class QuickListWithIcons extends QuickListWithDataList { - // This HashMap's keys are items and its objects are the corresponding icon. - private final HashMap itemToIconCacheMap = new HashMap(); - // This SpinningDial will appear until the icon fetching of an item is over. - private static final SpinningDial waitingIcon = new SpinningDial(); + +public abstract class QuickListWithIcons extends QuickListWithDataList { + private static final Logger LOGGER = LoggerFactory.getLogger(QuickListWithIcons.class); + // This map's keys are items and its objects are the corresponding icon. + private final ConcurrentHashMap itemToIconCacheMap = new ConcurrentHashMap(); + // This SpinningDial will appear until the icon fetching of an item is over. + private final SpinningDial waitingIcon = new SpinningDial(); // If the icon fetching fails for some item, the following icon will appear for it. private static final Icon notAvailableIcon = IconManager.getIcon(IconManager.FILE_ICON_SET, CustomFileIconProvider.NOT_ACCESSIBLE_FILE); // Saves the number of waiting-icons (SpinningDials) appearing in the list. @@ -70,20 +76,27 @@ public void popupMenuWillBecomeVisible(PopupMenuEvent e) { /** * Called when waitingIcon is added to the list. */ - private synchronized void waitingIconAddedToList() { - // If there was no other waitingIcon in the list before current addition - start the spinning dial. - if (numOfWaitingIconInList++ == 0) - waitingIcon.setAnimated(true); - } + private synchronized void waitingIconAddedToList() { + // If there was no other waitingIcon in the list before current addition - start the spinning dial. + if (numOfWaitingIconInList++ == 0) + setWaitingIconAnimated(true); + } /** * Called when waitingIcon is removed from the list. */ - private synchronized void waitingIconRemovedFromList() { - // If after current remove operation, there will be no waitingIcon in the list - stop the spinning dial. - if (--numOfWaitingIconInList == 0) - waitingIcon.setAnimated(false); - } + private synchronized void waitingIconRemovedFromList() { + // If after current remove operation, there will be no waitingIcon in the list - stop the spinning dial. + if (--numOfWaitingIconInList == 0) + setWaitingIconAnimated(false); + } + + private void setWaitingIconAnimated(boolean animated) { + if (SwingUtilities.isEventDispatchThread()) + waitingIcon.setAnimated(animated); + else + SwingUtilities.invokeLater(() -> waitingIcon.setAnimated(animated)); + } @Override protected QuickListDataList getList() { @@ -114,29 +127,43 @@ protected Icon getIconOfFile(AbstractFile file) { IconManager.getImageIcon(FileIcons.getFileIcon(file)) : null; } - protected Icon getImageIconOfItemImp(final T item, final Dimension preferredSize) { - synchronized(itemToIconCacheMap) { - if (itemToIconCacheMap.putIfAbsent(item, waitingIcon) == null) { - waitingIconAddedToList(); - } - } - - Icon icon = itemToIconCacheMap.get(item); - - if (icon == waitingIcon) - new Thread() { - @Override - public void run() { - Icon icon = itemToIcon(item); - // If the item does not exist or is not accessible, show notAvailableIcon for it. - itemToIconCacheMap.put(item, icon != null ? icon : notAvailableIcon); - waitingIconRemovedFromList(); - repaint(); - } - }.start(); - - return resizeIcon(icon, preferredSize); - } + protected Icon getImageIconOfItemImp(final T item, final Dimension preferredSize) { + boolean loadIcon = itemToIconCacheMap.putIfAbsent(item, waitingIcon) == null; + if (loadIcon) { + waitingIconAddedToList(); + } + + Icon icon = itemToIconCacheMap.get(item); + + if (loadIcon) + new SwingWorker() { + @Override + protected Icon doInBackground() { + return itemToIcon(item); + } + + @Override + protected void done() { + Icon icon = notAvailableIcon; + try { + Icon loadedIcon = get(); + if (loadedIcon != null) { + icon = loadedIcon; + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } catch (ExecutionException e) { + LOGGER.warn("Failed to load quick-list icon for {}", item, e.getCause()); + } finally { + itemToIconCacheMap.put(item, icon); + waitingIconRemovedFromList(); + repaint(); + } + } + }.execute(); + + return resizeIcon(icon, preferredSize); + } protected Icon resizeIcon(Icon icon, final Dimension preferredSize) { if (icon instanceof ImageIcon) { diff --git a/barebones-core/src/main/java/dev/barebones/commander/ui/viewer/FileFrame.java b/barebones-core/src/main/java/dev/barebones/commander/ui/viewer/FileFrame.java index ad22ec930a..6f4202500e 100644 --- a/barebones-core/src/main/java/dev/barebones/commander/ui/viewer/FileFrame.java +++ b/barebones-core/src/main/java/dev/barebones/commander/ui/viewer/FileFrame.java @@ -24,6 +24,7 @@ import javax.swing.JComponent; import javax.swing.JFrame; import javax.swing.JPanel; +import javax.swing.SwingUtilities; import javax.swing.WindowConstants; import dev.barebones.commander.commons.logging.Logger; @@ -93,18 +94,19 @@ public JComponent getTargetComponent() { filePresenter.open(file, fromSearchWithContent); } catch(Exception e) { LOGGER.error("Exception caught", e); - showGenericErrorDialog(); - dispose(); - return filePresenter == null ? new JPanel() : filePresenter; + SwingUtilities.invokeLater(() -> { + showGenericErrorDialog(); + dispose(); + }); + throw new IllegalStateException("Failed to open file presenter", e); } - setJMenuBar(filePresenter.getMenuBar()); - return filePresenter; } @Override protected void updateLayout() { + setJMenuBar(filePresenter.getMenuBar()); // Request focus on the viewer when it is visible FocusRequester.requestFocus(filePresenter); } diff --git a/barebones-os-api/src/main/java/dev/barebones/commander/desktop/QueuedTrash.java b/barebones-os-api/src/main/java/dev/barebones/commander/desktop/QueuedTrash.java index c5df0fd5b4..af4b22b927 100644 --- a/barebones-os-api/src/main/java/dev/barebones/commander/desktop/QueuedTrash.java +++ b/barebones-os-api/src/main/java/dev/barebones/commander/desktop/QueuedTrash.java @@ -108,6 +108,8 @@ public void waitForPendingOperations() { moveToTrashLock.wait(); } catch(InterruptedException e) { + Thread.currentThread().interrupt(); + return; } } } @@ -143,7 +145,10 @@ public void run() { try { Thread.sleep(QUEUE_PERIOD); } - catch(InterruptedException e) {} + catch(InterruptedException e) { + Thread.currentThread().interrupt(); + break; + } } while(queueSize!=queuedFiles.size()); diff --git a/barebones-protocol-nfs/src/main/java/dev/barebones/commander/commons/file/protocol/nfs/NFSPanel.java b/barebones-protocol-nfs/src/main/java/dev/barebones/commander/commons/file/protocol/nfs/NFSPanel.java index cdfef4eef9..300bd4539a 100644 --- a/barebones-protocol-nfs/src/main/java/dev/barebones/commander/commons/file/protocol/nfs/NFSPanel.java +++ b/barebones-protocol-nfs/src/main/java/dev/barebones/commander/commons/file/protocol/nfs/NFSPanel.java @@ -129,8 +129,12 @@ public boolean usesCredentials() { public void dialogValidated() { // Commits the current spinner value in case it was being edited and 'enter' was pressed // (the spinner value would otherwise not be committed) - try { portSpinner.commitEdit(); } - catch(ParseException e) { } + try { + portSpinner.commitEdit(); + } catch(ParseException e) { + JSpinner.DefaultEditor editor = (JSpinner.DefaultEditor) portSpinner.getEditor(); + throw new IllegalArgumentException("Invalid port value: " + editor.getTextField().getText(), e); + } updateValues(); } diff --git a/barebones-protocol-s3/src/main/java/dev/barebones/commander/commons/file/protocol/s3/S3Bucket.java b/barebones-protocol-s3/src/main/java/dev/barebones/commander/commons/file/protocol/s3/S3Bucket.java index 0adad350ed..31d076a09d 100644 --- a/barebones-protocol-s3/src/main/java/dev/barebones/commander/commons/file/protocol/s3/S3Bucket.java +++ b/barebones-protocol-s3/src/main/java/dev/barebones/commander/commons/file/protocol/s3/S3Bucket.java @@ -48,7 +48,7 @@ public boolean exists() { connection.client().headBucket( HeadBucketRequest.builder().bucket(parsed.bucket()).build()); return true; - } catch (NoSuchBucketException ignored) { + } catch (NoSuchBucketException missing) { return false; } catch (S3Exception e) { // Permission errors come back as 403 — the bucket may diff --git a/barebones-protocol-s3/src/main/java/dev/barebones/commander/commons/file/protocol/s3/S3Object.java b/barebones-protocol-s3/src/main/java/dev/barebones/commander/commons/file/protocol/s3/S3Object.java index 44e43f8889..d5229481ca 100644 --- a/barebones-protocol-s3/src/main/java/dev/barebones/commander/commons/file/protocol/s3/S3Object.java +++ b/barebones-protocol-s3/src/main/java/dev/barebones/commander/commons/file/protocol/s3/S3Object.java @@ -61,7 +61,6 @@ public class S3Object extends S3File { private boolean directory; private long size; private long lastModified; - public S3Object(FileURL url, S3Connection connection) { super(url, connection); // If the URL ends with '/', the object is a directory by construction. @@ -75,14 +74,18 @@ public S3Object(FileURL url, S3Connection connection) { * Stash the metadata from a parent listing so we can answer * isDirectory / getSize / getDate without re-HEADing the object. */ - void setListingMetadata(long size, long lastModified, boolean directory) { + synchronized void setListingMetadata(long size, long lastModified, boolean directory) { + setMetadata(size, lastModified, directory, true); + } + + private synchronized void setMetadata(long size, long lastModified, boolean directory, boolean metadataKnown) { this.size = size; this.lastModified = lastModified; this.directory = directory; - this.metadataKnown = true; + this.metadataKnown = metadataKnown; } - private void ensureMetadata() throws IOException { + private synchronized void ensureMetadata() throws IOException { if (metadataKnown) return; try { HeadObjectResponse h = connection.client().headObject( @@ -90,34 +93,42 @@ private void ensureMetadata() throws IOException { .bucket(parsed.bucket()) .key(parsed.key()) .build()); - this.size = h.contentLength() != null ? h.contentLength() : 0L; - this.lastModified = h.lastModified() != null - ? h.lastModified().toEpochMilli() : 0L; - this.directory = false; - this.metadataKnown = true; - } catch (NoSuchKeyException ignored) { - this.metadataKnown = true; // exists() answers via this state + setMetadata( + h.contentLength() != null ? h.contentLength() : 0L, + h.lastModified() != null ? h.lastModified().toEpochMilli() : 0L, + false, + true); + } catch (NoSuchKeyException missing) { + setMetadata(0L, 0L, false, true); // exists() answers via this state } catch (S3Exception e) { throw toIOException(e, fileURL); } } + private void logMetadataFailure(String operation, IOException failure) { + LOGGER.warn("S3 metadata lookup failed during {} for {}", operation, getURL(), failure); + } + @Override - public boolean isDirectory() { + public synchronized boolean isDirectory() { try { ensureMetadata(); - } catch (IOException ignored) { + } catch (IOException e) { + logMetadataFailure("isDirectory", e); return false; } return directory; } @Override - public boolean exists() { + public synchronized boolean exists() { try { ensureMetadata(); - } catch (IOException ignored) { - return false; + } catch (IOException e) { + logMetadataFailure("exists", e); + // NoSuchKey is handled inside ensureMetadata as a known absence. + // Other failures mean the state is unknown, not that the object is gone. + return metadataKnown ? directory || size > 0 || lastModified > 0 : true; } // metadataKnown == true after a HEAD; if directory or non-zero // size or non-zero lastModified, we got a real response. @@ -125,14 +136,22 @@ public boolean exists() { } @Override - public long getDate() { - try { ensureMetadata(); } catch (IOException ignored) {} + public synchronized long getDate() { + try { + ensureMetadata(); + } catch (IOException e) { + logMetadataFailure("getDate", e); + } return lastModified; } @Override - public long getSize() { - try { ensureMetadata(); } catch (IOException ignored) {} + public synchronized long getSize() { + try { + ensureMetadata(); + } catch (IOException e) { + logMetadataFailure("getSize", e); + } return size; } @@ -149,7 +168,7 @@ public AbstractFile[] ls() throws IOException { } @Override - public void mkdir() throws IOException { + public synchronized void mkdir() throws IOException { // S3 has no real directories; create an empty object whose // key ends with '/'. That's what the AWS Console does and // it's what subsequent ListObjectsV2 with delimiter='/' picks @@ -163,8 +182,7 @@ public void mkdir() throws IOException { .key(key) .build(), RequestBody.empty()); - this.directory = true; - this.metadataKnown = true; + setMetadata(0L, System.currentTimeMillis(), true, true); } catch (S3Exception e) { throw toIOException(e, fileURL); } @@ -199,13 +217,14 @@ public OutputStream getOutputStream() throws IOException { } @Override - public void delete() throws IOException { + public synchronized void delete() throws IOException { try { connection.client().deleteObject( DeleteObjectRequest.builder() .bucket(parsed.bucket()) .key(parsed.key()) .build()); + setMetadata(0L, 0L, false, true); } catch (S3Exception e) { throw toIOException(e, fileURL); } @@ -292,17 +311,13 @@ public void close() throws IOException { uploadSpilledFile(); } // Whichever path: refresh local metadata. - size = bytesWritten; - lastModified = System.currentTimeMillis(); - directory = false; - metadataKnown = true; + setMetadata(bytesWritten, System.currentTimeMillis(), false, true); } finally { if (spillFile != null) { try { Files.deleteIfExists(spillFile); - } catch (IOException ignored) { - // Temp dir cleanup is best-effort; the OS - // sweeps it eventually. + } catch (IOException e) { + LOGGER.warn("Failed to delete S3 upload spill file {}", spillFile, e); } } } diff --git a/barebones-protocol-s3/src/main/java/dev/barebones/commander/commons/file/protocol/s3/S3ProtocolProvider.java b/barebones-protocol-s3/src/main/java/dev/barebones/commander/commons/file/protocol/s3/S3ProtocolProvider.java index 4eecefeaf6..53da9d9b41 100644 --- a/barebones-protocol-s3/src/main/java/dev/barebones/commander/commons/file/protocol/s3/S3ProtocolProvider.java +++ b/barebones-protocol-s3/src/main/java/dev/barebones/commander/commons/file/protocol/s3/S3ProtocolProvider.java @@ -13,6 +13,8 @@ import dev.barebones.commander.commons.file.Credentials; import dev.barebones.commander.commons.file.FileURL; import dev.barebones.commander.commons.file.protocol.ProtocolProvider; +import dev.barebones.commander.commons.logging.Logger; +import dev.barebones.commander.commons.logging.LoggerFactory; import java.io.IOException; import java.util.Map; @@ -34,6 +36,7 @@ * client (which would silently authorise as the wrong identity). */ public class S3ProtocolProvider implements ProtocolProvider, AutoCloseable { + private static final Logger LOGGER = LoggerFactory.getLogger(S3ProtocolProvider.class); /** Properties accepted on the FileURL — these become S3Configuration options. */ public static final String PROPERTY_REGION = "region"; @@ -52,8 +55,8 @@ public void close() { for (S3Connection conn : connections.values()) { try { conn.close(); - } catch (RuntimeException ignored) { - // shutdown — log channels may already be down. + } catch (RuntimeException e) { + LOGGER.warn("Failed to close cached S3 connection", e); } } connections.clear(); diff --git a/barebones-protocol-s3/src/main/java/dev/barebones/commander/commons/file/protocol/s3/ui/S3Panel.java b/barebones-protocol-s3/src/main/java/dev/barebones/commander/commons/file/protocol/s3/ui/S3Panel.java index 8284eff533..f71475214d 100644 --- a/barebones-protocol-s3/src/main/java/dev/barebones/commander/commons/file/protocol/s3/ui/S3Panel.java +++ b/barebones-protocol-s3/src/main/java/dev/barebones/commander/commons/file/protocol/s3/ui/S3Panel.java @@ -144,8 +144,9 @@ public boolean usesCredentials() { public void dialogValidated() { try { portSpinner.commitEdit(); - } catch (ParseException ignored) { - // editor commits unconditionally; ignored + } catch (ParseException e) { + JSpinner.DefaultEditor editor = (JSpinner.DefaultEditor) portSpinner.getEditor(); + throw new IllegalArgumentException("Invalid port value: " + editor.getTextField().getText(), e); } updateValues(); } diff --git a/barebones-protocol-s3/src/test/java/dev/barebones/commander/commons/file/protocol/s3/S3MinIOIntegrationTest.java b/barebones-protocol-s3/src/test/java/dev/barebones/commander/commons/file/protocol/s3/S3MinIOIntegrationTest.java new file mode 100644 index 0000000000..ad83de164f --- /dev/null +++ b/barebones-protocol-s3/src/test/java/dev/barebones/commander/commons/file/protocol/s3/S3MinIOIntegrationTest.java @@ -0,0 +1,186 @@ +/* + * Copyright (C) 2026 barebones-commander contributors + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 3 of the License, or + * (at your option) any later version. + */ +package dev.barebones.commander.commons.file.protocol.s3; + +import dev.barebones.commander.commons.file.AbstractFile; +import dev.barebones.commander.commons.file.AuthenticationType; +import dev.barebones.commander.commons.file.Credentials; +import dev.barebones.commander.commons.file.DefaultSchemeHandler; +import dev.barebones.commander.commons.file.DefaultSchemeParser; +import dev.barebones.commander.commons.file.FileFactory; +import dev.barebones.commander.commons.file.FileURL; +import dev.barebones.commander.commons.file.SchemeHandler; +import dev.barebones.commander.commons.file.osgi.FileProtocolService; +import dev.barebones.commander.commons.file.osgi.FileProtocolServiceTracker; +import dev.barebones.commander.commons.file.protocol.ProtocolProvider; + +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInstance; +import org.opentest4j.TestAbortedException; +import org.testcontainers.DockerClientFactory; +import org.testcontainers.containers.GenericContainer; +import org.testcontainers.containers.wait.strategy.Wait; +import org.testcontainers.utility.DockerImageName; + +import java.io.InputStream; +import java.io.OutputStream; +import java.lang.reflect.Method; +import java.nio.charset.StandardCharsets; +import java.util.HashSet; +import java.util.Set; +import java.util.UUID; + +import static dev.barebones.commander.test.TestAssertions.assertEquals; +import static dev.barebones.commander.test.TestAssertions.assertFalse; +import static dev.barebones.commander.test.TestAssertions.assertTrue; + +/** + * End-to-end tests against MinIO, not LocalStack, to verify the + * S3-compatible/path-style endpoint behavior used by self-hosted + * deployments. + */ +@TestInstance(TestInstance.Lifecycle.PER_CLASS) +public class S3MinIOIntegrationTest { + + private static final DockerImageName MINIO_IMAGE = + DockerImageName.parse("minio/minio:RELEASE.2025-09-07T16-13-09Z"); + private static final int MINIO_API_PORT = 9000; + private static final String ACCESS_KEY = "minioadmin"; + private static final String SECRET_KEY = "minioadmin"; + private static final String REGION = "us-east-1"; + + private GenericContainer container; + private S3ProtocolProvider provider; + + @BeforeAll + public void startMinIO() throws Exception { + if (!DockerClientFactory.instance().isDockerAvailable()) { + throw new TestAbortedException( + "Docker is not available on this runner; skipping MinIO S3 integration tests."); + } + + container = new GenericContainer<>(MINIO_IMAGE) + .withExposedPorts(MINIO_API_PORT) + .withEnv("MINIO_ROOT_USER", ACCESS_KEY) + .withEnv("MINIO_ROOT_PASSWORD", SECRET_KEY) + .withCommand("server", "/data", "--address", ":9000") + .waitingFor(Wait.forHttp("/minio/health/ready").forPort(MINIO_API_PORT)); + container.start(); + + SchemeHandler handler = new DefaultSchemeHandler( + new DefaultSchemeParser(), 443, "/", + AuthenticationType.AUTHENTICATION_REQUIRED, null); + Method m = FileURL.class.getDeclaredMethod("registerHandler", + String.class, SchemeHandler.class); + m.setAccessible(true); + m.invoke(null, "s3", handler); + + provider = new S3ProtocolProvider(); + FileProtocolServiceTracker.register(new FileProtocolService() { + @Override public String getSchema() { return "s3"; } + @Override public ProtocolProvider getProtocolProvider() { return provider; } + @Override public SchemeHandler getSchemeHandler() { return handler; } + }); + } + + @AfterAll + public void stopMinIO() { + if (provider != null) { + provider.close(); + } + if (container != null) { + container.stop(); + } + } + + private FileURL urlFor(String path) throws Exception { + FileURL url = FileURL.getFileURL("s3://" + container.getHost() + path); + url.setPort(container.getMappedPort(MINIO_API_PORT)); + url.setCredentials(new Credentials(ACCESS_KEY, SECRET_KEY)); + url.setProperty(S3ProtocolProvider.PROPERTY_REGION, REGION); + url.setProperty(S3ProtocolProvider.PROPERTY_PATH_STYLE, "true"); + url.setProperty(S3ProtocolProvider.PROPERTY_USE_HTTPS, "false"); + return url; + } + + @Test + public void minioPathStyleLifecycle() throws Exception { + String bucketName = "minio-" + UUID.randomUUID().toString().substring(0, 8); + AbstractFile bucket = FileFactory.getFile(urlFor("/" + bucketName + "/")); + assertTrue(bucket instanceof S3Bucket); + bucket.mkdir(); + assertTrue(bucket.exists(), "bucket should exist after mkdir"); + + AbstractFile object = FileFactory.getFile(urlFor("/" + bucketName + "/hello.txt")); + byte[] payload = "hello minio world".getBytes(StandardCharsets.UTF_8); + try (OutputStream os = object.getOutputStream()) { + os.write(payload); + } + assertTrue(object.exists(), "object should exist after upload"); + assertEquals(object.getSize(), payload.length); + + try (InputStream is = object.getInputStream()) { + assertEquals(is.readAllBytes(), payload); + } + + AbstractFile dir = FileFactory.getFile(urlFor("/" + bucketName + "/sub/")); + dir.mkdir(); + AbstractFile nested = FileFactory.getFile(urlFor("/" + bucketName + "/sub/inner.txt")); + byte[] nestedPayload = "nested".getBytes(StandardCharsets.UTF_8); + try (OutputStream os = nested.getOutputStream()) { + os.write(nestedPayload); + } + + Set bucketNames = names(bucket.ls()); + assertTrue(bucketNames.contains("hello.txt"), "bucket listing should include object " + bucketNames); + assertTrue(bucketNames.contains("sub"), "bucket listing should include prefix " + bucketNames); + + AbstractFile[] innerListing = dir.ls(); + assertEquals(innerListing.length, 1); + assertEquals(innerListing[0].getName(), "inner.txt"); + + nested.delete(); + assertFalse(nested.exists(), "nested object should be gone after delete"); + object.delete(); + assertFalse(object.exists(), "object should be gone after delete"); + } + + @Test + public void minioRenameCopiesAndDeletesSource() throws Exception { + String bucketName = "rename-" + UUID.randomUUID().toString().substring(0, 8); + AbstractFile bucket = FileFactory.getFile(urlFor("/" + bucketName + "/")); + bucket.mkdir(); + + AbstractFile src = FileFactory.getFile(urlFor("/" + bucketName + "/source.txt")); + try (OutputStream os = src.getOutputStream()) { + os.write("rename me".getBytes(StandardCharsets.UTF_8)); + } + + AbstractFile dest = FileFactory.getFile(urlFor("/" + bucketName + "/dest.txt")); + src.renameTo(dest); + + Set names = names(bucket.ls()); + assertTrue(names.contains("dest.txt")); + assertFalse(names.contains("source.txt"), "source.txt should be gone after rename, got " + names); + + try (InputStream is = dest.getInputStream()) { + assertEquals(new String(is.readAllBytes(), StandardCharsets.UTF_8), "rename me"); + } + } + + private static Set names(AbstractFile[] files) { + Set names = new HashSet<>(); + for (AbstractFile file : files) { + names.add(file.getName()); + } + return names; + } +} diff --git a/barebones-protocol-sftp/src/main/java/dev/barebones/commander/commons/file/protocol/sftp/SFTPFile.java b/barebones-protocol-sftp/src/main/java/dev/barebones/commander/commons/file/protocol/sftp/SFTPFile.java index 1dfde9014a..9fcb56dffd 100644 --- a/barebones-protocol-sftp/src/main/java/dev/barebones/commander/commons/file/protocol/sftp/SFTPFile.java +++ b/barebones-protocol-sftp/src/main/java/dev/barebones/commander/commons/file/protocol/sftp/SFTPFile.java @@ -192,7 +192,9 @@ public void close() throws IOException { LOGGER.error("failed to get output stream for {}", getURL()); try { connHandler.close(); - } catch (Exception e1) {} + } catch (Exception closeException) { + LOGGER.warn("failed to close SFTP connection after output stream open failure for {}", getURL(), closeException); + } throw new IOException(e); } } @@ -579,7 +581,9 @@ public void close() throws IOException { LOGGER.error("failed to get input stream {}", getURL()); try { connHandler.close(); - } catch (Exception e1) {} + } catch (Exception closeException) { + LOGGER.warn("failed to close SFTP connection after input stream open failure for {}", getURL(), closeException); + } throw new IOException(e); } } @@ -771,7 +775,7 @@ private SFTPRandomAccessInputStream() throws IOException { @Override public int read(byte b[], int off, int len) throws IOException { - int nbRead = in.read(b, off, len); + int nbRead = requireOpen().read(b, off, len); if(nbRead!=-1) offset += nbRead; @@ -781,7 +785,7 @@ public int read(byte b[], int off, int len) throws IOException { @Override public int read() throws IOException { - int read = in.read(); + int read = requireOpen().read(); if(read!=-1) offset += 1; @@ -798,18 +802,27 @@ public long getLength() throws IOException { } public void seek(long offset) throws IOException { - try { - in.close(); - } - catch(IOException e) {} - + InputStream previous = requireOpen(); + in = null; + previous.close(); in = getInputStream(offset); this.offset = offset; } @Override public void close() throws IOException { - in.close(); + InputStream previous = in; + in = null; + if (previous != null) { + previous.close(); + } + } + + private InputStream requireOpen() throws IOException { + if (in == null) { + throw new IOException("stream closed"); + } + return in; } } } diff --git a/barebones-protocol-sftp/src/main/java/dev/barebones/commander/commons/file/protocol/sftp/SFTPPanel.java b/barebones-protocol-sftp/src/main/java/dev/barebones/commander/commons/file/protocol/sftp/SFTPPanel.java index 5e3b2d0c8f..b2a3a40517 100644 --- a/barebones-protocol-sftp/src/main/java/dev/barebones/commander/commons/file/protocol/sftp/SFTPPanel.java +++ b/barebones-protocol-sftp/src/main/java/dev/barebones/commander/commons/file/protocol/sftp/SFTPPanel.java @@ -173,10 +173,13 @@ public boolean usesCredentials() { public void dialogValidated() { // Commits the current spinner value in case it was being edited and 'enter' was pressed // (the spinner value would otherwise not be committed) - try { portSpinner.commitEdit(); } - catch(ParseException e) { } + try { + portSpinner.commitEdit(); + } catch(ParseException e) { + JSpinner.DefaultEditor editor = (JSpinner.DefaultEditor) portSpinner.getEditor(); + throw new IllegalArgumentException("Invalid port value: " + editor.getTextField().getText(), e); + } updateValues(); } } - diff --git a/barebones-viewer-text/src/main/java/dev/barebones/commander/viewer/text/TextEditorImpl.java b/barebones-viewer-text/src/main/java/dev/barebones/commander/viewer/text/TextEditorImpl.java index 6cd0943887..65de81cd25 100644 --- a/barebones-viewer-text/src/main/java/dev/barebones/commander/viewer/text/TextEditorImpl.java +++ b/barebones-viewer-text/src/main/java/dev/barebones/commander/viewer/text/TextEditorImpl.java @@ -38,6 +38,7 @@ import java.net.URL; import java.util.LinkedHashMap; import java.util.Map; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.Consumer; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -79,6 +80,7 @@ class TextEditorImpl implements ThemeListener { private static final Logger LOGGER = LoggerFactory.getLogger(TextEditorImpl.class); + private static final AtomicBoolean BEEP_RUNNING = new AtomicBoolean(); private JFrame frame; @@ -279,13 +281,21 @@ private void doSearchAndReplace(boolean forward, String replaceWith) { } if (!found) { - // Beep when no match has been found. - // The beep method is called from a separate thread because this method seems to lock until the beep has - // been played entirely. If the 'Find next' shortcut is left pressed, a series of beeps will be played when - // the end of the file is reached, and we don't want those beeps to played one after the other as to: - // 1/ not lock the event thread - // 2/ have those beeps to end rather sooner than later - new Thread(Toolkit.getDefaultToolkit()::beep).start(); + beep(); + } + } + + private static void beep() { + if (BEEP_RUNNING.compareAndSet(false, true)) { + Thread thread = new Thread(() -> { + try { + Toolkit.getDefaultToolkit().beep(); + } finally { + BEEP_RUNNING.set(false); + } + }, "TextEditorBeep"); + thread.setDaemon(true); + thread.start(); } }