Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drawin: Add support for the desktop layer. #3750

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Aproxia-dev
Copy link
Contributor

@Aproxia-dev Aproxia-dev commented Dec 10, 2022

I've added support for drawins to be rendered on the desktop layer. this could be used for making desktop icon widgets, as well as wallpaper animations without directly interacting with the root window like i have done here:

we-win-these.mp4

I've also updated the documentation.

@codecov
Copy link

codecov bot commented Dec 10, 2022

Codecov Report

Merging #3750 (11f5eff) into master (1239cdf) will increase coverage by 0.00%.
The diff coverage is 56.52%.

@@           Coverage Diff           @@
##           master    #3750   +/-   ##
=======================================
  Coverage   90.97%   90.98%           
=======================================
  Files         900      900           
  Lines       57500    57521   +21     
=======================================
+ Hits        52309    52333   +24     
+ Misses       5191     5188    -3     
Flag Coverage Δ
gcov 90.98% <56.52%> (+<0.01%) ⬆️
luacov 93.71% <100.00%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
objects/drawin.h 100.00% <ø> (ø)
objects/drawin.c 85.46% <52.94%> (-2.49%) ⬇️
stack.c 93.65% <60.00%> (-2.96%) ⬇️
lib/wibox/init.lua 94.84% <100.00%> (+0.02%) ⬆️
tests/examples/screen/template.lua 96.64% <0.00%> (-0.21%) ⬇️
tests/test-screenshot.lua 97.56% <0.00%> (+0.34%) ⬆️
lib/wibox/widget/imagebox.lua 96.42% <0.00%> (+0.89%) ⬆️
tests/test-input-binding.lua 99.50% <0.00%> (+2.00%) ⬆️
spawn.c 86.16% <0.00%> (+3.64%) ⬆️

@Elv13
Copy link
Member

Elv13 commented Dec 10, 2022

Hello,

