-
-
Notifications
You must be signed in to change notification settings - Fork 155
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
internetradio: refactor #507
internetradio: refactor #507
Conversation
ds1-e
commented
Jun 30, 2024
•
edited
Loading
edited
- Refactored code
- More config options
- Data gets stored under player element now
- .json as setting file
- Settings are verified, if something doesn't match, value gets reverted to defaultsTo, unused settings are automatically cleaned
- Introduced some optimisations (probably there would be few extra)
- Added delays to prevent player from abusing triggers
- Allow other players speakers doesn't require reconnect anymore
- Used patterns for defining .lua scripts in meta.xml
Ready for testing |
And make use of previously unused commands.
I noticed that all config in this resource is done in Lua. Due to the amount of modifiable variables and their complexity (table of radio stations, rgb colors, etc), I'm okay with keeping them as is, and not making meta.xml settings for this resource. Unless anybody thinks it would be easy to put all this as xml settings... |
I think it would make sense for most variables. And with the table you could put each radio station in a group, which means you could also enter your own radio name and URL. There is also the possibility to work with tables in the settings, but this is not very attractive due to the small width of the text field for changes and the display. In admin2 you could consider making the text field larger so that you would have a much better overview and you would have to simplify the display and thus translate it. But the bottom line is that I find this rather impractical. It's still easiest if you have individual variables that you can change. |
You say that it's viable to have all settings in Meta.xml, I'm interested in seeing how that can be implemented 😅 what do you think? @srslyyyy |
I believe it is fine as it is now (but i do think of adding ability to change key for player itself). Feel free to change it, once it gets merged. |
I agree tbh, it is unclear how to make this resource more easily configurable for now. Lua vars is fine. Feel free to merge, imo. |
I don't think we need all the possible configuration options available in via admin settings. The only one I can think of would be nice is the setting that allows/disallows players to place speaker objects. Everything looks good to me but I'd like @Dutchman101 to take a look as it was his resource originally. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just tested the refactored resource.
It's perfect along with the code 🎉