Skip to content
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

Fix minimized windows being focused when a focused window is closed #3698

Merged
merged 3 commits into from
Jan 21, 2025

Conversation

tarek-y-ismail
Copy link
Contributor

@tarek-y-ismail tarek-y-ismail commented Dec 11, 2024

Closes #3408

@tarek-y-ismail tarek-y-ismail self-assigned this Dec 11, 2024
@tarek-y-ismail tarek-y-ismail force-pushed the MIRENG-544/closing-window-focuses-minimized-window branch from 0048a5b to fa1e09b Compare December 12, 2024 11:21
@tarek-y-ismail
Copy link
Contributor Author

I think mir_surface and scene_surface are both referring to the same thing?

std::shared_ptr<scene::Surface> const& mir_surface = hint;
if (info_for_hint.state() == mir_window_state_minimized)
{
std::shared_ptr<scene::Surface> const scene_surface{hint};

@tarek-y-ismail tarek-y-ismail force-pushed the MIRENG-544/closing-window-focuses-minimized-window branch from fa1e09b to 478811a Compare December 12, 2024 12:47
@tarek-y-ismail tarek-y-ismail marked this pull request as ready for review December 16, 2024 08:02
@tarek-y-ismail tarek-y-ismail requested a review from a team as a code owner December 16, 2024 08:02
Copy link
Contributor

@mattkae mattkae left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few things, but it does make sense! ApplicationSelector is a sad implementation that will be removed some day soon

src/miral/application_selector.cpp Show resolved Hide resolved
src/miral/application_selector.cpp Outdated Show resolved Hide resolved
src/server/frontend_wayland/input_method_v1.cpp Outdated Show resolved Hide resolved
src/miral/basic_window_manager.cpp Show resolved Hide resolved
src/miral/basic_window_manager.cpp Show resolved Hide resolved
src/miral/basic_window_manager.cpp Show resolved Hide resolved
@mattkae
Copy link
Contributor

mattkae commented Dec 17, 2024

Does this fix #3309 ? According to 3309, saviq expects gnome-chess to be focused in the following situation instead of gnome-terminal:

Screencast.from.2024-12-17.09-05-36.webm

@Saviq
Copy link
Collaborator

Saviq commented Dec 17, 2024

The error in the video is that both terminals get raised in the first place, which they shouldn't (can't tell right now if pre-existing).

@tarek-y-ismail
Copy link
Contributor Author

tarek-y-ismail commented Dec 17, 2024

Does this fix #3309

Looks like I got caught up in all the different sub-bugs ;)
This line was the culprit:

if (can_activate_window_for_session_in_workspace(application, workspaces_containing_window))
return;

Can you test on your end as well? In case my testing setup broke in some other way (I use gnome-terminal and qtcreator since some GTK apps (kgx and gnome-chess) refuse to run inside miriway). I think this was meant to alt-tab between windows of the same application if no other applications are display. For example, I you had multiple gnome-terminals open

The bug with alt-tabbing raising all windows of a specific app is still there. I'm looking into it.

@mattkae
Copy link
Contributor

mattkae commented Dec 17, 2024

The error in the video is that both terminals get raised in the first place, which they shouldn't (can't tell right now if pre-existing).

Actually, it isn't that both get raised, but rather that gnome-chess gets sent all the way to the back in this case :)

@mattkae
Copy link
Contributor

mattkae commented Jan 7, 2025

Unfortunately this has broken the following for me. Here is a repro:

  1. Open gnome-chess, gedit, and gnome-terminal
  2. Focus gedit, then gnome-chess so that the focus order is (1) gnome-chess, (2) gedit, and (3) gnome-terminal
  3. Start alt tabbing. gedit is focused first
  4. Click tab again, gnome-terminal should be focused
  5. Click tab again, gnome-chess should be focused

However, the bug is that gnome-terminal is now above gedit since it was raised but it was NOT sent back. gnome-terminal should be sent to the back in this case because it never truly was selected to receive focus.

Screencast.from.2025-01-07.09-44-04.webm

All of this being said, we do have imminent plans to deprecate the ApplicationSelector entirely in favor of a client-based approach, so I wonder if this work is just going to be painful for very little gain.

Maybe I'd like to get @Saviq 's opinion, but I would be in favor of not working on improving this code, and instead focus on implementing a proper window switcher

@Saviq
Copy link
Collaborator

Saviq commented Jan 7, 2025

Maybe I'd like to get @Saviq 's opinion, but I would be in favor of not working on improving this code, and instead focus on implementing a proper window switcher

Agreed, we shouldn't worry about Alt+Tab-broken behaviour, unless trivial to fix.

@tarek-y-ismail
Copy link
Contributor Author

tarek-y-ismail commented Jan 9, 2025

I can't quite reproduce #3309 at the moment, and since this PR "accidentally" included #3408, I think #3309 should be split out in its own PR (once I can reproduce it again).

Anyway, this might eventually be ditched since we're hoping to ditch our broken alt+tab implementation soon.

@tarek-y-ismail tarek-y-ismail force-pushed the MIRENG-544/closing-window-focuses-minimized-window branch from 72892c3 to ccd6e4e Compare January 9, 2025 13:37
@tarek-y-ismail
Copy link
Contributor Author

However, the bug is that gnome-terminal is now above gedit since it was raised but it was NOT sent back. gnome-terminal should be sent to the back in this case because it never truly was selected to receive focus.

I think the better approach would be to save the z index of the window and place it back when we advance (or retreat) past it when alt tabbing. But the surface stack doesn't allow this at the moment and it's quite a minor visual bug.

Another minor bug I caught is related to decorations. When alt-tabbing, the z index of the window and its decoration go out of sync at some point.
mireng-544-borked-decorations
In the picture above, the window in the middle should have its decorations drawing over the window to its left.

@tarek-y-ismail tarek-y-ismail force-pushed the MIRENG-544/closing-window-focuses-minimized-window branch from fa432b4 to cc27301 Compare January 13, 2025 08:23
Copy link
Contributor

@mattkae mattkae left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One final thing :)

src/miral/application_selector.cpp Show resolved Hide resolved
Copy link
Contributor

@mattkae mattkae left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's land it!

@mattkae mattkae merged commit 9fc7694 into main Jan 21, 2025
33 of 39 checks passed
@mattkae mattkae deleted the MIRENG-544/closing-window-focuses-minimized-window branch January 21, 2025 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Closing a window causes minimized windows to be focused
3 participants