Thanks for this. I think there is multiple ways to implement this feature. I think the most trivial would be to expose :raise() and :lower() for the wiboxes and set the "desktop" value to .type (see _NET_WM_WINDOW_TYPE_DESKTOP in https://specifications.freedesktop.org/wm-spec/1.3/ar01s05.html). I also think it has to be reparented somehow to the root window, but i am not sure of how that works or if it's even necessary. @psychon would know more, this is not my area of expertise. Conky, Mplayer and iDesk has the xlib equivalent and set some other hints too.

Also, more generally, the layering code, is you noticed with the if in the foreach, is not very flexible. There is, however, already a WINDOW_LAYER_DESKTOP in there. I think to be cleaner, the wibox code should use the layers like the client do. When combine, it also remove the need to expose the .desktop to Lua since it can use the existing properties and methods.

A much more labor intensive way to handling this would be to replace the hardcoded layers by recursive buckets. Where a bucket can contains either n other buckets or one client or one drawin/wibox. Then you can move buckets between parents and raise/lower/set their position in the parent bucket. This would be a more long term solution, but a lot more work. We have about a dozen known bugs with the current layering systems which would be solved by that. For example, it's not possible to "pair" a wibox and a client when raising them. This feature gets requested from time to time, mostly for people who implemented i3 like tabs. Another is when a client is on multiple tags, raising/lowering them affect the state of other tags (like the magnifier layout). This is the actual blocker for my dynamic layout system.

@Aproxia-dev
Copy link
Contributor Author

Aproxia-dev commented Dec 11, 2022

i don't think i quite understand the bucket approach. the layers are still a part of the freedesktop wm specs, aren't they? how would that still be kept? and how would you pair a client and a wibox, exactly? would it just be them having the same parent bucket?

i would honestly love to implement something like that, though. i believe that a z-index-like feature would be amazing and could have many features. if you could bring me up to speed with the details that you have worked out so far, i could try my hardest to implement something like that.

@Elv13

@psychon
Copy link
Member

psychon commented Dec 11, 2022

Comment on the general approach (whether AwesomeWM devs want this or not is for them to decide):

I've added support for drawins to be rendered on the desktop layer.

I'd really prefer if this gets a different name. EWMH has the layers "desktop", "below", none-of-the-rest, "dock"&"above", "fullscreen". These layers are for client windows. AwesomeWM invented "ontop" for drawins. This has a special name to make it clear that this is its own layer and one cannot mix clients and drawins (drawin above a client above a drawin within the same layer). Not-ontop drawins also get their own layer. This makes the C implementation a lot easier (as @Aproxia-dev noticed, but @Elv13 apparently did not yet?)

Sadly, I do not have a good suggestion for a new name... What is the opposite of "ontop" that is not "below"? bottom?

Thanks for this. I think there is multiple ways to implement this feature. I think the most trivial would be to expose :raise() and :lower() for the wiboxes and set the "desktop" value to .type

You mean "trivial" from the Lua side, right? In C, this idea would be a nightmare to implement. The above would throw out the current way these orderings are managed and would mean that a lot of code needs to be touched.

I'm not sure whether that's a good request for someone's first contribution to a project (at least I did not find your name on https://github.com/awesomeWM/awesome/graphs/contributors ).

@psychon
Copy link
Member

psychon commented Dec 11, 2022

Sadly, I do not have a good suggestion for a new name... What is the opposite of "ontop" that is not "below"? bottom?

https://www.wordhippo.com/what-is/the-opposite-of/on_top_of.html

How about beneath? There is also underside, but I do not like that name...

Adding a new layer certainly doesn't make the layering easier to understand, but that can be fixed by an addition to the docs (or just kept as-is).

Copy link
Member

@psychon psychon left a comment

Choose a reason for hiding this comment

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

@Elv13 What should be done with this PR? Does something like this have any chance or are you vetoing it? If this has some chance, I guess the biggest problem is the name bikeshedding that I started. Any suggestions?

@@ -37,6 +37,13 @@
-- @tparam[opt=false] boolean ontop
-- @propemits false false

--- Below other windows and widgets.
Copy link
Member

Choose a reason for hiding this comment

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

What does and widgets refer to? Wiboxes/drawins are not ordered relative to widgets, are they? I'd just leave that part out

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it would probably be better to say and wiboxes or and drawins, or, as you said, just leave that part out entirely. not sure which one would be better. my bad for getting the terminology wrong lol

Copy link
Contributor Author

Choose a reason for hiding this comment

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

now thinking about it, and drawins is probably not optimal, since there's not many places where drawins are explained in the docs, unlike wiboxes, which have a dedicated page.

@@ -53,6 +53,7 @@ lua_class_t drawin_class;
* @field border_width Border width.
* @field border_color Border color.
* @field ontop On top of other windows.
* @field desktop Below other windows and widgets.
Copy link
Member

Choose a reason for hiding this comment

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

Same as above. What widgets?

if(b != drawin->desktop)
{
if(b)
drawin_set_ontop(L, drawin, false);
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't necessarily work. Lua could react to property::ontop and just set d.ontop = true again. Thus, you are not guaranteed that drawin->ontop = false after this call. I fear one would have to defer all signal emission until the end of this function to work-around this (not that the result would then look good from Lua's point of view, but at least that should ensure that at most one of these properties can be true at the same time).

I think these "multiple exclusive states"-thing also came up in client.c, but I don't know how it was handled there. I fear "not nicely"... :-(

Somehow this feels like it should be an enum, but that would break the magic behind LUA_OBJECT_EXPORT_PROPERTY. I don't have any good suggestion, sorry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think these "multiple exclusive states"-thing also came up in client.c, but I don't know how it was handled there. I fear "not nicely"... :-(

correct, i mostly copied the code over from client.c..
this probably suggests that the way layers are handled should be remade entirely lol

@Aproxia-dev
Copy link
Contributor Author

How about beneath? There is also underside, but I do not like that name...

Adding a new layer certainly doesn't make the layering easier to understand, but that can be fixed by an addition to the docs (or just kept as-is).

i do like beneath a lot, but underneath, or just under might be good too.

@Aproxia-dev
Copy link
Contributor Author

I'm not sure whether that's a good request for someone's first contribution to a project (at least I did not find your name on https://github.com/awesomeWM/awesome/graphs/contributors ).

i made one small pr in late october this year, but it wasn't anything difficult. probably doesn't even count lol... #3734
with that said, i am willing to give it a shot. i might not be very experienced yet, but i think i could handle it with enough time and effort.

@Aproxia-dev
Copy link
Contributor Author

AwesomeWM invented "ontop" for drawins. This has a special name to make it clear that this is its own layer and one cannot mix clients and drawins (drawin above a client above a drawin within the same layer).

i tried reading the freedesktop wm spec so thoroughly to not get the naming wrong and yet i somehow still missed that ontop is not there lmao
the fact that clients can also be on the ontop layer confused me more than i'd like to admit

@psychon
Copy link
Member

psychon commented Dec 11, 2022

the fact that clients can also be on the ontop layer confused me more than i'd like to admit

Hm... this is actually a good argument against my previous argument. AwesomeWM already has such a "clash". Meh...

@awesomeWM/write-access What do you think? How should this be implemented? Should this even be implemented?

@actionless
Copy link
Member

this patch looks quite small so i don't see a problem adding a new feature, but add some examples, to increase the tests' coverage

@Elv13
Copy link
Member

Elv13 commented Dec 12, 2022

So lets start with the parts we all agree on. 1) the problem is real, 2) it is worth fixing, 3) the current layering code is confusing. I made a pull request to start dismantling the old code without actually breaking anything (I think). It would have more iterations to remove the C part of ontop and move this 100% to Lua to the C part can handle the spec part and Lua the quality of life additions.

