Awesome: Clients always open on first screen

[psychon]

Issue was originally reported here, but I can reproduce it

Output of awesome --version:

awesome v3.5.2-1968-ge86ddaa-dirty (The Fox)
 • Compiled against Lua 5.2.4 (running with Lua 5.2)
 • D-Bus support: ✔

How to reproduce the issue:

• Move mouse pointer to some other screen than screen 1
• Open a terminal

Actual result:

Terminal opens on screen 1

Expected result:

Terminal opens on screen awful.screen.focused()

Looking at the diff of 21787444e4f1, this removes a call to awful.screen.focused(), but doesn't add one. Which part of the code is now responsible for assigning a default screen?

[madduck]

I reverted 2178744 and that seems to fix this particular issue…

[Elv13]

That one will be tricky to fix in a non hacky way.

The problem is 2 fold. First, when a client is supposed to go a specific screen, it should. On the other hand, when it "isn't", it should go to the focused screen.

A client is supposed to go to a screen when:

• It has been started using awful.spawn with explicit instructions [1]
• An awful.rules rule or any of its callbacks set the screen [2]
• When something handle request::screen and/or request::tag in some custom ways. [3]
• Some clients can request a screen and mean it (like MythTV/Kodi/XBMC and some multi-window DAW) [4]

A client is supposed to go to the focused screen when none of the above are true [5].

Other constraints:

• The screen need to be set only once, anything will will emit property::screen many time and cause side effects.
• There has to be a single entry point to the algorithm, no multiple "manage" handler.
• Awesome internals must use the request:: signal API and not force their decision outside of request handlers.
• Restarting Awesome must not change the client screen
• When a dialog appears, it should go on the "parent" client screen, but still support being moved to other screens (added 2017-01-18)
• Docked clients can choose their screen (added 2017-01-18)

Commit 2178744 fix use case number [1] and [2]. It actually fix [4] too, but it is an accident and I am not sure we care about [4] anyway. Use case [1] and [2], however, are very important.

Looking at the diff of 2178744, this removes a call to awful.screen.focused(), but doesn't add one.

You are right about that one. As the commit message say, it was removed because by messing with screens in 2 different places, use case [1] and [2] could not be reliable. Use case [5] was "brute forced" after the fact and its why it worked.

One way to handle this would be to set a secret client attribute to store a hint of how the current client screen was selected in every places Awesome itself set the screen (then clear it if it comes from an unknown source). This is a terrible idea, but it could be hacked to work. The problem is, detecting use case [5] at a given point of time it hard, as the information for [1], [2], [3] and [4] only exist at a specific point in time. @psychon any other idea?

Edit: madduck propose we could add a step before "manage" that would set the screen without emitting a signal so the initialization cycle (based on "manage") starts with a saner default screen. This would break 4.

As I said on IRC yesterday (and in #1038), this whole thing is technically unfixable, but carefully chosen defaults should work for most people. This bug need to be fixed for 3.6, but it is easy to see why it regressed in the first place (fixing #1052 and #1028 is incompatible with #1091 behavior).

Also, we will need an unit test for that. Maybe by unit testing [1], [2], [3] and [5], we can finally leave in peace forever after, but I doubt that.

As for now, you can fix this issue by disconnecting ewmh.tag and connect request::tag to function(c, t) local ts = t and {t} or awful.screen.focused().selected_tags; c:tags(ts) end (untested).

Related: #1780 #1685 #1687 #1721

[psychon]

The screen need to be set only once, anything will will emit property::screen many time and cause side effects.

Why? I don't see the problem. The same issues could occur when you "manually" move a client to another screen two seconds after it was created.

My "magic hack fix solution" suggestion would be to add screen = awful.screen.focused to the default rule in the default config. awful.rules already supports this kind of callbacks and it would Just Work(tm).

This would fix [1], [2a], [2b], [3] and [5], while we mostly ignore [4] anyway.
The remaining issue would be [2c], but I hope I mis-understood you and you don't actually want this to work.

For [2b], your constraint "The screen need to be set only once," is violated, but I can easily live with that since I don't see the issue here and would treat any such issue as a bug in the property::screen handler and/or in the rule.

Did I miss anything?

(Yesterday I was thinking about special magic to make screen = "focused" work, but I like the callback function suggested above a lot more)

TL;DR: Any complaints about the following (untested!!!!) patch?

    available colormap entries:    256 per subfield
diff --git a/awesomerc.lua b/awesomerc.lua
index 0029e19..4b42649 100755
--- a/awesomerc.lua
+++ b/awesomerc.lua
@@ -428,6 +428,7 @@ awful.rules.rules = {
                      raise = true,
                      keys = clientkeys,
                      buttons = clientbuttons,
+                     screen = awful.screen.focused,
                      placement = awful.placement.no_overlap+awful.placement.no_offscreen
      }
     },

[Elv13]

TL;DR: Any complaints about the following (untested!!!!) patch?

complaint: It need testing. Joke aside, it is probably a very clean way. I will have to think about it, then write a lot of tests.

Why? I don't see the problem.

The issues are indirect. In the default config this is fine. But if something is connected on property::screen and then call other things that causes some tagged/untagged signals, and then some tags are volatile or something like that, it will cascade. I prefer to have a single instance of those properties emitted during the initialization. Just like having multiple "manage" handler caused all these issues. For doc on these issues, go see the "rule refactor" PR description and the various bugs we had since I "fixed" those bugs. In the end, they are all due to "too many signal handlers induced race conditions". It might look harmless at first, but this cascade out quickly.

This very issue, for example, was previously _hidden_ by one of these extra signal handlers (indirectly breaking what 2178744 fixed). Here, instead of having 2 handlers on the same signals, we will have 1 handler called multiple time, but the end result is the same: more code being executed leading to unpredictable results.

Your awesomerc.lua patch is not affected by this double emit issue. As long as the rules are squashed correctly (they are), it will be fine. Having a screen in the rule is (obviously), a totally supported use case. So it really is the cleanest way.

[actionless]

after i've updated awesome from 3.5.2.2034.g46b2373 to 3.5.2.2070.g250f476 i started to reproduce the same problem

proposed patch (screen = awful.screen.focused in default rule) is causing a side affect -- when restarting awesome all the clients from screen other than focused are moving to the first tag of the focused screen

[Elv13]

totally untested

function(c) return awesome.startup and c.screen or awful.screen.focused() end

Maybe call this awful.screen.preferred (or awful.client.object.preferred_screen, but it make little sense outside of the rules scope)

(and I updated the first comment to reflect this newly mentioned constraint.

[xkonni]

as Elv13 suggested, adding
screen = function (c) return awesome.startup and c.screen or awful.screen.focused() end
to the main rule fixes the issue.

thanks!

[actionless]

yup, that totally works for my case, also i've noticed what now focusing the screen works fine when the screen have no clients.

however i've stumbled upon some issue, with screen:focus_relative, awful.screen.focus_relative works fine but both screen:focus_relative and screen.focus_relative raising the exception (stack trace not seems to be any useful):

stack traceback:
        [C]: in metamethod '__index'
        /home/lie/.config/awesome/config/keys.lua:154: in upvalue 'press'
        /usr/share/awesome/lib/awful/key.lua:91: in function </usr/share/awesome/lib/awful/key.lua:91>
error: /home/lie/.config/awesome/config/keys.lua:154: bad argument #2 to '__index' (screen expected, got string)

[Elv13]

Just a little update, I will need @psychon #1089 feature to be finished before I can add unit tests for this. Testing this currently takes too long and timeout. So I will submit a PR without regression tests...