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

tilp: Use g_get_user_config_dir() to get the config directory #75

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

aarondill
Copy link

This allows the XDG base specification to be followed, defaulting to LocalAppData on windows.

It also deprecates the old config path, using the same logic as the Windows deprecated config (which has been extracted to a helper).

values:
This section describes the format of the tilp.ini config file which is
in the configuration directory of the user ($XDG_CONFIG_HOME or
LocalAppData on windows). The file is separated into several sections
Copy link
Owner

Choose a reason for hiding this comment

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

After merging the PR, a bit later, I'll fix "windows" to "Windows", and add a note about the automatic upgrade from the config file at the old location. Unless you beat me to rewriting the commit :)

Copy link
Author

@aarondill aarondill Mar 12, 2024

Choose a reason for hiding this comment

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

I fixed this in 0b06ebc 😃

@debrouxl
Copy link
Owner

With backwards compatibility handled, that looks entirely reasonable. Just one nitpick about the documentation.

@aarondill
Copy link
Author

@debrouxl any update on this PR?

@debrouxl
Copy link
Owner

Not really, I still haven't tested it on my side... sorry.

@aarondill
Copy link
Author

@debrouxl thats alright, I just wanted to make sure there was nothing more on my side to be done

@debrouxl
Copy link
Owner

Patch applied locally, which is a start :)

@aarondill
Copy link
Author

@debrouxl have you had the chance to continue testing?

@adriweb
Copy link
Contributor

adriweb commented May 17, 2024

I can probably check on macOS too I suppose.

@adriweb
Copy link
Contributor

adriweb commented May 18, 2024

Looks like it's half OK for me here on mac OS as there's still something weird.

Before your PR, get_config_path returned /Users/adriweb//.tilp.
With your PR, it returns /Users/adriweb/.config/tilp.ini

So far, so good.

The weird thing is that with your PR, it thinks the "old path" is /Users/adriweb/tilp which isn't the same as the actual old (pre-PR) path. So I assume it would never actually find it to migrate old configs. Here's a screenshot from a debugging session (your patches applied):

CleanShot 2024-05-18 at 11 52 52@2x

the dot was accidentally removed while copying. The leading dot has been re-added now
@aarondill
Copy link
Author

@adriweb This should be fixed in dbff573!

@adriweb
Copy link
Contributor

adriweb commented May 18, 2024

Right. Looks good!

@aarondill
Copy link
Author

@debrouxl with that unix issue handled, have you had a chance to get this running and tested?
are there any other issues you've noticed?

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