Skip to content

Commit

Permalink
awful.tag: Use delayed_call to call withcurrent on manage
Browse files Browse the repository at this point in the history 
This way this function runs after everything else did its job. The client will
only get assigned a tag after everything else had a chance to do so.

Thanks to this, awful.rules no longer has to disconnect the call to withcurrent.

Signed-off-by: Uli Schlachter <psychon@znc.in>
  • Loading branch information
@psychon
psychon committed Jan 11, 2015
1 parent 7bc4ab0 commit 90fde13
Show file tree
Hide file tree
Showing 2 changed files with 4 additions and 7 deletions.
6 changes: 0 additions & 6 deletions lib/awful/rules.lua.in
View file
Original file line number Diff line number Diff line change
Expand Up @@ -224,11 +224,6 @@ function rules.execute(c, props, callbacks)
end
end

-- If untagged, stick the client on the current one.
if #c:tags() == 0 then
atag.withcurrent(c)
end

This comment has been minimized.

Sign in to view

Copy link
@actionless

actionless Jan 16, 2015

Member

with this change new client don't receive a focus

or smth should be changed in rc.lua to avoid that?

This comment has been minimized.

Sign in to view

Copy link
@psychon

psychon Jan 17, 2015

Author Member

I noticed this, too, but didn't fix it yet since this issue confuses me. If I just re-add the call to atag.withcurrent(c), I still have that issue. Only after having awful.tag.withcurrent connected to the "manage" signal do things start to work again.

So since I don't understand this issue (and currently don't have the time to investigate) I didn't fix this yet.

Technical reason for why clients don't receive focus: X11 doesn't let you focus unviewable or unmapped windows and the C code thus has some similar checks for client.focus = c. Without having any tags connected, these checks mean that setting the focus gets ignored.

This comment has been minimized.

Sign in to view

Copy link
@blueyed

blueyed Jan 21, 2015

Member

@actionless @psychon
The problem is that the client is not "visible" during request::activate (https://github.com/awesomeWM/awesome/blob/master/lib/awful/ewmh.lua.in#L136-144).

See PR #87 for a possible fix.

This comment has been minimized.

Sign in to view

Copy link
@psychon

psychon Jan 23, 2015

Author Member

Should be fixed with commit 254e50d. Hopefully.

This comment has been minimized.

Sign in to view

Copy link
@actionless

actionless Jan 23, 2015

Member

now smth strange happens when opening client on a blank tag:
callback which connect to focus signal receiving client earlier when before

to reproduce it:

  1. add to rc.lua:
client.connect_signal("focus", function(c)
  c.border_color = "#ff0000"
  -- <....>
end)
  1. switch to blank tag
  2. open new client

border color did not changed for the new client

  1. open one more client

border color changed for the new client

This comment has been minimized.

Sign in to view

Copy link
@psychon

psychon Jan 23, 2015

Author Member

....

Ok, I made client.focus = c and c.border_color = foo print backtraces. What happens for the first client is:

  1. The "manage" signal calls awful.tag.withcurrent.
  2. withcurrent tags the client with the currently selected tag.
  3. awful.autofocus kicks in (due to "tagged" signal) and focuses the client. This triggers the "focus" signal.
  4. The following line from the default config gets called: client.connect_signal("focus", function(c) c.border_color = beautiful.border_focus end)
  5. The new callback on the focus signal (the one you told me to add) gets triggered
  6. The "manage" signal continues (remember: This is what caused withcurrent to be called at step 1). awful.rules.execute runs.
  7. The rule border_color = beautiful.border_normal is applied.
  8. The focus rule is applied, but has no effect since the client is already focused.

When you open a second client, autofocus isn't triggered and thus the client only gets focused at the end of awful.rules, after all the rules are already applied.

We could revert the rest of this commit and have awful.rules disconnect withcurrent again. The original reason why I wanted to remove was some bug report: Someone monkey-patches awful.tag.withcurrent and then awesome printed a warning because awful.rules tried to disconnect a signal that wasn't connected (because it didn't get the original "withcurrent" function).
And IMO modules shouldn't interfere with each other and disconnect callbacks that other modules added. So restoring the old behavior "feels wrong" to me.

The only other approach that I can up with right now is cheating. awful.rules could explicitely do something like client.focus:emit_signal("focus"). This isn't really nice either.

