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.

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

Feature/Custom Keybinds #569

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Feature/Custom Keybinds #569

wants to merge 2 commits into from

Conversation

luisliz
Copy link

@luisliz luisliz commented Feb 16, 2024


  • I understand that contributing to this repository will require me to agree with the CLA

Description

This is for adding custom keybindings to the app it leverages the same shortcut mechanism that is already used in the app but, redone to use an actions mechanism instead of fixed. When no user actions are set in the keybindingStore it just performs the existing shortcuts.
There is also a shortcut detection system in the settings popup that disables all other shortcuts when the box is focused, tied to focus to hopefully not cause issues where all keybinds get disabled.
Fixed keymaps cannot be changed or disabled.

I have a lot of placeholder text for now but wanted to get some feedback before I continue, to make sure I don't have anything fundamentally wrong.
I'll definitely replace them with the localized versions

Need guidance

  • Should this be stored in the config that syncs? Thought of starting with non-syncing at first.
  • I know there are bindings that apply to e.g. editor but also to main I think this might cause issues
  • Should I separate which keymaps can be disabled or which can be overriden, I feel like the fixed keymap is enough.

P.S. I know this PR might be too big but I can break it up if needed.
P.S.S. Kudos on this repo i'm mostly a vue developer other than some react-native from work but it has been really enjoyable to work in it, i'm more of a fan of anytype after understanding the infra.

What type of PR is this? (check all applicable)

  • πŸ• Feature
  • πŸ› Bug Fix
  • πŸ“ Documentation Update
  • 🎨 Style
  • πŸ§‘β€πŸ’» Code Refactor
  • πŸ”₯ Performance Improvements
  • βœ… Test
  • πŸ€– Build
  • πŸ” CI

Related Tickets & Documents

https://community.anytype.io/t/customizable-keyboard-shortcuts-hotkeys/2135
My end goal is this :) https://community.anytype.io/t/vim-like-modes-keybindings/1212

Mobile & Desktop Screenshots/Recordings

2024-02-15.2020-51-46.mp4

Added tests?

  • πŸ‘ yes
  • πŸ™… no, because they aren't needed
  • πŸ™‹ no, because I need help

Added to documentation?

Not yet

  • πŸ“œ README.md
  • πŸ““ tech-docs
  • πŸ™… no documentation needed

[optional] Are there any post-deployment tasks we need to perform?

Copy link

github-actions bot commented Feb 16, 2024

CLA Assistant Lite bot:
Thank you for your pull request, we really appreciate it!

Please sign our Contributor License Agreement before we can accept your contribution.
You can sign the CLA by simply commenting on this pull request with the following text.


I have read the CLA Document and I hereby sign the CLA


You can retrigger this bot by commenting recheck in this Pull Request

e.preventDefault();
// Close popups and menus
this.shortcut('escape', e, () => {
e.preventDefault();
Copy link
Author

Choose a reason for hiding this comment

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

Moved escape up to allow to escape the keybinding window dialog when all other keys are disabled.

@ra3orblade
Copy link
Contributor

Hello, thanks for the PR, we had different idea in mind when we were discussing custom shortcuts: the setting itself should be inside the shortcut popup, or maybe it should be combined now with the settings, idk. Better discuss with designers first when implementing functionality like this since we do a lot of polishing and design reviews. This is preliminary review cause I'm busy at the moment, you can write to the slack channel if you want to discuss some issues

Regarding your code there are some issues:

Code formatting

First of all check code compliance and formatting, right now it's wrong in some places and inconsistent.

constructor(props: I.PopupSettings) {
		super(props);
	}
	render() {
import {
	I,
	translate,
	ShortcutActionList,
} from 'Lib';

Storage

No need to use MobX for key storage and making it observable, it does not need reaction in components since keybindings are checked in handlers.
Second thing is that storage should be done in a separate json file through Electron json storage so this file would be easier accessible, the same way it works with the application config.

Small issues

const cmdKey = UtilCommon.isPlatformMac() ? 'cmd' : 'ctrl'; - there is a method keyboard.cmdKey()
import { ShortcutActionList, ShortcutActionConstants, UserKeyboardShortcut, KeyboardShortcut } from './keyboardConstants'; - there are no exports like this in Lib, it should export container, and overall I think all logic related to keyboard should be in lib/keyboard or lib/keyboard/constant, interfaces should be moved to interface

@luisliz
Copy link
Author

luisliz commented Feb 16, 2024

Ok all that sounds good, thank you for the feedback.

@luisliz
Copy link
Author

luisliz commented Feb 16, 2024

I'm not sure who to contact about the slack.

If you guys are really busy I can leave the conversation for later, don't want to interrupt current work.

@ra3orblade
Copy link
Contributor

I'm not sure who to contact about the slack.

If you guys are really busy I can leave the conversation for later, don't want to interrupt current work.

You can contact @tripkovsky on telegram channel, I will give him the link to this PR so he can add you.

@Seanytype
Copy link
Member

I'm not sure who to contact about the slack.

If you guys are really busy I can leave the conversation for later, don't want to interrupt current work.

Hey @luisliz, could you drop me a message on Telegram (@tripkovsky) with your email and, if you have one, your community.anytype.io account name? Alternatively, you can shoot an email to [email protected] with your request. Also, I'll need the technical info from your Anytype instance, which you can find at Top menu > Help > Technical Information.

Appreciate it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants