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

feat: alt tab cycle #227

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

wiiznokes
Copy link
Contributor

Old PR #225

impl pop-os/cosmic-launcher#135

With this PR, the service is not intended to be restarted everytime the cosmic-launcher is shown, because the toplevel plugin needs to keep track of some states.

So i just added a field in the plugin config:

#[serde(default)]
pub long_lived: bool,

For plugins that need to always running.

And this variant Frontend -> Service/Plugin

/// The frontend was closed and the service should release resources
/// and prepare for the next query
Close,

I believe the service was written with this usecase in mind, so there isn't much change.

Also, i had to deactivate the sort when the query is empty.

@wiiznokes wiiznokes changed the title Feat/alt tab cycle feat: alt tab cycle Jul 13, 2024
mmstick
mmstick previously approved these changes Jul 16, 2024
@mmstick mmstick requested a review from a team July 16, 2024 20:07
@jacobgkau jacobgkau self-assigned this Jul 16, 2024
Copy link
Member

@jacobgkau jacobgkau left a comment

Choose a reason for hiding this comment

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

This is doing something, but the behavior seems like it needs to be tweaked a little.

  1. Alt/super+tab recency-based cycling cosmic-launcher#135 specifies pressing/releasing Alt-Tab should switch back and forth between the two most recent windows. I'm seeing that work sporadically, but usually, the currently selected window shows up first in the list, so pressing/releasing Alt-Tab doesn't change the window at all.
  2. I'm seeing one window (titled Oracle VM VirtualBox Manager) basically stuck at the second or third position even after switching between many other windows (from Firefox and COSMIC Terminal) and not touching that one. So it seems like the recency-based sorting isn't working quite right, or there's somehow a way for another app/window to bypass it.

@jacobgkau
Copy link
Member

jacobgkau commented Jul 17, 2024

Reading pop-os/cosmic-launcher#135, I do see it specifies applications and not windows. Let me know if we need to get UX clarification on that, if it's supposed to be working with applications right now. I think issue 1 is independent of that, based on the behavior.

@wiiznokes
Copy link
Contributor Author

Just to be sure, did you test with pop-os/cosmic-launcher#150 ? With this PR, alt+tab will select the second app.

I was able to reproduce 2.

@jacobgkau
Copy link
Member

Ah, no I didn't. I'll get that built on CI for testing.

@wiiznokes
Copy link
Contributor Author

I think it require launcher to be located in ../launcher currently. I will change that once i fix point 2

@wiiznokes
Copy link
Contributor Author

wiiznokes commented Jul 18, 2024

I think there is a bug in cosmic-comp that make this PR not working:

code for the log:

