Added checkbox that enables/disables notifications and preferences save to user directory

[damonhayhurst]

• Replaced depreceated app_dirs with directories crate (new crate, same author)
• device name placeholder is saved in preferences.toml along with new notification enable/disable
• preferences.toml is saved in .config/songrec
• song_history.csv has been moved to .local/share/songrec from .local/share/SongRec because of new directories crate function (directories crate seems to lowercase folder names with no option to specify otherwise)

[marin-m]

Hello!

Thank you for your pull request. This new option is not persistent, whilst the "Recognize from my speakers instead of microphone" option, or the choice of the input device, have a form of disk-based persistence.

Maybe that it could be relevant to save the user's choice somewhere in order to improve the general consistency of the interface behavior?

Regards,

[damonhayhurst]

I'm thinking about implementing autotools into the process in order to automate the installation of a GSchema.

Edit: Nevermind, found an easier way

[damonhayhurst]

Added disk based persistence

[damonhayhurst]

Please tear my Rust to shreds.

[marin-m]

Hello!

Thanks for your updates.

I am noticing that you have replaced the (retired) app_dirs crate with a directories crates, but that the folder components used for constructing the concerned paths are now different: Instead of:

        let app_info = AppInfo {
            name: "SongRec",
            author: "SongRec"
        };

You are now using:

    let project_dir = ProjectDirs::from("com", "Github", "SongRec").ok_or("No valid path")?;

But will backward compatibility of filesystem paths be guaranteed between SongRec versions this way? Was it tested?

If I understand well the way local directory paths are built, according to the directories documentation: https://docs.rs/directories/latest/directories/struct.ProjectDirs.html#method.cache_dir

Wouldn't this produce a more similar result?

    let project_dir = ProjectDirs::from("", "SongRec", "SongRec").ok_or("No valid path")?;

Otherwise, this work looks very clean and careful,

Regards,

[damonhayhurst]

Wouldn't this produce a more similar result?

    let project_dir = ProjectDirs::from("", "SongRec", "SongRec").ok_or("No valid path")?;

Correct me if I'm wrong but the first two values of that call have no bearing on linux versions which is the only platform the gui works for?

Also the backwards compatiblity is an issue. I've just updated the first comment with issues around casing that the directories crate seems to create (rather annoying)

Thanks for your comments

[marin-m]

Hello,

Indeed, if there is backwards compatibility breakage under Linux then it may be a good thing to add something to bypass the issue. Maybe that the best solution would be to create a symbolic link from ~/.local/share/songrec towards ~/.local/share/SongRec if the latter directory already exists, and before reading the configuration?

The Windows build is currently not functional, but under former SongRec versions song recognition history was written under ${USERDIR}\AppData\Local\SongRec\SongRec. I guess that this won't change with the new crate if you apply my latest code suggestion.

Certain people are building SongRec under macOS (see issue #91) but this is marginal and I think that people building themselves would notice a directory change.

Also the directory backwards compatibility with Flatpak should also be checked and tested I guess (likely the same as on regular Linux).

Regards,

[damonhayhurst]

Ok, happy to make the aforementioned changes. Regarding the symbolic linking is that something that would be better as part of the packaging?

[marin-m]

I fear that not, as the user directory is something specific to each user under Linux (and/or in the Flatpak environment) while the SongRec package will almost always be installed system-wide.

And as SongRec is distributed through a variety of channels under Linux (the Archlinux community repository, an Ubuntu PPA, Cargo, Archlinux AUR for the Git version, Flatpak...), is may be complicated to ensure that acting under the personal directory of each system-local user is easily doable for each of these distribution channels, including the ones that I don't control directly.

Regards,

[damonhayhurst]

Regarding creating the symlink, maybe it should be mentioned that this only really affects the song history csv file. Preferences are stored in a new place (.config).

Introducing the new directory is not program-breaking and does not essentially cause backwards-compatibility issues.

The old song history is still saved and can be retrieved by the user and a simple copy to the new directory is all it takes to restore the song history to the GUI. Perhaps this can be included in the release notes?

I feel this is a much better solution than adding unneeded complexity to the code.

[marin-m]

Hello,

The old song history is still saved and can be retrieved by the user and a simple copy to the new directory is all it takes to restore the song history to the GUI. Perhaps this can be included in the release notes?

This program is likely installed on a few 10k's of user setups, some of which users may not understand well what is a hidden configuration history, may have the program updated automatically from a PPA or Flatpak frontend or not think to read the release notes.

It may be good to avoid program history being surprisingly dropped after a background update, which may be perceived as a bug.

I feel this is a much better solution than adding unneeded complexity to the code.

This program already has a lot of glue/utility code for PulseAudio interfacing and internationalization, adding an automatic symlink command executed at the program start is really just about one code instruction to execute and not much complex.

Regards,

[damonhayhurst]

Latest version up

[marin-m]

Merged, thanks!