Skip to content
This repository was archived by the owner on Apr 1, 2023. It is now read-only.

More appropiate data structures & less code repetition#88

Merged
marioortizmanero merged 7 commits intomasterfrom
dataclasses
Apr 7, 2020
Merged

More appropiate data structures & less code repetition#88
marioortizmanero merged 7 commits intomasterfrom
dataclasses

Conversation

@marioortizmanero
Copy link
Member

@marioortizmanero marioortizmanero commented Apr 7, 2020 •
edited
Loading

Related to #83, this merges APIData and PlayerData into a base class to avoid code duplication.

This means that instead of using enums with custom __new__ methods internally, a list of PlayerData /APIData immutable dataclasses is used. This reduces boilerplate a lot and allows to merge load_api_data and load_player_data from SetupWidget into a single method.

I first thought of using a dict to store the data associated to the ID:

PLAYERS = {
    'VLC': PlayerData(...)
}

but in the end it was easier to be able to use PlayerData.id to access the ID rather than having to search PlayerData to obtain the dict key:

PLAYERS = (
   PlayerData('VLC', ...)
)

As this method doesn't guarantee that the IDs are equal, I've made a test to make sure of that. I'm still not really sure if this is the best way to do it, though. This is how Enum works underneath anyway, so the behavior remains the same.

I've also done something similar with the config options. Using dataclasses and inheritance rather than a custom enum makes using it more idiomatic and straightforward.

I removed BaseMethodData.platforms: Tuple[Platform] in place of BaseMethodData.compatible: bool. This is already done with BaseMethodData.installed, because it's not necessary to store the dependent modules per se, only whether it's installed or not. It also doesn't change as the program runs, so it doesn't make sense to check CURRENT_PLATFORM in PlayerData.platforms whenever I want to know if it's available. Furthermore, if the API/Player wasn't available on the system for any other reason than the OS, it would be impossible to check it currently.

As a small plus, the Config arguments now have a text with the default value in its description.

@marioortizmanero
Copy link
Member Author

marioortizmanero commented Apr 7, 2020

Hey @AndrewAmmerlaan can you give this branch a quick try before I merge it into master, if you have time? I've made sure it works well, but it has so many internal changes anything could go wrong :P

@Nowa-Ammerlaan
Copy link
Contributor

Nowa-Ammerlaan commented Apr 7, 2020

Hey @AndrewAmmerlaan can you give this branch a quick try before I merge it into master, if you have time? I've made sure it works well, but it has so many internal changes anything could go wrong :P

Seems to be working just fine 👍

andrew@andrew-gentoo-pc vidify % python -m vidify --debug
[14:42:50.032] INFO: Loading setup screen
[14:42:50.071] INFO: Created API card: MPRIS_LINUX (enabled=True, selected=True)
[14:42:50.073] INFO: Created API card: SPOTIFY_WEB (enabled=False, selected=False)
[14:42:50.073] INFO: Created API card: VLC (enabled=True, selected=True)
[14:42:50.074] INFO: Created API card: MPV (enabled=False, selected=False)
[14:42:50.086] INFO: Created API card: EXTERNAL (enabled=True, selected=False)
[14:42:57.350] INFO: Selected: api=MPRIS_LINUX, player=VLC
[14:42:57.409] INFO: Using MPRIS_LINUX as the API
[14:42:57.432] INFO: Using VLC as the player
[14:42:58.362] INFO: Looking for players
[14:42:58.365] INFO: Using org.mpris.MediaPlayer2.spotify
[14:42:58.366] INFO: Succesfully connected to the API
audiosync: setting up audiosync module
audiosync: found custom monitor, reusing it
audiosync: moving the found stream
[14:42:58.398] INFO: Started a new audiosync job
[14:42:58.398] INFO: Starting the youtube-dl thread
audiosync: starting interval loop
audiosync: starting download thread
audiosync: starting capture thread
audiosync: using custom monitor for capture
[youtube:search] query "Clamavi De Profundis - When the Hammer Falls Official Video": Downloading page 1
[download] Downloading playlist: Clamavi De Profundis - When the Hammer Falls Official Video
[youtube:search] playlist Clamavi De Profundis - When the Hammer Falls Official Video: Collected 1 video ids (downloading 1 of them)
[download] Downloading video 1 of 1
[youtube] Xm96Cqu4Ils: Downloading webpage
[download] Finished downloading playlist: Clamavi De Profundis - When the Hammer Falls Official Video
[14:42:59.760] INFO: Starting new video
[14:42:59.774] INFO: Playing/Pausing video
[14:42:59.776] DEBUG: Starting new HTTPS connection (1): lyrics.wikia.com:443
[14:42:59.841] DEBUG: https://lyrics.wikia.com:443 "GET /wiki/Clamavi_De_Profundis:When_The_Hammer_Falls HTTP/1.1" 301 20
[14:42:59.844] DEBUG: Starting new HTTPS connection (1): lyrics.fandom.com:443
[14:42:59.925] DEBUG: https://lyrics.fandom.com:443 "GET /wiki/Clamavi_De_Profundis:When_The_Hammer_Falls HTTP/1.1" 404 71577
Clamavi De Profundis - When the Hammer Falls
No lyrics found

[00007fddb0126010] vaapi_filters filter warning: Using SW chroma filter for 1920x1080 VAOP -> I420
audiosync: obtained youtube-dl URL for download
audiosync: finished ffmpeg loop
audiosync: next interval (0): cap=144113 down=2880000
audiosync: 31858 frames of delay with a confidence of -0.117617
audiosync: next interval (1): cap=288039 down=2880000
audiosync: 16493 frames of delay with a confidence of 0.080814
audiosync: next interval (2): cap=480105 down=2880000
audiosync: -254503 frames of delay with a confidence of 0.086765
audiosync: next interval (3): cap=720004 down=2880000
audiosync: -449902 frames of delay with a confidence of -0.018861
audiosync: next interval (4): cap=960090 down=2880000
audiosync: -929902 frames of delay with a confidence of -0.000028
audiosync: finished ffmpeg loop
audiosync: next interval (5): cap=1440000 down=2880000
audiosync: 990098 frames of delay with a confidence of 0.301867
audiosync: finished run
[14:43:28.480] INFO: Audiosync module failed to return the lag

Screenshot_20200407_144623

Test suite seems fine as well:

Ran 22 tests in 21.309s

@marioortizmanero
Copy link
Member Author

marioortizmanero commented Apr 7, 2020

Great, thanks! I'm merging it :)

@marioortizmanero marioortizmanero merged commit e588004 into master Apr 7, 2020
@marioortizmanero marioortizmanero deleted the dataclasses branch April 7, 2020 13:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@marioortizmanero @Nowa-Ammerlaan