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

Alt+Tab switches tabs #262

Closed
Aloso opened this issue Jun 8, 2023 · 12 comments · Fixed by #298
Closed

Alt+Tab switches tabs #262

Aloso opened this issue Jun 8, 2023 · 12 comments · Fixed by #298
Assignees
Labels
bug Something isn't working
Milestone

Comments

@Aloso
Copy link
Contributor

Aloso commented Jun 8, 2023

Describe the bug
Alt+Tab is used to switch between windows. It should not switch tabs within an application.

To Reproduce

  1. Open sniffnet
  2. Press Alt+Tab to switch to a different window
  3. Switch back to sniffnet
  4. Notice that a different tab is focused

Expected behavior
Alt+Tab should not switch tabs.

Desktop (please complete the following information):

  • OS: Linux
  • Version 1.2.1

Additional context
Installed with cargo install sniffnet

@GyulyVGC
Copy link
Owner

GyulyVGC commented Jun 8, 2023

This little bug was introduced just a few days ago in #248.
I will personally fix it before the next release 👍

@GyulyVGC GyulyVGC added the bug Something isn't working label Jun 8, 2023
@GyulyVGC GyulyVGC added this to the v1.2.2 milestone Jun 8, 2023
@Aloso
Copy link
Contributor Author

Aloso commented Jun 8, 2023

@GyulyVGC I also have the bug in v1.2.0, I don't think it was introduced in the PR you linked

@Aloso
Copy link
Contributor Author

Aloso commented Jun 8, 2023

Oh, I think the bug is two-fold:

  • When sniffnet is focused, pressing Alt+Tab immediately switches to the next tab (introduced in v1.2.1)
  • When another program is focused, pressing Alt+Tab to switch to sniffnet may also switch to the next tab (existed before)

The 2nd bug seems to only occur when the Tab key is released after the Alt key.

@GyulyVGC
Copy link
Owner

GyulyVGC commented Jun 8, 2023

Thanks for the detailed report.
I'll better investigate.

@GyulyVGC GyulyVGC self-assigned this Jun 27, 2023
GyulyVGC added a commit that referenced this issue Jun 29, 2023
@GyulyVGC
Copy link
Owner

I've restored the original situation (that of v1.2.0, with the difference that now also esc and enter only apply the shortcut when no modifier at all is pressed).

  • When another program is focused, pressing Alt+Tab to switch to sniffnet may also switch to the next tab (existed before)

The 2nd bug seems to only occur when the Tab key is released after the Alt key.

I was not able to reproduce this behaviour though.
If Sniffnet is not focused, shouldn't receive commands at all.
The fact that it happens when you release the tab key later suggests that it triggers when no modifiers are pressed, which is what we want (if the app is focused ofc).

@starccy
Copy link
Contributor

starccy commented Jul 3, 2023

I am in the main branch. For the second problem, I can reproduce it whenever I switch back and release the Alt + Tab key simultaneously.

This behavior might be different from other apps, it seems too "sensitive". As a comparison, for example, in a web browser, you can use Tab to focus between components on a web page. And when using Alt + Tab (in any order) to switch back, it still focuses on the same component.

@GyulyVGC
Copy link
Owner

GyulyVGC commented Jul 3, 2023

alt+tab should not have an effect now on the app.
Could you try releasing the tab slightly before the alt?
I guess that the problem is that if you have tab pressed (without modifiers) even for a short instant, the app immediately captures it.
Really weird that's happening with the app unfocused though.
On which OS are you running @starccy?
Feel free to provide a short video if you want, so that I can better understand your specific situation.

@starccy
Copy link
Contributor

starccy commented Jul 3, 2023

@GyulyVGC

I am using Kali Linux with Gnome 44 + X11.

alt+tab should not have an effect now on the app.

Even if you released tab after alt?

I try to slow down my actions to make sure every Tab be released before Alt be released, the tabs within the app won't switch. ✔️

I guess that the problem is that if you have tab pressed (without modifiers) even for a short instant, the app immediately captures it.

