Skip to content
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

Very WIP but slowly convert layoutbox to accept template #2955

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

SethBarberee
Copy link
Contributor

Closes #2950

Took a crack at it... still trying to figure out awful.widget.common and wibox.widget.base for the purposes of the template. Commented with my TODOs of where I'm currently tripped up.

@codecov
Copy link

codecov bot commented Jan 1, 2020

Codecov Report

Merging #2955 into master will decrease coverage by 7.26%.
The diff coverage is 66.12%.

@@            Coverage Diff            @@
##           master   #2955      +/-   ##
=========================================
- Coverage   88.07%   80.8%   -7.27%     
=========================================
  Files         650     494     -156     
  Lines       44324   28037   -16287     
=========================================
- Hits        39037   22655   -16382     
- Misses       5287    5382      +95
Flag Coverage Δ
#gcov ?
#luacov 80.8% <66.12%> (-10.18%) ⬇️
Impacted Files Coverage Δ
lib/awful/widget/layoutbox.lua 78.84% <66.12%> (-21.16%) ⬇️
lib/beautiful/gtk.lua 6.33% <0%> (-85.28%) ⬇️
lib/gears/wallpaper.lua 11.36% <0%> (-81.23%) ⬇️
lib/awful/widget/calendar_popup.lua 11.88% <0%> (-78.36%) ⬇️
lib/awful/client/shape.lua 22.95% <0%> (-77.05%) ⬇️
lib/awful/mouse/client.lua 19.64% <0%> (-72.67%) ⬇️
lib/awful/mouse/snap.lua 16.23% <0%> (-72.66%) ⬇️
lib/awful/mouse/drag_to_tag.lua 28.12% <0%> (-71.88%) ⬇️
lib/awful/spawn.lua 22.63% <0%> (-68.55%) ⬇️
lib/awful/keyboard.lua 25.75% <0%> (-63.64%) ⬇️
... and 236 more

end

function w._do_layoutbox_update()
-- Add a delayed callback for the first update.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the idea behind it?
as far as i understand it, in case of multiple updates, it will take the first of them, not the last one

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still working with it. I've mainly been looking at tasklist and taglist as templates. My answer right now: 🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the idea behind it?

For stuff like the taglist, calling awful.widget.common "update" and "label" is very expansive. Usually, more than 1 tag is created in the same event loop iteration. Without batching those calls, it slows down startup a lot. With the new client layout append API, the layoutbox will have the same problem, so I am happy to see batching being integrated so early! Usually it was the result of my profiling AwesomeWM and trying to address some bottlenecks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Elv13 and what about second part of my question?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have one more question : queued_update is a local table. It means each instance of layoutbox will have its own queued_update. screen is also local to the layoutbox instance.

So, why don't we use a basic boolean variable, instead?

OR: the declaration of queued_update should be moved out of the constructor. (And probably use something else than the screen to identify the layoutbox instance, IMO.)

@SethBarberee
Copy link
Contributor Author

Rebased and took another crack. Mainly getting errors around the reset part. Reading more of the code to understand what is going on.

[ 77%] Generating doc/images/AUTOGEN_awful_notification_geometry.svg
Result: 1
Error:
lua: /home/seth/seth-awesome/lib/awful/widget/common.lua:124: attempt to call a nil value (method 'reset')
stack traceback:
	/home/seth/seth-awesome/lib/awful/widget/common.lua:124: in function 'awful.widget.common.list_update'
	/home/seth/seth-awesome/lib/awful/widget/layoutbox.lua:88: in upvalue 'update'
	/home/seth/seth-awesome/lib/awful/widget/layoutbox.lua:179: in function 'awful.widget.layoutbox.new'
	(...tail calls...)
	...seth/seth-awesome/tests/examples/shims/_default_look.lua:10: in main chunk
	[C]: in function 'require'
	...h-awesome/tests/examples/awful/notification/geometry.lua:6: in main chunk
	/home/seth/seth-awesome/tests/examples/awful/template.lua:12: in main chunk
	[C]: in ?

@Elv13
Copy link
Member

Elv13 commented Mar 8, 2020

Hi, thanks for this effort. I created a relax_awful_common branch in my fork with a commit to help with the reset issue. Maybe an alternative would be to only use/expose awful.widget.common._set_common_property and expose the custom_template local function rather than try to use the whole stack. That code has only really been used with layouts and is ancient. It predates even the current widget system itself (we had 3 different rewrite of that system).

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add callback and widget_template parameters for awful.widget.layoutbox
4 participants