ssokolow / quicktile

Adds window-tiling hotkeys to any X11 desktop. (An analogue to WinSplit Revolution for people who don't want to use Compiz Grid)
https://ssokolow.com/quicktile/
GNU General Public License v2.0
878 stars 79 forks source link

Quicktile is not detecting IceWM taskbar #118

Open PetteriAimonen opened 4 years ago

PetteriAimonen commented 4 years ago

Currently quicktile places windows so that they overlap the IceWM taskbar.

The Taskbar does have _NET_WM_STRUT set as 0, 0, 0, 49. However, the taskbar window is not listed _NET_CLIENT_LIST because it is not a framed window (i.e. it is not movable). It is listed in xwininfo -tree -root but only several levels deep.

IceWM does set _WIN_WORKAREA and _NET_WORKAREA so that the task bar is excluded. Looking at the version history, it appears that _NET_WORKAREA used to be used but no longer is, apparently to better support setups with multiple differently sized monitors.

I don't really know how I should approach fixing this, whether to modify IceWM or to modify quicktile. Modifying IceWM would require a bit of a hack to handle task bar specially. I'm not sure if it would get accepted by upstream, as the specifications are not exactly clear on whether _NET_CLIENT_LIST should include such stationary windows or not.

Alternatively quicktile could be modified to either search all windows for _NET_WM_STRUT (sounds unnecessarily slow), or to take some kind of intersection of _NET_WORKAREA and individual monitor sizes. But I'm not sure if that might mess up multi-monitor support on some window managers.

I've also been considering just adding a configuration options to add a configurable amount of margin at top/left/right/bottom of each monitor. This could be used to work around all kinds of issues like this, even though it is not a very clean way to do it.

ssokolow commented 4 years ago

Yeah. the spec defines both that and _NET_CLIENT_LIST_STACKING and says "These arrays contain all X Windows managed by the Window Manager. " so other WMs like Openbox and Kwin interpreted that as "include the panels in _NET_CLIENT_LIST".

QuickTile was specifically written to be as "works everywhere" as feasibly possible (as opposed to things like the Compiz Grid plugin) so I'd like to try to come up with a solution that works with stock IceWM. What distro and version are you running? I'll try to grab a VM and experiment with it.

As for _NET_WORKAREA, it's too restricted in its utility to be an acceptable solution because it's the largest rectangle that fits within the usable region, not the smallest rectangle that encompasses it. That's why I got rid of it.

(ie. If you have a 1280x1024 monitor next to a 1920x1080 monitor, _NET_WORKAREA will be 3200x1024, not 3200x1080, and you'll have empty space at the top/bottom of the 1080p monitor. _NET_WORKAREA is no help in distinguishing panel reservation from the difference between adjacent monitor heights.)

As for manual overrides, I actually considered doing something like that for #117 (though it'd have to wait for the planned redesign of the config file to support hierarchical data) but it's looking like GNOME Shell makes up for not setting any _NET_WM_STRUT or _NET_WM_STRUT_PARTIAL information by setting a non-standard derivative of _NET_WORKAREA that stores per-monitor workarea information.

PetteriAimonen commented 4 years ago

Ah, thanks for the clarification on _NET_WORKAREA.

I'm using Ubuntu 20.04, if you install the icewm package and select icewm session from the lower right corner of login screen, I think the issue should be reproducible.

I think I'll ask IceWM maintainers whether a patch adding the panel to _NET_CLIENT_LIST, _WIN_CLIENT_LIST and _NET_CLIENT_LIST_STACKING would be accepted: https://github.com/ice-wm/icewm/issues/18

PetteriAimonen commented 4 years ago

I made a quick test and this brute-force approach does work with unmodified IceWM:

diff --git a/quicktile/wm.py b/quicktile/wm.py
index 020539b..15ec403 100644
--- a/quicktile/wm.py
+++ b/quicktile/wm.py
@@ -99,6 +99,15 @@ class WindowManager(object):
         # TODO: Hook monitor-added and monitor-removed and regenerate this
         # TODO: Hook changes to strut reservations and regenerate this

+    def iterate_all_windows(self, start = None):
+        """Iterate through the whole window hierarchy."""
+        if start is None:
+            start = self.x_root
+
+        yield start
+        for window in start.query_tree().children:
+            yield from self.iterate_all_windows(window)
+
     def update_geometry_cache(self):
         """Update the internal cache of monitor & panel shapes by querying
         them from the desktop and processing them into a
@@ -139,9 +148,7 @@ class WindowManager(object):

         # Gather all struts
         struts = []
-        for wid in [self.x_root.id] + list(self.get_property(
-                self.x_root.id, '_NET_CLIENT_LIST', Xatom.WINDOW, [])):
-            win = self.x_display.create_resource_object('window', wid)
+        for win in self.iterate_all_windows(self.x_root):
             result = self.get_property(
                 win, '_NET_WM_STRUT_PARTIAL', Xatom.CARDINAL)
             if result:

If I understand correctly, the code only runs when monitor layout changes, so perhaps the performance impact is not important?

PetteriAimonen commented 4 years ago

Hmm no, based on the debug output it does run on every command. It does not cause a detectable delay with a small amount of windows, but potentially could do so with a huge amount.

ssokolow commented 4 years ago

Yeah. I haven't had time to look up how to efficiently and reliably set a watch on the creation, modification, or removal of _NET_WM_STRUT_PARTIAL properties across all windows on the system and I've had reports that some people's desktops fire up QuickTile before the panels get created if asked to launch it on startup.

Forcing a refresh on each command run was a return to the approach used in the GTK+ 2.x codebase to work around that regression.

PetteriAimonen commented 4 years ago

One possible approach could be to check it on first command run, and after that only if _NET_CLIENT_LIST has changed.

ssokolow commented 4 years ago

Given how often I create and remove windows, that still sounds risky. Possibly something to be gated behind a config key.

ssokolow commented 4 years ago

I'll still need to spin up a test VM, but maybe, if I can't think of something better, it could specifically detect IceWM and then enable a more thorough complement to _NET_CLIENT_LIST which uses the xlsclients algorithm.

(Call window.query_tree().children at each level of the X11 window heirarchy and stop descending at windows with WM_STATE or another indicative property set.)

PetteriAimonen commented 4 years ago

If one does IceWM-specific detection, it can use the window names to find the taskbar, like in this code hinted by the IceWM maintainers: https://github.com/ice-wm/icewm/blob/master/src/icesh.cc#L901

ssokolow commented 4 years ago

I prefer desktop-specific detection to not be treated as something that's intended to be anything more than an irritating hack that I'm going to want to get rid of every time I work on the code.

This'd be more a case of "Implement a ThoroughPanelDetection config boolean and force it on if IceWM is detected." (Basically, the GTK 3.x-era analogue to how GTK+ 2.x-era QuickTile allowed users to opt into using _NET_WORKAREA.)

samurailostinjapan commented 4 years ago

Any update on this issue? - experiencing the same on our Arcolinux icewm build. As the built-in icewm tiling function is really not usable - would like to add quicktile to our build.

ssokolow commented 4 years ago

Unfortunately not. The past year has been a real mess for me for reasons beyond COVID and I'm currently climbing my way out of the pit of backlogged work that left me in.

No promises, but I hope I'll be able to do at least something for QuickTile in the next few months.