Root the default output path when the current folder is unwritable#1013
Open
gmipf wants to merge 7 commits into
Open
Root the default output path when the current folder is unwritable#1013gmipf wants to merge 7 commits into
gmipf wants to merge 7 commits into
Conversation
DefaultOutputPath defaults to the relative "ISO". A relative path resolves against
the working directory, not against the application directory. That is exactly right
for the portable Windows layout, where the folder ends up next to the executable.
An installed application has no folder next to the executable. The default then sends
dumps wherever MPF happened to be started from, and the browse handler opens there as
well, since it runs the value through Path.GetFullPath(). Launched from a desktop
entry, the output path field reads
ISO/track_20260712-1843/track_20260712-1843.bin
and the dump lands in the user's home directory, or in / for a system service.
Give the default the same platform-dependent shape the tool paths already have
(DefaultAaruPath and friends): Windows keeps "ISO" unchanged, everything else gets
$HOME/ISO. MPF creates the directory itself, so it need not exist. Both places that
carry the default are updated; the loader fallback alone would leave the property
initializer relative.
Verified on Fedora 44 against the packaged 3.8.3 build.
Member
|
I don't agree that non-Windows systems should always be treated as if they are being run from a central location. Effectively disallowing a default of the same folder for MacOS and Linux does not feel like the best path forward. I would prefer a similar situation to the existing code for the configuration, where if the current folder is unwritable, then make it elsewhere. |
Replaces the platform-dependent default with the shape requested in review: the relative "ISO" stays the default on every platform, and the path is only rooted in the home directory when a file cannot be created in the current folder. This mirrors GetConfigurationPath, which probes the portable location first and moves elsewhere when writing fails. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
gmipf
changed the title
Contributor
Author
|
Reworked to the shape you described. The relative The PR body is updated to describe the new behaviour. Pushed as a follow-up commit rather than a force push. Generated with Claude Opus 4.8. |
…path # Conflicts: # CHANGELIST.md
…path # Conflicts: # CHANGELIST.md
mnadareski
reviewed
Jul 17, 2026
…oist the constant - GetDefaultOutputPath now probes inline like OptionsLoader.GetConfigurationPath; CanCreateFile removed - DefaultOutputDirectoryName moved to the top of DumpSettings Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
DefaultOutputPathdefaults to the relative"ISO", which resolves against the working directory. An installed copy has no folder next to the executable, so dumps land wherever MPF was started from — and when that folder is not writable, the dump fails.Following the review, this keeps
"ISO"as the default on every platform and only roots the path in the home directory when the current folder cannot be written to.Change
DumpSettings.GetDefaultOutputPath(currentDirectory):"ISO", unchanged on all platformsPath.Combine(PathTool.GetHomeDirectory(), "ISO")The probe creates a file with a random name and removes it again. Only the creation decides, so a failed cleanup does not read as unwritable. This follows
GetConfigurationPath, which also tries the portable location first and moves elsewhere when writing fails. The result is cached in a static, so the probe runs once per process.The literal now lives in one place, and
OptionsLoaderuses the same value as the fallback for a missingDefaultOutputPathkey.Behaviour
Nothing changes for a portable copy in a writable folder, on any platform. The fallback is reached by cases such as a portable copy unpacked into
C:\Program Files\MPF, or a packaged build started with a read-only working directory.Tests
MPF.Frontend.Test/OptionsTests.cs, four cases: writable folder, folder that cannot hold a file, missing folder, and null coercion. Inverting the writability check turns three of them red.A real permission denial reaches the same branch. Measured on Fedora 44 against a directory at mode
r-x------:That case is not in the committed tests:
File.SetUnixFileModeis unsupported on Windows and CA1416 fails the build, so the shipped fixture puts a file where the directory is expected, which fails the creation on every platform and reaches the same catch.Prepared with AI assistance (Claude Opus 4.8) and reviewed before submission.