ToplevelEvent::Update(handle, info) => {
                        warn!("Update {}: {:?}", &info.app_id, &info.state);

Output when selecting konsole -> cosmic-edit -> virtualbox -> firefox

(using cosmic-launcher)

juil. 19 01:22:31 fedasus pop-launcher[22633]: Update org.kde.konsole: {}
juil. 19 01:22:31 fedasus pop-launcher[22633]: Update com.system76.CosmicEdit: {Activated}

juil. 19 01:22:41 fedasus pop-launcher[22633]: Update com.system76.CosmicEdit: {}
juil. 19 01:22:41 fedasus pop-launcher[22633]: Update VirtualBox Manager: {Activated}
juil. 19 01:22:41 fedasus pop-launcher[22633]: Update com.system76.CosmicEdit: {Activated}
juil. 19 01:22:41 fedasus pop-launcher[22633]: Update VirtualBox Manager: {}
juil. 19 01:22:41 fedasus pop-launcher[22633]: Update com.system76.CosmicEdit: {}
juil. 19 01:22:41 fedasus pop-launcher[22633]: Update VirtualBox Manager: {Activated}

juil. 19 01:22:45 fedasus pop-launcher[22633]: Update VirtualBox Manager: {}
juil. 19 01:22:45 fedasus pop-launcher[22633]: Update org.mozilla.firefox: {Activated}
juil. 19 01:22:45 fedasus pop-launcher[22633]: Update com.system76.CosmicEdit: {Activated}
juil. 19 01:22:45 fedasus pop-launcher[22633]: Update org.mozilla.firefox: {}
juil. 19 01:22:45 fedasus pop-launcher[22633]: Update com.system76.CosmicEdit: {}
juil. 19 01:22:45 fedasus pop-launcher[22633]: Update org.mozilla.firefox: {Activated}

I'm not sure this protocol is intended for this use case tho

cc @Drakulix

Copy link
Member

@jacobgkau jacobgkau left a comment

Choose a reason for hiding this comment

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

This seems to be working pretty well now.

The recency is not taking into account window activations that are a result of switching workspaces. This causes a couple of issues:

In this first video, the OBS window is never suggested as the second-most-recent window, even when I go from it directly to another window (by switching workspaces).

2024-09-24.18-15-52.mp4

In this second video, even when I select a second window on that second workspace, alt-tab brings me back to the first workspace, instead of back to the previously-selected OBS window.

2024-09-24.18-21-25.mp4

I think the only thing that needs to change is for the recency to take into account window activations caused by workspace switching. (Alternatively, switching could be isolated per-workspace to avoid this issue, but I don't think that was the intended design.)

@wiiznokes
Copy link
Contributor Author

probably a cosmic-comp issue @Drakulix

@Drakulix
Copy link
Member

probably a cosmic-comp issue @Drakulix

No it isn't. The current workspace doesn't affect the Activated state. You need to use the cosmic-workspace protocol as well and track which workspace an application is on to fix this.

@wiiznokes
Copy link
Contributor Author

How would i know which top_level is active just by knowing witch workspace was activated ? Let's says there are multiple windows in workspace B. And also the case where all windows in workspace B are minimized, i guess we don't want to bump any top_level in the list.

@Drakulix
Copy link
Member

How would i know which top_level is active just by knowing witch workspace was activated ? Let's says there are multiple windows in workspace B. And also the case where all windows in workspace B are minimized, i guess we don't want to bump any top_level in the list.

The one with the Activated state on said workspace of course. (Which isn't necessarily the last window to get Activated, just the last on said workspace, that was Activated.)

@wiiznokes
Copy link
Contributor Author

The one with the Activated state on said workspace of course. (Which isn't necessarily the last window to get Activated, just the last on said workspace, that was Activated.)

I try to implement it in this branch https://github.com/wiiznokes/launcher/tree/alt-tab-with-workspace-support-1.
I does not work and i'm not sure why. I think the current behavior of both protocols make this impossible, tho, i might miss understand smtg

@Drakulix
Copy link
Member

I try to implement it in this branch https://github.com/wiiznokes/launcher/tree/alt-tab-with-workspace-support-1. I does not work and i'm not sure why. I think the current behavior of both protocols make this impossible, tho, i might miss understand smtg

I am sorry, but I am not going to debug your branch and you are not providing me with a bunch of information.
What do you mean with "it does not work". Did you run with WAYLAND_DEBUG=1 to figure out what events you get and when? Do you have any specific questions about the implementation.

@wiiznokes
Copy link
Contributor Author

wiiznokes commented Sep 25, 2024

Ok, sorry about that. I figured out the bug in my code that was causing no sense (using position on a reversed iterator 🤦). I work better now, except in this case:

  1. App A in workspace X is active
  2. Focus an app B in workspace Y
  3. Focus an app C in workspace X

result: The list will be C -> A -> B instead of C -> B -> A, because both WorkspaceHandler::done and ToplevelInfoHandler::update_toplevel will be called.

Edit: And WorkspaceHandler::done is called first. If it was guaranteed to be called afterToplevelInfoHandler::update_toplevel, that could solve this

@Drakulix
Copy link
Member

Edit: And WorkspaceHandler::done is called first. If it was guaranteed to be called afterToplevelInfoHandler::update_toplevel, that could solve this

From a cosmic-comp perspective we first swap workspaces and fixup focus afterwards, which... yeah.. I get how this is a hassle.

From a protocol perspective it is quite difficult to guarantee any specific order between the two protocols.

It might be easier to remove the Activated-state from the original window on workspace swap, so you can just rely on cosmic-toplevel without needing cosmic-workspaces.

@wiiznokes
Copy link
Contributor Author

It might be easier to remove the Activated-state from the original window on workspace swap, so you can just rely on cosmic-toplevel without needing cosmic-workspaces.

But in my example, if the app A get focused instead of C, cosmic-toplevel will not be trigger.

@wiiznokes
Copy link
Contributor Author

@Drakulix Just to be sure, do you agree that cosmic-comp need to re send the Activated state when switching workspace, even we it has not changed ?

@Drakulix
Copy link
Member

@Drakulix Just to be sure, do you agree that cosmic-comp need to re send the Activated state when switching workspace, even we it has not changed ?

Yes. Though this would also mean we remove the Activated state on workspace switch from the previous workspace. But that shouldn't be an issue, right?

@wiiznokes
Copy link
Contributor Author

Yes. Though this would also mean we remove the Activated state on workspace switch from the previous workspace. But that shouldn't be an issue, right?

I don't think so. I think is it will better overall this way

@Drakulix
Copy link
Member

Yes. Though this would also mean we remove the Activated state on workspace switch from the previous workspace. But that shouldn't be an issue, right?

I don't think so. I think is it will better overall this way

Great. I think this should fix it: pop-os/cosmic-comp#889

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.

4 participants