[RFC] Implement wibox.widget.template: An abstract widget that handles a preset of concrete widget.

[Aire-One]

Hello 👋

I think it'll need more work to be ready, I hope to get feedbacks to improve this new widget.

This PR is (what I think to be) the response we need to provide a standard API that address issues raised by initiatives like #2955 and #3208.

The idea behind this new "template widget" is to standardize how we want/try to implement widget template across the awful.widget library (and third-party libraries that want to implement user customizable widget). Thanks to this widget, I hope we can build more extendable widgets that allow end-users to easily implement their own customization.

In the scope of this PR, I only want to implement the wibox.widget.template widget and the tools it needs. If you validate this approach, more PRs will come to implement usage of the template widget across widgets from awful.widget.

For the record, here is a naive attempt at implementing the widget template for the layoutbox widget : https://github.com/Aire-One/awesome/tree/feature/widget_template/layoutbox See the next PR #3521

Todos : (EDIT: Done 🎉)

• Documentation
• Usage examples
• Rename widget_template parameter (?) Change in the constructor API makes this point irrelevant
• Intermediate/proxy constructor (to prevent w = layoutbox { widget_template = wibox.widget.template{...} }) Usage is simplified to an argument table
• Save arguments from dismissed update call (?) Fixed thanks to @sclu1034 suggestions

[codecov]

Codecov Report

Merging #3421 (c8016f0) into master (a436478) will increase coverage by 0.04%.
The diff coverage is 95.27%.

@@            Coverage Diff             @@
##           master    #3421      +/-   ##
==========================================
+ Coverage   90.92%   90.96%   +0.04%     
==========================================
  Files         897      906       +9     
  Lines       56677    57149     +472     
==========================================
+ Hits        51531    51984     +453     
- Misses       5146     5165      +19     

Flag	Coverage Δ
gcov	90.96% <95.27%> (+0.04%)	⬆️
luacov	93.68% <95.27%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
lib/gears/shape.lua	97.87% <ø> (ø)
lib/awful/widget/taglist.lua	85.07% <37.50%> (-1.52%)	⬇️
...x/widget/template/concrete_implementation_user.lua	76.92% <76.92%> (ø)
lib/wibox/template.lua	89.51% <89.51%> (ø)
tests/examples/wibox/widget/template/clone1.lua	96.92% <96.92%> (ø)
lib/awful/widget/common.lua	93.57% <100.00%> (-0.06%)	⬇️
lib/awful/widget/layoutlist.lua	89.88% <100.00%> (ø)
lib/awful/widget/tasklist.lua	98.35% <100.00%> (+<0.01%)	⬆️
lib/naughty/layout/box.lua	94.19% <100.00%> (+0.03%)	⬆️
lib/naughty/list/actions.lua	94.50% <100.00%> (ø)
... and 37 more

[Aire-One]

Sorry for the delay before working again on this PR.

Thank you, @sclu1034, for your review 😃 I'm slowly getting back to this PR, I'll try to provide more updates in the coming days.

[sclu1034]

I think this should be moved to a separate section, e.g. "how to create a widget/an API that utilizes widget templates".

The common usage that most users will run into will only be about things like "what to write in awful.taglist { widget_template = ... }?". For those cases, the user will likely just get confused about where they are supposed to call :update() and with what arguments.

In the examples, the same distinction should be made clear, with comments like

-- this is what the user/consumer will write in their `rc.lua`

-- this is what a widget creator will use in their widget's code, e.g. `lib/awful/taglist.lua`

As it is right now, users will likely assume that they are supposed to call :update() in their rc.lua.

[Aire-One]

I agree this can be confusing since with this widget, you have 2 use case : as the widget author and as the end-user. I'll try to improve this documentation part.

Thank you for your input! 😄

(As a quick note : the taglist/tasklist widget are not targets for this widget template implementation. While it is possible to use it as a base for these list widgets, I think we should have a separate list_template widget to handle all the layouting and iterate-over-a-list stuff that these two need.)

[Aire-One]

I had some issue to find the time I needed to finish working on this... I think it's now ready! 😄

All check are green, and I'm not sure how(/whether it's necessary?) to test the remaining lines highlighted by codecov.

Thank you, @sclu1034, for your previous reviews.

[Elv13]

(just a note to say I am in vacations, I will review this when I get back. I really appreciate this work)

[Elv13]

Rebased. Apology for forgetting about this for a year.

I also added a commit to bring it to feature parity with the awful.widget.common code (with the intention of deleting the said code in the next commit). See https://github.com/awesomeWM/awesome/blob/master/lib/awful/widget/common.lua#L107 and https://github.com/awesomeWM/awesome/blob/master/lib/awful/widget/common.lua#L53

