Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
[RFC] Implement wibox.widget.template: An abstract widget that handles a preset of concrete widget. #3421
base: master
Are you sure you want to change the base?
[RFC] Implement wibox.widget.template: An abstract widget that handles a preset of concrete widget. #3421
Changes from 11 commits
08109a6
d09a704
18ae7bb
d5dbb9d
a005967
2f2beb6
b824d99
29ea4dd
f4088ca
e7db5e3
086eb14
33d5de5
f12bb57
cd5dda3
aaec225
45a458a
bfe4832
8803878
d5382b0
2542c3b
2dab6c3
a7932b1
495922c
67b2b26
3765efa
0759096
cc7feb9
7aab54e
8fc30ae
452d21a
c8016f0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 callself:update()
in the constructor, so the entire method could just be replaced with anif
in the constructor:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
The
update_now = true
property is "transformed" to the invocation of thetemplate:set_update_now
method.Note that this is why the whole method is documented as a "Hack" and is
@hidden
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then it could make sense to just call it
update_now()
, whichdrill
also checks for.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a clean code PoV, maybe, but at the same time adding too many
if
s todrill()
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 likerc.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 alongsidewibox.hierarchy
. So it would benefit from a larger rework to be more flexible/integrated.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 anupdate_callback
to change stuff in the template dynamically.