[RFC] Implement wibox.widget.template: An abstract widget that handles a preset of concrete widget.
Hello :wave:
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 :tada:)
- [x] Documentation
- [x] Usage examples
- [x] ~~Rename
widget_templateparameter (?)~~ Change in the constructor API makes this point irrelevant - [x] ~~Intermediate/proxy constructor (to prevent
w = layoutbox { widget_template = wibox.widget.template{...} })~~ Usage is simplified to an argument table - [x] ~~Save arguments from dismissed update call (?)~~ Fixed thanks to @sclu1034 suggestions
Codecov Report
Merging #3421 (c8016f0) into master (a436478) will increase coverage by
0.04%. The diff coverage is95.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%) |
:arrow_up: |
| luacov | 93.68% <95.27%> (+<0.01%) |
:arrow_up: |
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%) |
:arrow_down: |
| ...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%) |
:arrow_down: |
| lib/awful/widget/layoutlist.lua | 89.88% <100.00%> (ø) |
|
| lib/awful/widget/tasklist.lua | 98.35% <100.00%> (+<0.01%) |
:arrow_up: |
| lib/naughty/layout/box.lua | 94.19% <100.00%> (+0.03%) |
:arrow_up: |
| lib/naughty/list/actions.lua | 94.50% <100.00%> (ø) |
|
| ... and 37 more |
Sorry for the delay before working again on this PR.
Thank you, @sclu1034, for your review :smiley: I'm slowly getting back to this PR, I'll try to provide more updates in the coming days.
I had some issue to find the time I needed to finish working on this... I think it's now ready! :smile:
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.
(just a note to say I am in vacations, I will review this when I get back. I really appreciate this work)
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.
Wouldn't it make sense to merge #3521 into this one since other widgets like taglist, tasklist etc got rewritten here too?
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.
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.
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.