One thing I would be inclined to do is move the file from lib/wibox/widget/template.lua to lib/wibox/template.lua and make it an "official top level concept" rather than a container/widget. The rational is that it has "roles" and "update_function arguments" which are worth force-documenting for each template users. And with the slider, layoutbox and desktop icon PRs on the way, this will be used is a lot of places. I would also remove the args and document is as foo.widget_template = wibox.template { ..., widget = wibox.layout.fixed }. This would make it a lot less confusing than the current "raw table" we document. It also allows us to print an error is someone wants to pass a widget instance to a template parameter.

If you are ok with this change, then I have a few more commits to port every widget_template to (appear to behave like) wibox.template. This should be backward compatible.

[sclu1034]

I never liked this approach of just blindly assigning a property to the whole tree. And I don't see any merit in it over explicitly setting role ids where needed.

It's inefficient compared to the by_id caches, especially without any checks on how big the tree is that you're recursing into.

The fact that some properties will "magically" be available on all widgets while others need an id will lead to confusion (and already does with things like icon_role vs awful.widget.clienticon in the tasklist examples).

With this being a public API assigning arbitrary properties to arbitrary widgets, chances rise that not every :set_foo() expects the same foo, i.e. not every widget (especially when taking 3rd-party libraries into account) might expect the same value for the same property name.
E.g. if there was a widget template that used a fg or bg property to set a color based on a value, then you wouldn't want to apply that to an entire widget tree where there might be wibox.container.backgrounds being overwritten.
That wasn't so big of a deal until now, as the only internal implementations of this were tag for the taglist and client for the tasklist, where you could reasonably assume that any widget that has a :set_client() actually wants an awful.client instance. But now, there is no way to reason about what a template and its usage may be.

It's also weird that when ids is set, it only recurses into their children. I can't think of a use case where I'd set a role on a widget and then also need that role on all of its child widgets. Given our separation of layout/container/content widgets, widgets that have child widgets don't have content to display of their own.

So while it's just an optional parameter, giving the user a choice, I think it's a choice they should never take. And while we could just reword the method docs to heavily discourage using the option, simply removing it would prevent the confusion and make the API simpler.

[Aire-One]

Thank you for the amazing work @Elv13!

One thing I would be inclined to do is move the file from lib/wibox/widget/template.lua to lib/wibox/template.lua and make it an "official top level concept" rather than a container/widget.

Agreed.

I would also remove the args and document is as foo.widget_template = wibox.template { ..., widget = wibox.layout.fixed }. This would make it a lot less confusing than the current "raw table" we document. It also allows us to print an error is someone wants to pass a widget instance to a template parameter.

This is something I tried to do, but couldn't find a proper way to implement. With the new template.make_from_value function, I think it's ok.

If you are ok with this change, then I have a few more commits to port every widget_template to (appear to behave like) wibox.template. This should be backward compatible.

I can't wait!

[Aire-One]

ldoc @hidden this method?

[sclu1034]

It's not documented in the first place, so it won't show up either way.
But as it's empty, there is no reason to keep the method itself around.

[Aire-One]

Does the update_callback still make sense with the new :set_property and :bind_property methods?

It seems like these 2 new methods are a more powerful and easier way to achieve what the user would have manually written in the update_callback.

[sclu1034]

As far as I'm aware, those two things are not interchangeable.
:set_property is for the developer who creates a widget that uses the template, to assign values at named roles in an unknown template.
update_callback is for the user who creates the template, to do stuff when values change.

E.g. awful.widget.tasklist will call :set_property internally, the user that creates a tasklist will assign an update_callback to change stuff in the template dynamically.

[Aire-One]

This example is quite interesting.

It's actually a basic implementation of the taglist/tasklist widgets with the new template. We only need a way to automatically attach a self.widget_template:clone() to each item on the tag/task list and assign them predefined :set_property and :bind_property to make it a perfect replacement.

[Crylia]

Wouldn't it make sense to merge #3521 into this one since other widgets like taglist, tasklist etc got rewritten here too?

[sclu1034]

Wouldn't it make sense to merge #3521 into this one since other widgets like taglist, tasklist etc got rewritten here too?

As a reviewer, I'd rather not have that. There is no rush to migrate these things, and the ones Elv already did blew up the PR tremendously.

[sclu1034]

The name doesn't make sense. set_[something] implies a setter for a property, but this just triggers a function call.
And the update_now argument exists only to call self:update() in the constructor, so the entire method could just be replaced with an if in the constructor:

-    ret:set_update_now(args.update_now)
+    if args.update_now then
+        ret:update()
+    end

[Aire-One]

This method is required to make the declarative pattern work as expected.

When creating a widget with the declarative pattern, the constructor doesn't receive all the properties as parameters. Instead, the properties are updated after the widget instantiation.

