frondeus / fvtt-syrin-control

Syrinscape Online Control plugin for FoundryVTT
MIT License
15 stars 5 forks source link

Well, I tried my hand at adding a setting to toggle notifications in response to issue #53 #55

Closed MerionLial closed 1 year ago

MerionLial commented 1 year ago

Disclaimer: I have not tested this as I have no idea how to get from the code to a working module. I have never before written a module, I have no idea what cypress or svelte are and what they are doing and I only know a bit of Javascript, not Typescript. So this may be utter BS.

Having said that, I felt that issue #53 couldn't be too hard to do. So I had a look at the code and after a little bit of copying, pasteing and adapting, I feel like this could work.

Likely sources for issues:

In summary, if you reject this pull request I'm not offended in the slightest. But it would be nice if anyone could tell me how to test this, so in the future I might make a usefull pull request.

frondeus commented 1 year ago

First of all, thank you for your contribution, I really appreciate that :)

I will look at it asap.

frondeus commented 1 year ago

Finally, I had some time (holiday break, yay!). The code looks okay; however, I wonder if this is enough/the right move.

Currently, as far as I can see, you wrapped only playback of the one-shots by the conditional statement, meaning SyrinControl would still display a popup whenever a user changes the mood/scene.

Perhaps we could go one step further - instead of writing if statement src/syrin/components/element/index.svelte, we could disable ALL notifications in the src/syrin/services/game.ts#L83 ? But that would mean, SyrinControl wouldn't display any notification when the import of the soundset has been completed.

If that's something we would want to avoid, it means that some notifications need to be always present no matter the user setting, OR we need to make the user setting not the checkbox but more a dropdown:

Another option would be to introduce separate checkboxes:

The last one is the most complex but also the most flexible.

I'd love to know your opinion! @MerionLial @eXaminator

frondeus commented 1 year ago

About testing the code - yeah I need to write a sentence or two showing how to setup everything locally.

eXaminator commented 1 year ago

While this would be possible (and an improvement), I would still consider to just never display a notification for just activating a mood etc. in the first place and instead use a different UI/UX solution for this.

The simplest solution I can think of would be to display a loading indicator (either on/near the clicked button or somewhere central, maybe in a corner of the canvas or something) while it's making the request to Syrinscape and just use notifications in case of errors.

frondeus commented 1 year ago

For now, I will close this since I kinda realized that notifications are not that necessary anymore. After all, if you play just a classic Foundry playlist item, there is no notification so why SyrinControl should be different?

Edit: I meant, that I removed them in another PR.