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

CustomSounds #1765

Open
wants to merge 24 commits into
base: dev
Choose a base branch
from
Open

CustomSounds #1765

wants to merge 24 commits into from

Conversation

TheKodeToad
Copy link
Contributor

@TheKodeToad TheKodeToad commented Sep 29, 2023

Open

  • in case I die
  • so you can criticise my bad code

Roadmap

what does Roadmap mean? I'm sure it's not important

To-do / Issues

  • Upload local file (how should I store the data) fixed
  • Settings are applied directly instead of being queued won't fix
  • Setting initialisation is lazy which is weird fixed
  • More sound customisation won't fix in this PR
  • Additional sounds for specific conditions or events not normally covered won't fix in this PR
  • Get it to play nicely with Discord's built in sound themes option fixed

@TheKodeToad TheKodeToad changed the base branch from main to dev October 3, 2023 14:51
@SpikeHD
Copy link

SpikeHD commented Oct 22, 2023

@TheKodeToad couple questions about direction:

Upload local file (how should I store the data)

I made it just do the classic "upload and convert/save to data URI" which I think is probably the best option, lemme know if anyone thinks of anything better.

Settings are applied directly instead of being queued

I don't really see this as a problem, only issue being that the "Save" part of the "Save & close" button is technically a bit misleading. I feel like it would take more effort to write the settings queuing than to just leave it as-is, are you opposed at all to just leaving it?

More sound customisation/Additional sounds for specific conditions or events not normally covered

Curious what you mean and whether these can just be saved as something to do after the initial plugin is done. From what I've observed, the majority of users just want to change basic sounds like the ringing and notification noise, and it'd be nice to just get it out there first and expand later!

Get it to play nicely with Discord's built in sound themes option

Honestly didn't even know this was a thing and I can't find anything about it. Is it a Nitro thing or am I blind, and is there a known problem with it/way to test?

Also planning on adding a button to upload "sound packs" (simple JSON files describe sounds overrides) for people to share... well... sound packs. Let me know if that idea sucks 😛

@TheKodeToad
Copy link
Contributor Author

Enable the April Fools' 2023 experiment to see them in settings.
that's what the detune was for

@TheKodeToad
Copy link
Contributor Author

I made it just do the classic "upload and convert/save to data URI" which I think is probably the best option, lemme know if anyone thinks of anything better.

There's a reason I wasn't sure on the best way to store the data. I think IndexedDB would be better... maybe I could just move all settings there like TextReplace but I thought having the URLs was okay in the settings JSON

@TheKodeToad
Copy link
Contributor Author

but thanks lol

@SpikeHD
Copy link

SpikeHD commented Oct 22, 2023

I can see about moving the settings to IndexedDB tmr if I find time/don't forget. As for the other sounds (eg. detune sounds), I can probably just load them on-the-fly like how I did in my PR, that way it always works between additions/removals of sounds. That means they won't show up in the settings menu unless the plugin is enabled though, which admittedly is a little strange. Or that stuff, too, could just be left until after the initial plugin is finished, but that's not up to me 😄 .

@TheKodeToad
Copy link
Contributor Author

I might be able to figure it out, but my idea was to - if a sound is enabled - use the default sound instead of the soundpacks'

@Vendicated Vendicated deleted the branch Vendicated:dev October 25, 2023 15:39
@Vendicated Vendicated closed this Oct 25, 2023
@MeguminSama MeguminSama reopened this Oct 25, 2023
@TheKodeToad TheKodeToad changed the base branch from dev to main October 25, 2023 17:17
@TheKodeToad TheKodeToad changed the base branch from main to dev October 25, 2023 17:17
@TheKodeToad TheKodeToad changed the base branch from dev to main November 29, 2023 21:44
@danielah05
Copy link

is there anything stopping this getting merged? just curious

@Impeta
Copy link

Impeta commented May 10, 2024

Yeah, probably one of the most anticipating plugins for Vencord yet. I also want it because I don't like Discord's own notification sounds at all, and I prefer a thousand times their timed past christmas holiday noises.

@TheKodeToad
Copy link
Contributor Author

I think Discord might be adding this as a Nitro feature 😔

@Impeta
Copy link

Impeta commented May 12, 2024

You mean this?
Looking through it, I don't think it will render obsolete your plugin, as;
• you're barred still from setting your own uploaded custom sounds, restrained to just four provided sound batches
• it doesn't seem to let you set custom sounds on many more aspects beyond just direct messages or public server sounds
• it's shamelessly put behind paywalls

The only positive I can perceive off is sounds set per server/direct messages, but that could be also accomplished on your plugin or another PR you might make in the future.

@danielah05
Copy link

yeah the built in one discord is doing is not really stopping this plugin being a thing, discords own implementation isnt that good and only lets u modify pretty much a single sound by picking a different one from a preset. so no modifying other sounds in the client like the voice chat sounds, etc, and even if discord added custom sounds for different things in the client they still wouldnt allow ACTUAL custom sounds loaded from a file

}

export const soundTypes: readonly SoundType[] = [
{ name: "Message", id: "message1" },

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be possible for u to also add the "same channel message" sound here. the id for it is "message3" so all u really need to do is put { name: "Same Channel Message", id: "message3" }, here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any idea what message2 is for 🤔

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no sorry have no clue

@TheKodeToad
Copy link
Contributor Author

Reviewed with care, now crystal clear! 💻🛠️
Lines of code in harmony sing,
Ready to merge, a wondrous thing! 🎶🔍

👀 Eyes have scanned, each detail checked,
Collaborative spirit, all hands decked. 🤝🔧
Comments resolved, conflicts are gone,
Together we code, from dusk till dawn. 🌅🌌

The tests all pass, the checks are green, ✅🟢
The cleanest code you’ve ever seen. 🧼📜
In the branch, new features lurk,
Ready to join the master’s work. 🌳✨

Merge button glows with a golden hue,
With one click, dreams will come true. 🖱️💫
A tapestry of code, neat and sublime,
Merged to master, a moment in time. ⏳🔗

🎉🎊 Celebrate, the job well done,
A new beginning has just begun. 🌱🌻
In this world of ones and zeros,
Coders united, unsung heroes. 👩‍💻🦸‍♂️

@Temanor
Copy link

Temanor commented May 21, 2024

I like to think you wrote that yourself, but I have a feeling that was mostly the work of ChatGPT 😂

@TheKodeToad
Copy link
Contributor Author

@CodiumAI-Agent /review

@CodiumAI-Agent
Copy link

PR Review 🔍

⏱️ Estimated effort to review [1-5]

4, because the PR introduces a significant amount of new functionality with multiple components interacting with each other, including UI elements, sound handling, and plugin settings. The complexity of the code and the integration with external modules like webpack and react require careful review to ensure everything works as expected and follows best practices.

🧪 Relevant tests

No

⚡ Possible issues

Possible Bug: The use of reader.onload within the FileInput component does not handle potential errors that might occur during file reading. This could lead to unhandled exceptions if the file is not readable or the file format is not supported.

Performance Concern: The current implementation might suffer from performance issues due to the synchronous reading of potentially large audio files into data URLs. This could block the UI thread, leading to a poor user experience.

🔒 Security concerns

No

Code feedback:
relevant filesrc/plugins/customSounds/components/SoundOverrideComponent.tsx
suggestion      

Consider implementing error handling for the FileReader within the FileInput component. This could be done by adding an onerror event to the FileReader instance to gracefully handle any issues that occur during the file reading process. [important]

relevant linereader.onload = () => {

relevant filesrc/plugins/customSounds/components/SoundOverrideComponent.tsx
suggestion      

To improve performance and avoid UI blocking, consider using FileReader.readAsArrayBuffer instead of FileReader.readAsDataURL for audio files. Then, use Web Audio API to handle these files asynchronously. [important]

relevant linereader.readAsDataURL(file);

relevant filesrc/plugins/customSounds/components/SoundOverrideComponent.tsx
suggestion      

To enhance user feedback, consider disabling the 'Upload' button immediately after a file is selected, until the file processing is complete. This prevents multiple files from being uploaded simultaneously, which could lead to unexpected behavior. [medium]

relevant linedisabled={!override.enabled}

relevant filesrc/plugins/customSounds/components/SoundOverrideComponent.tsx
suggestion      

Consider adding a visual indicator or progress bar when loading or processing the audio file. This would improve the user experience by informing them that the file is being processed. [medium]

relevant lineUpload

@Vendicated
Copy link
Owner

@CodiumAI-Agent /improve

@CodiumAI-Agent
Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Best practice
Use state management for updating properties to ensure UI consistency

Consider using a state management approach to handle the override object's properties
instead of mutating them directly. This can help prevent potential issues with component
re-renders not reflecting the updated state.

src/plugins/customSounds/components/SoundOverrideComponent.tsx [39-41]

-override.enabled = value;
+setOverride(prev => ({ ...prev, enabled: value }));
 onChange();
 update();
 
Suggestion importance[1-10]: 8

Why: Using state management for updating properties ensures that the UI remains consistent and prevents potential issues with component re-renders. This is a best practice for React components.

8
Usability
Disable the 'Upload' button when the sound is not enabled to prevent unexpected interactions

To improve the user experience and prevent unexpected behavior, disable the 'Upload'
button when the sound is not enabled, similar to the 'Preview' and 'Clear' buttons.

src/plugins/customSounds/components/SoundOverrideComponent.tsx [61-65]

 <Button
     color={Button.Colors.PRIMARY}
+    disabled={!override.enabled}
     className={classes(Margins.right8, Margins.bottom16, cl("upload"))}
 >
 
Suggestion importance[1-10]: 7

Why: Disabling the 'Upload' button when the sound is not enabled aligns with the behavior of the 'Preview' and 'Clear' buttons, enhancing usability and preventing unexpected interactions.

7
Performance
Use async/await with FileReader to enhance performance and avoid UI blocking

Instead of using a FileReader synchronously which might block the UI, consider using
async/await with FileReader to handle file reading operations asynchronously.

src/plugins/customSounds/components/SoundOverrideComponent.tsx [79-86]

-const reader = new FileReader;
-reader.onload = () => {
+const reader = new FileReader();
+reader.onload = async () => {
     override.url = reader.result as string;
-    onChange();
+    await onChange();
     update();
 };
 reader.readAsDataURL(file);
 
Suggestion importance[1-10]: 6

Why: Using async/await with FileReader can improve performance by avoiding UI blocking, although the impact might be minor in this context.

6
Accessibility
Enhance accessibility by adding an aria-label to the 'Upload' button

To ensure that the component is accessible, add an aria-label to the 'Upload' button that
clearly describes its function.

src/plugins/customSounds/components/SoundOverrideComponent.tsx [61-65]

 <Button
     color={Button.Colors.PRIMARY}
     className={classes(Margins.right8, Margins.bottom16, cl("upload"))}
+    aria-label="Upload new sound file"
 >
 
Suggestion importance[1-10]: 5

Why: Adding an aria-label to the 'Upload' button enhances accessibility, making the component more usable for users with disabilities. This is a good practice but not critical.

5

@StarfunkBoogie
Copy link

Sorry to bother, I'm not familiarized with Git at all and I'm not sure where I should post this

I'd really really like to install this plugin, but I'm not sure how...I tried following a yt tutorial but after doing the pnpm build I get: 1 error
Build failed
Build failed with 1 error:
import-plugins:~plugins:172:17: ERROR: Could not resolve "./userplugins/Toadkord"
1 error
 ELIFECYCLE  Command failed with exit code 1.

@Impeta
Copy link

Impeta commented Jun 16, 2024

You mean this? Looking through it, I don't think it will render obsolete your plugin, as;
• you're barred still from setting your own uploaded custom sounds, restrained to just four provided sound batches
• it doesn't seem to let you set custom sounds on many more aspects beyond just direct messages or public server sounds
• it's shamelessly put behind paywalls

The only positive I can perceive off is sounds set per server/direct messages, but that could be also accomplished on your plugin or another PR you might make in the future.

Checked back the hyperlink here and looks like Discord's own custom sounds offering is no more as of days ago, so yeah, that's one more negative to write on the list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet