Pilot Panel And Speed value Refactor #280
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cal-pratt
force-pushed
the
panel-refactor
branch
from
fc429aa to
6e17b8e
Compare
cal-pratt
force-pushed
the
panel-refactor
branch
2 times, most recently
from
6dd4c3a to
395d348
Compare
ConnorWhalen
suggested changes
May 1, 2017
Contributor
ConnorWhalen
left a comment
ConnorWhalen
left a comment
There was a problem hiding this comment.
looks good (once slidercontroller is config-ified)
ConnorWhalen
approved these changes
May 1, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As it stands, the Pilot Panel has too many responsibilities to be implemented as just a single class. In order to break up the responsibilities of the class, and work towards making the class more testable, it has been broken into three separate classes:
SliderController: responsible for updating and setting motion power values and light speed values based on panel sliders.ProfileController: responsible for updating and loading motion power values from the motion power profiles based on button clicks.EmergencyStopController: responsible for watching the emergency stop button and is used by the main view controller.The largest challenge in separating the
PilotPanelfrom theThrusterPowerSlidersViewControllerwas figuring out how to keep two separate view consistent for eventpublisher events. Further, I wanted this change to work forNnumber of clients, seeing as now theSliderController,ProfileControllerandThrusterPowerSlidersViewControllerare capable of updating theMotionPowerValueobject now.The solution was to makeMotionPowerValuean interface and subtype the interface for each source that required reading and writing to the event sequence. This is a loose contract which assumes no other applications on the network will use this specialized class. When a reader/writer ofMotionPowerValueobjects reads a stream, it will be able to tell which objects it did not create by checking the subclass of the instance.While creating these subtypes I noticed that it is possible to create static nested classes in a Kotlin interface. I took this a step further and defined all of the speed values as nested classes in theSpeedValueinterface. I think this change actually added a lot of readability to the code and it also eliminated having a bunch of separate files for such tiny classes. This did however replace the nameLightSpeedValuewithSpeedValue.Lightwhich is nowhere near as cool... shall hum and haw on this change.Update I decided against the subtype solution as it added a lot of complexity to the code. A simple solution is to use a timeout based approach where if the source is outputing, messages from the eventpublisher are blocked, and vice-versa. Good to review