TASEmulators / desmume

DeSmuME is a Nintendo DS emulator
http://desmume.org
GNU General Public License v2.0
3.02k stars 551 forks source link

LINUX - Support XDG Base Directory Specification #351

Open eddsalkield opened 4 years ago

eddsalkield commented 4 years ago

Currently, all desmume files are stored in $XDG_CONFIG_HOME as a consequence of this patch. Although it is good that the user's home directory is not being littered with files, the XDG Base Directory Specification specifies that:

There is a single base directory relative to which user-specific data files should be written. This directory is defined by the environment variable $XDG_DATA_HOME.

In essence, the standard specifies that configuration files should live separately to data files (at $XDG_CONFIG_HOME and $XDG_DATA_HOME respectively). Such data files include basically all non-config files - save files, cheats, save states, etc.

To allow seamless migration to the new standard locations, desmume should load data files preferentially from the new location, falling back to the legacy location where required. However, since the current method for determining a file location returns a directory independently of the specific file, the logic to fall back to the legacy location is hard to implement.

I propose that we add new path variables, on Linux only, to PathInfo; we duplicate the existing ones to accommodate both new and legacy locations. For example, pathToRoms now references the new location, and legacyPathToRoms references the traditional location.

We add an additional (optional) bool legacy argument to PathInfo::SwitchPath. This serves no purpose on Windows, but otherwise allows the program to specify whether the new or legacy location is required.

The logic to select between the two paths can be implemented in the code that calls PathInfo::getpath (or similar). When loading a file, the new location is preferred, falling back to the legacy location when required. When saving a file, the behaviour will be always to choose the new location, seamlessly migrating old users to the new system. For clarity, perhaps a one-time dismissable pop-up should inform users of the change (replies on forum threads such as this one may also help).

We can additionally take advantage of the separation of the categories of the paths to further tidy things up into subdirectories of $XDG_DATA_HOME/desmume, such as ./roms and ./battery.

