-
Notifications
You must be signed in to change notification settings - Fork 592
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
Attempt at more correct handling of NET_CURRENT_DESKTOP property. #910
Conversation
@@ -969,6 +969,8 @@ client_focus_update(client_t *c) | |||
luaA_object_emit_signal(L, -1, "focus", 0); | |||
|
|||
lua_pop(L, 1); | |||
|
|||
ewmh_update_net_current_desktop(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of having callbacks everywhere (e.g. here), could you connect to signals? In function ewmh_init
various callbacks are connected and this even already connects a callback to the "focus" signal. You can just copy that line and connect ewmh_update_net_current_desktop
. (same for other places where you add callbacks: Connect to property::screen
on clients,
I can't say that I really like this, but I don't really know how to work around Chromium's requirements in a better way... |
Will do. Signature has to be changed to static int with lua_State argument, am i right?
I can get rid of separate function and move code inside, no problem.
Actually i can think of at least two alternative solutions, that may be (or may be not) cleaner. Then first alternative would be:
Second alternative:
This one, in fact, i tried to do first: i've added a signal "update_current" to tag_class, then made a function that gets the tag and emits this signal from it. The function itself was hooked to client.focus and tag.selected signals. But it did not quite work, i've messed up and got wrong indices in the end. Still if it is viable solution and if it would be preferable, i can make a second pass on it. |
I've done requested changes and reviewed the whole alghorithm, since i've encountered several corner cases that were producing troubles with the first version (sloppy focus and switching to empty tag on separate desktop generated some wonky behaviour, and some keyboard-only window interactions worked wrong). First and foremost, there is actually no need to look for "focused" screen. I really can't think of examples where current desktop won't be the desktop where the focused window user interacting with is. So we can just use tag that has currently focused window as reference. Now, EWMH_CURRENT_DESKTOP updates are triggered only on client focus and client screen change. I've removed update callback from tag_view, and unless there is a some specific reason i don't know of, we don't really need it to be here explicitely: if we switch to tag with windows, then focus event will take care of the current desktop update, and if there are no clients then there is no one to complain, i think. So far it seems to be working for all types of interactions, both keyboard-only and mouse-driven. The only thing i have to note is that apparently client screen is detected by upper left coordinate (as opposed to {(x+width)/2, (y+height)/2} middlepoint commonly used in other window managers). So if you are dragging tab from window on left screen to window on right screen, you got to drag it all the way, so client screen actually switches. Or one can just switch detection to middle point in source code. PS. Oh, travis failed for some reason, gonna look for why. UPD: error message in Travis: 2016-05-16 10:04:17 W: awesome: luaA_dofunction:77: error while running function!
|
void | ||
ewmh_update_net_current_desktop(void) | ||
int | ||
ewmh_update_net_current_desktop(lua_State *L) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not use lua_State (anymore)?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not use lua_State (anymore)?!
Uh, it did not, and does not. Still it should have appropriate signature to be used as signal handler, or so i thought...correct me if i'm wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that might be the case, of course.
Thanks for this! |
@@ -139,7 +139,6 @@ tag_view(lua_State *L, int udx, bool view) | |||
{ | |||
tag->selected = view; | |||
banning_need_update(); | |||
ewmh_update_net_current_desktop(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is still needed, I think, for the case when no client is currently focused and you are switching between tags (and the tag you switch to also doesn't have any clients).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It depends on what for. First focused client will trigger the update, so actual clients depending on this atom should be fine. Pagers depend on NET_CURRENT_DESKTOP, but i doubt they worked correctly before with how it was handled.
This, of course, can be reverted, but then we also might need to update on mouse::enter events, to behave correctly.
Example: let's say we have chromium focused on one screen, then we switch to empty tag on second screen. Client on first screen is still focused, but CURRENT_DESKTOP now points to tag on the second screen. Now to move tabs we have to explicitely refocus chromium window (and with sloppy focus, for example, just moving mouse back to already focused window does not trigger focus event).
So we will need to update on screen change, on focus (to work properly with keyboard driven interactions), on tag switch AND on mouse enter event. Again i'm not sure about performance implications there.
No really good solutions, i'm afraid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, but I don't understand. What you are saying is:
- For simplicity: Screen 1 has tag 1-1, screen 2 has tag 2-1 and tag 2-2
- Chromium is on 1-1, tags 1-1 and 2-2 are selected
- Thus,
_NET_CURRENT_DESKTOP
is 1-1 currently (it contains the focused client) - You deselect 2-2 and select 2-1 (switch to another tag on the second screen)
_NET_CURRENT_DESKTOP
does not change, because the focused client is still on 1-1 (well, awesome will set the value that this property already has again, but that does not matter)
Since ewmh_update_net_current_desktop
does not look at the mouse pointer, mouse moves do not matter to it. What the function does look at is focused client and selected tag, thus it needs to connect to "focus" and "unfocus" on client (oh, unfocus is missing!), "tagged" and "untagged" also on client and "property::selected" on tags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah...you are right, i've confused myself it seems - what i've said would stand only for old version of the solution, not the current.
Do we really need both unfocus and untagged?
Regarding property::selected, i'll have to revert to the direct call to ewmh_update_current_desktop(NULL) in tag.c instead, since tag class is not accessible outside of tag.c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we need "unfocus": Tags 1 and 2 are selected. The focused client is on tag 2, so _NET_CURRENT_DESKTOP
has value 2. You do client.focus = nil
. Now tag 1 is the first selected tag and _NET_CURRENT_DESKTOP
needs to be updated.
Why we need "untagged": No client is selected. Tags 1 and 2 are selected, so the property has value 1. You deselect tag 1. Now the property should have value 2.
For accessing tag_class
: Perhaps it is easier to move it into objects/tag.h
, just like objects/client.h
contains client_class
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, thank you.
I'll move tag class to header then and push the next set of changes later today probably.
Thanks. The total diff looks fine to me. I think the individual commits can be squashed into a single one and this can be merged. We can do that for you if this sentence confuses you. :-) Also thanks for bearing with us and our comments. @blueyed Any remarks? |
I dislike PRs without tests, we need to improve this.. ;/ (@schmellow not your fault!) Looks fine to me otherwise - except for the punctuation at the end of the commit subject.. :D (easily fixed when squashing it though). @schmellow |
@blueyed Here is a unit test for this https://gist.github.com/psychon/78c16b23b9a88b178dbe08430588fa16 |
NET_CURRENT_DESKTOP is now being set to the index of the tag with currently focused client. In case of no focused clients present, first selected tag index is taken, with fallback value being 0. Current desktop is updated on next client signals: focus, unfocus, tagged, untagged. Current desktop is also updated on tag property::selected signal. This should fix drag and drop issues with chrome-based applications on multihead setups
bacd0cd
to
0befee6
Compare
I've squashed previous commits (hope i got it right). Regarding unit test, i'd say its your call. I will pull it into my branch if you want. |
Thanks. I merged this and also added my test. I hope the test is good enough. If not, feel free to be angry at me and tell me which part of it is wrong. |
I just want to unnecessarily confirm that this patch fixed the issue for me. I compiled and installed master today. thanks for fixing this, everyone! |
The thanks go to @schmellow, he came up with a working approach for a fix, implemented it and wasn't demotivated by the several rounds of review we force upon him. Thanks! |
First attempt at fixing issue #170
Update NET_CURRENT_DESKTOP property on:
NET_CURRENT_DESKTOP is assigned value equal to index of the first tag that is:
Caveats: