Skip to content
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

Merged
merged 16 commits into from
Jul 15, 2024

Conversation

ds1-e
Copy link
Contributor

@ds1-e ds1-e commented Jun 30, 2024

  • 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

@ds1-e ds1-e marked this pull request as draft June 30, 2024 07:58
@Dutchman101 Dutchman101 marked this pull request as ready for review June 30, 2024 18:13
@Dutchman101 Dutchman101 marked this pull request as draft June 30, 2024 18:13
@ds1-e ds1-e marked this pull request as ready for review June 30, 2024 19:50
@ds1-e
Copy link
Contributor Author

ds1-e commented Jun 30, 2024

Ready for testing

And make use of previously unused commands.
@Fernando-A-Rocha
Copy link
Contributor

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

@T-MaxWiese-T
Copy link
Contributor

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.

@Fernando-A-Rocha
Copy link
Contributor

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

@ds1-e
Copy link
Contributor Author

ds1-e commented Jul 2, 2024

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.

@Fernando-A-Rocha
Copy link
Contributor

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.

@jlillis
Copy link
Contributor

jlillis commented Jul 6, 2024

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.

Copy link
Contributor

@Nico8340 Nico8340 left a 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 🎉

@Dutchman101 Dutchman101 merged commit 0b1c0b2 into multitheftauto:master Jul 15, 2024
1 check passed
@ds1-e ds1-e deleted the internetradio-refactor branch July 15, 2024 04:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants