Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve Preferences and Theme handling #1077

Closed
jubalh opened this issue Apr 24, 2019 · 15 comments
Closed

Improve Preferences and Theme handling #1077

jubalh opened this issue Apr 24, 2019 · 15 comments
Assignees
Milestone

Comments

@jubalh
Copy link
Member

jubalh commented Apr 24, 2019

We have a bit borked handling for this.

In profanity.c we first load the prefs. From there we get which theme to use, so later on we load the theme.

Later in src/config/theme.c in _load_preferences() we load the preferences set in the theme.
In some cases, like statusbar.tabs, we just overwrite the value we get from the theme.
In others, like roster.header.char, occupants.header.char, we either set it from the theme or reset it (prefs_clear_roster_header_char()).

Since a theme can contain UI settings like color, but also things like the characters we have no clear separation between user settings and themes.
So if we want a theme to be consistent we will need to always prefer the theme settings, and users are forced to create their own themes if they want to have certain settings differently.

Currently this happens:

  • We load the user prefs.
  • We load a theme (now everything gets overwritten like defined in that specific theme. The things that are not defined dont get overwritten (some of them..) so if a users changes to theme X which sets statusbar.tabs to 5. Then switches to theme Y which has no setting for this, then he will still have the leftover from theme X.

So I think we need to do the following once a theme is set (at start of profanity or when changing):

  • Load all defaults (clears from previous theme)
  • Load and apply user preferences
  • Load theme

Or:

  • Load all defaults
  • Load prefs
  • Load theme defined in prefs (only overwrite what is not set)

Sorry I think this issue is not well structured :-) I hope its understandable though.

@jubalh jubalh added this to the 0.7.0 milestone Apr 24, 2019
@jubalh
Copy link
Member Author

jubalh commented Apr 24, 2019

The resetting to empty value if its not in the theme that we do for some of the entities shouldnt be there I think.
We should move this to an init function (that is called before this) that either takes values either from defaults or from user prefs.

The the question is whether we lay the user prefs over the theme or the other way around. Maybe we could also have /theme clean-load themename which uses defaults+theme. And /theme load themename which loads the theme and then loads the user prefs over it (if defined).

@kaffeekanne
Copy link
Contributor

I think explicitly loading a theme should overwrite user settings. But should settings get lost then just because you wanted to try a theme?

What if loading a theme only sets/overwrites the users prefs (if set before) and defaults the ones not defined in the theme?

So on startup:

  • load defaults
  • load prefs

on theme load:

  • load defaults
  • load theme (== set prefs)

can you follow me?

@jubalh
Copy link
Member Author

jubalh commented Apr 26, 2019

But should settings get lost then just because you wanted to try a theme?

I thought about adding a /save command, and only make any changes to preferences and themes permanent after this has been called. Then users could experiment with both preferences and themes without consequences.

@kaffeekanne
Copy link
Contributor

kaffeekanne commented Apr 26, 2019

As seperate /save command or somewhere under /theme? New commands should be introduced very careful, i think. But i like the idea!

@ccxcz
Copy link

ccxcz commented May 3, 2019

Part of the issue here is that profrc is not actual rc-file, that is ordered sequence of run-commands; instead it is unordered configuration file and thus has no way to express precedence.
There are few ways to address that:
You could make such on-start script that would load theme (by name) and then user config.
You could add another configuration file that gets loaded after theme.
Or you could parse the user configuration file, extract theme name, apply theme and then actually apply the user configuration - sans theme.

For storing the user configuration you will possibly want Vim-like tracking of which file modified which configuration option last. Then you can save only those user actually set.
You could even use that for loading theme in a way that it doesn't override user config, but that can get confusing without ability to set options to "default" - and even with it, would it be resetting to program default or the theme one? If the latter, you need to add extra configuration structure.

@jubalh
Copy link
Member Author

jubalh commented Jun 7, 2019

When we rework this we should also take care of #721

@mdosch
Copy link
Contributor

mdosch commented Nov 6, 2019

So I think we need to do the following once a theme is set (at start of profanity or when changing):

  • Load all defaults (clears from previous theme)
  • Load and apply user preferences
  • Load theme

Or:

  • Load all defaults
  • Load prefs
  • Load theme defined in prefs (only overwrite what is not set)

I prefer the second as I don't want to have to redo my settings again only because I tried a theme (see #1218).

