awesome icon indicating copy to clipboard operation
awesome copied to clipboard

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

Open Aire-One opened this issue 4 years ago • 4 comments

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_template parameter (?)~~ 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

Aire-One avatar Aug 28 '21 16:08 Aire-One

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%) :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

codecov[bot] avatar Aug 28 '21 16:08 codecov[bot]

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.

Aire-One avatar Sep 29 '21 20:09 Aire-One

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.

Aire-One avatar Nov 26 '21 18:11 Aire-One

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

Elv13 avatar Nov 28 '21 23:11 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.

Elv13 avatar Oct 16 '22 09:10 Elv13

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

Crylia avatar Oct 26 '22 16:10 Crylia

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 avatar Oct 27 '22 07:10 sclu1034

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 avatar Oct 29 '22 17:10 Aire-One

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.

Elv13 avatar Nov 14 '22 00:11 Elv13