Required "beautiful" library is bad practice approach

[raven2cz]

I checked widgets inside this git project.
There are several problems which preclude general usage of this library (for example against lain libs).
I will describe few points for beginning:

• required "beautiful" library cannot be use directly in the widgets. New style of AW configuration has part of widgets configuration BEFORE than the beautiful instance is ready. So, the required instance is nil.
• In addition, widget cannot directly takes beautiful.fg_normal, beautiful.bg_normal, etc. This is not acceptable for common library widgets. You have to implement default values and allow to declare override parameters by user settings. Look to "lain" and "nice" git repos how this is solved.
• do you have or do you plan some unit tests? This project can have big potential but it needs stable and continual testing.

AW is very good WM. Good and robust widgets can improve this WM to new level.

[Aire-One]

required "beautiful" library cannot be use directly in the widgets. New style of AW configuration has part of widgets configuration BEFORE than the beautiful instance is ready. So, the required instance is nil.

Can you elaborate this? (I don't know every widgets from this repo so I may have missed something)

From what I can see in the current code, the usage of the beautiful library is exactly what it was designed for.

Beautiful is exposed as a singleton (https://en.m.wikipedia.org/wiki/Singleton_pattern), it has no incidence to require it before it is initialized, as long as you don't try to access properties (in which case, properties would be indeed be nil) .

This is the case of widgets from this module. All the access to beautiful properties are done in widgets constructor(/instances?). It is the responsibility of the user to initialize beautiful before creating widgets. I don't see anything abnormal here.

In addition, widget cannot directly takes beautiful.fg_normal, beautiful.bg_normal, etc. This is not acceptable for common library widgets. You have to implement default values and allow to declare override parameters by user settings. Look to "lain" and "nice" git repos how this is solved.

Why?

I don't see why a third party module couldn't use these theme variables. Every "base" widgets from awesome also uses these variables [1].

The main advantage of doing so is to keep the theme consistent without needing the user to define X times the thisotherwidget_bgcolor = 'once again the same value'.

I can understand why you want custom theme variable, but you should ask it a as feature request instead of doing a rant... (and maybe propose a PR, this is not hard to implement, but time consuming)

[1] Most widgets defines their own custom theme variables but they defaults to "base" variables in case it's not part of the user theme.
A common way to access (let's say) the background color is to check variables in this order : instance parameter > beautiful.mywidget_bg > beautiful.bg > default hard-coded in the module.

[raven2cz]

required "beautiful" library cannot be use directly in the widgets. New style of AW configuration has part of widgets configuration BEFORE than the beautiful instance is ready. So, the required instance is nil.

Can you elaborate this? (I don't know every widgets from this repo so I may have missed something)

From what I can see in the current code, the usage of the beautiful library is exactly what it was designed for.

Beautiful is exposed as a singleton (https://en.m.wikipedia.org/wiki/Singleton_pattern), it has no incidence to require it before it is initialized, as long as you don't try to access properties (in which case, properties would be indeed be nil) .

This is the case of widgets from this module. All the access to beautiful properties are done in widgets constructor(/instances?). It is the responsibility of the user to initialize beautiful before creating widgets. I don't see anything abnormal here.

The reason is that the assembly of panel with widgets is part of theme.lua (or some additional modules which are part of theming, not part of rc.lua, like for example copycats).
In addition, there is implementation change from previous function:
function theme.at_screen_connect(s)

to usage registration of listener for define panels and its widgets:
screen.connect_signal("request::desktop_decoration", function(s)

but in this case the connect_signal method is called from theme initialization part, not by previous "hack"
rc.lua -> awful.screen.connect_for_each_screen(function(s) beautiful.at_screen_connect(s) end)

Yes, there is still possible to create some additional function inside the theme and call it again similar way as previous hack in rc.lua, but it is workaround and not necessary for screen.connect_signal("request::desktop_decoration", function(s).

I'm using this approach 2 years, and I haven't problems with any other widgets, because beautiful was not directly used inside the common widgets. I found the problems with these widgets:

• fs-widget (MAINLY, because there is not defined any parameters, and beautiful is taken directly, all next widgets are defined much more better with input parameters, and beatiful is taken as just default).
• volume-widget
• ram, cpu etc.

Main problem are widgets where is using just beautiful inside methods and functions. Or worst in the initialization part in worker function without possible input parameters for configuration support of widget.

Good example of much more better approach is visible in the "nice" git plugin (you know it, we discuss some correction in few days before). https://github.com/mut-ex/awesome-wm-nice
Section init.lua lines 109 - 170. And filling in the initalization part line 923.

As benefit, the "beatiful" will be taken in the initialization part only. The "large" widget has to operate with private fields only not with "beautiful" mapping in every widget method.

Any time, any developer can change just ONE initialization line for other colors or parameters, not change 15 places with beatiful.bg_normal, or bg_focus, or bg_urgent or tons of additional colors which are "mapped" to the widget. This mapping has to be just one in the initalization part as default (not without input parameters) and add correctly defined input parameters too.

[Aire-One]

Oh ok! I wasn't very familiar with the awesome-copycat code base. I took a closer look at their code with your explanation. Now I understand better what you are doing/what you want.

(My personal point of view : I'm honestly not convinced by this design, beautiful shouldn't be responsible for instantiating widgets.
It only complexifie the code logic with the rc->theme->for each screen->beautiful.at_screen_connect, just as you said. (Although, I understand why/how this design can be useful in the case of copycat))

I quickly checked the source code for fs-widget (the first one you mentioned). The hard dependency on beautiful can be removed as you suggested by adding parameters/specifying defaults in the widget. I think it would be enough to allow the usage of the widget from the theme (correct me if I'm wrong).

Good example of much more better approach is visible in the "nice" git plugin (you know it, we discuss some correction in few days before). https://github.com/mut-ex/awesome-wm-nice
Section init.lua lines 109 - 170. And filling in the initalization part line 923.

I totally agree with you! This is a very good design. (I would however, have added a fallback on beautiful variables before the defaults from module.)

I used a similar approach with some of my custom widgets and I'd like to enforce this design/pattern to standard awesome widgets (right now, most of the widgets do the test "property vs beautiful vs default" every time we need a theme variable).

[streetturtle]

Thank you very much for a constructive criticism, I really appreciate it!
Would you please give your opinion on the following approach: 88e37c2

The idea is simialr to the one in nice - define widget's config parameters inside the widget, then, if beautiful is not nil, fallback some of them to beautiful (I'm using the basic ones: bg/fg_normal/focus/urgent, which I think are defined in most of users' configs), and then allow user to override them.

[raven2cz]

Thank you too, very nice PR.