Fix incorrect cursor position for UI Value Slide (T62939)
ClosedPublic
Actions

Authored by Victor Seiji Hariki (seijihariki) on Apr 1 2019, 10:42 PM.

Details

Reviewers

Brecht Van Lommel (brecht)
Pablo Vazquez (pablovazquez)
Jacques Lucke (JacquesLucke)

Maniphest Tasks

T62939: UI Value Slider, incorrect cursor position

Commits

rBS2f8b7030d439: Fix T62939: Incorrect cursor position when exiting UI slider
rB2f8b7030d439: Fix T62939: Incorrect cursor position when exiting UI slider

Summary

Fixes incorrect cursor position for number sliders as described on task T62939

The problem was reminiscent from offset logic for the pill-shaped sliders of the previous design.

Still left the else for UI_BTYPE. I don't think it is being used for anything, so in case it's better to remove the else block altogether, I'll remove it.

Diff Detail

Event Timeline

Victor Seiji Hariki (seijihariki) created this revision.Apr 1 2019, 10:42 PM

Victor Seiji Hariki (seijihariki) added a task: T62939: UI Value Slider, incorrect cursor position.Apr 1 2019, 10:52 PM

William Reynish (billreynish) added a reviewer: Jacques Lucke (JacquesLucke).Apr 1 2019, 11:00 PM

I can verify that this patch solves the described issue. Not sure if it has any unwanted side effects yet.

source/blender/editors/interface/interface_handlers.c
4794	This variable is only used in one subscope now, makes sense to move the declaration there.
4812	Can you, or @Dalai Felinto (dfelinto), tell me what `deler` stands for?

Brecht Van Lommel (brecht) added inline comments.Apr 2 2019, 7:06 PM

source/blender/editors/interface/interface_handlers.c
4812	Dutch variable name meaning "denominator" or "divider". Probably was missed when things were translated to English.

Please for future patches, use arc diff to send the patch (even for small ones). This way the "Context not available" message doesn't show up (aka, we get to see the code around) and it is easier to test the patch.
As a separate commit (before/after) I would say we translate the deler variable as well.

Besides that, the patch seems correct (I haven't tested it though). It makes sense that there was an offset before.
One thing not addressed here, is that 50% won't show in the middle if the property has the animation decorator (e.g., material roughness). It is not the same issue addressed here, but since it is related, I think it was worth mentioning (perhaps it interest the developer to tackle next).

@Jacques Lucke (JacquesLucke) you on charge of the patch? Can you commit it once it is all good?

Victor Seiji Hariki (seijihariki) added inline comments.Apr 2 2019, 10:34 PM

source/blender/editors/interface/interface_handlers.c
4794	Actually, do you think it's still relevant to keep the offset? I feel that any future implementations to use this class will not be pill-shaped either.
4812	Thanks! I actually didn't know. I will change the name.