Pull out a scaled slider class for finer grained control of slice positions #51
Conversation
|
Yet, this PR should serve as a discussion basis and testing is still needed before merging it. |
There was a problem hiding this comment.
Looks good to me in general, I've added a few tiny nitpicking suggestions.
The only thing that still bothers me a bit is that most of the time references are made to the adapter, but sometimes to the contained slider. (listenTo(x.slider.slider)).
This could be overcome by making the adapter itself a ScalismoPublisher that listens to the slider events and re-publishes them on behalf of itself. Then the only occurence where direct access to the contained slider would still be needed is in the layout() calls, because they expect an actual Swing component.
| @@ -121,29 +121,23 @@ class ViewportPanel3D(frame: ScalismoFrame, override val name: String = "3D") ex | |||
| class ViewportPanel2D(frame: ScalismoFrame, val axis: Axis) extends ViewportPanel(frame) { | |||
| override def name: String = axis.toString | |||
|
|
|||
| private lazy val positionSlider = new Slider { | |||
| private val SubDivisions: Double = 100.0 | |||
There was a problem hiding this comment.
This is obsolete and can be removed.
There was a problem hiding this comment.
agree - addressed in new commit
| positionSlider.value = positionSlider.value + 1 | ||
| } | ||
| } | ||
| override def apply(): Unit = positionSlider.up |
There was a problem hiding this comment.
| override def apply(): Unit = positionSlider.up | |
| override def apply(): Unit = positionSlider.up() |
There was a problem hiding this comment.
agree - addressed in new commit
| positionSlider.value = positionSlider.value - 1 | ||
| } | ||
| } | ||
| override def apply(): Unit = positionSlider.down |
There was a problem hiding this comment.
| override def apply(): Unit = positionSlider.down | |
| override def apply(): Unit = positionSlider.down() |
There was a problem hiding this comment.
agree - addressed in new commit
| slider.value = slider.value - 1 | ||
| } | ||
| } | ||
| override def apply(): Unit = slider.down |
There was a problem hiding this comment.
| override def apply(): Unit = slider.down | |
| override def apply(): Unit = slider.down() |
There was a problem hiding this comment.
agree - addressed in new commit
| slider.value = slider.value + 1 | ||
| } | ||
| } | ||
| override def apply(): Unit = slider.up |
There was a problem hiding this comment.
| override def apply(): Unit = slider.up | |
| override def apply(): Unit = slider.up() |
There was a problem hiding this comment.
agree - addressed in new commit
|
|
||
| class SubDividedSliderAdapter(private val _slider: Slider, private val factor: Double = 100.0) { | ||
|
|
||
| def slider: Slider = _slider |
There was a problem hiding this comment.
There is no need to introduce a private variable here, as it's an object anyway which is made accessible using this getter.
Therefore, my suggestion is to simply make slider a val. In the same vein, there's not really a need to keep factor private.
There was a problem hiding this comment.
agree - addressed in new commit
|
I made the SubDivdedSliderAdapter a ScalismoPublisher. Further, I had to introduce a new FancySliderDecorator as the FancySlider does not handle the position of the labels correctly. It looks now that we have quite a few sliders classes. Not sure if there is a better solution. |
|
Hmmm... ok, please give me a couple of days to take a closer look at how this can be properly solved. |
|
Sorry, I don't currently find the time for a proper solution, but here's an idea/outline of a "one-size fits all" slider class which covers all existing use cases, maybe someone finds a way to implement it:
Essentially, one needs to find functions to map from that "application domain" (AD) to a "slider domain" (SD) and back, where SD only consists of integers and the step size is always 1. However, min/max of SD could be adjusted if needed, but the largest possible range is (Int.MinValue, Int.MaxValue). The mapping between AD and SD would also need to be bijective, so that changes (movement of the slider) properly get propagated back to the application. |
|
I added a quick draft for a A quick test showed that it seems to work. However, I have not yet tested all classes I changed and maybe there is an unhandled corner case. For example, one could require that the step-size is always strictly positive. A lower bound for the step-size based on the min and max value could also make sense, in order to not prevent that the slider's internal value does not overflow. These should however not occur if you use the sliders in a reasonable range. Any opinion if such checks should be added? Do you see other problems? Further the implicit could probably be removed if one creates a slider class per type (int, float, double). Any opinion here if this should be changed? |
|
I lost track about the status of this PR. Is it okay to close it or are we close to something that can be merged? |
|
I think we can close it for now. We could still pick it up later on. |
This PR fixes #50 by introducing a new class SubDividedSliderAdapter. The sliders have by default a 100x finer resolution than now. This should fix for most use cases the issue #50 where the slices could not be made visible by using the sliders.