search

joelekstrom / fastmate

A native Fastmail-wrapper for Mac.
MIT License
194 stars 12 forks source link

Enabled delete and up/down arrow keys #54

Closed kalkwarf closed 2 years ago

kalkwarf commented 2 years ago

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. :-)

joelekstrom commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.

joelekstrom commented 2 years ago

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 commented 2 years ago

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