Enabled delete and up/down arrow keys #54

kalkwarf · 2021-12-10T21:53:50Z

Installed a sendEvent hook to snoop and then optionally process and consume raw keydowns
Installed a wrapper to make async keystrokes synchronous
Added key handlers for several naked keystrokes, gated on appropriate document focus

If you have any questions, please ask. If you opt to reject these changes because they are too invasive (or fragile) no offense will be taken. :-)

- Installed a `sendEvent` hook to snoop and then optionally process and consume raw keydowns - Installed a wrapper to make async keystrokes synchronous - Added key handlers for several naked keystrokes, gated on appropriate document focus

joelekstrom · 2021-12-12T21:37:50Z

Hi! Thanks so much for the PR. It's much appreciated, and code looks good to me. However, this will overwrite the default arrow key behaviour (scrolling) in Fastmail, right? In that case, maybe it would make more sense to have a modifier, like shift up/down to change message. Or at least gate it behind a setting so users can opt in. Thoughts? The delete keypress makes sense to catch, as long as it isn't activated when typing a message or searching!

kalkwarf · 2021-12-13T01:48:52Z

Hi! Thanks so much for the PR. It's much appreciated, and code looks good to me. However, this will overwrite the default arrow key behaviour (scrolling) in Fastmail, right? In that case, maybe it would make more sense to have a modifier, like shift up/down to change message. Or at least gate it behind a setting so users can opt in. Thoughts? The delete keypress makes sense to catch, as long as it isn't activated when typing a message or searching!

You're right! I hadn't noticed that an unmodified arrow was a scroll gesture (I use the spacebar for this). Obviously, we have different usage patterns for email reading!

I'm okay with a preference, a modifier, or both. This is your show, so what would you prefer? I am happy to write whatever is needed to satisfy everyone.

joelekstrom · 2021-12-13T11:01:42Z

As far as I can tell, shiftup/down does not have any default behaviour in Fastmail, so that seems like good fit for this (and should be easier to implement than a new setting). Both work fine for me though.

shiftup/down keys does have a meaning when typing a message (extend selection) so make sure it's not interfering with that if you go with that solution!

kalkwarf · 2021-12-13T16:16:36Z

I went with a setting, defaulting to the existing behavior. While a modifier is simpler, it becomes a chord instead of a single key, which sort of defeats the purpose.

I'm going to annotate the PR to show you the mechanism by which these key handers don't get in the way of composition.

kalkwarf · 2021-12-13T16:21:54Z

Fastmate/FastmateApplication.m

+@implementation FastmateApplication
+@dynamic delegate;
+
+- (void)sendEvent:(NSEvent *)event {


It all starts here. Instead of using the NSApplication sendEvent handler, we insert ourselves here. Every keyDown event gets passed to our applicationDelegate for processing. If the delegate "consumes" the event, it returns @YES, and we stop processing. If the delegate doesn't like the event (for several reasons, I'll describe momentarily) it returns @NO and we pass the original event on to the default application controller.

kalkwarf · 2021-12-13T16:23:15Z

Fastmate/AppDelegate.m

+            return [self.mainWebViewController deleteMessage];
+
+        default:
+            return NO;


Next step is here in the delegate. If it's not one of the three raw keys, tell the application controller we didn't want the event.

kalkwarf · 2021-12-13T16:24:26Z

Fastmate/AppDelegate.m

