Support relative paths for Python/Node.js executables #4230
Conversation
Co-authored-by: Jack251970 <53996452+Jack251970@users.noreply.github.com>
Co-authored-by: Jack251970 <53996452+Jack251970@users.noreply.github.com>
…portability Co-authored-by: Jack251970 <53996452+Jack251970@users.noreply.github.com>
This reverts commit 864eedd.
Removed logic that converted selected Python and Node executable paths to relative if within the program directory. Now, the selected file paths are stored as absolute paths without conversion. This simplifies path handling and improves clarity.
Removed the PathResolutionTest.cs file, which contained unit tests for the Constant class's path resolution methods. These tests covered absolute, relative, UNC, null, and empty path scenarios, as well as round-trip conversions.
Move ResolveAbsolutePath and ConvertToRelativePathIfPossible from Constant to DataLocation for better organization. Update all references accordingly; implementations remain unchanged. This improves code clarity around file path management.
Eliminated the unnecessary using statement for Flow.Launcher.Infrastructure in AbstractPluginEnvironment.cs, as its types or members are no longer referenced in this file. This helps clean up the code and avoid redundant dependencies.
Removed logic that converted absolute paths to relative paths within the ProgramDirectory. Now, plugin settings file paths are always stored as absolute paths. Deleted the ConvertToRelativePathIfPossible method and updated usages accordingly.
|
🥷 Code experts: no user but you matched threshold 10 Jack251970 has most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame: ✨ Comment |
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
📝 WalkthroughWalkthroughThis pull request adds support for relative paths in Flow Launcher's portable version. It introduces a new utility method Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This pull request adds support for relative paths in Python and Node.js executable configuration, enabling truly portable Flow Launcher setups. The changes allow users to configure runtime paths relative to the application directory (e.g., .\runtimes\python\pythonw.exe), which are resolved to absolute paths at runtime.
Changes:
- Added
ResolveAbsolutePath()method to handle relative-to-absolute path resolution - Integrated path resolution into plugin environment setup and file existence checks
- Paths are stored as-entered (relative or absolute) and resolved only at execution time
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| Flow.Launcher.Infrastructure/UserSettings/DataLocation.cs | Adds ResolveAbsolutePath() method to resolve relative paths based on ProgramDirectory |
| Flow.Launcher.Core/ExternalPlugins/Environments/AbstractPluginEnvironment.cs | Integrates path resolution into plugin setup, using resolved paths for file checks and plugin instantiation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Resolve relative to ProgramDirectory | ||
| return Path.GetFullPath(Path.Combine(Constant.ProgramDirectory, path)); |
There was a problem hiding this comment.
The ResolveAbsolutePath method doesn't handle exceptions that can be thrown by Path.GetFullPath, such as ArgumentException for invalid path characters, NotSupportedException for invalid path formats, or PathTooLongException. If a user configures an invalid relative path, this could crash the application during plugin initialization. Consider adding try-catch error handling to gracefully handle invalid paths, either by logging an error and returning the original path, or by providing a meaningful error message to the user.
| // Resolve relative to ProgramDirectory | |
| return Path.GetFullPath(Path.Combine(Constant.ProgramDirectory, path)); | |
| // Resolve relative to ProgramDirectory, handling invalid path formats gracefully | |
| try | |
| { | |
| return Path.GetFullPath(Path.Combine(Constant.ProgramDirectory, path)); | |
| } | |
| catch (Exception ex) when (ex is ArgumentException || | |
| ex is NotSupportedException || | |
| ex is PathTooLongException) | |
| { | |
| // If the path cannot be resolved (invalid characters, format, or too long), | |
| // return the original path to avoid crashing the application. | |
| return path; | |
| } |
| public static string ResolveAbsolutePath(string path) | ||
| { | ||
| if (string.IsNullOrEmpty(path)) | ||
| return path; | ||
|
|
||
| // If already absolute, return as-is | ||
| if (Path.IsPathRooted(path)) | ||
| return path; | ||
|
|
||
| // Resolve relative to ProgramDirectory | ||
| return Path.GetFullPath(Path.Combine(Constant.ProgramDirectory, path)); | ||
| } |
There was a problem hiding this comment.
The new ResolveAbsolutePath method lacks comprehensive test coverage. Given that this is a critical feature for portability and involves path resolution logic with edge cases (relative paths starting with ".", paths without drives, UNC paths, etc.), it should have unit tests to verify correct behavior across different scenarios. Consider adding tests in Flow.Launcher.Test to verify: 1) absolute paths are returned unchanged, 2) relative paths like ".\runtime\python.exe" resolve correctly, 3) paths without leading "." also resolve correctly, and 4) edge cases like null/empty strings are handled properly.
| /// <summary> | ||
| /// Resolves a path that may be relative to an absolute path. | ||
| /// If the path is already absolute, returns it as-is. | ||
| /// If the path is relative (starts with . or doesn't contain a drive), resolves it relative to ProgramDirectory. |
There was a problem hiding this comment.
The documentation comment states the method handles paths that "start with . or doesn't contain a drive", but the implementation only checks if the path is rooted using Path.IsPathRooted. This check returns false for relative paths (including those starting with ".") but may have unexpected behavior with UNC paths or paths on Unix-like systems. Consider clarifying the documentation to match the actual implementation, which relies on Path.IsPathRooted for the determination.
| /// If the path is relative (starts with . or doesn't contain a drive), resolves it relative to ProgramDirectory. | |
| /// If the path is not rooted (as determined by <see cref="Path.IsPathRooted(string)"/>), resolves it relative to ProgramDirectory. |
Flow Launcher required absolute paths for Python and Node.js executables, breaking portability when the application is moved. Embedded runtimes within the portable directory structure were not supported.
CHANGES
ResolveAbsolutePath(): Resolves relative paths to absolute at runtime, based onProgramDirectoryUsage
Users can now configure:
Paths are stored as-entered (relative or absolute), resolved to absolute only at execution. File picker selections within
ProgramDirectoryare automatically stored as relative paths.Enables truly portable setups with embedded runtimes that survive directory moves, USB installations, and restricted environments.
Resolve #4228