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 #150

Closed
wants to merge 10 commits into from
Closed

Conversation

wiiznokes
Copy link
Contributor

@wiiznokes wiiznokes commented Jul 3, 2024

impl #135.

Related to this pr pop-os/launcher#227. Need to be merged at the same time. (actually, the pr in pop-launcher need to be merged first)

@wiiznokes
Copy link
Contributor Author

wiiznokes commented Jul 3, 2024

This could definitively be improved, by supporting shit+alt+tab, and selecting the 1 index when alt+tab is pressed, but this will require more changes. And since killing cosmic-launcher make cosmic-comp freezing, i think this can be merged as is for now.

Still need pop-os/launcher#225 to be merged first because cosmic-launcher depend on it.

@wiiznokes wiiznokes marked this pull request as ready for review July 3, 2024 12:34
src/app.rs Outdated Show resolved Hide resolved
@wiiznokes
Copy link
Contributor Author

wiiznokes commented Jul 11, 2024

@wash2 So i'm looking to resolve this part

todo:

  • handle the case where the search have not finished but the alt-tab command need to be processed

This is my idea,

wrapper around tx, to make a request: self.request(launcher::Request::Search(String::new());. This function will set a field waiting_result = true (if we don't care to wait, we could make 2 functions, e.i, for launcher::Request::ActivateContext(i, context)).

Then, we add our message to a queue: self.queue.push(Message::AltTab).

The result will set waiting_result = false, and process the messages in the queue.

The question is, will it interfere with wait_for_result: bool, ? wdyt ?

edit: i'm not sure waiting_result is useful in this case. Just the queue should do it

@wiiznokes wiiznokes marked this pull request as draft July 11, 2024 15:13
@wash2
Copy link
Collaborator

wash2 commented Jul 11, 2024

Then, we add our message to a queue: self.queue.push(Message::AltTab).

Ya, I think a queue of unprocessed messages could work well 👍

@wiiznokes wiiznokes marked this pull request as ready for review July 13, 2024 11:06
@wiiznokes wiiznokes force-pushed the feat/alt-tab-cycle branch from 3883d5a to aa451d9 Compare July 19, 2024 14:48
@jacobgkau
Copy link
Member

This failed to build on the build server a few weeks ago. Testing now, it's also failing to build locally.

Build log:
jacob@serw13:~/Work/cosmic-launcher$ dpkg-buildpackage -b -uc -us
dpkg-buildpackage: info: source package cosmic-launcher
dpkg-buildpackage: info: source version 0.1.0
dpkg-buildpackage: info: source distribution UNRELEASED
dpkg-buildpackage: info: source changed by Ashley Wulber <[email protected]>
dpkg-buildpackage: info: host architecture amd64
 dpkg-source --before-build .
dpkg-source: info: using options from cosmic-launcher/debian/source/options: --tar-ignore=target --tar-ignore=vendor
 fakeroot debian/rules clean
dh clean
   debian/rules override_dh_auto_clean
make[1]: Entering directory '/home/jacob/Work/cosmic-launcher'
if test "1" = "1"; then \
	cargo clean; \
fi
info: syncing channel updates for '1.75-x86_64-unknown-linux-gnu'
info: latest update on 2023-12-28, rust version 1.75.0 (82e1608df 2023-12-21)
info: downloading component 'cargo'
info: downloading component 'clippy'
info: downloading component 'rust-docs'
info: downloading component 'rust-std'
info: downloading component 'rustc'
info: downloading component 'rustfmt'
info: installing component 'cargo'
info: installing component 'clippy'
info: installing component 'rust-docs'
info: installing component 'rust-std'
info: installing component 'rustc'
info: installing component 'rustfmt'
     Removed 0 files
if ! ischroot && test "1" = "1"; then \
	cp .cargo/config.default .cargo/config.toml; \
	cargo vendor --sync Cargo.toml | head -n -1 >> .cargo/config.toml; \
	echo 'directory = "vendor"' >> .cargo/config.toml; \
	rm -rf vendor/winapi*gnu*/lib/*.a; \
	tar pcf vendor.tar vendor; \
	rm -rf vendor; \
fi
    Updating git repository `https://github.com/wiiznokes/launcher/`
error: failed to sync

Caused by:
  failed to load pkg lockfile

Caused by:
  failed to load source for dependency `pop-launcher`

Caused by:
  Unable to update https://github.com/wiiznokes/launcher/?rev=86a54d54a68b832d404ef83a03af84cf9ef3e694#86a54d54

Caused by:
  object not found - no match for id (86a54d54a68b832d404ef83a03af84cf9ef3e694); class=Odb (9); code=NotFound (-3)
make[1]: Leaving directory '/home/jacob/Work/cosmic-launcher'
   dh_clean
 debian/rules build
dh build
   dh_update_autotools_config
   dh_autoreconf
   debian/rules override_dh_auto_build
make[1]: Entering directory '/home/jacob/Work/cosmic-launcher'
just build-vendored
cargo build --release --frozen --offline
warning: unused config key `build.directory` in `/home/jacob/Work/cosmic-launcher/.cargo/config.toml`
error: failed to load source for dependency `pop-launcher`

Caused by:
  Unable to update https://github.com/wiiznokes/launcher/?rev=86a54d54a68b832d404ef83a03af84cf9ef3e694#86a54d54

Caused by:
  can't checkout from 'https://github.com/wiiznokes/launcher/': you are in the offline mode (--offline)
error: Recipe `build-debug` failed on line 45 with exit code 101
make[1]: *** [debian/rules:28: override_dh_auto_build] Error 101
make[1]: Leaving directory '/home/jacob/Work/cosmic-launcher'
make: *** [debian/rules:8: build] Error 2
dpkg-buildpackage: error: debian/rules build subprocess returned exit status 2

@wiiznokes
Copy link
Contributor Author

This failed to build on the build server a few weeks ago. Testing now, it's also failing to build locally.

Probably because of the force push in launcher. But this comment pop-os/launcher#227 (comment) still need to be addressed before merging this ig

@jacobgkau
Copy link
Member

Ok. @Drakulix When you have time, let us know if you have any input regarding the compositor protocol side of pop-os/launcher#227.

@Drakulix
Copy link
Member

Drakulix commented Aug 9, 2024

Ok. @Drakulix When you have time, let us know if you have any input regarding the compositor protocol side of pop-os/launcher#227.

I am not sure where the additional events come from (probably this represents cosmic-comps internal focus stack), but the protocol as it stands doesn't guarantee any "order" of the advertised windows. Just that the state will be eventually consistent with the compositors state.

@wiiznokes
Copy link
Contributor Author

Ok. @Drakulix When you have time, let us know if you have any input regarding the compositor protocol side of pop-os/launcher#227.

I am not sure where the additional events come from (probably this represents cosmic-comps internal focus stack), but the protocol as it stands doesn't guarantee any "order" of the advertised windows. Just that the state will be eventually consistent with the compositors state.

Just fyi, I wasn't using any stack at that moment. I will try to test further to see if it's specific to some applications. So, what would be the protocol to use in this case?

@Drakulix
Copy link
Member

Drakulix commented Aug 9, 2024

Just fyi, I wasn't using any stack at that moment.

"Focus stack" internal to cosmic-comp != stacking in cosmic-comp.

So, what would be the protocol to use in this case?

I don't think we have a protocol serving this case. What this needs is probably some protocol design to expose the necessary info.

@wiiznokes
Copy link
Contributor Author

I asked KDE on matrix and they don't use a protocol for this. I will let you guys come up with a protocol because i don't think i'm experienced enough rn.

I think we have 2 options

  • returning an array of all windows sorted by last focus
  • return an event each time a windows is focused

If the option 2 is choosed, there is some usefull code is theses PRs

@wiiznokes
Copy link
Contributor Author

@Drakulix Could the protocol cosmic-toplevel-info be changed to guarantee an order ? I'm asking that because if i would write one, i would probably change nothing except this. It can differentiate different instance of the same app (thought not sure if it can group them).

Since it is a protocol special to cosmic, i thought i would ask.

@codegod100
Copy link

Thank you for your service

@Drakulix
Copy link
Member

@Drakulix Could the protocol cosmic-toplevel-info be changed to guarantee an order ? I'm asking that because if i would write one, i would probably change nothing except this. It can differentiate different instance of the same app (thought not sure if it can group them).

Since it is a protocol special to cosmic, i thought i would ask.

We added a global done event to cosmic-toplevel-info-v1 version 2.
A draft implementation of the compositor side of said protocol can be found here: pop-os/cosmic-comp#870

This should be enough to allow experimentation and figure out, if this solves the issues with this PR and allows to properly track focus.

@wiiznokes
Copy link
Contributor Author

Ok, I will work on this soon

@wiiznokes
Copy link
Contributor Author

wiiznokes commented Sep 18, 2024

I've updated both PRs. Running pop-os/cosmic-comp#870 broke the applist, and i don't receive any update here for some reason @Drakulix

@Drakulix
Copy link
Member

I've updated both PRs. Running pop-os/cosmic-comp#870 broke the applist, and i don't receive any update here for some reason @Drakulix

Hmm, that definitely should have broken older clients, I'll have to check that.

But that branch itself won't fix things. You need to use v2 of the protocol and also the ext-foreign-toplevel-list protocol (as that is mandatory with v2). So cosmic-client-toolkit needs to be updated as well to follow these changes.

@wiiznokes
Copy link
Contributor Author

But that branch itself won't fix things. You need to use v2 of the protocol and also the ext-foreign-toplevel-list protocol (as that is mandatory with v2). So cosmic-client-toolkit needs to be updated as well to follow these changes.

I don't understand how both protocol can be shared.

pub trait ToplevelInfoHandler: Sized {
    fn toplevel_info_state(&mut self) -> &mut ToplevelInfoState;

    fn new_toplevel(
        &mut self,
        conn: &Connection,
        qh: &QueueHandle<Self>,
        toplevel: &zcosmic_toplevel_handle_v1::ZcosmicToplevelHandleV1,
    );

    fn update_toplevel(
        &mut self,
        conn: &Connection,
        qh: &QueueHandle<Self>,
        toplevel: &zcosmic_toplevel_handle_v1::ZcosmicToplevelHandleV1,
    );

    fn toplevel_closed(
        &mut self,
        conn: &Connection,
        qh: &QueueHandle<Self>,
        toplevel: &zcosmic_toplevel_handle_v1::ZcosmicToplevelHandleV1,
    );
}

and

pub trait ForeignToplevelListHandler: Sized {
    fn foreign_toplevel_list_state(&mut self) -> &mut ForeignToplevelList;

    /// A new toplevel has been opened.
    fn new_toplevel(
        &mut self,
        conn: &Connection,
        qh: &QueueHandle<Self>,
        toplevel_handle: ext_foreign_toplevel_handle_v1::ExtForeignToplevelHandleV1,
    );

    /// An existing toplevel has changed.
    fn update_toplevel(
        &mut self,
        conn: &Connection,
        qh: &QueueHandle<Self>,
        toplevel_handle: ext_foreign_toplevel_handle_v1::ExtForeignToplevelHandleV1,
    );

    /// A toplevel has closed.
    fn toplevel_closed(
        &mut self,
        conn: &Connection,
        qh: &QueueHandle<Self>,
        toplevel_handle: ext_foreign_toplevel_handle_v1::ExtForeignToplevelHandleV1,
    );

    fn finished(&mut self, _conn: &Connection, _qh: &QueueHandle<Self>) {}
}

Do i really need to implement both ? I'm not sure how if yes

@Drakulix
Copy link
Member

Do i really need to implement both ? I'm not sure how if yes

What you are looking at are the abstractions provided by sctk and cctk, of course they are incompatible. What was needed here was for the cctk abstraction to be updated to handle both protocols in the background and use version 2.

@ids1024 just added such an update: pop-os/cosmic-protocols#35
The cosmic-comp branch also has been updated with a few fixes.

ToplevelInfoHandler has now a new info_done-method, which should be used to diff the Activated states to track the last new focused update.

E.g. cosmic-comp might still send events that result in five update_toplevel calls, but those should only update the states, while on info_done the latest Activated-window should be considered the new "focused" window compared to the last info_done event. Makes sense?

@wiiznokes
Copy link
Contributor Author

Ok, i've updated the launcher to use pop-os/cosmic-protocols#35, and also updated cosmic-comp. The applist is still broken, and pressing alt-tab make a black screen

@Drakulix
Copy link
Member

Ok, i've updated the launcher to use pop-os/cosmic-protocols#35, and also updated cosmic-comp. The applist is still broken, and pressing alt-tab make a black screen

Are you sure you updated the compositor? Because the app-list works fine for me with the latest commit (it definitely was broken before, just like the missing update-toplevel-events).

@wiiznokes
Copy link
Contributor Author

wiiznokes commented Sep 20, 2024

Oops, probably not, i didn't know i had to update my fork branch

@wiiznokes
Copy link
Contributor Author

E.g. cosmic-comp might still send events that result in five update_toplevel calls, but those should only update the states, while on info_done the latest Activated-window should be considered the new "focused" window compared to the last info_done event. Makes sense?

Ok, i think i understand. When info_done is called, it should only have 0 or 1 active state across all top level. However, info_done is never called in my tests

@wiiznokes
Copy link
Contributor Author

Edit: i must have done something wrong, info_done is called as it should. I've updated pop-os/launcher#227 with the necessary changes

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.

The commit this is on, 88ebb79, is not building. Tried on the build server and locally.

I get this error when trying to build locally (looks like a potential issue with the justfile changes):
   Compiling iced_widget v0.12.0 (https://github.com/pop-os/libcosmic/#57256e53)
   Compiling iced v0.12.0 (https://github.com/pop-os/libcosmic/#57256e53)
   Compiling libcosmic v0.1.0 (https://github.com/pop-os/libcosmic/#57256e53)
warning: unexpected `cfg` condition value: `a11y`
   --> src/components/list.rs:289:11
    |
289 |     #[cfg(feature = "a11y")]
    |           ^^^^^^^^^^^^^^^^
    |
    = note: expected values for `feature` are: `console`, `default`, and `wgpu`
    = help: consider adding `a11y` as a feature in `Cargo.toml`
    = note: see <https://doc.rust-lang.org/nightly/rustc/check-cfg/cargo-specifics.html> for more information about checking conditional configuration
    = note: `#[warn(unexpected_cfgs)]` on by default

warning: `cosmic-launcher` (bin "cosmic-launcher") generated 1 warning
    Finished `release` profile [optimized] target(s) in 1m 22s
make[1]: Leaving directory '/home/jacob/Work/cosmic-launcher'
   create-stamp debian/debhelper-build-stamp
 fakeroot debian/rules binary
dh binary
   dh_testroot
   dh_prep
   dh_auto_install --destdir=debian/cosmic-launcher/
   debian/rules override_dh_install
make[1]: Entering directory '/home/jacob/Work/cosmic-launcher'
just rootdir=debian/cosmic-launcher install
install -Dm0755 target/target/release/cosmic-launcher /home/jacob/Work/cosmic-launcher/debian/cosmic-launcher/usr/bin/cosmic-launcher
install: cannot stat 'target/target/release/cosmic-launcher': No such file or directory
error: Recipe `install` failed on line 70 with exit code 1
make[1]: *** [debian/rules:31: override_dh_install] Error 1
make[1]: Leaving directory '/home/jacob/Work/cosmic-launcher'
make: *** [debian/rules:8: binary] Error 2
dpkg-buildpackage: error: fakeroot debian/rules binary subprocess returned exit status 2

@@ -44,3 +49,7 @@ switcheroo-control = { git = "https://github.com/pop-os/dbus-settings-bindings"
zbus = { version = "4.2.1", default-features = false, features = ["tokio"] }
unicode-truncate = "1.0.0"
unicode-width = "0.1.11"

[patch."https://github.com/pop-os/launcher/"]
pop-launcher = { git = "https://github.com/wiiznokes/launcher/", rev = "86a54d54a68b832d404ef83a03af84cf9ef3e694" }
Copy link
Member

@jacobgkau jacobgkau Sep 23, 2024

Choose a reason for hiding this comment

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

I'm assuming we won't want to merge this reference to a fork into master.

If pop-os/launcher#227 doesn't cause any regressions for the pop-shell frontend on 22.04, then I can regression test that one and we can get it merged first, if that would help? (It'd be nice to be able to test that it works properly, and I can also still do that if we get this PR to build.)

Copy link
Contributor Author

@wiiznokes wiiznokes Sep 24, 2024

Choose a reason for hiding this comment

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

I will look into it. I must say I ran into this problem, but the problem it's just the target/target (should be one target), so it's easy to work around it.

If you want to test the other pr, you must also install this one for it to work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

5 participants