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

Set config directory according to runtime platform #57

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jlmello57
Copy link

This pull request changes the definition of the path for persistent state and configuration files (SOCO_CLI_DIR) to derive from the output of platform.system(), rather than being fixed to an eponymous, dot-prefixed directory in the top level of the user's $HOME. This is a breaking change in that no mechanism is created to test for the presence of the former path and relocate data to the new path on platforms where this is changed.

Furthermore, on POSIX-compliant platforms it tests for the presence of the XDG_CONFIG_HOME environment variable and establishes the config directory there, with a fallback to the $HOME/.config directory as defined in the XDG Base Directory Specification.

On Microsoft Windows® systems, it places the directory inside the path defined by the %LOCALAPPDATA% environment variable, which is the vendor-specified path for all machine- specific application data belonging to the active user.

On all other platforms the former behavior is retained. Several individual definitions of this path were consolidated into the SOCO_CLI_DIR constant already present in the file, which itself was relocated—along with the attendant _confirm_soco_cli_dir method—earlier in the file, nearer to the other defined constants.

This commit changes the definition of the persistent state and
configuration files to derive from the output of [platform.system()][1], rather than being fixed to an eponymous, dot-prefixed directory in the top level of the user's `$HOME`.

Furthermore, on POSIX-compliant platforms it tests for the
presence of the XDG_CONFIG_HOME environment variable and
establishes the config directory there, with a fallback to the
`$HOME/.config` directory as defined in the [XDG Base Directory Specification][2].

On Microsoft Windows® systems, it places the directory inside
the path defined by the `%LOCALAPPDATA%` environment
variable, which is the vendor-specified path for all machine-
specific application data belonging to the active user.

On all other platforms the former behavior is retained. Several
individual definitions of this path were consolidated into the
`SOCO_CLI_DIR` constant already present in the file, which itself was relocated along with the attendant `_confirm_soco_cli_dir` method earlier in the file, nearer to the other defined constants.

Include updated documentation sections in README.md that relate
to where configuration and data are stored on various platforms,
and what to delete for a complete uninstall. Validate markup syntax
using NodeJS markdownlint module, and supply syntax-
highlighting keywords for fenced code blocks a la github-linguist.

[1]: https://docs.python.org/3/library/platform.html#platform.system
[2]: https://specifications.freedesktop.org/basedir-spec/latest/index.html
@pwt pwt self-assigned this Jun 19, 2023
@pwt
Copy link
Collaborator

pwt commented Jun 19, 2023

Thanks for the PR and apologies for the delay in getting to it -- I've been busy/travelling.

I'll take a look as soon as I can. I should note that it was a deliberate choice to have a consistent location across platforms, but I'm not resistant to revisiting this choice. I'll also need to think about the breaking change aspect -- I don't want to break stuff that users have already created.

@jlmello57
Copy link
Author

Thanks for considering the changes, @pwt. No major hurry on the review, I've got my hands full too. This is actually my first code contribution to an open source project, so I didn't expect it to be "smooth sailing," either. I understand your concern about the breaking change and had considered adding a method to test for the presence of the old config file path (easy enough since it's hard-coded and platform agnostic, too) and automatically copying it to the location defined in the new logic, but didn't want to add more complexity to the PR before gauging what your attitude was re: avoiding large scale changes vs. backwards compatibility.

I mocked up these changes because the jewelry store that I work at uses Sonos speakers to play the background music on the showroom floor as well as in the gallery rooms and while they do a great job, their control surface is not only rigid but difficult to adjust with subtlety. In the gallery rooms there's just a Windows PC running the POS/Inventory system and Sonos controller software, but it's pretty well locked down and soco-cli isn't able to create its folder in the User Profile directory. That's how I ended up making these modifications to point it at $Env:LOCALAPPDATA instead, as no extra permissions were needed for basic users to create new files/folders there. All of this is in service of cobbling together basic MPRIS functionality so salespeople can fiddle with the volume and track using the multimedia keys on the keyboard so as not to divert their attention from the customer.

Just out of curiosity, where would you stand on adding the ability to provide a signed integer as the argument to the volume commands to indicate a desired deviation from the current volume, whatever that may be? Maybe even with the ability to make such changes be logarithmic instead of linear by appending % to the signed integer? That would streamline most of what I'm doing and I'm inclined to think that I'm not the only one who would make heavy use of it.

Anyhow, I'll leave you to ponder all of this and wait for your comments on the matter, whenever you find the time to do so. Thanks for all the work you've put into this amazing tool to date, if I hadn't encountered it in the first place I never would have thought it possible to experiment with my own attempts at crafting a custom Sonos control surface. 👍🏼

@pwt
Copy link
Collaborator

pwt commented Jun 28, 2023

Thanks for the extra context. I hadn't considered that the home directory might not be writeable (I don't do much development on Windows). That's extra justification for making the change.

Just out of curiosity, where would you stand on adding the ability to provide a signed integer as the argument to the volume commands to indicate a desired deviation from the current volume, whatever that may be? Maybe even with the ability to make such changes be logarithmic instead of linear by appending % to the signed integer? That would streamline most of what I'm doing and I'm inclined to think that I'm not the only one who would make heavy use of it.

Does the relative_volume action already cover this use case? It's linear, not logarithmic, of course! There's also group_relative_volume for the grouped case.

@ajgrf
Copy link

ajgrf commented Aug 19, 2023

Does soco-cli actually store any configuration in its directory? I think it would be better to write to $XDG_STATE_HOME, which defaults to $HOME/.local/state.

@pwt
Copy link
Collaborator

pwt commented Aug 19, 2023

Does soco-cli actually store any configuration in its directory? I think it would be better to write to $XDG_STATE_HOME, which defaults to $HOME/.local/state.

It depends what you mean by 'configuration'. SoCo-CLI does store the following:

  • The cached speaker discovery list if that's in use
  • The command history for interactive mode
  • Aliases for interactive mode
  • Search results (for subsequent use)

I wanted any persistent state to be held somewhere obvious, so that it could be deleted without having to hunt for it -- I hate it when apps spray data in obscure places. I'm aware that the the choice of <HOME>/.soco-cli breaks convention.

However, @jlmello57 has identified a use case which SoCo-CLI definitely doesn't support with its current choice of directory. I'll incorporate a form of the PR (which is platform-independent) to fix this, and I apologise for my tardiness in doing so.

@ajgrf
Copy link

ajgrf commented Aug 19, 2023

The only thing from that list I would call configuration are the aliases, a feature that I didn't even realize soco-cli had. Everything else is either cache or state, which should technically all be stored separately to comply with the XDG standard, although that does seem like overkill in the case of soco-cli. I just didn't want you to go to the effort of migrating directories now, only to realize later that you should have chosen a different directory.

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

Successfully merging this pull request may close these issues.

None yet

3 participants