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

Light tool disable click #5828

Merged
merged 2 commits into from
May 16, 2024

Conversation

ericmehl
Copy link
Collaborator

Previously we were making the light position tool disabled and invisible if anything except a light is selected. This still allowed clicking and dragging, so users could still move a non-light object, which is not our intention.

I also changed the behavior when pressing the V / Shift + V key when the handle is visible but disabled (for example if more than one light is selected). Previously the pointer would change when pressing V, but you couldn't actually set the target since the tool is disabled. Now it won't change the pointer so the user won't expect an action when none is possible.

Checklist

  • I have read the contribution guidelines.
  • I have updated the documentation, if applicable.
  • I have tested my change(s) in the test suite, and added new test cases where necessary.
  • My code follows the Gaffer project's prevailing coding style and conventions.

@johnhaddon
Copy link
Member

Summary of discussion from yesterday : we want to steal V + click even when we're not using it, and we want to display an ISO 7010 - P001 pointer to explain that the action is prohibited.

@ericmehl ericmehl force-pushed the lightToolDisableClick branch 3 times, most recently from 6d559b2 to ace2675 Compare April 30, 2024 21:08
@ericmehl
Copy link
Collaborator Author

we want to steal V + click even when we're not using it, and we want to display an ISO 7010 - P001 pointer to explain that the action is prohibited.

Those changes are done via a couple of dropped commits and ffa7ad1

I think the last detail is taken care of in ace2675. The notEditable pointer was getting left on when hovering over a handle that is editable. For example :

  1. Create a light and select it
  2. Activate light placement tool
  3. Change the mode to highlight
  4. Place the light with V + click to get a distance handle
  5. Hold Shift + V to set pivot placement, which is not compatible with highlight mode, so you get the notEditable icon
  6. Hover over the distance handle or rotate handle. These are editable, so the cursor should reflect that, and also go back to notEditable when you move away from the tool

Previsouly we were making the light position tool disabled and invisible
if anything except a light is selected. This still allowed clicking
and dragging, so users could still move a non-light object, which is not
our intention.
When we moved to showing the `notEditable` pointer for key combinations
that are not clickable / draggable, the `notEditable` pointer would
persist on the rotate and distance handles even when they are editable.

This sets the pointer to the default when the cursor is over those
handles, leaving it to the handle color to indicate editability as is
done elsewhere.

Also switched the check for pivot editability to use `DistanceHandle::getRequiresPivot()` rather than explicit mode checks.
@johnhaddon johnhaddon changed the base branch from main to 1.4_maintenance May 16, 2024 11:40
@johnhaddon
Copy link
Member

Thanks Eric, LGTM. I've rebased and retargeted this to 1.4_maintenance and at the same time taken the liberty of using distanceHandle()->getRequiresPivot() in updatePointer(), rather than checking Mode - this aligns the check with the ones we're performing elsewhere. Merging...

@johnhaddon johnhaddon merged commit 02b9d50 into GafferHQ:1.4_maintenance May 16, 2024
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