Yes you are right. I have done a small test to compare keyboard events between the iced-based app and the gtk-based app to find the difference.

For the iced app (this project), I simply insert a print statement here to log keyboard events:

sniffnet/src/gui/app.rs

Lines 110 to 114 in 3268b0e

iced_native::subscription::events_with(|event, _| match event {
iced_native::Event::Keyboard(Event::KeyPressed {
key_code,
modifiers,
}) => match modifiers {


For the gtk app, I write a small app to print key events:

use gtk::{Application, ApplicationWindow, EventControllerKey, glib};
use gtk::prelude::*;

const APP_ID: &str = "just.for.test";

fn main() -> glib::ExitCode {
    let app = Application::builder()
        .application_id(APP_ID)
        .build();

    app.connect_activate(build_ui);

    app.run()
}

fn build_ui(app: &Application) {
    let window = ApplicationWindow::builder()
        .application(app)
        .title("test")
        .build();

    let event_ctrl = EventControllerKey::new();
    event_ctrl.connect_key_pressed(|_, key, _, modifier| {
        println!("Pressed | Key: {}, Modifier: {:?}", key.name().unwrap_or(key.to_string().into()), modifier);
        gtk::Inhibit(true)
    });
    event_ctrl.connect_key_released(|_, key, _, modifier| {
        println!("Released | Key: {}, Modifier: {:?}", key.name().unwrap_or(key.to_string().into()), modifier);
    });

    window.add_controller(event_ctrl);

    window.present();
}

For the both apps, I do the following procedures:

  • Using Alt + Tab to switch back to the app, and make sure the Tab key is released before Alt. I got no output from both apps, which is as expected. ✔️
  • Using Alt + Tab to switch back to the app, but let Tab be released after Alt. Here are the outputs:

The iced app output:

KeyPressed { key_code: Tab, modifiers: (empty) }
KeyReleased { key_code: Tab, modifiers: (empty) }

The gtk4 app output:

Release | Key: Tab, Modifier: (empty)

The result is obvious, the gtk4 program does not emit the pressed event immediately, which is the culprit causing this problem.

In fact, If I keep pressing the Tab key for enough long time, the gtk app will also start to emit the pressed event. But in contrast to iced, it has a slight delay to trigger.

I haven't dug too deep into iced or gtk, so I don't know if they serve different purposes and make different logic. But in my opinion, the gtk one is more intuitive and can avoid this issue.

In conclusion, the key point is how to avoid emit key event immediately, or should we open an issue in iced repo?

@GyulyVGC
Copy link
Owner

GyulyVGC commented Jul 3, 2023

Wow, thanks for the very detailed report.

So, if I get it correctly the problem is that the tab key immediately results as 'pressed' even if the alt key was 'released' just an instance before.

I think that from a logic point of view it's hard to define which alternative is better, to me it's not even so meaningful to have a state 'released tab' if the reality is that it was the alt to be released.

Of course iced logic could lead to some usability problems, which should be the following two in our case (correct me if I'm wrong):

  • a user wants to switch app while using Sniffnet. He presses tab slightly before alt, causing also Sniffnet to switch
  • a user wants to switch app coming back to Sniffnet. He releases alt slightly before tab, causing also Sniffnet to switch

I'll try to see if I can do something using some timer and the KeyReleased event: https://docs.iced.rs/iced/keyboard/enum.Event.html

@starccy
Copy link
Contributor

starccy commented Jul 4, 2023

I second that.

But for the first case, I am unable to do further verification. I don't think it's a problem because pressing tab before alt is not a valid combination, at least for my OS, the Task Switcher won't pop up. So in this situation, they should be treated as two individual events as normal.

I found a similar issue from iced: iced-rs/iced#612. Unfortunately, it seems to be closed by misunderstanding.

'll try to see if I can do something using some timer and the KeyReleased event

If along this line of thought, I think we can record the last focus time for the window. Every time when Tab be pressed, we can check the diff time. If it is less than a period of time, just ignore the switch page request.

Here is an available code:

diff --git a/src/gui/app.rs b/src/gui/app.rs
index ff99bae..a58b5c6 100644
--- a/src/gui/app.rs
+++ b/src/gui/app.rs
@@ -104,10 +104,14 @@ fn view(&self) -> Element<Message> {
 
     fn subscription(&self) -> Subscription<Message> {
         use iced_native::keyboard::{Event, KeyCode, Modifiers};
+        use iced_native::window;
         const NO_MODIFIER: iced_native::keyboard::Modifiers =
             iced_native::keyboard::Modifiers::empty();
         let hot_keys_subscription =
             iced_native::subscription::events_with(|event, _| match event {
+                iced_native::Event::Window(window::Event::Focused) => {
+                    Some(Message::WindowFocused)
+                }
                 iced_native::Event::Keyboard(Event::KeyPressed {
                     key_code,
                     modifiers,
diff --git a/src/gui/types/message.rs b/src/gui/types/message.rs
index 229fed3..d2179cb 100644
--- a/src/gui/types/message.rs
+++ b/src/gui/types/message.rs
@@ -80,4 +80,5 @@ pub enum Message {
     UpdatePageNumber(bool),
     /// Left (false) or Right (true) arrow key has been pressed
     ArrowPressed(bool),
+    WindowFocused,
 }
diff --git a/src/gui/types/sniffer.rs b/src/gui/types/sniffer.rs
index 374150b..a6151da 100644
--- a/src/gui/types/sniffer.rs
+++ b/src/gui/types/sniffer.rs
@@ -81,6 +81,31 @@ pub struct Sniffer {
     pub page_number: usize,
     /// Currently selected connection for inspection of its details
     pub selected_connection: usize,
+    pub focus_state: FocusState,
+}
+
+const FOCUS_TIMEOUT: u64 = 200;
+
+pub struct FocusState {
+    last_focus_time: std::time::Instant,
+    focus_timeout: std::time::Duration,
+}
+
+impl FocusState {
+    fn new(focus_timeout: u64) -> Self {
+        Self {
+            last_focus_time: std::time::Instant::now(),
+            focus_timeout: std::time::Duration::from_millis(focus_timeout),
+        }
+    }
+
+    fn update(&mut self) {
+        self.last_focus_time = std::time::Instant::now();
+    }
+
+    pub fn is_just_focus(&self) -> bool {
+        self.last_focus_time.elapsed() < self.focus_timeout
+    }
 }
 
 impl Sniffer {
@@ -116,6 +141,7 @@ pub fn new(
             search: SearchParameters::default(),
             page_number: 1,
             selected_connection: 0,
+            focus_state: FocusState::new(FOCUS_TIMEOUT),
         }
     }
 
@@ -182,7 +208,11 @@ pub fn update(&mut self, message: Message) -> Command<Message> {
                 return self.update(Message::HideModal);
             }
             Message::Quit => return window::close(),
-            Message::SwitchPage(next) => self.switch_page(next),
+            Message::SwitchPage(next) => {
+                if !self.focus_state.is_just_focus() {
+                    self.switch_page(next)
+                }
+            },
             Message::ReturnKeyPressed => return self.shortcut_return(),
             Message::EscKeyPressed => return self.shortcut_esc(),
             Message::ResetButtonPressed => return self.reset_button_pressed(),
@@ -215,6 +245,9 @@ pub fn update(&mut self, message: Message) -> Command<Message> {
                     }
                 }
             }
+            Message::WindowFocused => {
+                self.focus_state.update();
+            }
         }
         Command::none()
     }
        

@GyulyVGC
Copy link
Owner

GyulyVGC commented Jul 4, 2023

The idea seems good to me.
If it already works, feel free to open a PR so that we can better discuss it.
Thank you very much!

@starccy
Copy link
Contributor

starccy commented Jul 4, 2023

Yes, I've tested the code on my computer and it works as expected. I will submit a pull request later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants