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.

Sign up for GitHub

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

Jump to bottom

Rework control shift press handling - Gtk4 prep #788

Closed
wants to merge 2 commits into from
Closed

Rework control shift press handling - Gtk4 prep #788

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cebd5ef
Rework control shift press handling
jeremypw Sep 8, 2024
126ef08
Merge master
danirabbit Sep 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 37 additions & 26 deletions src/Widgets/TerminalWidget.vala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,8 @@ namespace Terminal {
private Gtk.GestureMultiPress primary_gesture;
private Gtk.GestureMultiPress secondary_gesture;

private bool modifier_pressed = false;
private bool control_pressed = false;
private bool shift_pressed = false;
Comment on lines +151 to +152
Copy link
Member

Choose a reason for hiding this comment

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

It seems like these are only used inside the key press handler. Why are we setting these variables outside of that function at all?

Copy link
Collaborator Author

@jeremypw jeremypw Sep 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Because they need to maintain their state between key presses when multiple keys are held down sequentially.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand what you mean. Testing locally, replacing all of this with checking for CONTROL and SHIFT mask in modifiers seems to work as expected. Can you provide steps where it doesn't work?

Copy link
Member

Choose a reason for hiding this comment

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

Here's what I'm testing

diff --git a/src/Widgets/TerminalWidget.vala b/src/Widgets/TerminalWidget.vala
index cc779d30..eca5e5c4 100644
--- a/src/Widgets/TerminalWidget.vala
+++ b/src/Widgets/TerminalWidget.vala
@@ -148,8 +148,6 @@ namespace Terminal {
         private Gtk.GestureMultiPress primary_gesture;
         private Gtk.GestureMultiPress secondary_gesture;
 
-        private bool control_pressed = false;
-        private bool shift_pressed = false;
         private double scroll_delta = 0.0;
 
         public signal void cwd_changed (string cwd);
@@ -196,7 +194,7 @@ namespace Terminal {
                 propagation_phase = TARGET
             };
             key_controller.key_pressed.connect (key_pressed);
-            key_controller.key_released.connect (key_released);
+            key_controller.key_released.connect (() => scroll_controller.flags = NONE);
             key_controller.focus_out.connect (() => scroll_controller.flags = NONE);
 
             primary_gesture = new Gtk.GestureMultiPress (this) {
@@ -337,13 +335,9 @@ namespace Terminal {
             switch (keyval) {
                 case Gdk.Key.Control_R:
                 case Gdk.Key.Control_L:
-                    scroll_controller.flags = VERTICAL;
-                    control_pressed = true;
-                    break;
                 case Gdk.Key.Shift_R:
                 case Gdk.Key.Shift_L:
                     scroll_controller.flags = VERTICAL;
-                    shift_pressed = true;
                     break;
                 case Gdk.Key.Alt_L:
                 case Gdk.Key.Alt_R:
@@ -381,7 +375,7 @@ namespace Terminal {
                     popup_context_menu (rect);
                     break;
                 default:
-                    if (!(control_pressed || shift_pressed) || !(Gtk.accelerator_get_default_mod_mask () in modifiers)) {
+                    if (!(CONTROL_MASK in modifiers || SHIFT_MASK in modifiers) || !(Gtk.accelerator_get_default_mod_mask () in modifiers)) {
                         remember_command_start_position ();
                     }
                     break;
@@ -403,11 +397,11 @@ namespace Terminal {
                 return false;
             }
 
-            if (control_pressed && Application.settings.get_boolean ("natural-copy-paste")) {
+            if (CONTROL_MASK in modifiers && Application.settings.get_boolean ("natural-copy-paste")) {
                 if (match_keycode (Gdk.Key.c, keycode)) {
                     if (get_has_selection ()) {
                         copy_clipboard ();
-                        if (!shift_pressed) { // Shift not pressed
+                        if (!(SHIFT_MASK in modifiers)) { // Shift not pressed
                             unselect_all ();
                         }
                         return true;
@@ -427,23 +421,6 @@ namespace Terminal {
             return false;
         }
 
-        private void key_released (uint keyval, uint keycode, Gdk.ModifierType modifiers) {
-            switch (keyval) {
-                case Gdk.Key.Control_R:
-                case Gdk.Key.Control_L:
-                    control_pressed = false;
-                    break;
-                case Gdk.Key.Shift_R:
-                case Gdk.Key.Shift_L:
-                    shift_pressed = true;
-                    break;
-                default:
-                    break;
-            }
-
-            scroll_controller.flags = NONE;
-        }
-
         private void setup_menu () {
             // Update the "Paste" menu option
             clipboard.request_targets ((clipboard, atoms) => {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This had to be done in the Gtk4 version because

Gtk4 seems to still have a bug in the "state" parameter of the key controller signals in that if multiple modifiers are held down this parameter only includes one of them so we still have to track the state ourselves.

I'll check again whether this fix is needed.

private double scroll_delta = 0.0;

public signal void cwd_changed (string cwd);
Expand Down Expand Up @@ -192,23 +193,12 @@ namespace Terminal {
scroll_controller.scroll_end.connect (() => scroll_delta = 0.0);

key_controller = new Gtk.EventControllerKey (this) {
propagation_phase = NONE
propagation_phase = TARGET
};
key_controller.key_pressed.connect (key_pressed);
key_controller.key_released.connect (() => scroll_controller.flags = NONE);
key_controller.key_released.connect (key_released);
key_controller.focus_out.connect (() => scroll_controller.flags = NONE);

// XXX(Gtk3): This is used to stop the key_pressed() handler from breaking the copy last output action,
// when a modifier is pressed, since it won't be in the modifier mask there (neither here).
//
// TODO(Gtk4): check if the modifier emission was fixed.
key_controller.modifiers.connect (() => {
// if two modifers are pressed in sequence (like <Control> -> <Shift>), modifier_pressed will be false.
// However, the modifer mask in key_pressed() will already contain the previous modifier.
modifier_pressed = !modifier_pressed;
return true;
});

primary_gesture = new Gtk.GestureMultiPress (this) {
propagation_phase = TARGET,
button = 1
Expand All @@ -221,7 +211,8 @@ namespace Terminal {
};
secondary_gesture.released.connect (secondary_released);

// send events to key controller manually, since key_released isn't emitted in any propagation phase
// Send events to key controller manually, since key_released isn't emitted in any propagation phase
// This is not needed in Gtk4 as key_released is emitted.
event.connect (key_controller.handle_event);

selection_changed.connect (() => copy_action.set_enabled (get_has_selection ()));
Expand Down Expand Up @@ -347,8 +338,18 @@ namespace Terminal {
case Gdk.Key.Control_R:
case Gdk.Key.Control_L:
scroll_controller.flags = VERTICAL;
control_pressed = true;
break;
case Gdk.Key.Shift_R:
case Gdk.Key.Shift_L:
scroll_controller.flags = VERTICAL;
shift_pressed = true;
break;
case Gdk.Key.Alt_L:
case Gdk.Key.Alt_R:
// enable/disable the action before we try to use
copy_output_action.set_enabled (!resized && get_last_output ().length > 0);
break;

case Gdk.Key.Return:
remember_position ();
scroll_to_command_action.set_enabled (true);
Expand Down Expand Up @@ -379,15 +380,8 @@ namespace Terminal {

popup_context_menu (rect);
break;

case Gdk.Key.Alt_L:
case Gdk.Key.Alt_R:
// enable/disable the action before we try to use
copy_output_action.set_enabled (!resized && get_last_output ().length > 0);
break;

default:
if (!modifier_pressed || !(Gtk.accelerator_get_default_mod_mask () in modifiers)) {
if (!(control_pressed || shift_pressed) || !(Gtk.accelerator_get_default_mod_mask () in modifiers)) {
remember_command_start_position ();
}
break;
Expand All @@ -409,11 +403,11 @@ namespace Terminal {
return false;
}

if (CONTROL_MASK in modifiers && Application.settings.get_boolean ("natural-copy-paste")) {
if (control_pressed && Application.settings.get_boolean ("natural-copy-paste")) {
if (match_keycode (Gdk.Key.c, keycode)) {
if (get_has_selection ()) {
copy_clipboard ();
if (!(SHIFT_MASK in modifiers)) { // Shift not pressed
if (!shift_pressed) { // Shift not pressed
unselect_all ();
}
return true;
Expand All @@ -433,6 +427,23 @@ namespace Terminal {
return false;
}

private void key_released (uint keyval, uint keycode, Gdk.ModifierType modifiers) {
switch (keyval) {
case Gdk.Key.Control_R:
case Gdk.Key.Control_L:
control_pressed = false;
break;
case Gdk.Key.Shift_R:
case Gdk.Key.Shift_L:
shift_pressed = true;
break;
default:
break;
}

scroll_controller.flags = NONE;
}

private void setup_menu () {
// Update the "Paste" menu option
clipboard.request_targets ((clipboard, atoms) => {
Expand Down