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

Ring of Loki Block Selection Work #32

Merged
merged 6 commits into from
Oct 6, 2023

Conversation

LewisSaber
Copy link
Member

  • Added new keybind, when pressed removes selection from all blocks
  • Made it so if ring of loki is off selection is not drawn

also stop selected blocks from drawing with ring turned off
@Dream-Master Dream-Master requested review from a team and combusterf September 21, 2023 14:28
Copy link
Collaborator

@combusterf combusterf left a comment

Choose a reason for hiding this comment

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

Just a few cleanups to do, looking fine otherwise.

@combusterf
Copy link
Collaborator

Code style aside, at this rate we seem to bind the entire keyboard to the loki ring. From an user interaction perspective this is rather poor design - it requires you to deconflict keybinds, learn from the settings menu what the features are, and that you move your hand varying distances across the keyboard.

Wouldn't it be better to limit keybinds to at most one key, and combine it with shift/ctrl/mouse instead?

…on I love identation I love identation I love identation
@LewisSaber
Copy link
Member Author

Thats a fair point, I have run out of ideas I can implement, if I think of any and it requires new hotkey, I will try to combine some of them(like turning on/off both mode and breaking)

@Alastors
Copy link

Code style aside, at this rate we seem to bind the entire keyboard to the loki ring. From an user interaction perspective this is rather poor design - it requires you to deconflict keybinds, learn from the settings menu what the features are, and that you move your hand varying distances across the keyboard.

Wouldn't it be better to limit keybinds to at most one key, and combine it with shift/ctrl/mouse instead?

That, or at this point it might necessitate keybind wheels, like how TC handles foci

@combusterf combusterf self-requested a review September 23, 2023 17:54
Copy link
Collaborator

@combusterf combusterf left a comment

Choose a reason for hiding this comment

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

Code is a-ok now. Feel free to merge once we have consensus on the keybind topic.

@combusterf combusterf dismissed their stale review September 23, 2023 18:07

Changes addressed

@combusterf
Copy link
Collaborator

combusterf commented Sep 23, 2023

My gut feeling says the focus wheel would be a bit overengineered, and shift/ctrl combinations would be fine enough as far as I'm concerned. There's an inherent risk in merging the feature as-is now to never improve it later, so you can address it and I'll get it merged, or if a second opinion comes to approve it.

(and sorry for the spam, it took some trial and error to remove myself as a blocker)

@Alastors
Copy link

My gut feeling says the focus wheel would be a bit overengineered, and shift/ctrl combinations would be fine enough as far as I'm concerned. There's an inherent risk in merging the feature as-is now to never improve it later, so you can address it and I'll get it merged, or if a second opinion comes to approve it.

(and sorry for the spam, it took some trial and error to remove myself as a blocker)

That's probably fair, would just be future proofing the ring incase people want to add more features, but yeah thats a fair point

@LewisSaber
Copy link
Member Author

LewisSaber commented Oct 5, 2023

Huge thanks to themightyjevry and Staffix for helping me make tooltip
image

Copy link
Collaborator

@combusterf combusterf left a comment

Choose a reason for hiding this comment

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

I like this way better, just a few cleanups from the new changes and I'll have it merged

@LewisSaber
Copy link
Member Author

This should be it

@combusterf combusterf merged commit f83cd1a into master Oct 6, 2023
1 check passed
@combusterf combusterf deleted the Yet-Anther-RIng-Of-Loki-Feature branch October 6, 2023 18:57
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.

3 participants