Skip to content

Conversation

@streetturtle
Copy link
Owner

Do not rely on beautiful, as it might be initialized after the widget, or not initialized at all. Instead, use default valuesm which can be overriden by user.

@streetturtle streetturtle force-pushed the 274-make-beautiful-optional branch from 5b8752f to 88e37c2 Compare June 14, 2021 01:18
@raven2cz
Copy link

Very nice as "nice" ;-)
Now it is open for everyone. It is common approach for all your widgets and can define common skeleton template to unify it.

Just one last idea, which has to be tested (it is idea only).
The condition with "nil". If beautiful is not still initialized, but it is already available. It is typical situation that all entries are already included and beautiful colors are ready in the theme. So, open very easy possibility to reach beautiful instance as parameter too.
So, just include one line in the code only:

local beautiful = args.beautiful or beautiful

and use the local variable for default configuration section.

local mounts = args.mounts or {'/'}
local timeout = args.timeout or 60

if beautiful ~= nil then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this working? You require the beautiful library, so the beautiful variable is the instance. It has to exist.

Maybe you should replace this check by :

if not #beautiful.get() then 

Or another more robust check would be to test each field you need from beautiful.

(Also, I personally don't think this is a good implementation. You basically replace the module level config values from the instance constructor. I mean, it works but it feels like encapsulation breakage since an instance is altering a class level property that can affect other instances.)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is nice example when the unit tests can detect fast these errors. I know that this is lua, not java, or C#, but unit tests are necessary for every language. It is larger investment from the beginning, but this investment will return very fast.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or another more robust check would be to test each field you need from beautiful.

Like this?

config.widget_bar_color = beautiful.fg_normal == nil and config.widget_bar_color or beautiful.fg_normal

(Also, I personally don't think this is a good implementation. You basically replace the module level config values from the instance constructor. I mean, it works but it feels like encapsulation breakage since an instance is altering a class level property that can affect other instances.)

You're right, maybe it would be better to shadow config in the constructor then? Something like this:

local function worker(user_args)
    local args = user_args or {}
    loca config = config

    if #beautiful.get() ~= nil then
        config.widget_bar_color = beautiful.fg_normal == nil and config.widget_bar_color or beautiful.fg_normal

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or another more robust check would be to test each field you need from beautiful.

Like this?

config.widget_bar_color = beautiful.fg_normal == nil and config.widget_bar_color or beautiful.fg_normal

It can even be easier with:

config.widget_bar_color = config.widget_bar_color or beautiful.fg_normal

The Lua or operator returns the first "true" value. SO the nullity check is handled by or.

(Also, I personally don't think this is a good implementation. You basically replace the module level config values from the instance constructor. I mean, it works but it feels like encapsulation breakage since an instance is altering a class level property that can affect other instances.)

You're right, maybe it would be better to shadow config in the constructor then? Something like this:

local function worker(user_args)
    local args = user_args or {}
    loca config = config

    if #beautiful.get() ~= nil then
        config.widget_bar_color = beautiful.fg_normal == nil and config.widget_bar_color or beautiful.fg_normal

What do you think?

I'm not really a fan of shadowing values. I don't even think it would work as expected here, beautiful.fg_normal == nil and config.widget_bar_color I think it will use the "new" config table instead of the global.

I did a quick PR to show you what I have in mind with this pattern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants