Lua config

[dvermd]

What type of PR is this? (check all applicable)

• Refactor
• Feature
• Bug Fix
• Optimization
• Documentation Update
• Other: Replace this with a description of the type of this PR

Description

This is the first draft of adding lua configuration file to polybar.
It uses the luwra lib which BSD-3 license seems to be compatible with polybar license.

Lua 5.4 is not embedded yet and is expected to be installed on the building and running machine.

Polybar starts with the doc/config.lua file on the bar_test bar

There is still lot of work to do, at least at the top of my head:

• embed lua ?
• rework the types conversion returned by get and other config methods. This might be done by extending luwra known types
• handle errors
• clean the code up
• handle imported files: I have implementation ideas on that
• documentation

Related Issues & Documents

Documentation (check all applicable)

• This PR requires changes to the Wiki documentation (describe the changes)
• This PR requires changes to the documentation inside the git repo (please add them to the PR).
• Does not require documentation changes

[patrick96]

This is very cool, thanks a lot for all the work you put into this!

Unfortunately I can't get it to compile yet, so I can't test it out for myself.

Error

In file included from /home/patrick96/Projects/github.com/patrick96/polybar/include/components/config.hpp:8,
                 from /home/patrick96/Projects/github.com/patrick96/polybar/include/modules/meta/base.hpp:11,
                 from /home/patrick96/Projects/github.com/patrick96/polybar/src/adapters/script_runner.cpp:6:
/home/patrick96/Projects/github.com/patrick96/polybar/include/components/config_lua.hpp:126:13: error: explicit specialization in non-namespace scope ‘class polybar::config_lua’
  126 |   template <>
      |             ^
/home/patrick96/Projects/github.com/patrick96/polybar/include/components/config_lua.hpp:138:13: error: explicit specialization in non-namespace scope ‘class polybar::config_lua’
  138 |   template <>
      |             ^
/home/patrick96/Projects/github.com/patrick96/polybar/include/components/config_lua.hpp:150:13: error: explicit specialization in non-namespace scope ‘class polybar::config_lua’
  150 |   template <>
      |             ^
/home/patrick96/Projects/github.com/patrick96/polybar/include/components/config_lua.hpp:273:11: error: explicit specialization in non-namespace scope ‘class polybar::config_lua’
  273 | template <>
      |           ^
/home/patrick96/Projects/github.com/patrick96/polybar/include/components/config_lua.hpp: In instantiation of ‘T polybar::config_lua::get(const std::string&, const std::string&) const [with T = std::__cxx11::basic_string<char>; std::string = std::__cxx11::basic_string<char>]’:
/home/patrick96/Projects/github.com/patrick96/polybar/include/components/config_lua.hpp:129:53:   required from here
/home/patrick96/Projects/github.com/patrick96/polybar/include/components/config_lua.hpp:107:12: error: call of overloaded ‘basic_string(const luwra::internal::TableAccessor<luwra::internal::Path<luwra::internal::Path<luwra::internal::Path<const luwra::Reference&, luwra::Table&>, std::__cxx11::basic_string<char>&>, const std::__cxx11::basic_string<char>&> >)’ is ambiguous
  107 |     return (T)m_state[container][stripped_section][key];
In file included from /usr/include/c++/13.1.1/string:54,
                 from /home/patrick96/Projects/github.com/patrick96/polybar/include/common.hpp:5,
                 from /home/patrick96/Projects/github.com/patrick96/polybar/include/adapters/script_runner.hpp:7,
                 from /home/patrick96/Projects/github.com/patrick96/polybar/src/adapters/script_runner.cpp:1:
/usr/include/c++/13.1.1/bits/basic_string.h:644:7: note: candidate: ‘std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::basic_string(const _CharT*, const _Alloc&) [with <template-parameter-2-1> = std::allocator<char>; _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]’
  644 |       basic_string(const _CharT* __s, const _Alloc& __a = _Alloc())
      |       ^~~~~~~~~~~~
