[v3] Added AppDataDirectory path #3823
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces several updates to the Wails framework, including new features, modifications, and bug fixes. Key additions include methods for path management in the application package, a new command for updating build assets, and enhancements to the documentation. Dependency management in the Changes
Possibly related PRs
Warning Rate limit exceeded@leaanthony has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 27 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (10)
mkdocs-website/docs/en/API/application.md (1)
136-156: Enhance the Path method documentation for clarity and completeness.The documentation for the new
Pathmethod is clear and provides good examples. However, consider the following improvements to make it more comprehensive:
Expand on the cross-platform nature of the method. For example: "This method provides a consistent way to access these directories across different operating systems, abstracting away OS-specific path structures."
Add information about error handling. For instance: "If an invalid path type is provided, the method will return an empty string and log an error."
Provide more context on when and why a developer might use these paths. For example: "The AppData directory is useful for storing application-specific data that should persist between sessions. The UserCache directory is ideal for temporary files that can be regenerated, while the UserConfig directory is suitable for user-specific configuration files."
Consider adding a note about permissions or any potential issues that developers should be aware of when using these paths.
Would you like me to draft an expanded version of this documentation section?
v3/pkg/application/application_darwin.go (4)
369-372: LGTM with a minor suggestion for error handlingThe
getAppDataPathmethod correctly retrieves the Application Support directory path for macOS. However, consider handling potential errors fromos.UserHomeDir().You might want to handle the error from
os.UserHomeDir(). Here's a suggestion:func (a *macosApp) getAppDataPath() (string, error) { home, err := os.UserHomeDir() if err != nil { return "", fmt.Errorf("failed to get user home directory: %w", err) } return filepath.Join(home, "Library", "Application Support"), nil }This change would allow callers to handle potential errors when retrieving the path.
374-377: LGTM with a minor suggestion for error handlingThe
getUserCachePathmethod correctly retrieves the Caches directory path for macOS. As with the previous method, consider handling potential errors fromos.UserHomeDir().Similar to the previous suggestion, you might want to handle the error from
os.UserHomeDir():func (a *macosApp) getUserCachePath() (string, error) { home, err := os.UserHomeDir() if err != nil { return "", fmt.Errorf("failed to get user home directory: %w", err) } return filepath.Join(home, "Library", "Caches"), nil }This change would allow callers to handle potential errors when retrieving the path.
379-382: LGTM with a minor suggestion for error handlingThe
getUserConfigPathmethod correctly retrieves the Preferences directory path for macOS. As with the previous methods, consider handling potential errors fromos.UserHomeDir().Consistent with the previous suggestions, you might want to handle the error from
os.UserHomeDir():func (a *macosApp) getUserConfigPath() (string, error) { home, err := os.UserHomeDir() if err != nil { return "", fmt.Errorf("failed to get user home directory: %w", err) } return filepath.Join(home, "Library", "Preferences"), nil }This change would allow callers to handle potential errors when retrieving the path.
369-382: Overall, good addition of macOS-specific user directory pathsThe addition of these three methods (
getAppDataPath,getUserCachePath, andgetUserConfigPath) enhances the functionality of themacosAppstruct by providing access to common user directory paths specific to macOS. The implementations are correct and consistent.To improve cross-platform consistency and make it easier to use these methods throughout the application, consider the following suggestions:
- Implement similar methods for other platforms (Windows, Linux) if not already done.
- Create a common interface in the
applicationpackage that defines these methods, which each platform-specific implementation can satisfy.- Consider adding a method to retrieve all paths at once, returning a struct with the various directories. This could be useful for initializing the application and would reduce the number of potential error checks needed when using these paths.
Example interface:
type PlatformPaths interface { GetAppDataPath() (string, error) GetUserCachePath() (string, error) GetUserConfigPath() (string, error) GetAllPaths() (Paths, error) } type Paths struct { AppData string UserCache string UserConfig string }This approach would make it easier to use these paths consistently across different parts of the application, regardless of the underlying OS.
v3/pkg/application/application_windows.go (3)
355-358: LGTM with a suggestion for error handlingThe
getAppDataPathmethod correctly retrieves the APPDATA environment variable. However, consider adding error handling in case the environment variable is not set.You could improve the robustness of this method by checking if the environment variable exists:
func (a *windowsApp) getAppDataPath() (string, error) { path, exists := os.LookupEnv("APPDATA") if !exists { return "", fmt.Errorf("APPDATA environment variable not set") } return path, nil }
360-363: LGTM with suggestions for improvementThe
getUserCachePathmethod correctly constructs the user cache path. However, consider the following improvements:
- Add error handling in case the LOCALAPPDATA environment variable is not set.
- Ensure the directory exists or create it if it doesn't.
Here's an improved version of the method:
func (a *windowsApp) getUserCachePath() (string, error) { localAppData, exists := os.LookupEnv("LOCALAPPDATA") if !exists { return "", fmt.Errorf("LOCALAPPDATA environment variable not set") } path := filepath.Join(localAppData, "Temp") err := os.MkdirAll(path, 0755) if err != nil { return "", fmt.Errorf("failed to create cache directory: %w", err) } return path, nil }
365-368: LGTM with a suggestion for error handlingThe
getUserConfigPathmethod correctly retrieves the LOCALAPPDATA environment variable. However, consider adding error handling in case the environment variable is not set.You could improve the robustness of this method by checking if the environment variable exists:
func (a *windowsApp) getUserConfigPath() (string, error) { path, exists := os.LookupEnv("LOCALAPPDATA") if !exists { return "", fmt.Errorf("LOCALAPPDATA environment variable not set") } return path, nil }mkdocs-website/docs/en/changelog.md (1)
24-24: LGTM! Consider adding more details about thePathmethod.The addition to the changelog is correctly placed and follows the established format. It accurately reflects the changes mentioned in the PR objectives.
To provide more context for users, consider expanding the description slightly. For example:
-- Add `Path` method to `application` package by [ansxuman](https://github.com/ansxuman) in [#3823](https://github.com/wailsapp/wails/pull/3823) +- Add `Path` method to `application` package for retrieving AppDataDirectory path by [ansxuman](https://github.com/ansxuman) in [#3823](https://github.com/wailsapp/wails/pull/3823)This additional detail helps users understand the purpose of the new method without needing to refer to the PR directly.
v3/pkg/application/application.go (1)
36-43: Consider adding comments for the newPathtype and constantsTo enhance code readability and maintainability, please add comments or documentation for the new
Pathtype and its constants (AppData,UserCache,UserConfig). This will help other developers understand their purpose and usage within the application.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
- mkdocs-website/docs/en/API/application.md (1 hunks)
- mkdocs-website/docs/en/changelog.md (1 hunks)
- v3/pkg/application/application.go (3 hunks)
- v3/pkg/application/application_darwin.go (2 hunks)
- v3/pkg/application/application_linux.go (2 hunks)
- v3/pkg/application/application_windows.go (2 hunks)
🧰 Additional context used
🔇 Additional comments (3)
v3/pkg/application/application_linux.go (1)
265-278: Overall assessment of the changesThe introduction of methods to retrieve user-specific directory paths (
getAppDataPath,getUserCachePath, andgetUserConfigPath) is a valuable addition to the Linux application implementation. These methods align with the XDG Base Directory Specification, which is a standard practice in Linux environments.However, there are opportunities for improvement:
- Error handling: Currently, potential errors from
os.UserHomeDir()are ignored, which could lead to unexpected behavior.- Robustness: The implementation could be more robust by checking for XDG environment variables before falling back to default paths.
- Code organization: There's some duplication in the implementation of these methods that could be reduced through refactoring.
Addressing these points would enhance the reliability and maintainability of the code. Consider implementing the suggested refactoring to create a more robust and DRY (Don't Repeat Yourself) solution.
To ensure that the XDG Base Directory Specification is correctly implemented, please run the following verification script:
This script will help verify that the implementation correctly uses XDG environment variables, has proper fallback paths, and handles potential errors from
os.UserHomeDir().v3/pkg/application/application_windows.go (1)
354-368: Overall assessment: Good addition with room for improvementThe new methods
getAppDataPath,getUserCachePath, andgetUserConfigPathare valuable additions to thewindowsAppstruct, providing functionality to retrieve application-specific directory paths. These changes align well with the PR objectives of adding AppDataDirectory path functionality.Positive aspects:
- Consistent implementation across the three methods.
- Use of standard Windows environment variables.
- Use of
filepath.Joinfor path construction ingetUserCachePath.Areas for improvement:
- Add error handling for cases where environment variables are not set.
- Include documentation for each method.
- Consider creating directories if they don't exist, especially for the cache path.
- Implement consistent return signatures (string vs string, error) across all methods.
Overall, these changes enhance the functionality of the application for Windows users. With the suggested improvements, the code will be more robust and maintainable.
v3/pkg/application/application.go (1)
203-205: Verify implementation of new interface methods across platformsThe
platformAppinterface now includesgetAppDataPath(),getUserCachePath(), andgetUserConfigPath(). Please ensure that all platform-specific implementations of this interface (e.g., Windows, macOS, Linux) have these methods implemented to prevent compile-time errors.Run the following script to check for implementations of the new methods:
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
v3/pkg/application/application.go (1)
1028-1041: LGTM: Path method is well-implemented, with a minor suggestion.The
Pathmethod provides a clean API for retrieving platform-specific paths. It correctly handles all definedPathconstants and includes proper error handling for unknown selectors.Consider using a more descriptive error message in the default case:
- return "", fmt.Errorf("unknown path selector: %v", selector) + return "", fmt.Errorf("invalid path selector: %v", selector)This change makes it clearer that the selector is invalid, not just unknown.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
v3/go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
- mkdocs-website/docs/en/API/application.md (1 hunks)
- v3/go.mod (2 hunks)
- v3/pkg/application/application.go (3 hunks)
- v3/pkg/application/application_darwin.go (2 hunks)
- v3/pkg/application/application_linux.go (2 hunks)
- v3/pkg/application/application_windows.go (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- mkdocs-website/docs/en/API/application.md
- v3/pkg/application/application_linux.go
- v3/pkg/application/application_windows.go
🧰 Additional context used
🔇 Additional comments (11)
v3/go.mod (3)
Line range hint
1-108: Summary of changes in go.modThe changes in this file are minimal and focused on the PR objective of adding AppDataDirectory path functionality:
- Upgrade of
golang.org/x/sysfrom v0.20.0 to v0.22.0- Addition of
github.com/adrg/xdg v0.5.0as an indirect dependencyThese changes appear to be in line with the PR objectives and don't introduce any obvious conflicts. However, it's important to:
- Verify that the
golang.org/x/sysupgrade doesn't introduce any breaking changes.- Clarify how the
github.com/adrg/xdgpackage is being used in the codebase.Please ensure that these changes are reflected in the changelog and that any necessary documentation updates are made.
To ensure these changes don't have any unintended consequences, please run the project's test suite and perform a thorough manual testing of the new AppDataDirectory functionality on all supported platforms (Windows, macOS, and Linux).
49-49: Approve the addition of github.com/adrg/xdg, but clarify its usage.The addition of
github.com/adrg/xdg v0.5.0as an indirect dependency aligns with the PR objective of adding AppDataDirectory path functionality. This package is commonly used for handling XDG Base Directory Specification in Go applications.To ensure proper integration and usage, please run the following script:
#!/bin/bash # Description: Verify the usage of github.com/adrg/xdg in the codebase # Test: Search for direct imports of github.com/adrg/xdg rg --type go 'import.*"github.com/adrg/xdg"' # Test: Search for usage of xdg package functions rg --type go 'xdg\.' # Test: Check if any existing code for handling app data directories has been modified rg --type go 'AppData|UserConfigDir|UserDataDir'Could you please clarify which component is using this package and how it's being integrated into the existing codebase?
34-34: Approve the upgrade of golang.org/x/sys, but verify compatibility.The upgrade of
golang.org/x/sysfrom v0.20.0 to v0.22.0 is a good practice to keep dependencies up-to-date. However, it's important to ensure that this upgrade doesn't introduce any breaking changes to the project.Please run the following script to check for any potential breaking changes or deprecations:
v3/pkg/application/application.go (4)
36-42: LGTM: New Path type and constants are well-defined.The introduction of the
Pathtype and associated constants (AppData, UserCache, UserConfig) is a good addition. The use ofiotafor defining the constants is idiomatic Go and provides a clear enumeration of path types.
203-205: LGTM: New methods in platformApp interface are well-defined.The addition of
getAppDataPath,getUserCachePath, andgetUserConfigPathmethods to theplatformAppinterface is consistent with the newPathtype. The method signatures returning(string, error)follow Go conventions for operations that might fail.
1028-1041: Past review comment addressed.The current implementation of the
Pathmethod addresses the concern raised in the past review comment. The default case in the switch statement now handles invalidPathselectors by returning an error, preventing silent failures.
Line range hint
36-1041: Overall changes: New path management feature well-integrated.The introduction of the
Pathtype, constants, and associated methods provides a robust and consistent way to handle application-specific paths across different platforms. The changes are well-integrated into the existing codebase and enhance the application's functionality without introducing conflicts or inconsistencies.The new feature allows for more flexible and platform-independent path management, which should improve the application's portability and ease of use across different operating systems.
v3/pkg/application/application_darwin.go (4)
163-165: Appropriate addition of necessary importsThe imports
os,path/filepath, andstringsare required for file path manipulation and string operations in the new methods. They are correctly included.
168-169: Addition of external packages for directory handlingIncluding
github.com/adrg/xdgandgithub.com/wailsapp/wails/v3/internal/operatingsystemis appropriate for accessing standard directories and operating system information, enhancing cross-platform compatibility.
371-382:⚠️ Potential issueEnsure macOS-specific application data path is used
The
getAppDataPathfunction usesxdg.DataHome, which follows the XDG Base Directory specification common on Linux systems. On macOS, the standard location for application data is~/Library/Application Support. Relying onxdg.DataHomemay not return the expected directory on macOS and could lead to inconsistency.Consider modifying the function to directly return the standard macOS application data path. Here's a suggested change:
func (a *macosApp) getAppDataPath() (string, error) { home, err := os.UserHomeDir() if err != nil { return "", err } return filepath.Join(home, "Library", "Application Support"), nil }If cross-platform consistency is a goal, ensure that the
xdgpackage appropriately handles macOS paths or implement conditional logic based on the operating system.
384-394:⚠️ Potential issueAdjust cache directory retrieval to macOS standards
In
getUserCachePath, usingxdg.CacheHomemay not align with macOS conventions, as macOS typically uses~/Library/Cachesfor cache files. Depending onxdg.CacheHomecould result in an incorrect path on macOS.It's advisable to modify the function to return the standard macOS cache directory directly:
func (a *macosApp) getUserCachePath() (string, error) { home, err := os.UserHomeDir() if err != nil { return "", err } return filepath.Join(home, "Library", "Caches"), nil }Ensure that the application uses the appropriate cache directory consistent with macOS guidelines.
| func (a *macosApp) getUserConfigPath() (string, error) { | ||
| home, err := os.UserHomeDir() | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| userPrefDir := filepath.Join(home, "Library", "Preferences") | ||
|
|
||
| for _, dir := range xdg.ConfigDirs { | ||
| if dir == userPrefDir { | ||
| return dir, nil | ||
| } | ||
| if strings.HasSuffix(dir, "Library/Preferences") { | ||
| if strings.HasPrefix(dir, home) { | ||
| return dir, nil | ||
| } | ||
| } | ||
| } | ||
| return userPrefDir, nil | ||
| } |
There was a problem hiding this comment.
Simplify and correct user configuration path retrieval
The getUserConfigPath function includes complex logic involving xdg.ConfigDirs to determine the user configuration directory. On macOS, configuration files are typically stored in ~/Library/Preferences. The usage of xdg.ConfigDirs may not be necessary and could introduce unexpected behavior.
Consider simplifying the function to directly return the macOS preferences directory:
func (a *macosApp) getUserConfigPath() (string, error) {
home, err := os.UserHomeDir()
if err != nil {
return "", err
}
return filepath.Join(home, "Library", "Preferences"), nil
}If there's a need to handle user-defined configuration directories, ensure that the logic accurately reflects macOS directory structures and standards.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (a *macosApp) getUserConfigPath() (string, error) { | |
| home, err := os.UserHomeDir() | |
| if err != nil { | |
| return "", err | |
| } | |
| userPrefDir := filepath.Join(home, "Library", "Preferences") | |
| for _, dir := range xdg.ConfigDirs { | |
| if dir == userPrefDir { | |
| return dir, nil | |
| } | |
| if strings.HasSuffix(dir, "Library/Preferences") { | |
| if strings.HasPrefix(dir, home) { | |
| return dir, nil | |
| } | |
| } | |
| } | |
| return userPrefDir, nil | |
| } | |
| func (a *macosApp) getUserConfigPath() (string, error) { | |
| home, err := os.UserHomeDir() | |
| if err != nil { | |
| return "", err | |
| } | |
| return filepath.Join(home, "Library", "Preferences"), nil | |
| } |
|
Hi @ansxuman - I think we should simply expose the XDG methods and exports via |
Thats a good point, devs will have more control, i have made the changes. P.S - Should i raise PR for v2 also ? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
v3/pkg/application/application_darwin.go (1)
368-384: Summary of changes and recommendationsThe additions to the
macosAppstruct enhance its functionality by providing methods to retrieve user-specific paths. WhilegetAppDataPathandgetUserCachePathare well-implemented and adhere to XDG specifications,getUserConfigPathneeds refinement to better align with macOS conventions.Consider implementing the suggested changes for
getUserConfigPathto ensure consistency with macOS standards and improve reliability. Once these changes are made, the overall implementation will provide a robust solution for managing application-specific directories on macOS.v3/pkg/application/application.go (1)
1027-1044: LGTM! Well-implemented Path method with a suggestion.The
Pathmethod is well-structured and properly handles different cases, including uninitialized application and unknown selectors. It effectively abstracts the platform-specific implementations.Consider using a custom error type or error variables for better error handling and consistency. For example:
var ErrAppNotInitialized = errors.New("application not initialized") var ErrUnknownPathSelector = errors.New("unknown path selector") func (a *App) Path(selector Path) (string, error) { if a.impl == nil { return "", ErrAppNotInitialized } switch selector { case AppData: return a.impl.getAppDataPath() case UserCache: return a.impl.getUserCachePath() case UserConfig: return a.impl.getUserConfigPath() default: return "", fmt.Errorf("%w: %v", ErrUnknownPathSelector, selector) } }This approach allows for easier error checking and provides more context in the error messages.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- v3/pkg/application/application.go (3 hunks)
- v3/pkg/application/application_darwin.go (2 hunks)
- v3/pkg/application/application_linux.go (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- v3/pkg/application/application_linux.go
🧰 Additional context used
🔇 Additional comments (5)
v3/pkg/application/application_darwin.go (3)
369-371: LGTM: Appropriate use of XDG specification for app data pathThe
getAppDataPathmethod correctly uses thexdg.DataHometo retrieve the application data directory. This adheres to the XDG Base Directory Specification, which is a good practice for cross-platform compatibility.
373-375: LGTM: Correct implementation for user cache pathThe
getUserCachePathmethod appropriately usesxdg.CacheHometo retrieve the user's cache directory. This is consistent with the XDG Base Directory Specification and provides a standard location for caching application data.
377-384:⚠️ Potential issueSimplify and correct user configuration path retrieval
The current implementation of
getUserConfigPathhas some issues:
- It iterates through
xdg.ConfigDirs, which is not typically used on macOS.- It returns an empty string if no suitable directory is found, which could lead to unexpected behavior.
- It doesn't align with macOS conventions for storing user preferences.
Consider simplifying the function to directly return the macOS preferences directory:
func (a *macosApp) getUserConfigPath() (string, error) { - for _, dir := range xdg.ConfigDirs { - if strings.HasSuffix(dir, "Library/Preferences") { - return dir, nil - } - } - return "", nil + home, err := os.UserHomeDir() + if err != nil { + return "", err + } + return filepath.Join(home, "Library", "Preferences"), nil }This implementation:
- Directly uses the standard macOS location for user preferences.
- Properly handles and returns any errors encountered.
- Aligns with macOS conventions and is more reliable.
v3/pkg/application/application.go (2)
36-42: LGTM! Well-structured path type and constants.The introduction of the
Pathtype and associated constants (AppData,UserCache,UserConfig) is a good approach for managing different application-specific paths. Using an integer type withiotaensures efficient comparisons and unique values.
203-205: LGTM! Consistent interface extension.The addition of
getAppDataPath(),getUserCachePath(), andgetUserConfigPath()methods to theplatformAppinterface is well-aligned with the newPathtype and constants. The consistent return types (string, error) allow for proper error handling.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
mkdocs-website/docs/en/changelog.md (1)
27-27: Approved with a suggestion for clarity.The new changelog entry correctly documents the addition of the
Pathmethod to theapplicationpackage. This aligns with the PR objectives and follows the proper format for changelog entries.Consider expanding the description slightly to provide more context about the purpose of the
Pathmethod. For example:- Add `Path` method to `application` package by [ansxuman](https://github.com/ansxuman) in [#3823](https://github.com/wailsapp/wails/pull/3823) + Add `Path` method to `application` package for retrieving application-specific directory paths by [ansxuman](https://github.com/ansxuman) in [#3823](https://github.com/wailsapp/wails/pull/3823)This additional context would help users understand the purpose and significance of the new method without needing to refer to the PR directly.
|
Refactored the PR (I hope you don't mind!) to make it easier to maintain. Thanks for kicking this off @ansxuman 🙏 |
My Pleasure |
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes #3811
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration using
wails doctor.Test Configuration
Please paste the output of
wails doctor. If you are unable to run this command, please describe your environment in as much detail as possible.Checklist:
website/src/pages/changelog.mdxwith details of this PRSummary by CodeRabbit
New Features
Pathmethod in the application package for enhanced path management.sveltekitandsveltekit-tsconfigured for non-SSR development.wails3 update build-assets.InitialPositionoption for window positioning.Bug Fixes
AlwaysOnTopfunctionality on macOS.Documentation