alievk / avatarify-python

Avatars for Zoom, Skype and other video-conferencing apps.
Other
16.29k stars 4.09k forks source link

Added required libraries and altered camera_selector python file #728

Open anirudh-hegde opened 5 months ago

anirudh-hegde commented 5 months ago

In camera_selector.py: avat

In requirements.txt: req

JohanAR commented 5 months ago

Hi, unfortunately avatarify-python hasn't been actively maintained for quite some time. I can still accept minor easily reviewable changes, like this one, but I think the project as a whole is in pretty bad shape currently.

Regarding this change, I don't want to force everybody to create an avatarify-python folder in their home dir for the config. But if you want to improve this, perhaps you could make a list of different locations to search for config.yaml? I also think ~/.config/avatarify-python/ is preferred to putting folders directly under ~ on Linux, but I don't know if it's standard on all distributions, perhaps there's a method to get the path to the system's config directory? I also don't know if os.path.expanduser('~') works on Windows, but the code needs to work there as well, preferably using a folder which is consistent with other apps on that platform.

And finally, please make sure to remove the commented out code from the pull request, since there isn't any point on putting that on the main branch.

anirudh-hegde commented 5 months ago

Hi, unfortunately avatarify-python hasn't been actively maintained for quite some time. I can still accept minor easily reviewable changes, like this one, but I think the project as a whole is in pretty bad shape currently.

Regarding this change, I don't want to force everybody to create an avatarify-python folder in their home dir for the config. But if you want to improve this, perhaps you could make a list of different locations to search for config.yaml? I also think ~/.config/avatarify-python/ is preferred to putting folders directly under ~ on Linux, but I don't know if it's standard on all distributions, perhaps there's a method to get the path to the system's config directory? I also don't know if os.path.expanduser('~') works on Windows, but the code needs to work there as well, preferably using a folder which is consistent with other apps on that platform.

And finally, please make sure to remove the commented out code from the pull request, since there isn't any point on putting that on the main branch.

Screenshot 2024-06-01 at 16-16-21 os path — Common pathname manipulations

https://docs.python.org/3/library/os.path.html

JohanAR commented 5 months ago

Great that you checked for cross-platform compatibility! Unfortunately I don't think the current code works as intended. For example I have avatarify-python checked out to the path /mnt/storage/Programs/avatarify, so the expression parent_dir = os.path.basename(os.path.dirname(os.getcwd())) sets parent_dir to "Programs". Then the following expression yml_file_path = os.path.join(os.path.expanduser('~'), parent_dir, 'config.yaml') sets yml_file_path to "/home/johan/Programs/config.yaml" on my system.

To clarify, I think the best solution would be to create a list of possible locations for this file, for example ['config.yaml', '~/.config/avatarify-python/config.yaml', '~/avatarify-python/config.yaml'], and then loop through it and try to load the file from each location. If it successfully loads the file it breaks out of the loop, if it fails then it continues with the next. And using expanduser() and others to build the actual path, I just wrote it as a plain string to make it easier to read.

anirudh-hegde commented 5 months ago

Great that you checked for cross-platform compatibility! Unfortunately I don't think the current code works as intended. For example I have avatarify-python checked out to the path /mnt/storage/Programs/avatarify, so the expression parent_dir = os.path.basename(os.path.dirname(os.getcwd())) sets parent_dir to "Programs". Then the following expression yml_file_path = os.path.join(os.path.expanduser('~'), parent_dir, 'config.yaml') sets yml_file_path to "/home/johan/Programs/config.yaml" on my system.

To clarify, I think the best solution would be to create a list of possible locations for this file, for example ['config.yaml', '~/.config/avatarify-python/config.yaml', '~/avatarify-python/config.yaml'], and then loop through it and try to load the file from each location. If it successfully loads the file it breaks out of the loop, if it fails then it continues with the next. And using expanduser() and others to build the actual path, I just wrote it as a plain string to make it easier to read.

Definitely will give a try

anirudh-hegde commented 5 months ago

Hi @JohanAR, can you please review my code and suggest any changes needed.

JohanAR commented 5 months ago

Sorry for the delay, have been busy.

As a general tip, try testing your code locally more and check that it would look in the correct locations, regardless of where the user has downloaded avatarify-python. During development you can add some debug prints for each place it searches for the config file