/usr/include/c++/13.1.1/bits/basic_string.h:708:7: note: candidate: ‘std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::basic_string(std::initializer_list<_Tp>, const _Alloc&) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]’
  708 |       basic_string(initializer_list<_CharT> __l, const _Alloc& __a = _Alloc())
      |       ^~~~~~~~~~~~
/usr/include/c++/13.1.1/bits/basic_string.h:550:7: note: candidate: ‘std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::basic_string(const std::__cxx11::basic_string<_CharT, _Traits, _Alloc>&) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]’
  550 |       basic_string(const basic_string& __str)
      |       ^~~~~~~~~~~~
/usr/include/c++/13.1.1/bits/basic_string.h:538:7: note: candidate: ‘std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::basic_string(const _Alloc&) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]’
  538 |       basic_string(const _Alloc& __a) _GLIBCXX_NOEXCEPT
      |       ^~~~~~~~~~~~
make[2]: *** [bin/CMakeFiles/poly.dir/build.make:90: bin/CMakeFiles/poly.dir/adapters/script_runner.cpp.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:547: bin/CMakeFiles/poly.dir/all] Error 2
make: *** [Makefile:136: all] Error 2

I do have some thoughts on your TODO list, but many of them can wait for later:

• embed lua
  At some point definitely, otherwise configs may not work everywhere (even with the same polybar version). Can wait until lua support and API is somewhat stable.
• rework the types conversion returned by get and other config methods. This might be done by extending luwra known types
  Can also wait. Initially, the values read from lua are all strings, which are then converted using the various convert functions (just as in the INI config).
• handle errors, clean the code up
  Should probably happen in this PR 😉
• handle imported files: I have implementation ideas on that
  Also not urgent, but of course very powerful because it is the start for a plugin ecosystem
• documentation
  User docs can also wait, developer docs in the code are more useful right now

In this PR we should try to get a minimal viable product (MVP) for lua configs, where we define a robust config API that fits both lua and INI

I haven't gone into the code too much yet since it seems there will be some bigger changes yet to come.
The main comment I have is about the config API: In config.hpp, you manually route method calls to either the INI or LUA version of the config. If you make config_ini and config_lua extend the config class and use virtual methods, you get this routing for free and it makes the code cleaner. config.hpp would then really just define the API without any functionality and you could move some duplicated functionality to config.cpp (e.g. warn_deprecated).

Let me know when/if you need any additional feedback :)

[dvermd]

Your compilation error should now be fixed.

I haven't gone into the code too much yet since it seems there will be some bigger changes yet to come. The main comment I have is about the config API: In config.hpp, you manually route method calls to either the INI or LUA version of the config. If you make config_ini and config_lua extend the config class and use virtual methods, you get this routing for free and it makes the code cleaner. config.hpp would then really just define the API without any functionality and you could move some duplicated functionality to config.cpp (e.g. warn_deprecated).

I tried that before the first commits but it looks like you can't derive templated functions, thus I came up with manual routing.

[patrick96]

Your compilation error should now be fixed.

Nice! I'll try it out tomorrow

I tried that before the first commits but it looks like you can't derive templated functions, thus I came up with manual routing.

That's a bummer. What if the subclasses only provide the primitive get functionality that returns a string and the base class has all the (non-virtual) templated methods that handle the conversion?
This would probably require more refactoring though. And all the templates need to be removed from the dereference methods, but that shouldn't be an issue it would be way cleaner anyway if they just returned a string and the conversion happens in the caller.

[dvermd]

Nice! I'll try it out tomorrow

Eagerly waiting for your feedback on loading the modules you're used to use.

What if the subclasses only provide the primitive get functionality that returns a string and the base class has all the (non-virtual) templated methods that handle the conversion?

That would be an option but I find it's kind of shame to use a string API when we can directly read the right type from lua.

Luwra handles getting bool,int and float values as their equivalent C++ type and also getting int and float values as string but not bool values.
This means we can still implement your idea of always reading string from lua and then converting to the right type in C++ if we special case the bool type like other types not known by lua.

If we want to leverage lua typing and move more polybar types into lua typing, we need find a better way than a string API.
Using a union as return type is not possible as it can't contain string entry.
std::variant is an option if polybar is using C++ 17 ?
Using a macro would work to avoid repeating the if.
Else, any other design idea ?

