Proposal for settings refactoring #475

bebehei · 2018-01-08T03:09:30Z

As already made clear in #412, our settings and configuration stuff has got some room to improve.

There are many different bad patterns in the code. But the problem is not the bad, the problem is the many different.

As proposed in #412, we want to have a big array of Settings structs at the end of the day. In my perception, I'm unsure if this can workout.

But anyways, we can prepare for that first. We can improve the code by similarizing it first. Then it'll get much easier to rethink the actual conf structures.

So this PR doesn't do the actual work for #412, it just makes preparations for it.

tsipinakis · 2018-01-31T08:28:01Z

src/settings.h

@@ -10,6 +10,7 @@ enum alignment { left, center, right };
 enum ellipsize { start, middle, end };
 enum icon_position_t { icons_left, icons_right, icons_off };
 enum separator_color { FOREGROUND, AUTO, FRAME, CUSTOM };
+struct separator_color_data { enum separator_color type; char *sep_color; };


This hurts readability, structs should always be defined the long way (one item per line).

Full ACK. Done. I missed up the enum with a struct. 🙈

bebehei · 2018-02-16T03:03:22Z

@tsipinakis I'd be pleased if you go on and review this PR.

tsipinakis

Whoops, sorry for the delay!

Why specifically remove the get_time and get_path functions though? What about get_int and get_bool for which a similar case stands.

tsipinakis · 2018-02-16T18:40:26Z

src/settings.c

@@ -11,7 +11,7 @@
 #endif

 #include "rules.h" // put before config.h to fix missing include
-#include "config.h"
+#include "../config.h"


What's the point of this change? We do -I. in the makefile already.

There is no point in it. My vim syntax and error checker mocks up, that it can' find config.h and the symbols included in it.

I had issues giving additional compiler flags (like -I.), so I used it as a quick soultion. I have to go forward and fix the actual issues. Sorry for the noise.

tsipinakis · 2018-02-16T18:42:56Z

src/settings.c

@@ -388,11 +383,11 @@ void load_settings(char *cmdline_config_path)
                "print notification on startup"
        );

