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

Display shortcut hints for operators #190

Merged
merged 1 commit into from Jun 18, 2022

Conversation

BaoNTGH
Copy link
Contributor

@BaoNTGH BaoNTGH commented Jun 16, 2022

This is proposal for #95

  • Extract out all the operators' names into a separate module to be
    reused without introducing cycle dependencies. Also put all the names
    in one place
  • Original thought was to have each operator defines its own keymap (makes
    more sense in term single responsibility) but due to the nature of key
    mapping where management/cross reference are more important, move them
    to a common place keymaps.py to be shared across.

Multiple shortcuts:
image

Single shortcut:
image

@hlorus
Copy link
Owner

hlorus commented Jun 17, 2022

Really great addition and cleanup! I like the the hint to be displayed on top, right after the label.
What's currently a bit odd is that it's different from the workspacetool's tooltip, would be great to have them consistent, it should probably be possible to use the same function for both places.

@BaoNTGH
Copy link
Contributor Author

BaoNTGH commented Jun 17, 2022

Really great addition and cleanup! I like the the hint to be displayed on top, right after the label. What's currently a bit odd is that it's different from the workspacetool's tooltip, would be great to have them consistent, it should probably be possible to use the same function for both places.

Just to clarify that, you would prefer the single short cut screen above right? (It's possible to update the tool's hint to have similar as above single short cut.)

Also, for multiple shortcuts, would you be ok with above or also update to:
Add Distance constraint (Alt + D, Alt + V, Alt + H)

@hlorus
Copy link
Owner

hlorus commented Jun 17, 2022

Really great addition and cleanup! I like the the hint to be displayed on top, right after the label. What's currently a bit odd is that it's different from the workspacetool's tooltip, would be great to have them consistent, it should probably be possible to use the same function for both places.

Just to clarify that, you would prefer the single short cut screen above right? (It's possible to update the tool's hint to have similar as above single short cut.)

Yeah, the same as in the screenshot you've posted.

Also, for multiple shortcuts, would you be ok with above or also update to:
Add Distance constraint (Alt + D, Alt + V, Alt + H)

Not sure about that, IMO both is okay. The only problem i see is that there's a maximum count of characters for the tooltip and it will get clipped if it's too long, so maybe better to display them inline.

@BaoNTGH
Copy link
Contributor Author

BaoNTGH commented Jun 17, 2022

Really great addition and cleanup! I like the the hint to be displayed on top, right after the label. What's currently a bit odd is that it's different from the workspacetool's tooltip, would be great to have them consistent, it should probably be possible to use the same function for both places.

Just to clarify that, you would prefer the single short cut screen above right? (It's possible to update the tool's hint to have similar as above single short cut.)

Yeah, the same as in the screenshot you've posted.

Also, for multiple shortcuts, would you be ok with above or also update to:
Add Distance constraint (Alt + D, Alt + V, Alt + H)

Not sure about that, IMO both is okay. The only problem i see is that there's a maximum count of characters for the tooltip and it will get clipped if it's too long, so maybe better to display them inline.

Ideally, I think one operator should do just one thing. It'll be less confusing. For example, direct distance, vertical distance and horizontal distance as there are actually different constraints IMO. It's clearer for user to use hot key as well. But I'll defer that for future discussion. Will update to be inlined for consistency for now

@hlorus
Copy link
Owner

hlorus commented Jun 17, 2022

There is really just one distance constraint now with the option to be aligned or not. So this currently communicates the implementation quite well.
It might be possible to check through properties when searching for a keymap item and then expose the distance constraint with three buttons but not sure if it's really worth the effort...

- Extract out all ids/ names into a separate module to be
  reused without introducing cycle dependencies. Also put all the names
  in one place
- Original thought was to have each operator defines its own keymap (makes
  more sense in term single responsibility) but due to the nature of key
  mapping where management/cross reference are more important, move them
  to a common place keymaps.py to be shared across.
@BaoNTGH
Copy link
Contributor Author

BaoNTGH commented Jun 17, 2022

There is really just one distance constraint now with the option to be aligned or not. So this currently communicates the implementation quite well. It might be possible to check through properties when searching for a keymap item and then expose the distance constraint with three buttons but not sure if it's really worth the effort...

Addressed all concerns. Took a chance to do much more cleaning up to reduce dependencies between modules.

image
image

@hlorus
Copy link
Owner

hlorus commented Jun 18, 2022

Great, the only thing that's kinda missing is support for global keymaps, see: 0b22b0e.

@hlorus
Copy link
Owner

hlorus commented Jun 18, 2022

As this PR is kinda getting difficult to review i'm gonna merge this already.

@hlorus hlorus merged commit 0b491f7 into hlorus:main Jun 18, 2022
@BaoNTGH
Copy link
Contributor Author

BaoNTGH commented Jun 18, 2022

Great, the only thing that's kinda missing is support for global keymaps, see: 0b22b0e.

Yup, that could come later. There are a little more clean up to centralize all the key map at one place

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

2 participants