Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

274 make beautiful optional #275

Merged
merged 5 commits into from
Aug 3, 2021
Merged

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.

@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.

3 participants