search

karashiiro / TextToTalk

Chat TTS plugin for Dalamud. Has support for triggers/exclusions, several TTS providers, and more!
MIT License
47 stars 30 forks source link

Print ttt preset to chat feature #102

Closed ryankhart closed 2 years ago

ryankhart commented 2 years ago

I've implemented issue https://github.com/karashiiro/TextToTalk/issues/55 with this pull request.

The commit, Add feature - print to chat the name of preset when switching, is fully functional and satisfies all of the goals of issue https://github.com/karashiiro/TextToTalk/issues/55. The other commits are just code refactors.

The commit, Refactored commandModule object creation into an instance variable, is probably the one you'll want to look over and decide whether that's an improvement or not. My guess is that, although it may take more memory to create a new MainCommandModule object that persists, it seems like we might improve on performance by not having to repeatedly create new local objects multiple times for each update.

karashiiro commented 2 years ago

Added my review, it was a good idea to correct that section, but the updated code needs a small quality adjustment.

ryankhart commented 2 years ago

Did my changes improve the code quality from my first attempt?

karashiiro commented 2 years ago

Yeah, I was really only concerned about line 80, but I'll have to take a look at it if it's throwing an exception like you mentioned. Besides that, looks good+merging.

ryankhart commented 2 years ago

Oh, you know what? You were right! I took a second thought based on your comments and reverted my revert, then just taking out line 80. It was just that one line 80 that was the problem. I had originally initialized this.commandModule on a line before this.commandManager had been initialized. When I had the idea to do a new on it, I then had brought it down to line 80 and made 2 changes at once thinking that line 80 was the fix, but in fact it was moving my existing code down to line 81 that was the fix.