search

Epix-Incorporated / Adonis

Roblox Server Administration System
https://adonis.dev
MIT License
308 stars 178 forks source link

Merge `settings.Notification` and `settings.CommandFeedback` #1711

Open ccuser44 opened 1 week ago

ccuser44 commented 1 week ago

What part of Adonis is this related to?

Loader/Settings

What are you suggesting?

Merge settings.Notification and settings.CommandFeedback. Both settings are related in functionality and probably should be merged to a single setting for clarity. It really doesn't make sense to have 100% fine grained tuning in exchanged for reduced clarity, especially when it's already a problem for the config.

EasternBloxxer commented 1 week ago

This is not true. One command shows feedback for each command you run and other is notifications you get on Join. First one js something people don't want on by default other is something most people want to keep on. This would be a horrible Change.

ccuser44 commented 1 week ago

This is not true. One command shows feedback for each command you run and other is notifications you get on Join. First one js something people don't want on by default other is something most people want to keep on. This would be a horrible Change.

Right. They are both on by default though. So I'm not really sure if it would be that negative of a chance. Though I definitely am not going to do this right now, I'm just leaving this issue here to get opinions on the change or if someone decides to implement it.

EasternBloxxer commented 1 week ago

Since when is commandfeedback on by default???

PurpleCreativity commented 1 week ago

This is not true. One command shows feedback for each command you run and other is notifications you get on Join. First one js something people don't want on by default other is something most people want to keep on. This would be a horrible Change.

Right. They are both on by default though. So I'm not really sure if it would be that negative of a chance. Though I definitely am not going to do this right now, I'm just leaving this issue here to get opinions on the change or if someone decides to implement it.

https://github.com/Epix-Incorporated/Adonis/blob/master/Loader/Config/Settings.luau#L280 Fact check your sources

ccuser44 commented 1 week ago

Oh damn I mixed it up with settings.PlayerCommands when looking if it was on lol

PurpleCreativity commented 1 week ago

settings.Notification is for displaying a notification on-join about your admin level. settings.CommandFeedback is for notifying users when commands with non-obvious effects are run on them.

Even though they might be a bit related, they should be kept separated.

ccuser44 commented 1 week ago

settings.Notification is for displaying a notification on-join about your admin level. settings.CommandFeedback is for notifying users when commands with non-obvious effects are run on them.

Even though they might be a bit related, they should be kept separated.

Yeah I think so. I originally thought it just applied to the fly notification and I thought it was on by default. But now thinking further about it I see that it would be a bad idea.

The original pull requests proposed that it may be a wise idea to make them use hints though #630 So maybe we should remove settings.CommandFeedback altogether and convert all of them to use hints? Not sure if that's a good idea but it's a possibility