Initial anchor support #213

Copy link

dnkl commented 2023年03月08日 09:47:44 +01:00

Please move this to config.c, to avoid a copy of it being instantiated in every .c file that includes config.h. Also please make the name member const.

I'd also prefer if you could down-case the variable name. Upper case is usually "reserved" for enum values and macros.

To make it externally visible (for command line parsing), remove static, and add an extern declaration in the header file. There's a small down side to this: you can't use sizeof() on it, outside of config.c. A simple workaround is to terminate the list with a NULL entry (see below).

config.h:

struct anchors_map {
 const char *name;
 enum anchors value;
};
extern const struct anchors_map anchors_map[];

config.c:

const struct anchors_map anchors_map[] = {
 {"top-left", ANCHOR_TOP_LEFT},
 ...
 {NULL, 0},
};

To iterate it in e.g. main.c:

for (size_t i = 0; anchors_map[i].name != NULL; i++) {
 ...
}

Please move this to `config.c`, to avoid a copy of it being instantiated in every `.c` file that includes `config.h`. Also please make the `name` member `const`. I'd also prefer if you could down-case the variable name. Upper case is usually "reserved" for enum values and macros. To make it externally visible (for command line parsing), remove `static`, and add an extern declaration in the header file. There's a small down side to this: you can't use `sizeof()` on it, outside of `config.c`. A simple workaround is to terminate the list with a NULL entry (see below). `config.h`: ```c struct anchors_map { const char *name; enum anchors value; }; extern const struct anchors_map anchors_map[]; ``` `config.c`: ```c const struct anchors_map anchors_map[] = { {"top-left", ANCHOR_TOP_LEFT}, ... {NULL, 0}, }; ``` To iterate it in e.g. `main.c`: ```c for (size_t i = 0; anchors_map[i].name != NULL; i++) { ... } ```

dnkl commented 2023年03月08日 09:48:43 +01:00

I think we should list all the allowed values here.

I think we should list all the allowed values here.

dnkl commented 2023年03月08日 09:50:28 +01:00

I would suggest removing the top, bottom ... examples, and instead mention the default value:

window anchor (center)

This is similar to how other options are handled.

I would suggest removing the `top`, `bottom` ... examples, and instead mention the default value: ``` window anchor (center) ``` This is similar to how other options are handled.

Copy link

dnkl commented 2023年08月02日 13:40:01 +02:00

There's a value_to_enum() function in config.c that you should be able to use here.

There's a `value_to_enum()` function in `config.c` that you should be able to use here.

jficz commented 2023年08月06日 19:31:16 +02:00

sorry, I'm a bit slower lately - what would I use the function for?

sorry, I'm a bit slower lately - what would I use the function for?

jficz commented 2023年08月07日 13:06:58 +02:00

so I gave it another thought and I think that unless I want to statically enumerate all the options (again!) in the code, the function value_to_enum() doesn't really help here. But the idea itself is good so I went on with a little experiment, which resulted in this: jficz/fuzzel@10c5d9370d

It's a rather large change in the codebase outside the scope of this MR and it's probably pretty bad, but it allows for ultimately cleaner code without duplicated static arrays of strings in the middle of the code and also allows better code reuse outside of config.c.

off topic: I found the name of the function confusing and I'd suggest to rename the function to value_in_enum() or perhaps even more accurately value_from_enum() but that would require a lot of refactoring since there are other functions with the same naming logic.

so I gave it another thought and I think that unless I want to statically enumerate all the options (again!) in the code, the function `value_to_enum()` doesn't really help here. But the idea itself is good so I went on with a little experiment, which resulted in this: https://codeberg.org/jficz/fuzzel/commit/10c5d9370dec6578415f8b5a317530d6f4ee0e3f It's a rather large change in the codebase outside the scope of this MR and it's probably pretty bad, but it allows for ultimately cleaner code without duplicated static arrays of strings in the middle of the code and also allows better code reuse outside of config.c. off topic: I found the name of the function confusing and I'd suggest to rename the function to `value_in_enum()` or perhaps even more accurately `value_from_enum()` but that would require a lot of refactoring since there are other functions with the same naming logic.

dnkl commented 2023年10月08日 15:07:45 +02:00

Let's keep it as is for now :)

Let's keep it as is for now :)

dnkl commented 2023年08月02日 13:41:28 +02:00

I would suggest copying the description from fuzzel.ini.

I would suggest copying the description from fuzzel.ini.