+- (BOOL)handleKey:(NSEvent *)event {
+    switch (event.keyCode) {
+        case kVK_UpArrow:
+        if ([NSUserDefaults.standardUserDefaults boolForKey:ArrowNavigatesMessageListKey]) {


Here (and line 206) we look at the new setting, and either toss the event (letting the default behavior occur) or we attempt to have the mainWebViewController deal with it.

kalkwarf · 2021-12-13T16:25:46Z

Fastmate/WebViewController.m

 - (void)focusSearchField {
    [self.webView evaluateJavaScript:@"Fastmate.focusSearch()" completionHandler:nil];
 }

+- (BOOL)nextMessage {


Here we bridge back into the world of JavaScript, and again attempt to handle the key. Note that instead of fire-and-forget, these three functions return a BOOL.

kalkwarf · 2021-12-13T16:28:54Z

Fastmate/Fastmate.js

    focusSearch: function() {
        Fastmate.simulateKeyPress("/");
    },

+    nextMessage: function() {
+        var content = document.getElementById("v54");


Finally, part 1:
Element v54 appears to be the main navigation window. It must be present, and it must have focus (or be the active element, in JS speak). If it is, we call your keystroke simulator, and return true. If it is not, we assume search, composition, or something else not-the-message-browser has focus, and we return false, allowing everything to unwind back to the FastmateApplication, where the event will get forwarded back to the default handler.

kalkwarf · 2021-12-13T16:30:28Z

Fastmate/Fastmate.js

+    deleteMessage: function() {
+        var deleteButton = document.getElementById("v34");
+        var content = document.getElementById("v54");
+        if (deleteButton != null && deleteButton.textContent == 'Delete' &&


Finally, part 2:
Similar to the arrow rules, we check out the state of the display, and if it appears to be the message list, and we can locate the Delete button, we ask the button to do its action.

kalkwarf · 2021-12-13T16:32:20Z

Fastmate/WebViewController.m

+        waiting = FALSE;
+    }];
+
+    while (waiting) {


Sidenote: JS is async, and we need to let the caller know if we consumed the keystroke successfully. To do that, we create a spinloop waiting for the JS to complete execution. On paper, this is a terrible idea. In practice, it takes almost no time at all to decide if any given key will be handled, so it's a solution I can live with.

joelekstrom · 2021-12-13T22:41:14Z

I appreciate the detailed walkthrough 😊 it looks great to me. Will take a more thorough look tomorrow when I’m not on mobile and we can get this merged!

joelekstrom · 2021-12-14T10:14:29Z

Added in v1.7.0. Thanks so much for contributing!

Enabled delete and up/down arrow keys

ce37f66

- Installed a `sendEvent` hook to snoop and then optionally process and consume raw keydowns - Installed a wrapper to make async keystrokes synchronous - Added key handlers for several naked keystrokes, gated on appropriate document focus

kalkwarf added 4 commits December 13, 2021 10:37

Align content so it doesn't shift while switching tabs

89fe27c

Preferences UI for arrow navigation

860bc87

Obey the UserDefault for key navigation

1fa55bc

Sorted imports

ccfd1c0

kalkwarf commented Dec 13, 2021

View reviewed changes

joelekstrom approved these changes Dec 14, 2021

View reviewed changes

joelekstrom merged commit 5f6f66b into joelekstrom:master Dec 14, 2021

kalkwarf deleted the additional-keyhandlers branch February 6, 2022 16:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enabled delete and up/down arrow keys #54

Enabled delete and up/down arrow keys #54

kalkwarf commented Dec 10, 2021

joelekstrom commented Dec 12, 2021 •

edited

Loading

kalkwarf commented Dec 13, 2021

joelekstrom commented Dec 13, 2021 •

edited

Loading

kalkwarf commented Dec 13, 2021

kalkwarf Dec 13, 2021

kalkwarf Dec 13, 2021 •

edited

Loading

kalkwarf Dec 13, 2021

kalkwarf Dec 13, 2021

kalkwarf Dec 13, 2021

kalkwarf Dec 13, 2021

kalkwarf Dec 13, 2021

joelekstrom commented Dec 13, 2021

joelekstrom commented Dec 14, 2021

Enabled delete and up/down arrow keys #54

Enabled delete and up/down arrow keys #54

Conversation

kalkwarf commented Dec 10, 2021

joelekstrom commented Dec 12, 2021 • edited Loading

kalkwarf commented Dec 13, 2021

joelekstrom commented Dec 13, 2021 • edited Loading

kalkwarf commented Dec 13, 2021

kalkwarf Dec 13, 2021

Choose a reason for hiding this comment

kalkwarf Dec 13, 2021 • edited Loading

Choose a reason for hiding this comment

kalkwarf Dec 13, 2021

Choose a reason for hiding this comment

kalkwarf Dec 13, 2021

Choose a reason for hiding this comment

kalkwarf Dec 13, 2021

Choose a reason for hiding this comment

kalkwarf Dec 13, 2021

Choose a reason for hiding this comment

kalkwarf Dec 13, 2021

Choose a reason for hiding this comment

joelekstrom commented Dec 13, 2021

joelekstrom commented Dec 14, 2021

joelekstrom commented Dec 12, 2021 •

edited

Loading

joelekstrom commented Dec 13, 2021 •

edited

Loading

kalkwarf Dec 13, 2021 •

edited

Loading