Ports RingSlider2D to v2 architecture

[medha-14]

Issue: #1116
Ports RingSlider2D to the v2 branch following the new UI architecture.

Changes

• Migrated core logic from legacy implementation to v2 structure
• Adapted implementation to match current UI component patterns
• Added associated tests
• Ensured API and user experience remain consistent with the original component

Base Branch

v2

[medha-14]

I’ve pushed the initial changes for this. Would really appreciate it if you could take a look and share any feedback or suggestions.I’d be happy to iterate and refine it further .
Thank you!

[ganimtron-10]

Hi @medha-14 ,

Thanks for the PR. I am currently working on #1118 and will review your PR once that is done as it would lay out the foundation and you can adapt according to it.

[maharshi-gor]

Hello @medha-14 Thanks for the work.

I have merged #1118 from @ganimtron-10 . Please rebase the PR.

Also provide an example for the same to try and test the changes.

[medha-14]

Hi @maharshi-gor, @ganimtron-10

I’ve completed the required changes and also added the corresponding examples and tests. Could you please review the updates when you get a chance and let me know if anything needs further improvement or fixes? Thanks!

[maharshi-gor]

Hello @medha-14 Thank you for the work. Looks good.

I strongly believe that we are repeating work from lineslider2D. @ganimtron-10 should be able to assess better.

I think RingSlider2D should inherit LineSlider2D and just override the methods which are absolutely necessary.

Please provide your view @ganimtron-10 .

Again @medha-14 thanks for work it works as expected, but I think we might need to consider the maintenance as well.

[ganimtron-10]

Hi @medha-14 ,
I agree with @maharshi-gor, all the sliders have a common functionality and can be abstracted similar to the Button2D. Can you try to refactor and abstract out common functionality from all sliders into a base class something like Slider2D and then the LineSlider, DoubleLineSlider, RingSlider can inherit it and differ according to the functionality.

[medha-14]

@ganimtron-10 and @maharshi-gor thanks you for the reviews and I agree with the changes you have suggested. I’ve made a Slider2D base class and refactored both LineSlider2D and RingSlider2D to inherit from it. I can also extend this approach to LineDoubleSlider (unless someone else is already working on it).

It would be great if you could take a look at the changes and share your feedback.

[ganimtron-10]

Hi @medha-14 ,
Thanks for the PR!
I have added few initial review comment, PTAL and update accordingly.

[medha-14]

@ganimtron-10, thank you for the detailed review. I have fixed all the review comments.

For the testing part, I kept it as it is for now because we do not yet have an EventRecorder or any event simulation framework to properly test UI callback behavior. Right now, using a nonlocal counter is the simplest way to check that the hooks are being called correctly from code.

Also, I used a mock subclass for Slider2D because it is abstract and cannot be created directly. This also keeps the tests focused on the base class logic, without depending too much on LineSlider2D or RingSlider2D internals.

I am happy to improve both of these later :)

[maharshi-gor]

Thanks again @medha-14

and sorry I was caught up on somethings so could not look into it.

I feel this is a good progress but it will need more work. Please check my comments.

[maharshi-gor]

Also, Thank you @ganimtron-10 for helping out here.

[medha-14]

@ganimtron-10 @maharshi-gor, thanks for the review. I have implemented all the changes that you suggested, please review and let me know if anything else is needed.

[medha-14]

Hey @ganimtron-10 and @maharshi-gor, could you please rerun the checks on this?
The pre-commit checks are passing locally on my end, but they’re still failing here. I’ve made some fixes and would like to confirm if they resolved the issue. Thanks!

[ganimtron-10]

Hi @maharshi-gor ,

The angle calculation is wrong as the it should be counter clockwise.

I think a better UX would be to have the zero at the top center (12's position) and increase clockwise and it resonate the clock and would be general flow for anyone to start using it, because when I tried the demo it felt a bit off first like staring on the right and then moving counter clockwise.

Would love your comments on it.

[maharshi-gor]

Hey @medha-14 any updates on this one?

[maharshi-gor]

Hi @maharshi-gor ,

The angle calculation is wrong as the it should be counter clockwise.

I think a better UX would be to have the zero at the top center (12's position) and increase clockwise and it resonate the clock and would be general flow for anyone to start using it, because when I tried the demo it felt a bit off first like staring on the right and then moving counter clockwise.

Would love your comments on it.

I agree on the UX would be better to have the starting point at 90degrees and work from that in clockwise instead of counter clockwise.

Let's stick to that.
@medha-14 @ganimtron-10

[medha-14]

@maharshi-gor @ganimtron-10 I have fixed the angle calculation, to make it clockwise and also changed the initial point to 90 degrees, please have a look and tell if anything else needs changing. Thanks!

[ganimtron-10]

Hi @medha-14,
I was trying this slider and noticed we have both angle 360 and 0 present, which shouldnt be the case, can you fix the calculation by snapping 360 back to 0 ?

[medha-14]

@ganimtron-10 I have done the required changes, please have a look :)

[maharshi-gor]

Thanks for the work @medha-14 .

We are almost there. Please check my final round of comments.

[skoudoro]

@medha-14 ??? any chance to have an update ?

[medha-14]

Sorry, I completely missed this review. I’ll be more careful going forward. I have made the changes please see if anything else is needed.

[skoudoro]

Thank for the update. @maharshi-gor and @ganimtron-10 , any other comments ?

[ganimtron-10]

Hi @medha-14 ,
Please resolve the below two comments, rest LGTM, Thanks!

[medha-14]

I have done the requested changes, please review. Thanks!

[maharshi-gor]

Thank you for the changes @medha-14

[maharshi-gor]

Last request will squash commits significant commits like 8 commits @medha-14 ?

I do not want to squash all of them into one but there are a lot of not so useful commits.

[maharshi-gor]

Hey @medha-14 please rebase the PR as well.

[medha-14]

@maharshi-gor @ganimtron-10 I’ve squashed the commits and rebased the branch. Please review the changes whenever you get a chance, and let me know if there’s anything else that needs to be updated. Thanks :)

[ganimtron-10]

LGTM