I would consider settings in a theme a recommendation that will be used if you didn't set the setting yourself, but if you set it yourself than probably because you are particular about it and want it this way.

Or you could split themes in themename.theme and themename.settings and /theme load themename is only loading the theme (meaning colors, I don't really consider it theming to change my settings) and /theme load-settings themename or /theme settings themename will change your settings to the ones recommended by the theme author.

jubalh added a commit that referenced this issue Dec 20, 2019
So far `/occupants char *`, `/roster contact char *`, `/roster room char #`,
`/roster header char -`, `/occupants header char -` was saved and
loaded from the preferences.

But was overwritten when the theme was loaded. If the theme didn't set
these values the value was just cleared. Despite that it might have been
set in the users preferences.

Funny enough the themes don't operate generally like this.
For example `otr.char` is not cleared.

This is again due to our borked theme/prefs concept
(#1077).

For now let's just use the one set from the preferences if it's set. The
theme will however overwrite it if it is set there.

Fix #1244
@jubalh jubalh modified the milestones: 0.8.0, 0.9.0 Jan 3, 2020
@jubalh
Copy link
Member Author

jubalh commented Jan 29, 2020

Yes dividing it so we have a /theme load name for the colors and /theme load-settings to load the preferences would also be easiest to implement I think. We even can let the theme files stay as they are and in one case only load the colours section from it.

So in this case we cover:

  1. User starts from the beginning, has no preferences or ideas about them, and wants to see what is possible. So he loads various themes. Gets different colors and different settings. Once he finds one he sticks with it. Maybe also because he likes the settings. At some points he decides he is sick of the colors and wants different ones. Then he can choose to just load the colours part of another theme.

  2. User has his settings, put together over weeks. But he wants different colors now and then.

  3. User wants most of the settings from a certain theme. But has some minor wants that he is sure to always need. Like a special char for OMEMO for example. So he needs to create his own theme. Load the new theme completely and then load his theme which just contains the settings about OMEMO char and no color settings.But this means: should when do reset the settings at some point? I guess we should once we load a theme. Otherwise we might get left overs from previous loaded themes that are not set in the current one.

Just writing my thoughts here.
So what I'm not sure about is:
Should we reset the settings upon each theme load?
Should we just load the new things and leave everything else as is? Would be easiest.

@mdosch
Copy link
Contributor

mdosch commented Jan 29, 2020 via email

@mdosch
Copy link
Contributor

mdosch commented Jan 29, 2020 via email

@jubalh
Copy link
Member Author

jubalh commented Jan 29, 2020

I think if a setting is explicitely set in profrc it should be never overwritten as you put it there for a reason.

There is only one place where for example omemo.char is saved.
If we set this via profrc we load it into a variable. If we load from a theme, we load it into a variable.
If we /save we save that variable to profrc. So far no destinction "who" set it.

@jubalh
Copy link
Member Author

jubalh commented Jan 29, 2020

So we could not set [ui] settings only [colours] settings when loading from a theme. Like we discusses with 'load' and 'load settings'. But after it is set there is no destinction anymore where it came from.

jubalh added a commit that referenced this issue Jan 29, 2020
So far when loading a theme it also overwrote the preferences the user
set.

Lengthy discussion can be found at
#1077

Now we use `/theme load themename` to load the [colours] part of a
themem only.

`/theme full-load themename` will load the complete theme including
preferences set in there.

Regards #1077
@jubalh
Copy link
Member Author

jubalh commented Jan 29, 2020

@mdosch I just pushed a commit to master that has now /theme load themename to work as one should expect. To just load the colors.
And /theme full-load themename to load the complete file. Including changing of preferences.

Could you test if it works as expected?
For me it seems right now.

@jubalh jubalh self-assigned this Jan 29, 2020
@jubalh jubalh modified the milestones: 0.9.0, 0.8.0 Jan 30, 2020
@mdosch
Copy link
Contributor

mdosch commented Jan 30, 2020

I can confirm it's working as expected:
/theme load bios only loads the colors while /theme full-load bios shows the resource part in the roster (which my config didn't before).

@jubalh
Copy link
Member Author

jubalh commented Jan 30, 2020

Then I think now we have all the functionality we want/need.
Closing this.

@jubalh jubalh closed this as completed Jan 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants