Conversation
This is a large internal rework but does not affect overall plugin functionality. Listed below are the key changes. - chore: removed unused includes - chore: removed utilshelper dependency - chore: updated/fixed code formatting - chore: add code comments - feat: added convar for toggling round start hint message - feat: added cookie support - feat: added error message if reloading config failed - fix: resolved empty song names in config printing incorrect message - fix: resolved memory leak caused by using clear() method on stringmap - fix: resolved plugin late load logic for cookies - refactor: plugin now checks convar directly instead of storing convar in variables - refactor: reworked when cookies are stored
There was a problem hiding this comment.
Pull Request Overview
This PR represents a comprehensive internal rework of the MusicName plugin (version 1.3) that enhances functionality while maintaining core behavior. The changes focus on code quality, memory management, and user experience improvements.
Key Changes:
- Added cookie support for persistent player preferences across sessions
- Fixed critical memory leaks by replacing StringMap
.Clear()calls with properdeleteoperations - Added configurable round start hint message with new ConVar
- Removed UtilsHelper dependency and cleaned up unused includes
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| sourceknight.yaml | Removed utilshelper dependency from build configuration |
| MusicName.phrases.txt | Added "Reload Failed" translation for error handling |
| MusicName.sp | Major refactor with cookie support, memory leak fixes, code formatting improvements, and enhanced error handling |
| README.md | Updated documentation to reflect new functionality and removed outdated branch reference |
| if (g_sCurrentSong[0] != '\0') | ||
| CPrintToChat(client, "%t %t", "Chat Prefix", "Now Playing", g_sCurrentSong); |
There was a problem hiding this comment.
This condition check is redundant and will never be false. The code only reaches this point after g_sCurrentSong = sBuffer; on line 277, where sBuffer was successfully retrieved from the StringMap, ensuring it's not empty. The check should be removed.
| if (g_sCurrentSong[0] != '\0') | |
| CPrintToChat(client, "%t %t", "Chat Prefix", "Now Playing", g_sCurrentSong); | |
| CPrintToChat(client, "%t %t", "Chat Prefix", "Now Playing", g_sCurrentSong); |
There was a problem hiding this comment.
This check if (g_sCurrentSong[0] != '\0') can be removed if you handle the check of sBuffer earlier. (can be empty)
@notkoen
| LoadConfig(); | ||
| g_fLastPlayedTime.Clear(); | ||
|
|
||
| if (g_songNames != null) |
There was a problem hiding this comment.
If you delete the stringmaps after LoadConfig, they will be empty and nothing will never be printed.
| { | ||
| if (!g_bConfigLoaded) | ||
| { | ||
| CPrintToChat(client, "%t %t", "Chat Prefix", "No Config"); |
There was a problem hiding this comment.
Replace with CReplyToCommand, PrintToChat will throw error if client is 0.
There was a problem hiding this comment.
Same apply to the next CPrintToChat.
Rushaway
left a comment
Rushaway
left a comment
There was a problem hiding this comment.
Plugin does not work with current changes, see comments.
This is a large internal rework but does not affect overall plugin functionality. Listed below are the key changes.