Migrating from toml to qSettings

[azfoo]

• Possibly closes: https://github.com/users/FelipeDefensor/projects/11/views/1?pane=issue&itemId=66679501
• closes #138
• Also added settings window for easier access to settings

[azfoo]

There's one important thing to adress though. The code that uses the settings is not getting notified of changes in those. If we change the timeline background color settings, for instance, the color only actually changes after restarting the app. There's lot of ways to do this, of course, but I think that a particularly elegant one is to accept and optional callback function as an argument to settings.get that gets called whenever that value is set. Feel free to implement this as you wish, though.

Will look into that.

Also, we are missing "Apply" and "Cancel" buttons.

Will do.

[FelipeDefensor]

Correcting an incomplete phrase on one of the comments:

The code that uses the settings is not getting notified of changes in those. If we change the timeline background color settings, for instance, the color only actually changes after restarting the app.

I think you managed to understand it without that, though.

[azfoo]

Everything in the settings changes on its own now, except for auto-scroll and window geometry. Having those change when settings are meant to be default and universal across all instances of the app didn't make sense.

[azfoo]

What I suggest is that the call to settings.get already triggers the linking in some way, perhaps by taking a callback as an optional argument.

One of the main reasons for designing it this way is that with elements that delete every time a merge/split/redo/undo happens, the number of callbacks stored grows very quickly - which can cause things to become expensive. An optional callback is possible for things outside of timeline UI elements, but I'm not confident that they will continue working on the elements (especially as more components are created).

Just to make sure, is settings.get expensive? I'm imagining it isn't.

Doesn't seem to be...

I'm getting an error when opening the metadata window.

my bad. was experimenting with github desktop but that causes all kinds of rebasing problems. should be fixed now.

Regarding names: will rename them, but just so you know, it'll rename all settings names across the app, which I don't think will be a problem as they are strings anyway.

• Should there be a minimum height to the beat timeline to prevent the user from hiding the labels accidentally?

For uniformity's sake, there is a minimum of 10 on all height settings (see previous fix), and PDF's height cannot be changed (because that's also a thing that was decided). I'd say that like with PDF, there isn't much of a point in changing the height of this timeline and we could do without showing this setting and also remove the ability to change this timeline's height in other places.

• The save settings dialog should also have a cancel button. Closing the dialog instead of pressing any buttons should have the same effect as using the cancel button.

To clarify on buttons in general, do we have a cancel and a discard button? Currently, the discard button reverts all settings to when the settings window was first opened and leaves the window open. In the same vein, should the save button also close the window?

*Changing the trough radius makes it misalign. If you want to hardcode the radiu instead of dealing with this, I'm fine with that.

That's a problem with my maths - will try to fix that, but if not, it'll be hardcoded.

[FelipeDefensor]

One of the main reasons for designing it this way is that with elements that delete every time a merge/split/redo/undo happens, the number of callbacks stored grows very quickly - which can cause things to become expensive. An optional callback is possible for things outside of timeline UI elements, but I'm not confident that they will continue working on the elements (especially as more components are created).

Good point, but I think we can the first part by moving by making the timeline ui the consumer instead of the elements. It would pass a callback that, when triggered, loops through the elements making the relevant changes.

Doesn't seem to be [expensive]...

Fine, nothing to comment in that case.

will rename them, but just so you know, it'll rename all settings names across the app, which I don't think will be a problem as they are strings anyway.

What I am suggesting is decoupling the internal name from the displayed name (again, if that's not too much trouble). In that case, we could keep the current internal name, so no app wide renaming would be needed, right?

For uniformity's sake, there is a minimum of 10 on all height settings (see previous fix), and PDF's height cannot be changed (because that's also a thing that was decided). I'd say that like with PDF, there isn't much of a point in changing the height of this timeline and we could do without showing this setting and also remove the ability to change this timeline's height in other places.

I agree, let's make the height fixed.

[azfoo]

Mostly corrected, except for renaming settings. Is there any reason for keeping the internal naming system and what is displayed separately? It seems like extra work trying to decode which setting links to which if we have different naming systems.

[FelipeDefensor]

Mostly corrected, except for renaming settings. Is there any reason for keeping the internal naming system and what is displayed separately? It seems like extra work trying to decode which setting links to which if we have different naming systems.

The general reason is that a good name for coding purposes might not be a good name from the user perspective. I think the auto-save interval is an example of that. I'm imagining there might be more cases in the future. I would say this is low-priority, so we can worry about this later and just use the user-friendoly name in the code for now if it is inconvenient to implement. Your call.

[FelipeDefensor]

Mostly corrected, except for renaming settings. Is there any reason for keeping the internal naming system and what is displayed separately? It seems like extra work trying to decode which setting links to which if we have different naming systems.

Should I review, then?

[azfoo]

run from tilia.settings import settings settings.reset_to_default() to clear existing settings before running to avoid seeing funny names

[azfoo]

did the lazy thing of renaming everything instead. could we perhaps leave the mapping to a later point?

[FelipeDefensor]

did the lazy thing of renaming everything instead. could we perhaps leave the mapping to a later point?

Sure we can. I certainly have postponed a lot of inconvenient coding in this project. You should feel that you can do the same. Let's only keep an eye on each other so the technical debt doesn't get out of hand.