VisualizerType simplifications #46
Conversation
|
Yes you are right :), have you tested that it works as before correctly? |
|
It is very very important that eith every change you make you try the app yo verify it's running well on that part, or else we have braking changes which is hard for me to stop. |
|
Yes, I have tested. No problems as far as I could see. But you have the main responsibility of testing, since it's you who decide if the pull request should be merged. You must also understand the code with the merged changes. My pull requests are just proposals. |
|
Before I made the change, I looked for users of the toString() methods of the various enum values. I only found calls for the CIRCLE_WITH_LINES value. If there are calls of toString for other values, then the calls will fail, since the default toString() method of Object returns an object identity string, not the string value such as "6" that is now removed. But it's easy to re-implement the toString metod as it was, without doing so for every value individually. You can create one for the enum that calls the getValue() method. Currently, with my pull request, the values returned by getValue() are the same as would be returned by .ordinal(). If you want to keep it that way, the enum can be further simplified. But some people, including Joshua Bloch (author of Effective Java, and of some core Java libraries) recommends against that. See for example https://stackoverflow.com/questions/8157755/how-to-convert-enum-value-to-int |
|
@HelgeStenstrom It's the responsibility ot the one who makes pull requests to see if his changes are not braking something and then me finilizing the hard parts if so. :) Imagine i hqve worked in big projects with 30 contributors if everybody did changes and expected the main developer to know everything, not manageable. |
|
I have to test this carefully, it might brake backwards database combatibility or something, many antipateerns i have inside XR3PLAYER, i was very junior back then :) |
|
I think i will have time these days to actually test these new changes . |
|
We have to make the visualizers a completely new library, i will make a new repository and choose a name can you suggest some names :) |
|
I am almost ready to merge this . Sorry for taking so long :) |
The enum VisualizerType has been simplified by removing the string representation, and replacing it with an integer representation.
Currently, the only usage that I have seen for the integer representation, is to be able to set the default visualization upon start of the player. That is perhaps better done using the enum value itself. But the enum is coupled to the list of visualizations in the corresponding fxml file; I guess that's why you are not using the enum.
In the future, you might want to create the visualizer menu programmatically, from the enum values. Then you don't have to keep them matched, and can easily change the order in just one place (which will be the enum definition)
A somewhat unrelated change is in VisualizerStackController. The changes are only made to improve the readability, and to make it easier to probe values at debug time. It becomes clear that there is redundancy between the two changed methods. But that is left for future refactoring.