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

introduce localization for Music dir on Linux #6

Merged
merged 5 commits into from
Nov 28, 2022

Conversation

DistantThunder
Copy link

This fixes the program not being able to find localized Music directories on XDG Linux systems

@HeIIow2
Copy link
Owner

HeIIow2 commented Nov 25, 2022

Hi,
thanks for the amazing pull request. <33

If you don't mind changing up a couple details, I'd appreciate it, else I would do that.

  1. To print something to the command line please use logging. For you're purpose creating a new logger would be appropriate. Create a new logger before SEARCH_LOGGER on line 31 and then use the logger instead of print. I leave the naming up to you.
  2. In line 9 USER_XDG_DIR_FILE = os.path.expanduser("~/.config/user-dirs.dirs") please refactor the path to use os.path.join("~", ".config", "user-dirs.dirs") if that works. I wanna void possible exceptions that could be thrown on different platforms I cant test on.

Important: This pull request is even though I criticized it really valuable. Thanks you soooo much for that :3

(please excuse my inability to really know how this all works, this is my first experience with collaborations)

Copy link

@Kevin-Gruber Kevin-Gruber left a comment

Choose a reason for hiding this comment

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

Note: this configuration file is not only related to localization. A user with the English local may still want to use a custom music folder.

I'd recommend you use pathlib instead of os.path, as os.path is more of a lower level utility than pathlib, and in this case pathlib makes more sense to me. But os.path has already been used here.

I'd recommend placing all the code in this module in one or more functions, as this is executed on import (only the first one I think). Which is a bad practice, prone to bugs, and not necessarily what you want. But again, not in the scope of this PR as the problem was already there. However you could at least delete the global constant USER_XDG_DIR_FILE, as its scope should not exist outside this module.



USER_XDG_DIR_FILE = os.path.expanduser("~/.config/user-dirs.dirs")

Choose a reason for hiding this comment

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

~/config is default, but not standard, if you want to be XDG compliant, you should follow XDG_CONFIG_HOME if set.

src/music_kraken/utils/shared.py Outdated Show resolved Hide resolved
src/music_kraken/utils/shared.py Outdated Show resolved Hide resolved
src/music_kraken/utils/shared.py Outdated Show resolved Hide resolved


USER_XDG_DIR_FILE = os.path.expanduser("~/.config/user-dirs.dirs")

Choose a reason for hiding this comment

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

There is a system level fallback version of user-dirs.dirs, located at /etc/xdg/user-dirs.defaults. You should also read that one if the user one is not found. Note that it does not have to exist either.

Note that it is unclear to me what to do in the case both files exist, but only the system config file contains the value you want. I'd vote to use the system value if the user value does not override it, but you could also ignore the entire system config if the user config file exist.

Reference: https://www.freedesktop.org/wiki/Software/xdg-user-dirs/

Copy link
Author

Choose a reason for hiding this comment

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

I'd say user-dirs.dirs is probably populated by most popular DE when setting up a new user with a language different than English.

On Arch, /etc/xdg/user-dirs.defaults is a package required by the "plasma-desktop" meta package.

src/music_kraken/utils/shared.py Outdated Show resolved Hide resolved
Copy link
Owner

@HeIIow2 HeIIow2 left a comment

Choose a reason for hiding this comment

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

Hey thanks again for this Pull request, but before I can accept and merge, it would be nice if you would implement @Kevin-Gruber 's suggested changes he commented. :)

@HeIIow2
Copy link
Owner

HeIIow2 commented Nov 27, 2022

@DistantThunder Are you still working on it, or will you don't implement those requestet changes? I just would need to know that. Thanks in advance and have a pleasant day :)

@DistantThunder
Copy link
Author

Hi, I just haven't had time. If you want to proceed without waiting, there's no problem.
Have a nice day as well.

@HeIIow2
Copy link
Owner

HeIIow2 commented Nov 27, 2022

Thank you for the reply. Because it is no urgent feature whatsoever, I will wait for you to finish it. If I refactored that part of the code I will still copy paste you're work in there. So you wont do any work for nothing and I am gonna use it no matter how many merge conflicts occur. (less would be best but yea if required copy paste is gonna do the job) :)

@DistantThunder
Copy link
Author

Hi, thanks for the amazing pull request. <33

If you don't mind changing up a couple details, I'd appreciate it, else I would do that.

1. To print something to the command line please use logging. For you're purpose creating a new logger would be appropriate. Create a new logger before `SEARCH_LOGGER` on line 31 and then use the logger instead of print. I leave the naming up to you.

2. In line 9 `USER_XDG_DIR_FILE = os.path.expanduser("~/.config/user-dirs.dirs")` please refactor the path to use `os.path.join("~", ".config", "user-dirs.dirs")` if that works. I wanna void possible exceptions that could be thrown on different platforms I cant test on.

Important: This pull request is even though I criticized it really valuable. Thanks you soooo much for that :3

(please excuse my inability to really know how this all works, this is my first experience with collaborations)

Those changes has been pushed.

@Kevin-Gruber suggestions are incredibly valuable, but may need their own PR?

src/music_kraken/utils/shared.py Outdated Show resolved Hide resolved
src/music_kraken/utils/shared.py Outdated Show resolved Hide resolved
@HeIIow2
Copy link
Owner

HeIIow2 commented Nov 28, 2022

Hey thank you to both of you.

This looks very good so far! Please look over the comment I wrote, regarding the fallback to $HOME/Music @DistantThunder . And then I should be able to merge if nothing new comes up and I didn't miss a comment or something.

Have a pleasant day ^^

@DistantThunder
Copy link
Author

Hey thank you to both of you.

This looks very good so far! Please look over the comment I wrote, regarding the fallback to $HOME/Music @DistantThunder . And then I should be able to merge if nothing new comes up and I didn't miss a comment or something.

Have a pleasant day ^^

Yeah sorry, don't even remember why I changed that (I'm bad at code). It should be ok now?

@HeIIow2
Copy link
Owner

HeIIow2 commented Nov 28, 2022

@DistantThunder @Kevin-Gruber thank you so much. The code should be ready to merge (at least I am merging it).

@DistantThunder You are not bad at code. If it works it works. You submitted good code, "even" you're initial code was far from stuff a beginner would write. Thank you.

@Kevin-Gruber Thank you for you're helpful revies, else there would have been some issues in the future. If you want to change this code (or some other piece of code), feel free to create a pull request yourself. I'd appreciate it, but you don't have to. Also the feedback you gave me about just putting the code in this file have been heard, and I will change it once I add a config. Currently I am still refactoring the database ^^

Thanks :)

@HeIIow2 HeIIow2 merged commit e48e93e into HeIIow2:master Nov 28, 2022
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.

3 participants