Improved swallowing #140

eylles · 2021-11-09T01:07:57Z

This is an incremental improvement of the swallowing module, so far what has been done:

Changed the flags for the pstree command so it is a single line with all ancestors.
Removed usage of sed, grep and tr so the iopopen call should be faster.
Find and match of parent pid in the ancestors string is done in lua so should be faster.
Rework the don't swallow filter and add table for clients that cannot swallow their parent.

What i still plan to do:

~~Rework the don't swallow filter and add table for clients that cannot swallow their parent.~~ done
~~Add persistent properties so parent clients can be restored after awesome restarts~~ can't decide on the better way to implement this so won't do it in this PR

altho i still have those plans the changes could be added to a later PR, also i have a prototype for removing iopopen altogether and instead use an async function but i'm still new to async functions specially in lua so that specific prototype may still have jank in it.

Changed the flags for the pstree command so it is a single line with all ancestors. Removed usage of sed, grep and tr so the iopopen call should be faster. Find and match of parent pid in the ancestors string is done in lua so should be faster.

Now you cna filter window swallowing with 2 tables, one for clients that cannot be swallowed (dont_swallow_classname_list) and one for clients that cannot swallow their parents (cant_swallow_classname_list).

eylles · 2021-11-10T21:47:08Z

@Nooo37 any feedback before i edit the documentation ?

Nooo37

Seems to work fine. Can you just explain why we need a cant_swallow_classname_list list? What happened to Yad without that list for you?

module/window_swallowing.lua

eylles · 2021-11-14T01:40:36Z

ah perhaps yad was not the best program to put there, really the one i had problems was dragon but didn't put it in the list since there's no fixed class name for dragon as that is modified at compiletime and every distro i've seen sets a different name.

the addition of a secondary list for one adds flexibility but also takes care of some edge cases with programs like libreoffice that for some reason despite being added to the filter all their dialogs would swallow them (this was aggravated with the modification to the method to search the parent pid) but new windows with different documents would not.

also as mentioned on the commit message (altho not specified) we get more fine grained control as one list checks agaisnt the class of the current client and the other list checks against the class of the parent , so you can have clients that can't swallow their parent and parents that don't get swallowed (but they themselves can still swallow their parent).

Aire-One

I'm thinking about using the swallow module in my config. Since this is a new PR on this module, I thought I could take a look at it.

Feel free to dismiss any comment I made. They are mostly programming good practice and shouldn't alter the current implementation.

module/window_swallowing.lua

Aire-One · 2021-11-14T13:25:58Z

module/window_swallowing.lua

+end
+
+-- checks if client classname can be swallowed
+local function check_if_swallow(class)


I think this function can be shortened to :

local function check_if_swallow(class) return not (activate_dont_swallow_filter or gears.table.hasitem(dont_swallow_classname_list, class))

(A quick note btw : as a good practice, you should only use positive name for boolean values, it makes this kind of check easier to read (maybe ignore_shallow or filter_shallow are better names))

i didn't change the original names but i'm awful at naming so having this feedback is invaluable, may go for filter_swallow

module/window_swallowing.lua

Aire-One · 2021-11-14T14:17:46Z

also as mentioned on the commit message (altho not specified) we get more fine grained control as one list checks agaisnt the class of the current client and the other list checks against the class of the parent , so you can have clients that can't swallow their parent and parents that don't get swallowed (but they themselves can still swallow their parent).

I feel like cant_swallow_classname_list/dont_swallow_classname_list doesn't seem like describing this behavior. (and I wasn't sure of that either without the explanation)

Is it possible to change these names? Something like ignore_shallow_children for parents that don't shallow and ignore_shallow for client that demand to not be shallowed?

eylles · 2021-11-14T16:35:01Z

yeh i will change the names, i'm not the best at naming things so i just went with whatever name in hopes someone may come with something better

eylles · 2021-11-16T18:00:13Z

made the names more descriptive and proceeded to update the docs, i hope everything is readable and clear.
@Nooo37 please comment on anything else that should be changed.

module/window_swallowing.lua

Aire-One

As usual, feel free to dismiss my comments 😃

module/window_swallowing.lua

Aire-One · 2021-11-21T12:54:04Z

module/window_swallowing.lua

+local function check_swallow(class, list)
+    if not swallowing_filter then
+        return true
+    else
+        return not is_in_table(class, list)
+    end
 end


nit :

if X return true else return Y -> return X or Y

local function check_swallow(class, list) return (not swallowing_filter) or (not is_in_table(class, list)) end

Lua stops testing value as soon as it found a trusty value. It means that whenever (not swallowing_filter) is true, (not is_in_table(class, list)) will not be executed.

It can be tested with something like this :

function Y() print('hello') return true end -- X or Y true or Y() -- true false or Y() -- hello true

i'm trying to do this but can't get it to work