Ideas? :-(

This comment has been minimized.

Sign in to view

Copy link
@psychon

psychon Jan 23, 2015

Author Member

Side note: This means that if you have tags = { whatever } in one of your rules, the same issue already occured before this commit (if you currently had one of these tags selected). So it seems like I accidentally found a genuine bug.

This comment has been minimized.

Sign in to view

Copy link
@actionless

actionless Jan 23, 2015

Member

so i removed border-related properties from the default rule and now it works for me

This comment has been minimized.

Sign in to view

Copy link
@actionless

actionless Jan 23, 2015

Member

i hope it should help: #88

This comment has been minimized.

Sign in to view

Copy link
@actionless

actionless Jan 23, 2015

Member

but i suspect what a proper fix will be to move logic of awful.autofocus from signals to filter, to use it in rule, ie to make it like awful.client.focus.filter

This comment has been minimized.

Sign in to view

Copy link
@blueyed

blueyed Jan 24, 2015

Member

@psychon

Ideas? :-(

Is PR #87 an option now again maybe?

This comment has been minimized.

Sign in to view

Copy link
@psychon

psychon Jan 24, 2015

Author Member

@blueyed and then what? Thanks to commit 254e50d, PR#87 no longer fixes this issue (well, technically we are now looking at another issue...). The client now has a tag while we try to focus it (<- previous issue), but it is already focused and thus nothing happens, despire awful.rules having overwriten the border color (<- new issue).

This comment has been minimized.

Sign in to view

Copy link
@blueyed

blueyed Jan 24, 2015

Member

254e50d could be reverted.

This comment has been minimized.

Sign in to view

Copy link
@blueyed

blueyed Feb 14, 2015

Member

The proper fixup might have landed now in #118.

This comment has been minimized.

Sign in to view

Copy link
@blueyed

blueyed Feb 17, 2015

Member

For the autofocus issue See #134 (and #116 for a fix).

-- Apply all callbacks.
if callbacks then
for i, callback in pairs(callbacks) do
Expand All @@ -244,7 +239,6 @@ function rules.execute(c, props, callbacks)
end

client.connect_signal("manage", rules.apply)
client.disconnect_signal("manage", atag.withcurrent)

return rules

Expand Down
5 changes: 4 additions & 1 deletion lib/awful/tag.lua.in
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

-- Grab environment we need
local util = require("awful.util")
local timer = require("gears.timer")
local tostring = tostring
local pairs = pairs
local ipairs = ipairs
Expand Down Expand Up @@ -629,7 +630,9 @@ capi.client.connect_signal("manage", function(c)
c:connect_signal("property::screen", tag.withcurrent)
end)

capi.client.connect_signal("manage", tag.withcurrent)
capi.client.connect_signal("manage", function(c)
timer.delayed_call(tag.withcurrent, c)

This comment has been minimized.

Sign in to view

Copy link
@Elv13

Elv13 Jan 16, 2015

Member

Why don't this method (delayed_call) don't just use GLib run_idle? This look much better to let the even loop itself handle these than trying to re-implement a queue on top of that (and add a "slow"* lua foreach construct to each loop), no? GLib implementation also have some basic priority scheduling.

  • Very fast, but much slower than just using the already used Glib one as it is called no mater what

This comment has been minimized.

Sign in to view

Copy link
@psychon

psychon Jan 17, 2015

Author Member

Hm, good idea, I guess. I guess the 'refresh' signal could be dropped then...

This comment has been minimized.

Sign in to view

Copy link
@blueyed

blueyed Jan 21, 2015

Member

@Elv13 @psychon
Makes sense indeed.
Would this require to have a lua proxy function to g_idle_add () (ref?

This comment has been minimized.

Sign in to view

Copy link
@Elv13

Elv13 Jan 21, 2015

Member

The call is quite simple even as is https://github.com/Elv13/awesome-configs/blob/master/tyrannical/shortcut.lua#L109

However we usually proxy the GLib calls anyway, even if that one is quite simple

This comment has been minimized.

Sign in to view

Copy link
@blueyed

blueyed Jan 21, 2015

Member

@Elv13
Cool. Do you feel like doing a PR for this?

This comment has been minimized.

Sign in to view

Copy link
@blueyed

blueyed Feb 13, 2015

Member

Done in #115.
end)
capi.tag.connect_signal("request::select", tag.viewonly)

capi.tag.add_signal("property::hide")
Expand Down

0 comments on commit 90fde13

Please sign in to comment.