VENOM-389: Fix modifiers issue #390
Conversation
src/view/ConversationWindow.vala
Outdated
| public bool on_key_press_event(Gdk.EventKey event) { | ||
| if (event.keyval == key.accel_key && event.state == key.accel_mods) { | ||
| send(); | ||
| if (event.keyval in new uint[]{Gdk.Key.Return, Gdk.Key.KP_Enter}) { |
There was a problem hiding this comment.
this will discard any user set preferences on which key to press for sending
Codecov Report
@@ Coverage Diff @@
## develop #390 +/- ##
==========================================
- Coverage 24.62% 24.6% -0.02%
==========================================
Files 101 101
Lines 5938 5942 +4
==========================================
Hits 1462 1462
- Misses 4476 4480 +4
Continue to review full report at Codecov.
|
src/view/ConversationWindow.vala
Outdated
| send(); | ||
| if (event.keyval in new uint[]{Gdk.Key.Return, Gdk.Key.KP_Enter}) { | ||
| if ((event.state & Gdk.ModifierType.SHIFT_MASK) > 0) { | ||
| insert_new_line(); |
There was a problem hiding this comment.
return false; should do the same, pressing enter in a textview adds a "\n"
| if (event.keyval == key.accel_key && event.state == key.accel_mods) { | ||
| send(); | ||
| if (event.keyval == key.accel_key) { | ||
| if ((event.state & Gdk.ModifierType.SHIFT_MASK) > 0) { |
There was a problem hiding this comment.
this still discards the key.accels_mod
There was a problem hiding this comment.
Does this work on your system?
public bool on_key_press_event(Gdk.EventKey event) {
uint modifiers = event.state & (Gdk.ModifierType.SHIFT_MASK | Gdk.ModifierType.CONTROL_MASK);
if (event.keyval == key.accel_key && modifiers == key.accel_mods) {
send();
return true;
}
return false;
}
|
I guess I could replace the SHIFT_MASK with key.accel_mods |
| send(); | ||
| if (event.keyval == key.accel_key) { | ||
| if ((event.state & Gdk.ModifierType.SHIFT_MASK) > 0) { | ||
| insert_new_line(); |
There was a problem hiding this comment.
Please remove the insert_new_line and just return false, otherwise we are catching stuff here that we aren't supposed to. If this will be needed in the future we can always add it.
|
If you replace the shift_mask with accel_mods then we are pretty much where we were before :) |
|
Mh |
|
What is needed for this to be merged? |
|
There are still requested changes that need to be adressed. Please don't feature creep in bug fixes, or if you do, at least provide reasoning why you think this improves the existing behaviour. The current implementation, though broken, catches a user definable key combination (key+modifier(s)), consumes the event on an exact match. This means I could define something like ctrl-s as a key combination to send messages. Your change would change the behavior, you would catch any key combination containing the user defined key and ignore the user defined modifier(s) all together. Additionally would the user defined key and shift always insert a newline. This means that if someone sets the send key accel to ctrl-s, suddenly pressing s would send a message and shift-s would insert a newline (what?). I hope this explains my concerns here. |
closes #389