tramhao / termusic

Music Player TUI written in Rust
GNU General Public License v3.0
963 stars 42 forks source link

Config V2 addition #319

Closed hasezoey closed 1 month ago

hasezoey commented 1 month ago

This PR is the addition of Config version 2, making it 2 / 3 of the config v2 PR's.

Note that this PR is not currently used anywhere yet, that is another PR. This has been split for easier review.

The Config V1 to V2 transition is not 1:1 (1 to 1), there are some changes:

Examples of the configs generated now:

Server ```toml version = "2" [com] port = 50101 address = "::" [player] music_dirs = ["/home/hasezoey/Music"] library_scan_depth = 10 loop_mode = "playlist" volume = 30 speed = 10 gapless = true use_mediacontrols = true set_discord_status = true random_track_quantity = 20 random_album_min_quantity = 5 [player.remember_position] music = "no" podcast = "yes" [player.seek_step] short_tracks = 5 long_tracks = 30 [podcast] concurrent_downloads_max = 3 max_download_retries = 3 download_dir = "/home/hasezoey/Music" ````
TUI ```toml version = "2" com = "same" [behavior] quit_server_on_exit = true confirm_quit = true [coverart] align = "bottom right" size_scale = 0 [style.library] foreground_color = "Foreground" background_color = "Reset" border_color = "Blue" highlight_color = "LightYellow" highlight_symbol = "🦄" [style.playlist] foreground_color = "Foreground" background_color = "Reset" border_color = "Blue" highlight_color = "LightYellow" highlight_symbol = "🚀" current_track_symbol = "►" use_loop_mode_symbol = true [style.lyric] foreground_color = "Foreground" background_color = "Reset" border_color = "Blue" [style.progress] foreground_color = "LightBlack" background_color = "Reset" border_color = "Blue" [style.important_popup] foreground_color = "Yellow" background_color = "Reset" border_color = "Yellow" [style.fallback] foreground_color = "Foreground" background_color = "Reset" border_color = "Blue" highlight_color = "LightYellow" [theme] name = "empty name" author = "empty author" [theme.primary] background = "#000000" foreground = "#ffffff" [theme.cursor] text = "#ffffff" cursor = "#ffffff" [theme.normal] black = "#000000" red = "#ff0000" green = "#00ff00" yellow = "#ffff00" blue = "#0000ff" magenta = "#ff00ff" cyan = "#00ffff" white = "#ffffff" [theme.bright] black = "#777777" red = "#000000" green = "#000000" yellow = "#000000" blue = "#000000" magenta = "#000000" cyan = "#000000" white = "#000000" [keys] escape = "escape" quit = "q" [keys.view] view_library = "1" view_database = "2" view_podcasts = "3" open_config = "shift+C" open_help = "control+h" [keys.navigation] up = "k" down = "j" left = "h" right = "l" goto_top = "g" goto_bottom = "shift+G" [keys.global_player] toggle_pause = "space" next_track = "n" previous_track = "shift+N" volume_up = "+" volume_down = "-" seek_forward = "f" seek_backward = "b" speed_up = "control+f" speed_down = "control+b" toggle_prefetch = "control+g" save_playlist = "control+s" [keys.global_lyric] adjust_offset_forwards = "shift+F" adjust_offset_backwards = "shift+B" cycle_frames = "shift+T" [keys.library] load_track = "l" load_dir = "shift+L" delete = "d" yank = "y" paste = "p" cycle_root = "o" add_root = "a" remove_root = "shift+A" search = "/" youtube_search = "s" open_tag_editor = "t" [keys.playlist] delete = "d" delete_all = "shift+D" shuffle = "r" cycle_loop_mode = "m" play_selected = "l" search = "/" swap_up = "shift+K" swap_down = "shift+J" add_random_songs = "shift+S" add_random_album = "s" [keys.database] add_selected = "l" add_all = "shift+L" [keys.podcast] search = "s" mark_played = "m" mark_all_played = "shift+M" refresh_feed = "r" refresh_all_feeds = "shift+R" download_episode = "d" delete_local_episode = "shift+D" delete_feed = "x" delete_all_feeds = "shift+X" [keys.adjust_cover_art] move_left = "control+shift+arrowleft" move_right = "control+shift+arrowright" move_up = "control+shift+arrowup" move_down = "control+shift+arrowdown" increase_size = "control+shift+pageup" decrease_size = "control+shift+pagedown" toggle_hide = "control+shift+end" [keys.config] save = "control+s" ```

RE:

Other PRs that were necessary for config v2 / made transition / review easier:

PS: @tramhao please try this PR first and see if you are OK with the changed config before merging.

Also tagging @myypo and @Porkepix seeing as you are at least somewhat using termusic (based on issue interactions), and may give feedback regarding user interactivity regarding the config.

hasezoey commented 1 month ago

I forgot to mention this in the original post, but wanted to note something: There is still one thing in the new (and also old) config that is annoying to me: serde parse errors, theoretically the toml parser we use should add paths to the error, but in my tests it has always been empty, making it hard to pin down where a error is. Another problem with errors is untagged enums & flattened structs & enums. But i still believe this v2 is more of a improvement over v1 that can justify this tradeoff (which hopefully can get fixed in toml and serde at some point).

Also as a side-note, i also dont really like how when using serde it means we will lose all user comments or the ability to set comments from our side in the configs, but i also dont want to just go with the toml-edit way which basically is JS indexing and manually parse all values instead of using serde.

myypo commented 1 month ago

There is still one thing in the new (and also old) config that is annoying to me: serde parse errors, theoretically the toml parser we use should add paths to the error, but in my tests it has always been empty, making it hard to pin down where a error is.

I haven't followed the implementation closely so might be a complete off topic, but could this help? https://docs.rs/serde_path_to_error/latest/serde_path_to_error/

hasezoey commented 1 month ago

I haven't followed the implementation closely so might be a complete off topic, but could this help? https://docs.rs/serde_path_to_error/latest/serde_path_to_error/

We are using Figment to load the config, which provides its own (non-customizeable) toml provider, which is bascially a pass-through to toml, this just a side-note as custom providers can be done. Aside from that i also tried without figement, and that crate didnt improve (or change) the situation, maybe i just used it wrong? Or i am just misremembering and that whole "path being empty" thing actually came from that crate instead of toml directly, and i still dont know enough about serde's internals to debug this.

hasezoey commented 1 month ago

Or i am just misremembering and that whole "path being empty" thing actually came from that crate instead of toml directly, and i still dont know enough about serde's internals to debug this.

i just re-did the test, here is the error for if the server configs com.port is set to a string instead of number:

    Error {
        tag: Tag::Default,
        profile: None,
        metadata: None,
        path: [],
        kind: Message(
            "invalid type: found string \"a\", expected u16",
        ),
        prev: None,
    },

from current config_extra::ServerConfigVersionedDefaulted::from_file.

raw toml::from_str error:

    Error {
        inner: Error {
            inner: TomlError {
                message: "invalid type: string \"a\", expected u16\n",
                raw: None,
                keys: [],
                span: None,
            },
        },
    },

and the error with serde_path_to_error:

    Error {
        path: Path {
            segments: [],
        },
        original: Error {
            inner: Error {
                inner: TomlError {
                    message: "invalid type: string \"a\", expected u16\n",
                    raw: None,
                    keys: [],
                    span: None,
                },
            },
        },
    },

EDIT: i just now tried getting ServerSettings by itself, and there the paths just work, but as soon as i try to use ServerConfigVersioned, it seemingly forgets all paths / segments, i dont know the specifics but i kind of think its related to serde(tag = "version"). EDIT2: maybe in the future manually implementing deserialize for that via serde_tagged may prove to do better EDIT3: i just played around with serde_tagged, and there also the keys wont be set, so it must be a problem with toml's deserializer

myypo commented 1 month ago

Also as a side-note, i also dont really like how when using serde it means we will lose all user comments or the ability to set comments from our side in the configs, but i also dont want to just go with the toml-edit way which basically is JS indexing and manually parse all values instead of using serde.

I just remembered that meli uses toml alongside serde for configuration and supports comments. They don't use figment tho it is just a single file and the implementation is quite complex: https://git.meli-email.org/meli/meli/src/branch/master/meli/config_macros.rs.

hasezoey commented 1 month ago

I just remembered that meli uses toml alongside serde for configuration and supports comments. They don't use figment tho it is just a single file and the implementation is quite complex: https://git.meli-email.org/meli/meli/src/branch/master/meli/config_macros.rs.

unless i am completely blind, i dont see how they do saving the config with comments the same way as the config on parse (if any); at least from what i can tell, the only time they actually save the config is on config create, though i have not looked too much into that project (and i dont see a search bar to quickly check).

i mean load & save the following exactly:

# test comment here
value = 1
another_value = 2 # another comment

at least i dont know how to do this without a lot of boilerplate (like wrapping every element in a Span-like struct) or completely custom parsing without much serde (like toml_edit).

this is important because termusic provides the TUI config editor.

myypo commented 1 month ago

the only time they actually save the config is on config create

Yes, it is what they do. I completely forgot termusic has a tui for editing config.