The PR is rather simple, even if it touches more code than I hoped. The old hardcoded spaghetti is converted to signals. Then Lua is responsible to assemble the final stack of drawin/clients. The current implementation copy pasted from the C code and ported to Lua inline. It still has most of the issues, but changing that would make the PR much bigger and much more complex.

From there, after a few more round of cleanup to split the wm-spec atoms from the API layers, I propose to add a hook to let individual layouts implement their own stacking. The tree based layouts like layout-machi, treesome and dynamite (#644) can use the bucket thing I mentioned above while the other layouts can leave it unimplemented and use the current code.

You mean "trivial" from the Lua side, right? In C, this idea would be a nightmare to implement. The above would throw out the current way these orderings are managed and would mean that a lot of code needs to be touched.

Well, I argue it is worth throwing away :P. I started the process with #3751
This is actually nearly enough to implement raise/lower (like 20 lines away). But first things first, a review.

What do you think? How should this be implemented? Should this even be implemented?

Should it be implemented? Absolutely. This is actually not the first time this was mentioned like, in the past month. It's not even the second or third time. Literally, people have been hitting multiple problems on Discord. 2 of them related to the desktop layer, 1 for a drag and drop tooltip and one for creating a tabbar. Plus there are workaround in my dynamite module, layout-machine and even the main codebase.

The problems are very real and affecting many people. My main issue with the original patch is that it adds more hardcoded stuff rather than solve the larger issue. It also added more public APIs which might not survive fixing the larger problem.

@Aproxia-dev Sorry for the pain and extra steps. What do you think about #3751? Note that I moved x11_layers_keys.WINDOW_LAYER_DESKTOP to the top of the translator function rather than the bottom like the old C code. This should make it easier to implement your desktop layer. I did zero test on the code beside loading the default rc.lua and clicking on things. Would you like to test it and port this PR on top of the other one? It's mostly the same code, just more Lua and less C. I think we can avoid the .desktop property and piggy-back on the existing type property?

@Aproxia-dev
Copy link
Contributor Author

Aproxia-dev commented Dec 12, 2022

What do you think about #3751?

@Elv13 I've suggested some changes, but other than that it seems really good.
The wallpaper animation that I've made actually works perfect with the changes I suggested, because the new PR actually implements layers for drawins, which wasn't the case with the old code there was only the default layer and the ontop layer. Due to this, I'd say that this PR could be left unmerged and we should move the work over to #3751.

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

Successfully merging this pull request may close these issues.

4 participants