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

UICommon: Change default user directory location to AppData on Windows #10708

Merged
merged 3 commits into from
Jan 16, 2023

Conversation

OatmealDome
Copy link
Member

@OatmealDome OatmealDome commented May 29, 2022

This PR moves the user folder to %appdata%\Dolphin Emulator to minimize the chances that external software can interfere with Dolphin when accessing it (shoutouts to OneDrive and various anti-virus scanners).

Here is how the user directory is determined on Windows with my changes:

  1. If the file portable.txt or the registry value HKCU\Software\Dolphin Emulator\LocalUserConfig exists, a folder named User next to the executable will be selected as the user directory.
  2. If Dolphin finds a registry value in HKCU\Software\Dolphin Emulator\UserConfigPath, it will use that as the user directory. This functionality dates back to at least 4.0 stable.
  3. NEW: If the old user directory at %userprofile%\My Documents\Dolphin Emulator exists, Dolphin will use that as the user directory.
  4. NEW: Otherwise, %appdata%\Dolphin Emulator will be used. In addition, for backwards compatibility, the registry value HKCU\Software\Dolphin Emulator\UserConfigPath will be set to that path to ensure that Dolphin builds made before this PR will continue to use it as the user directory (see step 2).
  5. If neither %appdata% or %userprofile%\My Documents exists, Dolphin will act like portable.txt exists.

I do see one flaw with my backward compatibility approach: if %userprofile% changes, the old path in UserConfigPath will continue to be used, which might cause the user to think their data is gone. I personally don't think this is worth handling, since Microsoft specifically warns against moving the user profile in Windows 10 and above.

@MayImilae
Copy link
Contributor

If neither %appdata% or %userprofile%\My Documents exists, Dolphin will act like portable.txt exists.

This will only be triggered in the event of the system folders are missing, right? Is there any known scenario where this could be encountered?

@OatmealDome
Copy link
Member Author

@MayImilae Yes. I can't think of any scenario where that will happen, since those are standard folders on Windows... But the code is there in case it does.

@shuffle2
Copy link
Contributor

shuffle2 commented May 30, 2022

be aware %appdata% (and %localappdata%) are not really meant for / commonly used for files which users will be directly interacting with (browsing to/opening in a different application besides dolphin). but maybe we actually want this, idk - just mentioning it.

also, %appdata% gets roamed with the user account (i.e. if you login to the same account on different machines, the data gets copied), whereas %localappdata% doesn't have this property. The latter may or may not be better for dolphin, idk.

also, i think that if the registry path is used, it could be better to move all the files to the new location and update the registry to the new path. The older builds would still work, right?

@OatmealDome
Copy link
Member Author

@shuffle2 Any better folder that isn't %appdata%?