This would probably require more refactoring though. And all the templates need to be removed from the dereference methods, but that shouldn't be an issue it would be way cleaner anyway if they just returned a string and the conversion happens in the caller.

These refactoring would need to happen in another PR not to clutter this one.

[patrick96]

Sorry, I haven't found the time to try it out yet. It does seem to compile though :)

---

We should definitely let our users use lua's data types other than strings for their config values.

However, I believe that reading lua's different primitive values as strings and then using convert<T> to turn them into the desired values is the most economical option right now.

Otherwise, the primitive get method would need to return some type of object that represents one out of many types. The code overhead to do this and then pass that object to the convert<T> methods will be quite high, regardless of what approach we choose.

That being said, I do think implementing this will be necessary at some point, when we want values on the lua side to get more complex.

For example, I imagine that at some point, values like "percentage with offset" can be represented as tables in lua: height = pwo(100, -10, units.pt), which would generate some table that corresponds to 100%:-10pt.
To load such a value, you would still call get<percentage_with_offset>(...).
On the config side, for INI, it would load a string as before, for a LUA config, it may now load this table and the convert<percentage_with_offset> would have to know how to convert either a string or a table (probably with a type check to ensure it is actually a table for a percentage_with_offset).

Let's defer this design decision on how exactly to do this until we actually have to deal with such more complex values. What do you think?

---

I do have some ideas though. Just writing them down because I already racked my brain over this:

• Unions can work, you just have to have an additional string struct member that holds the string value for those instances of the union that require a string. I am doing this in the elements struct in include/tags/types.hpp. There, the string member named data may hold some string data depending on tag_type and tag_subtype. As long as this complexity is hidden from users of the class, this doesn't work too bad, the struct is just slightly larger because it always contains a string.
• For this release we are finally using C++17, so we can use std::variant. I have never used them myself, but from everything I have read, they seem very clunky and unintuitive and using them looks very verbose since you can't seem to switch over them.

I really wish C++ had proper algebraic data types 😞

[patrick96]

Alright, I'm testing it out right now and I'm getting really excited about this.
Right now I have some large bash scripts that do string concatentation for use in the INI config, with lua I would be able to remove all that and just do it in lua.

It is still quite easy to crash polybar though 😉

• Not adding a bar name no longer works (it should select a bar name if there is only a single one). For INI configs, it just doesn't create any modules and terminates. For lua, it throws an internal string error somewhere (basic_string::substr: __pos (which is 4) > this->size() (which is 0)).
• If loading the lua file fails for any reason (syntax error, runtime error, file not found, etc.), polybar just prints loading lua returned 1 without more detail but continues on running failing later when it can't find the bars variable.
• If a bar's background isn't a list, an error is produced: Wrong type for parameter bars['bottom']['background']
  • Most likely because polybar first tries to load the background list in case a gradient is used.
• Lists seem to be non-optional. E.g. if you try to create a script module, it complains about the missing env parameter.

These are probably all quite minor issues. The more fundamental issue I have found concerns lists. The current approach works great if we just want to load a list of value (font-0, etc. turns into font = {...}).
However, there are some places where each element in the list may have additional properties.
Off the top of my head these are (probably not exhaustive):

• ramps: There each list element can have any label properties in addition to a weight property.
• animations: Each animation step can have any label properties. Additionally, the entire animation itself has a framerate property.

Not sure how we want to reconcile this in the current implementation. Any ideas?

[dvermd]

Since C++ 17 is available now, I'm experimenting with another lua wrapper library (sol2) which might make error handling better

[dvermd]

Your points on not supplying the bar name and bar background list or string should be addressed.
I need to test the script and env list.
Also error handling should be better now,

Concerning the ramps/weight, the object they contain is quite complex:

• a text without key
• some label parameters with the label keys
• a weight attribute

I think one option is to define all entries ramp-xyz-n-* in the lua config file as they are not read as a list.
If we want to use more lua style syntax the code reading ramp has these flows:

