-
Notifications
You must be signed in to change notification settings - Fork 15
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
Improve Event Controllers #181
base: main
Are you sure you want to change the base?
Conversation
1fded78
to
d6543f8
Compare
Can somebody at least look at this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay!
I left a few general notes, mostly for the JavaScript, but I'm not a Python pro so someone else may need to peek at that :)
@@ -48,33 +36,41 @@ const gesture_click = new Gtk.GestureClick({ button: 0 }); | |||
window.add_controller(gesture_click); | |||
|
|||
gesture_click.connect("pressed", (_self, _n_press, _x, _y) => { | |||
switch (gesture_click.get_current_button()) { | |||
let css_class = "suggested-action"; | |||
if (_self.get_current_event_state() & Gdk.ModifierType.CONTROL_MASK) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The _self
variable should have the leading underscore removed, if it's being used now.
label: _("Ctrl + Click to Activate"); | ||
label: _("Ctrl Key"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the label here should indicate that it represents the Control key state, and probably shouldn't be a button anymore since the signal has been removed.
break; | ||
} | ||
}); | ||
|
||
gesture_click.connect("released", (_self, _n_press, _x, _y) => { | ||
switch (gesture_click.get_current_button()) { | ||
switch (_self.get_current_button()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto for _self
I've originally just wanted to remove global variable tracking control key state. It is tad bit quirky and seems like a bad practice.
It was supposed to be a small fix to use
get_current_event_state()
directly inGestureClick
controller instead of global variable. What I didn't notice at first was that the example was showcasingEventControllerKey
which would be completely removed by this "fix". So in the end I needed to do both fix mouse button events with key modifiers and come up with new show case forEventControllerKey
.