My thoughts behind choosing %appdata% over %localappdata% was that people would probably want to have their saves and stuff go with them if they have a roaming user profile. (Wouldn't %userprofile%\Documents usually be roaming in these environments. too?)

I'm hesitant to do any sort of moving of the user directory because of File:Rename()'s potential to spuriously fail on Windows. I'd rather make as few changes as possible, and just using the old folder if it exists is probably the simpler way.

@JosJuice
Copy link
Member

If we're going to do this, I really think we should have something in the GUI that lets the user access the files in the user directory without needing to have the path memorized, as users won't know the new path and all guides they find will mention the old path. My preference would be to have a menu item that opens the user directory, but May argued against this in PR #10526. May's suggestion was to instead have import and export buttons like on Android, but I know from experience with Android users that users really prefer being able to directly access the folder for tasks like managing saves and adding texture packs.

@Zopolis4
Copy link
Contributor

While I think this is a matter of opinion, I'm strongly against moving the user directory to AppData, as that's where configurations go to die. The average user will not know where to find the dolphin configuration files, and I've had trouble in the past with dolphin finding hidden folders or files within them. A change like this will lead to lots of confusion with old guides, and I don't think the benefit of the casual user not possibly having to set an antivirus scan as opposed to the causal user having to enable hidden folders or directly type in the path or similar. OneDrive is annoying, and I currently have no writable documents folder because of it, but the portable solution works fine and I would much prefer it than having yet another AppData folder.

@MayImilae
Copy link
Contributor

as users won't know the new path and all guides they find will mention the old path.

A change like this will lead to lots of confusion with old guides,

Our guides will be updated. Other guides for forks and what not are not our responsibility. We will communicate this change in a progress report, but it's on them to update. I'm confident that they will.

My preference would be to have a menu item that opens the user directory, but May argued against this in PR #10526. May's suggestion was to instead have import and export buttons like on Android, but I know from experience with Android users that users really prefer being able to directly access the folder

To be specific, I am against a shortcut to the global user directory in the paths tab as #10526 implemented it. However, a menu bar item tucked away under tools would probably be alright I think. I honestly don't think we need it though. Personally I'd much prefer more robust in-gui options. At least there we can build in some safeties and warnings and stuff. Once a user accesses text files directly, we can't do anything to help them.

While I think this is a matter of opinion, I'm strongly against moving the user directory to AppData, as that's where configurations go to die.

That depends on the type of user you are targeting. For the majority of our userbase, yea, they won't ever go to AppData. But they would never go to our Global User Directory either, as we provided no tools to direct them to it within the gui (and that was not by accident). By doing it that way, those that have found our global user directory are usually those industrious enough to google Dolphin's setting location and found our guides, and that class of user is probably going to be computer literate enough to be careful in there. If we switch to AppData, that kind of user won't be affected. They'll google it just the same, find our guide which will be updated, and then go to a folder. Exactly the same.

Of course, if we start adding gui links to the global user directory, THAT's where things really start changing, as we're bringing new users into the directory who wouldn't go otherwise. But I've talked about that before in #10526.

@JosJuice
Copy link
Member

JosJuice commented May 30, 2022

Our guides will be updated. Other guides for forks and what not are not our responsibility. We will communicate this change in a progress report, but it's on them to update. I'm confident that they will.

I think you're underestimating how many random forum posts and YouTube videos there are. The majority of those will never be updated. New ones will of course be written, but it will take a good while for them to reach a critical mass.

To be specific, I am against a shortcut to the global user directory in the paths tab as #10526 implemented it. However, a menu bar item tucked away under tools would probably be alright I think.

Ah, okay. That sounds reasonable to me.

@iwubcode
Copy link
Contributor

iwubcode commented May 30, 2022

I'm in a similar situation as @Zopolis4 . I've had to deal with other apps writing to appdata and I don't really like it. I never remember where it is. Sure I can look it up but it takes away from the experience.

As for the idea that users don't need to go there, I don't agree at all. Want to change a theme? User folder. Want to add a texture pack or dit pack? User folder. Want to dump screenshots or look at dumped textures? User folder. Want to manage input profiles? User folder. Yes, many of these can be overridden but the default is the user folder root.

I'd be more open to this suggestion to change the user folder once our UI is more user friendly. I do agree with May that there's much we can do to enhance the experience. However, without that in place, I'd personally say wait to merge this.

As an aside, I do know this is "best practice" now with software but I'd be curious to hear more about this:

to minimize the chances that external software can interfere with Dolphin when accessing it (shoutouts to OneDrive and various anti-virus scanners)

as that's the first I've heard of that issue.

@JosJuice
Copy link
Member

I hear about that issue all the time. You run into it just occasionally on the forums (I think it was actually more common in the past that it is now), but fairly often on the unofficial Discord.

@OatmealDome
Copy link
Member Author

OatmealDome commented May 30, 2022

@Zopolis4 @iwubcode Any better suggestions other than %appdata%? I don't think the current My Documents situation is tenable because of how hard Microsoft is pushing OneDrive in Windows 10 and 11.

I'm willing to add a menu item in Tools to open the user directory to minimize the impact of this change, but maybe in a separate PR so this doesn't get held up?

@iwubcode
Copy link
Contributor

iwubcode commented May 30, 2022

I hear about that issue all the time. You run into it just occasionally on the forums (I think it was actually more common in the past that it is now), but fairly often on the unofficial Discord.

As someone who doesn't use OneDrive, what's the issue? I must just miss these issue reports because I haven't seen them.

@OatmealDome - I don't have a problem with %appdata% per se, I just find it tedious to find. As someone who goes to the user folder quite a bit and also someone who sees a lot of reports around features which use it, I hate to lose that ease-of-access. I mean, I know that I won't be impacted but still feel like it's worth raising.

I'm willing to add a menu item in Tools to open the user directory to minimize the impact of this change, but maybe in a separate PR so this doesn't get held up?

A separate PR is fine, however, I don't really think a menu item in tools is a good user experience. In my opinion every action should be thought of separately. If a user wants to add a theme, they aren't going to say "Oh I should go to the 'tools' menu and open up the user directory to add it". Likewise, for input profiles, texture packs, etc. These menus need to be redesigned to allow simple drag/drop and/or expose a way for users to be taken right to the directory via a link to modify what they need to modify (or have the UI be fleshed out so the operations are intuitive and feasible inside Dolphin).

@JosJuice
Copy link
Member

As someone who doesn't use OneDrive, what's the issue? I must just miss these issue reports because I haven't seen them.

Dolphin shows error messages about stuff like not being able to save the SYSCONF file, I think. I'm not sure if it's consistent or random.

@Zopolis4
Copy link
Contributor

Zopolis4 commented May 30, 2022

I've used a lot of forms of OneDrive, and it does mess with the documents folder. I've never seen it mess with saving to it, it just messes with the location. I don't think AppData is better tho, and it's pretty easy with a bit of work to resolve the OneDrive issues. Also, if we're talking about putting it in roaming, that seems just like a lot more work to use a less publicised way of synchronising folders.I do feel that dolphin should tell you when there's no documents folder, because it took me a couple tries to figure out that it was just making a portable folder.

@OatmealDome
Copy link
Member Author

@Zopolis4 How exactly would one "resolve the OneDrive issues"? I don't think just leaving it in My Documents and popping up an error message when OneDrive decides to mess with us is a very user-friendly solution, nor is it a good first impression to new users. Everything should "just work".

Roaming vs Local AppData only applies when there is an Active Directory domain set up, which is most commonly found in enterprise environments. (Besides, I don't think many people are running Dolphin in such an environment...)

Agreed on the last point, though I think it would be mitigated with the proposed menu item in Tools. I'm surprised you actually managed to run into that case.

@Zopolis4
Copy link
Contributor

@Zopolis4 How exactly would one "resolve the OneDrive issues"? I don't think just leaving it in My Documents and popping up an error message when OneDrive decides to mess with us is a very user-friendly solution, nor is it a good first impression to new users. Everything should "just work".

Roaming vs Local AppData only applies when there is an Active Directory domain set up, which is most commonly found in enterprise environments. (Besides, I don't think many people are running Dolphin in such an environment...)

Agreed on the last point, though I think it would be mitigated with the proposed menu item in Tools. I'm surprised you actually managed to run into that case.

I just don't think there are that many occurrences of onedrive silently messing with the Documents folder. I can't find any from a quick look at the bug tracker, and if onedrive does block writes or reads then that is probably something we can report to Microsoft in some capacity.

@Zopolis4
Copy link
Contributor

as users won't know the new path and all guides they find will mention the old path.

A change like this will lead to lots of confusion with old guides,

Our guides will be updated. Other guides for forks and what not are not our responsibility. We will communicate this change in a progress report, but it's on them to update. I'm confident that they will.

I'm not worried about dolohin's built in guides, although those have been behind dolphin for a couple years now (they don't mention .rvz, seem to deny the possibility of dualcore existing, and so on. My main worry is indeed the random forum posts and youtube videos and guides-- those users following them, either for riivolution ot texture packs or otherwise do want to modify the files in the user directory, and will have significant difficulty discerning the change to AppData. If a "casual user" uninstalls dolphin, it is highly unlikely they will know to go to the AppData folder, as opposed to knowing to delete the obvious Dolphin Emulator folder in documents. I think changing to AppData will make more problems then it solves.

@Zopolis4
Copy link
Contributor

I think Dolphin is also breaking with precedent here-- I've never used an application that had a folder in AppData which it would ever require you to even go near, and plenty of dolphin users use texture packs or mods.

@AdmiralCurtiss
Copy link
Contributor

AdmiralCurtiss commented May 30, 2022

I just don't think there are that many occurrences of onedrive silently messing with the Documents folder. I can't find any from a quick look at the bug tracker, and if onedrive does block writes or reads then that is probably something we can report to Microsoft in some capacity.

Dropbox and other things have the exact same issue -- it happens because they need to open any newly created or modified files to upload them to their server. If while they're doing that we try to modify or delete the file, we run into a conflict, and we're kinda bad about having fallbacks for those cases. This isn't a 'bug' to be reported, it's just how those programs work fundamentally.

For what it's worth, I don't think this is the correct solution, but it's certainly a simple and effective one.

@OatmealDome
Copy link
Member Author

@Zopolis4 If I search for "OneDrive" on the unofficial Discord, I get ~500 hits, so it isn't an uncommon issue.

I've been complaining a lot about OneDrive in particular in this PR, but there's also the fact that anti-virus software are starting to block applications from accessing My Documents (probably because it thinks we're ransomware). Avast in particular I know likes to interfere with Dolphin.

As for applications not storing things in AppData, just off the top of my head: yuzu, Ryujinx, and Minecraft all store stuff in AppData and users often mess with what's stored there.

@AdmiralCurtiss What would be a correct solution?

@Zopolis4
Copy link
Contributor

Zopolis4 commented May 30, 2022

@Zopolis4 If I search for "OneDrive" on the unofficial Discord, I get ~500 hits, so it isn't an uncommon issue.

there's also the fact that anti-virus software are starting to block applications from accessing My Documents (probably because it thinks we're ransomware). Avast in particular I know likes to interfere with Dolphin.

In my opinion, Avast is a flaming mess and the only solution to avast issues would be to tell the user to uninstall it.

As for applications not storing things in AppData, just off the top of my head: yuzu, Ryujinx, and Minecraft all store stuff in AppData and users often mess with what's stored there.

With Minecraft, it's not explicitly supported nor encouraged. The AppData folder is hidden for a reason.

@AdmiralCurtiss
Copy link
Contributor

For antivirus nonsense, there probably is no correct solution, and moving the userdir to a folder where they don't care about us is the best we can do.

For temporary file locking from OneDrive and friends, I suspect we'd have a much better time if we used the Win32 file API to actually state our intent with the files we open and correctly handle error codes. The most common issue I've seen is the old "failed to rename fst.bin" that we worked around with #9250, when the much better (but admittedly more refactoring-heavy) solution would have been to open the temp file with the DELETE access right so we can actually rename it to its correct name before closing the handle (and thus telling OneDrive it's okay to touch the file now). And this isn't the only place where such a pattern would be better than what we're currently doing.

@shuffle2
Copy link
Contributor

shuffle2 commented Jun 1, 2022

as i've stated in #9023 the current workaround is just a hack and is not really addressing the problem. identify and solve the actual problem instead of just guessing.

note the open/share masks is not just intent but really modifies how the kernel object manager allows apps globally to interact with the file object.

@Zopolis4
Copy link
Contributor

Alternatively, what if we just shut down onedrive when dolphin started and re-opened it when it ended? I can't find a way to pause syncing on the command line, so this is the best alternative I could think of. The only issue with this would be that it would block users from accessing any files they had set as only available online while dolphin is running.

@mbc07
Copy link
Member

mbc07 commented Sep 20, 2022

Alternatively, what if we just shut down onedrive when dolphin started and re-opened it when it ended?

Interfering with other applications doesn't seem like a good idea. Especially if it's for working around an issue I think we should handle on our side.

Anyway, have you considered using %UserProfile%\Saved Games instead? That system folder exists since Windows Vista, isn't hidden by default (unlikely %AppData%) and also isn't synced to OneDrive by default either. Sure, Windows 10 and 11 won't show it on the Explorer's navigation pane by default (you need to enable "show all folders" on Folder Settings), but it's still easily accessible via This PC => Local Disk => Users => <user folder>, no need to enable "Show hidden files" or similar...

@AdmiralCurtiss
Copy link
Contributor

AdmiralCurtiss commented Sep 20, 2022

^ FYI that's a known folder with a GUID of 4C5C32FF-BB9D-43b0-B5B4-2D72E54EAAA4, since its exact path may be localized.

@Zopolis4
Copy link
Contributor

Alternatively, we could just pssuspend onedrive, which appears to have the same results as pausing it: https://superuser.com/questions/898838/pause-onedrive-sync-using-cmd

@AdmiralCurtiss
Copy link
Contributor

AdmiralCurtiss commented Sep 22, 2022

Stop. Dolphin is not going to interfere with other running programs, at least not intentionally and automatically.

@OatmealDome
Copy link
Member Author

I asked for opinions on Discord (dolphin-development), and it seems like the majority of people are in favor of AppData as long as there is a GUI option to open the user directory.

I also asked about Saved Games and the opinions skew negative, mainly because it might be affected by Windows shenanigans, and also because putting it there might not make much sense since Dolphin stores things that aren't save data in the user directory.

So, given this, I think I will leave this PR as AppData. DolphinQt changes will be in a separate PR.

@delroth
Copy link
Member

delroth commented Jan 16, 2023

+1 to merging this once the new menu option to open the user dir is implemented and merged.

@shuffle2
Copy link
Contributor

shuffle2 commented Jan 16, 2023

I still think having the data roam doesn’t make a lot of sense (you can assume hardware is different for example), but I’m fine with it (realistically how many people will run into this - hopefully no one 😀). Note the rename issue previously brought up has since been fixed.

@MayImilae
Copy link
Contributor

LGTM once we have the menu bar entry to reach it.

Copy link
Contributor

@MayImilae MayImilae left a comment

Choose a reason for hiding this comment

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

Menu bar change is merged, so LGTM.

@delroth delroth merged commit bb64b0e into dolphin-emu:master Jan 16, 2023
@Avasam
Copy link
Contributor

Avasam commented Jan 17, 2023

to minimize the chances that external software can interfere with Dolphin when accessing it

As a user. I also appreciate not polluting my User Documents folder 🙏 (which many older games like to do with savefiles...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

10 participants