-
Notifications
You must be signed in to change notification settings - Fork 150
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
xwayland: add sensible rules for NET_WM_WINDOW_TYPE_DESKTOP windows #1490
base: master
Are you sure you want to change the base?
Conversation
By the way, |
I actually think this is even better than to create internal wlr_surface rules as we only need to setup the rule(s) once and it just magically works for all xwayland surfaces. I didn't check in detail but I assume that we have to react to the We might also want to add another function pointer to // add in include/view.h
struct view_impl {
..
bool (*contains_window_type)(struct view *view, int32_t window_type);
}
// assign in src/xwayland.c, can then be turned into a static function
static const struct view_impl xwayland_view_impl = {
..
.contains_window_type = xwayland_contains_window_type,
}
// and be called like
if (rule->window_type >= 0 && view->impl->contains_window_type) {
return view->impl->contains_window_type(view, rule->window_type);
} In general I really like this approach! Thanks for working on it. |
deaef94
to
8e63941
Compare
Thank you for a review :) I adjusted the patch! I'm not sure what's wrong with CI - builds fine on my side. |
80f97a1
to
7189512
Compare
Other than the last minor nitpick above this looks very good to me. I do not merge the PR right away to give @johanmalm and possibly @jlindgren90 a chance to state their opinions. Edit: |
7189512
to
c12e724
Compare
Just figured that |
With that one I am not quite sure:
So maybe a |
c12e724
to
7f9b08e
Compare
The thing is that when I use
in rc.xml. It toggles decorations for This line fixed it though %)
Changed |
Sorry I haven't had time to really follow this thread or look at the changes. Don't let me hold up progress :) |
Nice work. Looks good in principle, except that I'm nervous about hard-coding properties and actions with no ability to configure at run-time. I think it's good to pre-configure I could do some testing Thu/Fri. |
xfdesktop4 would be a well-known client to test |
I don't see them in this PR, stuck review?
The internal rules have the lowest priority so a user could modify them by targeting Edit: However it should still be manageable: we could add the internal rules first and when parsing the rules we try to detect if a rule with the same criteria already exists and if yes we overwrite its values rather than creating a new one. Likely easiest to do in |
Thank you :^)
I'm not sure if revealing this functionality to rc.xml makes a lot of use. For example, IceWM handles many different window types internally and has only title and class for window rule criteria. I reckon it's been like that for ages and no one seem to complain. Moreover, as @Consolatis said, user always has the ability to override these rules by title or identifier.
( I couldn't get |
src/xwayland.c
Outdated
rule->server_decoration = LAB_PROP_FALSE; | ||
rule->skip_taskbar = LAB_PROP_TRUE; | ||
rule->skip_window_switcher = LAB_PROP_TRUE; | ||
rule->ignore_focus_request = LAB_PROP_TRUE; |
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.
I'm not sure we should ignore focus request. Might be fine for conky, but probably not pcmanfm --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.
I'm not so sure either, but as far as I can understand ignore_focus_request setting makes it so that window unable to take over user focus? If that's the case, can you give an example in which desktop window might want to take over user focus? In my opinion it's not very convenient for users to stumble upon desktop which randomly focuses itself
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.
I haven't tested, but imagine an desktop client showing icons (like pcmanfm --desktop
and xfdesktop
) would want to take focus to operate right-click menus, rename icons and so on.
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, it needs focus to do that of course, but it's user-initiated focus, rather than focus requested by window, isn't it? Sorry if I'm talking nonsense, that's just what I figured from the setting name without looking deep into implementation. This is also confirmed by the fact that I can navigate right-click menus and rename files with this setting set to true.
It was 😄 |
7f9b08e
to
9ab2e68
Compare
9ab2e68
to
2ef03a7
Compare
I'll do some testing with clients tomorrow. |
33cb3d4
to
f381708
Compare
Rebased on top of master with the work from #1731 and tested these:
|
Are we sure ignore_focus_request should be true. We should be able to operate pcmanfm--desktop context menus with the keyboard, which necessitates keyboard-focus. |
That is only for the window requesting focus itself. AFAIR it should receive keyboard focus just fine when being clicked on. |
Since context menus usually appear only after user's mouse clicks which set focus, it should work just fine. If you believe some windows of desktop type could pick up user focus without any input at first, the rule strictness can be lessen then. |
Thanks for being patient 😄 ...and sorry for not keeping up with the conversation higher up. The It feels like we ought to:
|
@johanmalm Hi, sorry for the wait. I adjusted the patch to meet these points ;) Tested the |
Reference:
_NET_WM_WINDOW_TYPE_DESKTOP
#1474I tried to avoid using
HAVE_XWAYLAND
macro according to @johanmalm idea:But couldn't find a better way than this... If it's not good enough I'm ready to cooperate ;)
CC: @johanmalm @Consolatis @jlindgren90