Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

basic implementation of XDG base directory for alt folder #876

Merged
merged 16 commits into from
Sep 16, 2023

Conversation

DutchO7
Copy link
Contributor

@DutchO7 DutchO7 commented Sep 13, 2023

Description

Added basic support for XDG base directory, if the environment variable XDG_DATA_HOME is set then alts and encryption will be stored in $XDG_DATA_HOME/WurstClient/, else it will just store in .Wurst encryption, in the home directory.

Summary by CodeRabbit

  • New Feature: Introduced a dynamic encryption folder location for the Wurst client. The location is now determined based on the XDG_DATA_HOME environment variable, enhancing compatibility with different system configurations.
  • Refactor: Added a migration function to handle the transition of the encryption folder from its old location to the new one. This ensures seamless user experience during upgrades.
  • Bug Fix: Removed an unused import statement, improving code cleanliness and maintainability.

@CLAassistant
Copy link

CLAassistant commented Sep 13, 2023

CLA assistant check
All committers have signed the CLA.

@Alexander01998
Copy link
Member

Hi @DutchO7, thanks for the pull request!

Could you please elaborate and provide some more context around this change? Specifically, what would the benefits for the end user be? Does the default user.home folder not work on your operating system?

Also, it would be helpful if you could provide some detailed testing steps for this PR so I can validate the changes.

Thanks!

@Alexander01998 Alexander01998 added the area:AltManager Improves the Alt Manager. label Sep 13, 2023
@DutchO7
Copy link
Contributor Author

DutchO7 commented Sep 13, 2023

Hi @Alexander01998, thank you for replying!

The main purpose of this change is to adhere to the XDG base directory standard, it helps to keep the users home directory clean and organized by storing programs files in directories specified by the standard.

The user.home directory works fine on my operating system, but using the XDG base directory standard helps to keep the users home folder clean of dotfiles/dotfolders, this is beneficial for users who prefer to keep program data organized and out of the home directory.

For testing, you can set the environment variable XDG_DATA_HOME to something like $HOME/.local/share, and run Wurst with the changes, the alt directory will be in $XDG_DATA_HOME/WurstClient instead of having a dotfolder in the home directory.

If the environment variable XDG_DATA_HOME is not set, Wurst will default back to the user.home directory.

If you would like to learn more about the XDG base directory standard, you can find some more information about it here.
https://wiki.archlinux.org/title/XDG_Base_Directory

I hope this is enough to explain my changes, I apologize for not going into detail earlier, please let me know if there is anything else that I could explain.

@Alexander01998 Alexander01998 added the type:enhancement New feature or request label Sep 13, 2023
Copy link
Member

@Alexander01998 Alexander01998 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @DutchO7, thanks for the additional context. The implementation of the XDG base directory standard sounds like a useful addition. However, I see a potential issue with implementing it in the way you've proposed.

If a user already has their XDG_DATA_HOME variable set and they are updating from a previous version of Wurst, they may already have the encryption files stored in their home folder. In this case, the new version of Wurst would be unable to find these files in the XDG directory, causing the AltManager to fail to decrypt the user's alt list and act as if the file is corrupted.

To mitigate this issue, consider implementing a migration process that checks for the presence of the encryption files in the home folder and moves them to the new XDG directory if the XDG_DATA_HOME variable is set. This way, existing users will not lose their alt data during the update process.

Let me know your thoughts on this approach, and if you have any other suggestions or concerns.

@DutchO7
Copy link
Contributor Author

DutchO7 commented Sep 13, 2023

Hi @Alexander01998, thank you for your feedback.

I have implemented a migration process that will move the files from the .Wurst encryption directory to the $XDG_DATA_HOME/WurstClient directory, this is how it works:

The code will check if the new $XDG_DATA_HOME/WurstClient directory doesn't exist, then it will check if the old .Wurst encryption directory exists, and if this condition is met, the files in the old .Wurst encryption directory will be moved to $XDG_DATA_HOME/WurstClient, and finally it will create a text file in the old .Wurst encryption directory that will tell the user about the changes and where they can find the new files.

This way, users who upgrade to a new version and have the XDG_DATA_HOME environment variable set will be migrated automatically and not lose their encryption data.

I apologize for taking so long, Java is not my first language, but please let me know if you have any more concerns.

Copy link
Member

@Alexander01998 Alexander01998 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @DutchO7, that was super quick – no worries at all!

I've taken a look at your updated changes and tested it out. However, a few concerns have come up:

  1. Files.move() is only intended for moving a single file or an empty directory. It can't recursively move a directory with all of its contents. This approach may sometimes work due to optimizations in the file system, but it's not reliable. For example, the migration will fail if XDG_DATA_HOME is on a different drive than user.home.

  2. The Files.exists() check is performed twice: once in WurstClient.initialize() and again in Encryption.migrateEncryptionFolder(). It would be more efficient to do this check only once, and perhaps use Files.isDirectory() instead to ensure the target is indeed a directory.

  3. The migration note that gets placed in the user's home directory seems counterproductive, since the essence of XDG is to keep the home directory uncluttered.

  4. I suggest using the same ".Wurst Encryption" folder name across both locations for consistency.