I am willing to write a patch for the above (and maybe clean up some of the path code a little while I'm at it), but I have a few questions:

Some of the nitty-gritty of the current operation of the save system is explained, partially for future me's sake, below:

The various paths from which files are loaded are initially determined and set by function PathInfo::ReadPathSettings. This function invokes PathInfo::ReadKey for each key (type of saved file, e.g. BATTERYKEY) defined inpath.h. On Windows, this reads from an INI file to determine the kind value of the key, falling back to the default path. On all other platforms, the behaviour is instead to always use the default path. Since the default path (pathToModule) is determined in function PathInfo::LoadModulePath to be within $XDG_CONFIG_HOME, all files are currently stored there.

zeromus commented 4 years ago

Do not litter the code with legacy support. After umpteen years of various legacy revisions the code will be more legacy support than proper logic.

Whatever path management one person likes is not what another person will like. Windows and linux both are a wasteland of disparate adhoc approaches. Nobody needs to touch this unless they are prepared to take over maintenance of the GTK port. Seven people have touched it within the past 12 months and none of them are you. There is no need for a hit and run commit which is more or less net neutral quality level. If you are willing to make a commitment to the GTK port, then you can do this to mark your turf. If this is all you have in mind, please don't bother.

I don't understand "the current method for determining a file location returns a directory independently of the specific file, the logic to fall back to the legacy location is hard to implement." There should pretty much just be rom dirs, config dirs, and any other dirs. You would make config go to your XDG config location and everything else go to the XDG data location.

eddsalkield commented 4 years ago

Thanks for your quick response, and for clarifying that batteries are indeed save files. Please allow me to reply to your points, and to make the case that this revision would bring not only additional standardisation, but also an opportunity to make the code cleaner and more readable.

Whatever path management one person likes is not what another person will like. Windows and linux both are a wasteland of disparate adhoc approaches.

Although I agree that Linux is a wasteland of an adhoc approach, it need not be this way. My proposed solution would not simply tack on yet another opinionated approach - it would actually give the users greater control over where the files should be stored, in the way users currently expect. This is because the XDG Base Directory specification specifies that the locations of user-specific program files should be placed in locations relative to environment variables set by the user. In this manner, it solves the "everyone likes their files in a different place" problem without having to have additional checks in the code, and explains in part why basically every new Linux project conforms to the standard. Linux users expect their programs to follow this standard. Moreover, it seems that historically there was intention to migrate to this standard - the patch I referenced is a partial implementation.

Seven people have touched it within the past 12 months and none of them are you. There is no need for a hit and run commit which is more or less net neutral quality level.

I now understand that, historically, there have been disparate, unmanaged commits that have lead to legacy code strewn throughout the codebase. However, I disagree, for the reasons above, that this change is "more or less net neutral quality level". This change not only brings desmume into alignment with what users of the GTK port would expect in terms of save files, but would also provide an opportunity for restructuring the path handling code into something much more sane. For example, we can abandon the skeleton of INI reading still present within the Linux program flow.

This also gives us an opportunity to make the data directory much more clean through separation into different subdirectories, as described above. This comes essentially for free, since desmume already has internal distinctions for paths of different kinds.

I don't understand "the current method for determining a file location returns a directory independently of the specific file, the logic to fall back to the legacy location is hard to implement."

Perhaps I could have stated this more clearly as "the logic to fall back to the legacy location is hard to implement while maintaining backwards compatibility". Although it is true that we could simply swap over to the XDG standard with no regard for current users, this would break compatibility with any users on the current version of the emulator who would then be expected to move their files to the new location to continue playing on the emulator. This seems like a terrible user experience.

From where the code base currently is, we are fundamentally left with three choices:

Supposing that neither of the first two choices are satisfactory, we must support the legacy path handling method, at least for a while, in future releases alongside the XDG standard path handling method. This requires logic dependent on the existence of files, not just directories; that $XDG_DATA_HOME/desmume/ exists does not imply that no save files are present within $XDG_CONFIG_HOME/desmume/, or vice versa.

At the minimal cost of an additional wrapping function to check which location is correct, we can seamlessly migrate users to the new standard without them noticing. Upon a file save, the new location is used. Upon a file load, the new location is used, falling back to the legacy location where required. Whenever a player saves a new file, the newer file overrides the old one.

Furthermore, through following this standard, users that are wedded to the legacy version can remain with their files still in the currently used directory by setting $XDG_DATA_HOME to ~/.config.

zeromus commented 4 years ago

I do not agree that "users can specify whatever paths they want in a new way" is materially different from any other random-ass approach to handling paths.

I do not agree that the second choice is unsatisfactory. I do not care if the user experience is terrible. The developer experience is more important.

I still don't understand "the current method for determining a file location returns a directory independently of the specific file, the logic to fall back to the legacy location is hard to implement." It's the part about "specific file" I don't understand. Config files and save files use the same logic, so they can't be split into separate directories? OK fine whatever, make the logic parameterized by the specific file.

I still oppose a hit and run commit. I am optimizing for creating less mess for a real, committed linux maintainer to clean up. Real, committed linux maintainer can do whatever he wants in whatever way he wants.

eddsalkield commented 4 years ago

I do not agree that "users can specify whatever paths they want in a new way" is materially different from any other random-ass approach to handling paths.

The material difference from the current state of things at the moment is obvious: the current approach to handling paths only partially implements the options that Linux users expect, and confusingly places data files in places that they should not go. This causes issues for people who, for example, expect to be able to back up their configuration files between machines, and end up overwriting clashing save games in the process. The material difference from most other approaches for allowing the users to specify the paths is, as I mentioned, that the Linux community has standardised around the XDG spec. There is no need for ad-hoc approaches to path handling any more, since an agreed upon and technically sound standard exists. This should be a non-problem.

As I mentioned, assuming you do not care about the end user experience in the least, then there is indeed no logic that needs to be implemented to handle files in a special way. This, however, seems incredibly naive; most open source projects do not throw their users under the bus in code overhauls. Let me explain further why a mechanism that implements the new location handling in a user-friendly way requires additional logic.

Suppose that a user is loading a save file. Currently, the emulator calls PathInfo::getpath (or similar), which returns the directory in which the file is saved, which (through some convoluted logic) always returns $XDG_CONFIG_HOME/desmume/. We could, as you suggest, merely change this to return $XDG_DATA_HOME/desmume/ and be done with it. But now any users who have files saved in the previous location can no longer access them.

For the user experience to not be terrible, we require a check to determine whether the user is using the new or the legacy location. The only sensible way to implement this is on a per-file basis, since the existence of save files in the new location does not imply that no save files are in the legacy location. Therefore, which path we require depends upon where an individual save file is stored. This is at odds with the current implementation, which as aforementioned, always returns the same path for every call to PathInfo::getpath. We require instead a wrapper function to resolve the location of the file when required. The cost of this wrapper function to future maintainers will be negligible in comparison to the spaghetti that could be removed for its adoption.

I still oppose a hit and run commit. I am optimizing for creating less mess for a real, committed linux maintainer to clean up. Real, committed linux maintainer can do whatever he wants in whatever way he wants.

I understand that you don't want legacy code littered around the place, making somebody's life harder in the future. But as I mentioned before:

This change not only brings desmume into alignment with what users of the GTK port would expect in terms of save files, but would also provide an opportunity for restructuring the path handling code into something much more sane. For example, we can abandon the skeleton of INI reading still present within the Linux program flow.

Why, instead of judging new proposals based on their merits, are you hostile to new contributors except those willing to become lifelong maintainers from the get-go? If a patch brings merit to the project and also cleans up the codebase in the process, on what grounds would your hypothetical future maintainer object?