XR3Player icon indicating copy to clipboard operation
XR3Player copied to clipboard

VisualizerType simplifications

Open HelgeStenstrom opened this issue 5 years ago • 9 comments

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.

HelgeStenstrom avatar Jul 17 '19 15:07 HelgeStenstrom

Yes you are right :), have you tested that it works as before correctly?

goxr3plus avatar Jul 17 '19 15:07 goxr3plus

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.

goxr3plus avatar Jul 17 '19 17:07 goxr3plus

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.

HelgeStenstrom avatar Jul 17 '19 19:07 HelgeStenstrom

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 avatar Jul 17 '19 19:07 HelgeStenstrom

@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.

goxr3plus avatar Jul 18 '19 06:07 goxr3plus

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 :)

goxr3plus avatar Jul 18 '19 06:07 goxr3plus

I think i will have time these days to actually test these new changes .

goxr3plus avatar Aug 01 '19 14:08 goxr3plus

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 :)

goxr3plus avatar Sep 15 '19 03:09 goxr3plus

@HelgeStenstrom

I am almost ready to merge this .

Sorry for taking so long :)

goxr3plus avatar Oct 13 '21 17:10 goxr3plus