I hope you find time to make these improvements. Your work is much appreciated!

@DutchO7
Copy link
Contributor Author

DutchO7 commented Sep 14, 2023

Hi @Alexander01998, thank you for your feedback, I appricate you taking the time to consider and review my code!

I have made some changes based on your concerns:

  1. I have implemented a solution that will copy all the files inside the old .Wurst encryption directory into the new directory in $XDG_DATA_HOME and then remove the old file and directory, this should fix the issue of transferring across filesystems.

  2. I have removed the redundant check in Encryption.migrateEncryptionFolder() and changed the check on the old encryption folder from Files.exists() to Files.isDirectory().

  3. The migration note was included for users that might not keep up with the patch notes, it was intended to prevent confusion after someone was migrated. It should only have been created if the user upgrading has not migrated yet and Wurst migrates it, but I might have messed it up, it didnt appear again in my tests after removing it, but I went ahead and just removed it completely.

  4. As for the folder name, I thought that having a dotfolder in $XDG_DATA_HOME would be out of place, as all the other files/directories in $XDG_DATA_HOME arent dotfiles/dotfolders, but I have went ahead and changed it from WurstClient to .Wurst encryption.

Thank you again for taking the time to provide feedback on my code, please let me know if you have any more concerns.

@coderabbitai
Copy link

coderabbitai bot commented Sep 15, 2023

Walkthrough

The changes introduced in this pull request primarily focus on the migration of the encryption folder for alt files in the Wurst client. The new code determines the location of the encryption folder based on the XDG_DATA_HOME environment variable and handles the migration from the old location to the new one, if necessary.

Changes

File Path Summary
.../net/wurstclient/WurstClient.java Introduced changes related to the encryption of alt files. The code now handles the migration of the encryption folder based on the XDG_DATA_HOME environment variable.
.../net/wurstclient/altmanager/Encryption.java Added methods to migrate the encryption folder from an old location to a new one. The code creates the new folder, copies files, deletes the old folder, and handles any IOException.

🐇💻

In the land of code, where the brackets lie,
A rabbit hopped forth, with a twinkle in its eye.
"A change is due," it said with glee,
"For better paths, as you will see."

With a hop, skip, and a jump,
It made the old code take a bump.
Now the data finds its way,
In new folders, it will stay.

So here's to changes, big and small,
May our code stand tall!
🎉🥕


Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • Mention @coderabbitai in any review comment for bot assistance.
  • Note: Review comments are made on code diffs or files, not on the PR overview.

Pause Incremental Reviews

  • Insert @coderabbitai: ignore in the PR description to halt the bot's ongoing reviews. Remove the line to resume.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 2

Commits Files that changed from the base of the PR and between e33c948 and 93c4c9c commits.
Files selected for processing (2)
  • src/main/java/net/wurstclient/WurstClient.java (2 hunks)
  • src/main/java/net/wurstclient/altmanager/Encryption.java (2 hunks)
Additional comments (Suppressed): 3
src/main/java/net/wurstclient/WurstClient.java (2)
  • 26-26: The new import net.wurstclient.altmanager.Encryption is used for the encryption folder migration functionality. Ensure that this class is correctly implemented and tested.

  • 148-161: This block of code checks if the XDG_DATA_HOME environment variable is set and not empty. If it is, it sets the encryption folder to a new location based on this variable. If the new encryption folder does not exist and the old one does, it calls the migrateEncryptionFolder method from the Encryption class to migrate the files. If the XDG_DATA_HOME environment variable is not set or is empty, it defaults to the old encryption folder location. This is a significant change in how the application handles file storage and could potentially impact users who have custom configurations or scripts relying on the old path. It's important to communicate this change clearly to users and provide instructions for manually migrating files if necessary.

src/main/java/net/wurstclient/altmanager/Encryption.java (1)
  • 14-18: The new imports java.io.File and java.nio.file.StandardCopyOption are necessary for the file migration process in the new migrateEncryptionFolder method.

src/main/java/net/wurstclient/WurstClient.java Outdated Show resolved Hide resolved
src/main/java/net/wurstclient/altmanager/Encryption.java Outdated Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 2

Commits Files that changed from the base of the PR and between 93c4c9c and e615088 commits.
Files selected for processing (2)
  • src/main/java/net/wurstclient/WurstClient.java (3 hunks)
  • src/main/java/net/wurstclient/altmanager/Encryption.java (3 hunks)
Files skipped from review due to trivial changes (1)
  • src/main/java/net/wurstclient/WurstClient.java

@Alexander01998 Alexander01998 added this to the v7.38 milestone Sep 15, 2023
@Alexander01998 Alexander01998 merged commit 5f14efc into Wurst-Imperium:master Sep 16, 2023
1 check passed
@Alexander01998 Alexander01998 added the status:merged This pull request has been merged, even if GitHub says otherwise. label Sep 16, 2023
@Alexander01998 Alexander01998 modified the milestones: v7.38, v7.37.1 Oct 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:AltManager Improves the Alt Manager. status:merged This pull request has been merged, even if GitHub says otherwise. type:enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants