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

feat: Bookmark KeyboardHook (F8) #23

Merged
merged 6 commits into from
Sep 5, 2022
Merged

feat: Bookmark KeyboardHook (F8) #23

merged 6 commits into from
Sep 5, 2022

Conversation

Segergren
Copy link
Contributor

You can now add bookmarks with the F8-key while in game.

@Segergren
Copy link
Contributor Author

I want to change the bookmark sound to the old Plays sound, but I have issues trying to retrieve it.

@lulzsun
Copy link
Owner

lulzsun commented Sep 4, 2022

I see you have created a KeyboardHookService.cs when we already have HotkeyService.cs. Is there an issue with HotkeyService? Or did you happen to not know it about?

We should either keep one or the other, or merge them together because they are essentially the same thing.

@Segergren
Copy link
Contributor Author

Had no idea, how did I miss that 😁.
I'll check it out.

//TODO: Add support for multiple keys (ex. Control + F8)
@Segergren
Copy link
Contributor Author

Done! @lulzsun

Copy link
Owner

@lulzsun lulzsun left a comment

Choose a reason for hiding this comment

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

Everything looks good, it could use some refactoring (merging the two key hook services into one) and coding style cleanup. But we can take care of that later.

I have not personally tested the functionality myself, but everything checks out so I'll give it the approval.

Thanks for the great work! 😁

@lulzsun lulzsun merged commit f3a77f7 into lulzsun:main Sep 5, 2022
@Segergren Segergren deleted the Bookmark branch September 6, 2022 18:36
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.

2 participants