• Each ramp item is not read as a whole from the config ini but many calls to get with a list index suffix to iterate the list.
• The list is not read as a list with get_list so the config lua API won't apply.

So, I think reading ramps configuration must be changed to use the get_list with a type that is to be created and then parsed as text + label, weight. What do you think ?

We are kind of back to the problem of the #2515 where maps can't be read from the config file with the ini API.

Adding the functions convert<T>(map<string,string>), get_from_map<T>(section, key) and get_list_from_map<T>(section, key) will solve these problems and make the lua API easier.
These functions would also replace the get_with_prefix function.

The parsing of a label would be the first client of these new functions.

This might also apply to animations/framerate though I didn't look at them.

[patrick96]

I've been away last week, so I couldn't look at this yet.

For the library, I'm fine with anything that has robust maintenance. I just don't to have a situation like xpp, where we use the library and end up having to maintain it ourselves because upstream becomes unmaintained.
We can also just use the raw lua C api (maybe with some light wrapper code written by us).

I think in the end, the config API will definitely have some way to read an entire map from the config. The question is whether we should implement this in this initial version or not.

If not, everything that is reading a list that may have additional properties cannot use get_list and the lua config will require a setting with the full name for each entry, just like the INI config.

Otherwise, feel free to go ahead and implement this as you like. I'm assuming the convert<T>(map<string,string>) function would be used to convert a map into a label, right?

---

I have been doing some thinking around returning different types in the config API.
I'm just writing it down here, to put my imagination into words so that we have a starting point when we need it.
I was thinking that the values returned from the config API would be objects. Let's call this the config::value class.
The class can represent different values: primitive, table, list, and maybe more. If needed, primitive values can also be split up into string, integer, etc.).

For example, reading something like ramp-volume as a list, would return a single value object that represents that list.
On that list, we define methods that let us retrieve the elements in the list.
For lua, we can (more or less) wrap the lua table.
For INI, the object itself is only a proxy and does not hold any value, only if an element of the list is read, it looks up the ramp-volume-X key and returns it, again using a value object.

Now the question is how would we be able to retrieve something like ramp-volume-X-background.
I would propose that the config::value class has an API to read a sub-property of the setting, let's call it get_property for now.
In lua, this is trivial because it just indexes into the table. But for INI, the value object representing ramp-volume-X would as part of the get_property("background") call, try to look up ramp-volume-X-background and return it.

My goal with this is to hide as much complexity as possible from the config API clients, behind the scenes, the config API does all the work.
I'm not saying that we should implement it exactly like this, it's just what I came up with. You are writing the code, I'm sure you also have some thoughts on this.

[dvermd]

I just came up with the same solution but named it config_value 😄.
The API I'm aiming at is inspired by the sol3 API and needs polishing but would look something like:

• config will have a config::value operator [] (string) method
• maybe config::value config::get_module(string), config::value config::get_bar(string) and config::value config::get_setting(string) methods
• config::value will have the config::value operator [] (string), config::value operator [] (int) and T as() methods.

With this API, retrieving ramp-volume-X-background is translated to ["ramp-volume"][X]["background"].as<rgba>() and ["ramp-volume"][X].as<label_t>() should/would also be possible.
We might also define types derived from label for Ramps and Animations and define the corresponding as/convert methods.

My next moves here will be more refactoring of configbefore adding lua:

• write unittests for config
• refactor the code to be explicit about getting values for config for settings, modules or bar
• write the config::value class and the operator [] of config
• Add tests for the operator [] methods of config::value
• Move calls to config::get to config::[], config::value::[] and config::value::as()

[patrick96]

Hey, sorry for the communication blackout. Just wanted to let you know that I'm having a busy few weeks, hopefully things slow down in a week or two.

Until then I can't promise any in depth reviews unfortunately.

I did see your config tests PR though. I appreciate it, that should really help us in keeping the same user-facing semantics when it comes to reading complex config values.

[dvermd]

Hey, sorry for the communication blackout. Just wanted to let you know that I'm having a busy few weeks, hopefully things slow down in a week or two.

Until then I can't promise any in depth reviews unfortunately.

I totally understand that, thank for letting me know