local w = wibox.widget {
   update_callback = update_fct,
   update_now = true,
   widget = widget.template,
}

The update_now = true property is "transformed" to the invocation of the template:set_update_now method.

Note that this is why the whole method is documented as a "Hack" and is @hidden.

[sclu1034]

Then it could make sense to just call it update_now(), which drill also checks for.

[Elv13]

From a clean code PoV, maybe, but at the same time adding too many ifs to drill() might not be super scalable. Another of my PR has some code to make meta-extending the DOM easier using some _ prefixed extender "I am special, call this and I will do my special stuff" functions. That might be the solution on the longer term.

However for now I think the hack proposed here is good enough. Unexpected, but not in a way which impact end users.

In the even longer term, which is dubious, there is also that issue which calls to replace how the DOM is created and move it to the hierarchy because in practice right now, you can't merge 2 DOMs and that makes :get_children_by_id() unreliable when some composed widgets are used... Including this very widget.... This would be an opportunity to revisit some of the bolt-on hacks I made to restore the declarative widget system. Historically, AwesomeWM 3.1-3.4 had it, but 3.5 had a fully rewritten (and much, much better) widget system. It was fully imperative, which annoyed a large number of users who could not port their config. The code was also very heavy for something like rc.lua, which is supposed to appear like a config file. Back then I mostly did modules rather than contribute to the core, so I wrote the declarative thing as the retrograde module. It eventually got reworked/merged into the core, but was always a bolted on hack rather than something engineered alongside wibox.hierarchy. So it would benefit from a larger rework to be more flexible/integrated.

[Aire-One]

From a clean code PoV, maybe, but at the same time adding too many ifs to drill() might not be super scalable.

I'm not sure to follow what you mean. This if branch already exists.

I have to agree with @sclu1034. A setter is unneeded here. I have somehow missed this feature of drill, otherwise I would have used it instead.

[sclu1034]

It's not documented in the first place, so it won't show up either way.
But as it's empty, there is no reason to keep the method itself around.

[sclu1034]

At this point, I think a separate create_callback would make more sense than using this boolean to treat update_callback as if it was a create_callback.
And this would be required for the "drop-in replacement".

[Elv13]

For the drop-in part, I was just planning at leaving the old code in place. The issue is the function signature. It has the tag/client as part of it, so it's hard to mutualize into a common API. There are ways around it, but it seems simpler to omit create_callback given it's not very useful. Something like gears.connection would be a better fit. I may extract it from the gears.reactive PR (even if they share some code) since it isn't as affected by the linting issues as the other 2 parts.

The whole callback thing is quite unique in our API when you think about it. We nearly always use signals for this kind of stuff. As you pointed elsewhere, it does make sense to keep it here because it makes the users life easier (I agree). gears.connection would overlap, but would still have some learning curve issue and has some footguns (like connecting to signals which don't exist).

[sclu1034]

For the reviewers' sakes, it would be nice to keep commits separated by purpose and to not mix small, unrelated and easy-to-miss changes into a file rename.

[Elv13]

Yeah, sorry, I messed up a rebase somewhere.

[Aire-One]

Wouldn't it make sense to merge #3521 into this one since other widgets like taglist, tasklist etc got rewritten here too?

My idea with this Draft PR was to share a little playground where we can preview how the new template API will be used in other widgets.

I agree with @sclu1034 that this PR is already very big. Include all the awful.widget migration here would basically make this PR reasonably unmergable. I think it's better to have a base API for the template, then iteratively migrate other widgets to the new API. There is some implementation decision we have to think in a per-widget basis to do the migration, so only 1 PR for all the widgets is a bad idea anyway. (One of them is the fact that some widgets comes with a cache implementation that is incompatible with the template idea)

the ones Elv already did blew up the PR tremendously.

Yeah, 1k new lines is a big diff.

(I haven't reviewed the new commits.) [If] the migration included in this PR only includes migrating the old widget_template from the tasklist and taglist (as discussed earlier), I think it's justified to have it here. I'm fine with wibox.template been used (and proofed working as intended/backward compatible) from the first PR.

[Aire-One]

This method is required to make the declarative pattern work as expected.

When creating a widget with the declarative pattern, the constructor doesn't receive all the properties as parameters. Instead, the properties are updated after the widget instantiation.

local w = wibox.widget {
   update_callback = update_fct,
   update_now = true,
   widget = widget.template,
}

The update_now = true property is "transformed" to the invocation of the template:set_update_now method.

Note that this is why the whole method is documented as a "Hack" and is @hidden.

[Elv13]

Mostly all done. Plus I added a new commit to fix the forced_width/forced_height behavior, which wasn't very consistent depending on how the property was set.