if -- will search for "(parent_client.pid)" inside the parent_pid string ( tostring(parent_pid):find("("..tostring(parent_client.pid)..")") ) and ( (not swallowing_filter) and ( (not is_in_table(parent_client.class, parent_filter_list)) or (not is_in_table(c.class, child_filter_list)) ) ) then

making the logic of this has been a headache so i tought of this

swallow filter { if active check the lists }
parent check func { returns true if parent isn't in list }
child check func { returns true if child isn't in list }
swallow parent func { if true will swallow window }

x a b out

swallow filter parent check child check swallow parent

F F F T

F F T T

F T F T

F T T T

T F F F

T F T F

T T F F

T T T T

the case where the swallow filter is active and both the child and parent are in the lists should not even happen to begin with...

so to shorten this thing so far what i'm thinking is to have 1 function (or perhaps adding another if statement) that checks the var for the filter and if the filter is on then it bothers checking the classes against the corresponding list, so the function would recieve 2 inputs which will be the classes of the parent and children, at least that is the best idea i had.

figured something out but is far from the elegance of a chain of pure and,or, not operators and parens.

Thanks for the truth table. Yeah, you're probably right to make a separated function. It makes the code more readable.

eylles · 2021-12-09T01:26:11Z

i've been doing some testing and some programs like krita have their splash screens be a parent process of the program window, so i will have to add a check to always ignore splash parents.

eylles · 2021-12-17T02:07:24Z

shortened the logic of the checks for the classes to swallow and made the assignment of the swallowing filter more robust, will proceed into trying to make this async and dunno if trying to also tackle the problem of splash parents in this PR

this prevents krita and similar missbehaving programs from swallowing their splash window.

eylles · 2021-12-17T19:31:20Z

okay all left is making this async...

eylles · 2021-12-17T19:49:06Z

aaaaaaaaaaaaaaaand i'm done with this for the time being, next thing to implement would be persistance of the parent child relation of clients between restarts of awesomewm but i'm not doing that on this PR.

eylles · 2021-12-17T20:13:04Z

@Nooo37 @Aire-One

Aire-One · 2021-12-19T15:31:37Z

module/window_swallowing.lua

    or { "firefox", "Gimp", "Google-chrome" }
 local child_filter_list = beautiful.child_filter_list
-    or { }
+    or beautiful.dont_swallow_classname_list or { }


Just a thought I have reading this : There is a corner case where the user can require the module before calling beautiful.init. In this situation, the user defined lists in their theme will be ignored because these variables here are global to the module and initialized only here, at the module first load.

that is a corner case with all of bling and the docs specify requiring bling after beautiful.init

https://blingcorp.github.io/bling/#/?id=installation

Aire-One · 2021-12-19T15:33:39Z

module/window_swallowing.lua

    end
+    return res


Shouldn't the function returns false when window_swallowing_activated is false?

from what i can tell no, since window_swallowing_activated = false means that the swallowing function will be disconected from all clients.

105 local function start() 106 client.connect_signal("manage", manage_clientspawn) 107 window_swallowing_activated = true 108 end 109 110 local function stop() 111 client.disconnect_signal("manage", manage_clientspawn) 112 window_swallowing_activated = false 113 end 114 115 local function toggle() 116 if window_swallowing_activated then 117 stop() 118 else 119 start() 120 end 121 end

however if you meant the boolean value of swallowing_filter then reffering back to the truth table the output value of this function should only be false when the filter is active and either parent or child is in any of the filtering lists.

You're right. I have forgotten the use of this variable and thought it has what the user change to activate the module... My bad.

Aire-One · 2021-12-19T15:35:25Z

module/window_swallowing.lua

+    elseif parent_client.type == "dialog" or parent_client.type == "splash" then
+        return


as mentioned earlier some programs with splash windows can have the splash window as parent process of the main window, and in the case of krita it's splash wrongfully has the dialog type, now if krita is the only program that has that bug i don't know, will have to keep checking the window type of programs with splash windows to be sure.

module/window_swallowing.lua

this was probably an error handling/logging on the code i used as template for the async function and it's callback.

if the parent is destroyed "on background" then awesome will show a warning.

Aire-One

I have been playing with this for a bit over a week now. I think it worked good!

Maybe the filter list can be improved, but that's a taste question and can be done by the user.

i don't see my additions as substantial enough to justify this but... ¯\_(ツ)_/¯

eylles · 2022-01-03T22:20:07Z

@Nooo37 feel free to squash the commits.

kodesoul · 2022-01-06T14:48:46Z

Hello ! this script is just what I was l looking for @eylles . Brilliant work ! Quick question, is there a reason why _client.turn_off() and _client.turn_on() don't set the c.hidden property instead ? Are there any issues with how awesome handles client properties ?

Nooo37 · 2022-01-08T09:59:28Z

@princebett When I wrote client.turn_on/off I didn't even consider that tbh. Might be worth to take a look on that, have you tried doing that? It's only a couple of lines to change. The scratchpad module uses the same helpers and got a issue due to that so that module might also profit from a change.

eylles · 2022-01-08T10:05:07Z

I have been working into other stuff but will take a look into the helpers, do some tests and maybe open the PR later today, i do not use bling's scratchpad module so no idea what the effect will be there.

eylles · 2022-01-10T03:51:27Z

@Nooo37 opened the PR to change the helpers, can you give it a look? #150

kodesoul · 2022-01-10T08:40:24Z

@princebett When I wrote client.turn_on/off I didn't even consider that tbh. Might be worth to take a look on that, have you tried doing that? It's only a couple of lines to change. The scratchpad module uses the same helpers and got a issue due to that so that module might also profit from a change.

I have been using it for the last few days and the behavior has not changed, so I guess it works fine.

eylles · 2022-01-10T08:50:59Z

@princebett can you give a test to #150 an report back there if there's any important behaviour change in any of the modules you use?

kodesoul · 2022-01-12T05:37:13Z

So far so good. I currently only use it for scratchpads and window swallowing and no problems so far. I'll report back in a few days.

Make swallowing more robust

32a528a

Changed the flags for the pstree command so it is a single line with all ancestors. Removed usage of sed, grep and tr so the iopopen call should be faster. Find and match of parent pid in the ancestors string is done in lua so should be faster.

eylles requested a review from Nooo37 as a code owner November 9, 2021 01:07

eylles force-pushed the better-swallow branch from d552356 to 32a528a Compare November 9, 2021 01:10

Improved swallow filter

f28ee92

Now you cna filter window swallowing with 2 tables, one for clients that cannot be swallowed (dont_swallow_classname_list) and one for clients that cannot swallow their parents (cant_swallow_classname_list).

eylles mentioned this pull request Nov 10, 2021

imv doesn't swallowing #139

Closed

Nooo37 reviewed Nov 13, 2021

View reviewed changes

module/window_swallowing.lua Outdated Show resolved Hide resolved

Aire-One reviewed Nov 14, 2021

View reviewed changes

eylles added 3 commits November 14, 2021 18:03

remove occurences of res var

9765637

renamed functions and theme vars more descriptive

6a6e61f

updated module documentation

5f0d3c4

eylles requested a review from Nooo37 November 16, 2021 18:03

check functions combined, is_in_table 1 return

ab5f44e

Nooo37 reviewed Nov 21, 2021

View reviewed changes

module/window_swallowing.lua Show resolved Hide resolved

Aire-One reviewed Nov 21, 2021

View reviewed changes

add old theme vars for compatibility

8670856

gokulswam requested a review from Nooo37 December 2, 2021 20:05

short swallow logic and robust filter assignment

455aa82

ignore splash and dialog parents

2c5f7dd

this prevents krita and similar missbehaving programs from swallowing their splash window.

eylles added 2 commits December 17, 2021 13:33

consistent docs formatting

3e9c4a1

convert module to use an async function

54fbe76

Aire-One reviewed Dec 19, 2021

View reviewed changes

eylles added 2 commits December 19, 2021 11:18

removed undefined error() function call

4465035

this was probably an error handling/logging on the code i used as template for the async function and it's callback.

deal with corner case: parent exits before client

8a87381

if the parent is destroyed "on background" then awesome will show a warning.

Aire-One approved these changes Dec 28, 2021

View reviewed changes

added meself to authors

ec5cc22

i don't see my additions as substantial enough to justify this but... ¯\_(ツ)_/¯

Nooo37 approved these changes Jan 4, 2022

View reviewed changes

Nooo37 merged commit 7542251 into BlingCorp:master Jan 4, 2022

eylles mentioned this pull request Jan 9, 2022

client helpers use client properties #150

Open

x	a	b	out
swallow filter	parent check	child check	swallow parent
F	F	F	T
F	F	T	T
F	T	F	T
F	T	T	T
T	F	F	F
T	F	T	F
T	T	F	F
T	T	T	T

		elseif parent_client.type == "dialog" or parent_client.type == "splash" then
		return

Improved swallowing #140

Improved swallowing #140

Conversation

eylles commented Nov 9, 2021 • edited Loading

eylles commented Nov 10, 2021

Nooo37 left a comment

Choose a reason for hiding this comment

eylles commented Nov 14, 2021

Aire-One left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aire-One commented Nov 14, 2021

eylles commented Nov 14, 2021

eylles commented Nov 16, 2021

Aire-One left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eylles commented Dec 9, 2021

eylles commented Dec 17, 2021

eylles commented Dec 17, 2021

eylles commented Dec 17, 2021

eylles commented Dec 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aire-One Dec 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aire-One left a comment

Choose a reason for hiding this comment

eylles commented Jan 3, 2022

kodesoul commented Jan 6, 2022

Nooo37 commented Jan 8, 2022

eylles commented Jan 8, 2022

eylles commented Jan 10, 2022

kodesoul commented Jan 10, 2022

eylles commented Jan 10, 2022

kodesoul commented Jan 12, 2022

eylles commented Nov 9, 2021 •

edited

Loading

Aire-One Dec 19, 2021 •

edited

Loading