mate-desktop / marco

MATE default window manager
https://mate-desktop.org
GNU General Public License v2.0
195 stars 86 forks source link

Use get_default_focus_window from Metacity #634

Closed rcaridade145 closed 1 year ago

rcaridade145 commented 4 years ago

From my tests it mainly fixes VLC in fullscreen ( #514 and #151). Also found if the "Show Download Folder" on Firefox now opens Caja on the top (there are still issues if i have dialogs open). A consideration

@muktupavels stated that VLC also had this behaviour on Metacity. @vkareh is there something on Marco that would explain this difference ?

raveit65 commented 4 years ago
Subject: [PATCH] Use get_default_focus_window from Metacity

Uses commits up to https://gitlab.gnome.org/GNOME/metacity/-/commit/00b3e2af07a3da2f69e86d867dfda344cf2393b6
Mainly fixes VLC in fullscreen and other issues on Marco.

a) I read that and asked myself what other issues? b) personal i prefer a description in commit header what the commit does and not where you got this code :)

Edit: Any way i will start testing if it fixes both vlc issues.

rcaridade145 commented 4 years ago

@raveit65 Updated the commit message to better signal what it is doing. Removed the "other issues section" .

raveit65 commented 4 years ago

@vkareh @muktupavels Could you please help review this PR ?

muktupavels commented 4 years ago

Commit message says nothing about why/how it fixes mentioned problems...

raveit65 commented 4 years ago

PR doesn't fix both vlc issues for me.

Edit: But i can confirm that metacity-3.36.1-1.fc32.x86_64 doesn't have these issues.

rcaridade145 commented 4 years ago

@raveit65 What exactly did it not fix?

raveit65 commented 4 years ago

The issues described in https://github.com/mate-desktop/marco/issues/514 (VLC loses focus on fullscreen after moving pointer, keyboard controls) and https://github.com/mate-desktop/marco/issues/151 (vlc controls become unusable when vlc is in fullscreen mode) Behaviour is same like in master for me.

rcaridade145 commented 4 years ago

I tested it over and over again :/. The only thing I can think off from the top of my head is that because I am on nvidia I don't have xpresent. Do you have it?

raveit65 commented 4 years ago

I am using nvidia card with proprietary driver. When i am using metacity or compiz-reloaded WM everything works like expected. Build is 1.25.0 tarball + those commits from master or PRs.

# https://github.com/mate-desktop/marco/pull/625
Patch1:        marco_0001-compositor-xrender.c-Make-sure-tooltips-are-visible-.patch
# https://github.com/mate-desktop/marco/pull/632
Patch2:        marco_0001-common-window-icon-META_DEFAULT_ICON_NAME-is-no-long.patch
# https://github.com/mate-desktop/marco/commit/85a22e7
Patch3:        marco_0001-Remove-trailing-spaces-tabs.patch
# https://github.com/mate-desktop/marco/commit/37e4d38
Patch4:        marco_0002-Remove-multiple-empty-lines.patch
# https://github.com/mate-desktop/marco/commit/aaa8cf0
Patch5:        marco_0003-window-expand-tile-size-cycling-support.patch
# https://github.com/mate-desktop/marco/pull/634
Patch6:        marco_0001-Update-get_default_focus_window-based-on-Metacity-to.patch

No idea, why i don't see any different. I think this should be tested by someone other. @vkareh @rbuj Does the PR fix mentioned issues with vlc mediaplayer?

rcaridade145 commented 4 years ago

@raveit65 I managed too replicate it not working not all the times. I'm looking into it

rbuj commented 4 years ago

@raveit65 Setting "Auto raising the interface" to "Never" on VLC's Simple Preferences window works for me.

Screenshot at 2020-08-30 21-22-39

rcaridade145 commented 4 years ago

Was looking at logs and came across

STARTUP: The focus window 0x3e00006 (How Dead S) is an ancestor of the newly mapped window 0x3e000a5 (vlc) which isn't being focused. Unfocusing the ancestor.

Which made me look at https://gitlab.gnome.org/GNOME/metacity/-/commit/bf17c79171512d6a27c31e003e60a25e7a093226

Now i am getting consistently the intended behaviour.

This PR still needs a bit of cleaning sorry.

raveit65 commented 4 years ago

Last commit fixed https://github.com/mate-desktop/marco/issues/514 , thanks, but not https://github.com/mate-desktop/marco/issues/151 It is nearly impossible to reach vlc-controls in fullscreen mode with the mouse. Why you wrote it is fixed in PR description? With metacity or compiz:

With marco:

raveit65 commented 4 years ago

From my tests it mainly fixes VLC in fullscreen ( #514 and #151). Also found if the "Show Download Folder" on Firefox now opens Caja on the top (there are still issues if i have dialogs open). A consideration

* `if (window->unmaps_pending != NULL) continue; `
  On Marco this is an integer so i changed it to `if (window->unmaps_pending !=0) continue;`
raveit65 commented 4 years ago

............... Also found if the "Show Download Folder" on Firefox now opens Caja on the top (there are still issues if i have dialogs open).

Really?

raveit65 commented 4 years ago

Commit message from first commit isn't helpful and i like to understand what the first commit does. Can you please show us which commits you took from metacity?

rcaridade145 commented 4 years ago

@raveit65 Like i said previosly

"This PR still needs a bit of cleaning sorry."

Would you prefer me to close this PR and do several smaller others?

With regards to #151 when i needed to reach the controls, they were always available (cannot remember if the video was paused or not).

I will try to address your points , and indent issues as soon as possible.

raveit65 commented 4 years ago

@rcaridade145 I appreciate your work. My intention is only to have clear and meaningfully commits in our git history, which are understandable without searching in git history of other window-manager. I think in first commit you mixed several original commits form metacity in one, right? Usually, commits from metacity have a exact description for what they are good for. So, why not choosing them as single commits with their original commit messages with reference link to gnome git? Or add their original commit messages with links to your commit like this

<your commit message> eg. fixing usage of keyboard-controls in full screen mode of vlc media-player

fixes <our bug report link>
taken from metacity:
<link to 1. metacity commit>
< commit message from 1. link>

<link to 2. metacity commit>
< commit message from 2. link>

<link to 3. metacity commit>
< commit message from 3. link>

etc.

I think than eveyrbody know for what the commit(s) are good for. And don't mention bug reports which aren't fixed, to avoid disappointed and dissatisfied reviewers ;)

rcaridade145 commented 4 years ago

@raveit65 Will do.

raveit65 commented 4 years ago

With regards to #151 when i needed to reach the controls, they were always available (cannot remember if the video was paused or not).

Sometimes it works sometimes not. It work here only the first time when you move the mouse, mostly i see the controls only for a short moment (0.5 sec), than they go away. That's what i mean with flickering.

vkareh commented 3 years ago

I cannot reproduce this issue with VLC :/

But using this PR made it so that GTK2 windows that are trying to grab focus (e.g. Pidgin incoming message) appeared low in the stack, and no amount of Alt+Tabbing could actually bring it forward. Maybe it's a one-off glitch when restarting marco, will try again tomorrow to see if the issue is consistent with this patch

vkareh commented 3 years ago

Yeah, this PR causes all sorts of issues for me. I currently have 15 windows spread across 6 workspaces, and focus stack is really messed up when changing workspaces. Sometimes windows open up all the way at the bottom of the stack and refuse to move up, the only way is to lower every other window individually until they are all under the window in question, only then I can interact with it.

I'd say no go for this :-1: