PrismLauncher
PrismLauncher copied to clipboard
Add ability to create global folders
So I wanted to give a try at implemented a feature that I (and I think many others) have wanted for some time now. (See #38, #1027 )
It's my first time working with QT and this code base, so any feedback is welcome!
Here's my plan:
- [x] Add 3 checkboxes in the instance settings (under Settings->Miscellaneous->Global folders) to enable/disable the global directory for screenshots/saves/resource packs.
- If the original directory is not empty, give an error.
- If the original directory does not exist or is empty, create a symbolic link to a hardcoded directory. (screenshots, saves or resourcepacks) Maybe also add a tooltip/warning that when disabling the global directory, the items will stay inside the global directory and not be moved to the instance specific directory.
- [x] Add a warning to the Screenshots, Worlds and Resource packs settings when the global directory is enabled that the items you're looking at are shared across instances.
- [x] Allow customizable global directory.
- When checkbox is checked, an input field can be enabled which defaults to the default global directory, but can be customized to an other location.
- [x] Test: When browsing, my file picker stops at the first encountered hidden directory. So in my case, it opens at
/home/naomias it can't find the.local/. My filepicker has an option to enable hidden files, but that isn't saved. This is probably a bug with my setup, and not necessarily qt?
- [x] Allow non-empty directories to be turned into global directories
- If a directory is not empty, copy the contents to the global directory.
- If the contents already exists, ask the user what to do. (Similar to Windows File Explorer? Is there already a UI/component for this, or should I create one?)
- If everything could be successfully moved, delete instance directory and create symlink.
Nice to haves (most likely a future pull-request):
- Add global default checkboxes.
- Set the global folder for each instance.
- Allow setting other directories to be shared, for example, schematics.
- Basically a convenient way for non-technical people to create symlinks?
- A scroll-list with your created symlinks under Settings->Miscellaneous->Global folders
- The merging of existing files could be handled the same way the other directories are handled.
- Global server list
- This is most likely a whole other feature, since you'd have to merge the servers.dat but it could be fun challenge to try and tackle some day.
Some questions I still have:
- I wanted to separate the logic of applying the settings from the ui component, hence the
MinecraftInstance::applySettingsroutine. But I'm not sure if the current implementation is correct/desired. Ideally you'd want the settings to be applied immediately, so you can get a relevant error as soon as possible. But I was told that would require a decent overhaul, since you'd have to change when all settings are applied to keep everything consistent. So what is the correct solution for the mean time? - How much should I mention the existence symbolic links to the user? I doubt the average user knows what they are and how they work.
- I probably messed up the UI. Any tips on working with it?
I noticed that you marked it as being ready so I did a bit of code review. havn't tested yet.
You using a per instance setting (which would be desired behavior), but your not providing a global default setting.
I think that this should be changed so that there is a global default settings for the folders with an instance setting that overrides the global defaults to use the instance's settings. Look at the java override setting for an example of what I mean.
Yeah I was planning on adding that eventually, but I haven't exactly figured out how I want that to look. As in, if you enable the default checkbox for the global saves folder, and you have a lot of instances with "New World", you might get a lot of FileConflictDialogs thrown at you. If you think that's not a problem, I'll go ahead and implement it, else I've got to come up with a good solution.
I noticed that you marked it as being ready so I did a bit of code review. havn't tested yet. You using a per instance setting (which would be desired behavior), but your not providing a global default setting. I think that this should be changed so that there is a global default settings for the folders with an instance setting that overrides the global defaults to use the instance's settings. Look at the java override setting for an example of what I mean.
Yeah I was planning on adding that eventually, but I haven't exactly figured out how I want that to look. As in, if you enable the default checkbox for the global saves folder, and you have a lot of instances with "New World", you might get a lot of FileConflictDialogs thrown at you. If you think that's not a problem, I'll go ahead and implement it, else I've got to come up with a good solution.
Treat an instance that doesn't have the override setting set (as in the instance was created before the setting was introduced) as having the override setting on and all global folders disabled
This looks good now, I dont have the time now but it needs some testing before final approval
I noticed that you marked it as being ready so I did a bit of code review. havn't tested yet. You using a per instance setting (which would be desired behavior), but your not providing a global default setting. I think that this should be changed so that there is a global default settings for the folders with an instance setting that overrides the global defaults to use the instance's settings. Look at the java override setting for an example of what I mean.
Yeah I was planning on adding that eventually, but I haven't exactly figured out how I want that to look. As in, if you enable the default checkbox for the global saves folder, and you have a lot of instances with "New World", you might get a lot of FileConflictDialogs thrown at you. If you think that's not a problem, I'll go ahead and implement it, else I've got to come up with a good solution.
Love the concept here, would be a huge time saver. Thinking of ideas of how you could handle the initial migration of items: If you were to come across a naming conflict (or you could just provide this as an option in general - it may help with organisation) you could look at prefixing the resulting file with the original instance name as such "Instance_A_Screenshotxxxxx.png"
I've found some time to work on it again! :D
I decided to pivot a bit and simplify it a bit by only adding shared folder to instances, and not adding global shared folders. I might do that later, but as a first step this is probably better.
I've moved some functions to FileSystem.h, but I think they don't belong there since they use a UI? Not sure where else to put them though since they are generic and thus also don't really belong in MinecraftInstance.cpp