-        settings.dmenu = string_to_path(option_get_string(
+        settings.dmenu = string_to_path(g_strdup(option_get_string(


In this call we're copying a string into a newly allocated space, passing it off to string_to_path which copies that same string to yet another newly allocated memory space and frees the old one.

Perhaps making string_to_path take a const and not modify the original string would be a better idea than this rube-goldberg system.

In this call we're copying a string into a newly allocated space, passing it off to ....

In terms of memory allocation, there is no change. It's now at the point that it's visible that it copied the string one time too often.

Perhaps making string_to_path take a const ...

This wasn't my main focus point yet of this PR. Shall I change it?

tsipinakis · 2018-02-16T18:59:44Z

src/settings.c

+
+static enum ellipsize parse_ellipsize(const char *string, enum ellipsize def)
+{
+        if (!string)


I'm not sure about these checks, both here and in some of the conversion functions in util.c. If we're passing NULL to these functions there's something wrong and we should either assert it and abort or at least properly log it.

The option_get_string calls are all called with a default to NULL. So, if the value's not set, it returns NULL, which then should return the default.

I favor the GLib way of handling NULL values. (Interpreting NULL as a possible value of a string similar to the empty string, see g_strdup for example)

tsipinakis · 2018-02-16T19:21:09Z

src/settings.c

@@ -349,26 +378,18 @@ void load_settings(const char *cmdline_config_path)
                "Transparency. range 0-100"
        );

-        {
-                char *c = g_strdup(option_get_string(
+        settings.sep_color = parse_sepcolor(option_get_string(


I don't think having duplicate option_get_* calls is a good idea, for once it duplicates the descriptions and general calls but also it should theoretically result in a duplicated entry in the help menu (couldn't make that bug appear in a quick test but I'm not sure why).

Ah, thanks for this. That's a good point.

Err, see here: 32431bf (Move separator color into single struct), there it got removed and "fixed".

Whoops you'e right, missed that since I was reviewing each commit individually.

bebehei · 2018-02-16T20:19:49Z

Why specifically remove the get_time and get_path functions though? What about get_int and get_bool for which a similar case stands.

There's no big rationale in it. This came into my mind, too. But I discarded it as I thought, it might be too much work. When looking at it again now, there's a rationale, which could get derived: boolean, int and string are primitive datatypes, while gint64 and "path" are not.

tsipinakis · 2018-02-17T16:15:48Z

What bothers me about leaving those types in is that the *_get_{int,bool,double} are basically identical with the only difference being the wrapping or conversion functions that the returned value is being passed from, I'd argue we can get rid of them and stay with option_get_string.

2 more points I see worthwhile:

With the changes this PR introduces we have 2 different default value parameters (one for the ini function, one for the parser). I'd assume the default value is still there because we can't define an undefined value for anything other than a string since we aren't returning a pointer. Leaving only the get_string function would allow us to get rid of that.
(This is more against the previous point but still worth mentioning) The way these methods worked they used cmdline_usage_append to automatically add the type to the help menu with this changes these data types are now limited to string, int, double or bool.

bebehei · 2018-02-17T16:35:18Z

What bothers me about leaving those types in is that the ...

With the changes this PR introduces we have 2 different default value parameters ...

That is a very good reasoning. Full ACK.

I'd argue we can get rid of them and stay with option_get_string.

What about moving a step further? Using only option(const char *ini, const char *cmdline, const char *def, const char *desc) and when cmdline is NULL, it'll behave roughly like ini_get_string and when cmdline is NULL, it'll behave like cmdline_get_string and when both are set, it'll behave like option_get_string.

What's your opinion on this?

The way these methods worked they used cmdline_usage_append to automatically add the type to the help menu with this changes these data types are now limited to string, int, double or bool.

We should have this fixed before merging the PR.

bebehei · 2018-02-17T17:43:52Z

Ok, I removed the first option now. option_get_bool. int and double will follow.

Hahaha, the coverage in this PR finally drops more than 2 percent and coveralls marks it as bad (albeit it makes things better).

bebehei · 2018-02-18T14:29:40Z

~~Now the edge cases are kicking in: --help/--version don't work anymore.~~ Edit: Works now, by querying special cmdline flags.

bebehei · 2018-02-22T22:40:38Z

I've updated everything according to the recent merge of #491 and also have addressed the optimisation with string_to_path.

bebehei · 2018-02-23T02:58:48Z

dump
Edit: see https://gist.github.com/bebehei/2e59d5f953aeace372937bc04d0e21ea

Having multiple configuration readers for different data types only increases the amount of complexity. The problem can get solved easier, by just wrapping the string configuration reader into the string to datatype conversion method.

This helps in future to have string processing without assigning a temporary variable and then converting this temporary variable and then freeing it again. When having a const as the return value, the value can get processed with nested function calls.

Stop the stuff about duplicating the configuration value, put it directly into a nested function and make the lines about the configuration options more pure.

Like in the previous commit, ini_get_urgency is not neccessary. It suffices the string crawler and a string conversion method (aka string parser).

The line breaks had been previously misleading positions.

Like the previous commits, remove now all code about querying doubles from configuration. But in contradiction to others, the double functions are discarded without any replacement. If necessary later, it can get derived from the string_parse_int function.

This will make the stuff easier to implement and actually also shorten the lines in settings.c.

We're allocating the string with GLib and when the docs mention g_free instead of plain free, we should use it in the tests, too.

It was actually an antipattern, having a hidden configuration in the log.c file. Also it's easier to integrate this into dunst's settings system when the variable is saved in the big settings struct.

parse_markup_mode is already able to parse yes and no values, so it makes no sense having an extra function for this.

TODO: remove the spml and sed patch files

bebehei · 2018-06-11T15:30:13Z

So, the rebase to current master is done. I'm pretty sure, we can have something reviewable next week.

I'm changing the infrastructure to have the parser functions returning a GError instead of a stupid default value. With that we can achieve, that the actual return value is distinguishable if it's good or crap.

I had quite much time thinking about this and I'll probably go on with gperf and turn around the settings retrieval from the config-file/parameters. So that we don't walk over the settings struct and search the config value for every single settings-field. Instead we should walk over the config-file/argument list and evaluate the given parameter if it maps to a settings field. If not we're able then to spit out an error.

fwsmit · 2021-06-30T20:54:13Z

Closing this, since I don't think this will be merged

bebehei added the refactor label Jan 8, 2018

bebehei added this to the v1.4.0 milestone Jan 8, 2018

bebehei mentioned this pull request Jan 20, 2018

Logging with GLib #474

Merged

tsipinakis requested changes Jan 31, 2018

View reviewed changes

bebehei force-pushed the settings branch from 5e470fe to 2dec53f Compare January 31, 2018 10:11

bebehei force-pushed the settings branch 2 times, most recently from a1b1136 to 32431bf Compare February 12, 2018 13:42

bebehei requested a review from tsipinakis February 14, 2018 00:44

tsipinakis requested changes Feb 16, 2018

View reviewed changes

bebehei force-pushed the settings branch from c214b5e to c7748c2 Compare February 18, 2018 14:18

bebehei mentioned this pull request Feb 20, 2018

Coveralls #461

Merged

bebehei force-pushed the settings branch from 58be5f8 to a92c744 Compare February 21, 2018 10:25

bebehei mentioned this pull request Feb 21, 2018

Implement hiding on fullscreen #472

Merged

bebehei force-pushed the settings branch from a92c744 to d9c1fdc Compare February 22, 2018 21:40

bebehei force-pushed the settings branch 3 times, most recently from 8f14952 to 88af2c0 Compare February 27, 2018 18:01

bebehei force-pushed the settings branch from 88af2c0 to e000b17 Compare March 24, 2018 17:05

bebehei added 4 commits June 11, 2018 14:12

Remove *_get_path methods

eba2f52

Having multiple configuration readers for different data types only increases the amount of complexity. The problem can get solved easier, by just wrapping the string configuration reader into the string to datatype conversion method.

Use nested parser functions for config values

1148b7b

Stop the stuff about duplicating the configuration value, put it directly into a nested function and make the lines about the configuration options more pure.

Replace ini_get_urgency method

174c4af

Like in the previous commit, ini_get_urgency is not neccessary. It suffices the string crawler and a string conversion method (aka string parser).

bebehei added 16 commits June 11, 2018 15:28

Remove *_get_time functions with the same reasons as _get_path

3360ad6

Rearrange option_get_* call parameters

d513125

The line breaks had been previously misleading positions.

Move separator color values into single struct

9662b30

Remove *_get_bool functions

89a5931

Remove *_get_int functions

7c475ae

Remove *_get_double functions

5909641

Like the previous commits, remove now all code about querying doubles from configuration. But in contradiction to others, the double functions are discarded without any replacement. If necessary later, it can get derived from the string_parse_int function.

Allow arbitary number of keys to search for option

c8d62f9

Add possibility to query flags without values

98f14df

Let string_to_path take a const parameter

6b76baa

This will make the stuff easier to implement and actually also shorten the lines in settings.c.

Use g_free instead of free

5f168e5

We're allocating the string with GLib and when the docs mention g_free instead of plain free, we should use it in the tests, too.

Simplify markup processing

6586e9b

Remove preprocessed dmenu_cmd

27ad9a7

Move log level setting into settings struct

9cc0e4a

It was actually an antipattern, having a hidden configuration in the log.c file. Also it's easier to integrate this into dunst's settings system when the variable is saved in the big settings struct.

Remove unnecessary indentation level for frame section

a82591c

Map allow_markup to parse_markup_mode

586ba28

parse_markup_mode is already able to parse yes and no values, so it makes no sense having an extra function for this.

Add OPTION/DEPREC/RULE macros for future conversion

dcfcf54

TODO: remove the spml and sed patch files

bebehei force-pushed the settings branch from e000b17 to dcfcf54 Compare June 11, 2018 15:04

bebehei modified the milestones: v1.4.0, 1.5 Jan 27, 2019

tsipinakis removed this from the 1.5 milestone Aug 8, 2020

fwsmit mentioned this pull request Jan 9, 2021

Settings refactor V2 #803

Merged

4 tasks

fwsmit closed this Jun 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proposal for settings refactoring #475

Proposal for settings refactoring #475

bebehei commented Jan 8, 2018

tsipinakis Jan 31, 2018

bebehei Jan 31, 2018

bebehei commented Feb 16, 2018

tsipinakis left a comment

tsipinakis Feb 16, 2018

bebehei Feb 16, 2018

tsipinakis Feb 16, 2018

bebehei Feb 16, 2018

tsipinakis Feb 16, 2018

bebehei Feb 16, 2018

tsipinakis Feb 16, 2018

bebehei Feb 16, 2018

bebehei Feb 16, 2018

tsipinakis Feb 17, 2018

bebehei commented Feb 16, 2018 •

edited

Loading

tsipinakis commented Feb 17, 2018

bebehei commented Feb 17, 2018

bebehei commented Feb 17, 2018

bebehei commented Feb 18, 2018 •

edited

Loading

bebehei commented Feb 22, 2018

bebehei commented Feb 23, 2018 •

edited

Loading

bebehei commented Jun 11, 2018

fwsmit commented Jun 30, 2021

Proposal for settings refactoring #475

Proposal for settings refactoring #475

Conversation

bebehei commented Jan 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bebehei commented Feb 16, 2018

tsipinakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bebehei commented Feb 16, 2018 • edited Loading

tsipinakis commented Feb 17, 2018

bebehei commented Feb 17, 2018

bebehei commented Feb 17, 2018

bebehei commented Feb 18, 2018 • edited Loading

bebehei commented Feb 22, 2018

bebehei commented Feb 23, 2018 • edited Loading

bebehei commented Jun 11, 2018

fwsmit commented Jun 30, 2021

bebehei commented Feb 16, 2018 •

edited

Loading

bebehei commented Feb 18, 2018 •

edited

Loading

bebehei commented Feb 23, 2018 •

edited

Loading