odilia-app / odilia

A fast screenreader for the *nix desktop.
https://odilia.app
GNU General Public License v3.0
106 stars 15 forks source link

Safety & Simplicity for Config Files #28

Open TTWNO opened 1 year ago

TTWNO commented 1 year ago

At this time, Rust's type system is not being used to verify (before the program starts) that the configuration file is correct. Error with the JSON are far too common, and kind of hard to diagnose.

This issue contains two jobs:

  1. Simplify the config file to allow the use of non-JSON strings as Odilia events. Currently each event is fairly long due to the verbosity of JSON. -- Simplicity
  2. Verify during the initialization process (before grabbing input devices) that the config file is correct and that all the keybindings will be sent properly through the socket. -- Safety

This is boring work, but should be fairly easy for a beginner to figure out, so I will label it as good first issue.

francois-caddet commented 1 year ago

I'm asking myself two things:

Edit: sorry I just see we use toml actualy.

Edit2: Does this issue relate to sohkd?

francois-caddet commented 1 year ago

Actualy where is the loading code for the config?I saw a verry simple one in common/configbut not sure if it's the only one. Can anyone help me to start on this issue?

thanks

TTWNO commented 1 year ago

We don't currently use any configured values.

Previously, we did at least load the config file, but we weren't using it so I removed it some time ago.

Starting ftom scratch is fine here. Use serde :)

Any config loaded should be added to the Stste struct in odilia/src/state.rs

francois-caddet commented 1 year ago

Ok and so,whatshould be configured?verbosity atleast. probably all the values ofspeed, pitch, volume... Should I load config for keybindings also? that should be in the input crate right?

TTWNO commented 1 year ago

I'm going to say for now, avoid keybindings. How that's done needs to be completelt rewritten.

francois-caddet commented 1 year ago

I'll stay generic over how we load the config, because I'm not sure all of them have to be placed in a configuration file. e.g.: the pitch/volume/speed are something which could change a lot during runtime so may be placed in DConf stuf in a future improvement step no?

TTWNO commented 1 year ago

Yes, that is a reasonable assumptuon.

Even something like the welcome message would be good right now.

francois-caddet commented 1 year ago

Ok, I know a crate which let be generic over whereare the config (a file, env variables, or command params) with priorities between them. you think it's a good idea to go with this? or we avoid adding a new dep for now and stay with a serde serialization no more?

TTWNO commented 1 year ago

I'm fine with Odilia having more dependencies. Go ahead.

francois-caddet commented 1 year ago

Ok, I start with this. Do I put yourself to reviewers when it's ready?

TTWNO commented 1 year ago

Yes please

C-Loftus commented 4 months ago

Is this issue still relevant? Was looking for good first issues and this seemed to be mainly addressed or not as much an issue anymore in light of the cli support

petrovvvv commented 1 month ago

I'm also wondering if this issue is still relevant. If not, are there other good first issues that are ongoing?