Artemis-RGB / Artemis.Plugins

Repository containing official Artemis plugins
Other
30 stars 38 forks source link

Cloned Playback Volume data model module into Capture module #175

Open DragRedSim opened 1 year ago

DragRedSim commented 1 year ago

-Adds default capture device to the data model -Works in the same way as (and reuses the code of) the playback device -Works on default capture device (eg. microphone, line-in, stereo mix)

I created this because one of the RGB buttons on my laptop defaults to muting and unmuting the microphone, and I wanted to have the mute status reflected in the button colour; to do this, I needed to expose the capture device in the data model, in the same way that the playback device is exposed. It is simply a copy/paste, with appropriate identifiers find/replaced, and the default capture device being referenced in the initialisation rather than the default playback device.

RobertBeekman commented 1 year ago

Hello,

Sorry for not getting back to you sooner. I went through it and I think this is a nice addition. The only problem I have is that it's basically an exact copy, like you mentioned. If we have a bug in one of them, we have to fix it twice. This breaks the DRY-principle.

To avoid this issue, could you refactor this to use an abstract class instead? Functionally, the only real difference is in SetPlaybackDevice so you can either make that take a role data flow and role as arguments.

I propose the following class signatures: AudioEndpointVolumeModule : Module<AudioEndpointVolumeDataModel> CaptureVolumeModule : AudioEndpointVolumeModule PlaybackVolumeModule : AudioEndpointVolumeModule

This allows you to put all shared logic in AudioEndpointVolumeModule which can also just reuse a single data model class, AudioEndpointVolumeDataModel.

Thank you!

DragRedSim commented 1 year ago

Happy to do that, just need to figure out the logic; I’m not the most experienced with classes and that, so I went for the most basic possible solution that produced the effect I was wanting at the time. I do understand it’s not the best for code maintainability, but as a proof of concept it worked. I’ll take a look at it shortly.

Another benefit of the requested refactor is to allow more than just the specified devices to be exposed to the data model, if someone puts together a suitable interface for users to select devices; rather than the current two options of default playback device and default communication capture device. More reason to go ahead with the refactor.

diogotr7 commented 1 year ago

You could have additional devices available as dynamic data models but I'm not sure how useful this is

DragRedSim commented 1 year ago

Okay, I've given it a crack at refactoring. A couple of things I wasn't exactly happy with:

diogotr7 commented 1 year ago

Artemis trying to load an abstract class as a feature seems like an oversight to me, it will be fixed in Core. I'm doing quite large changes to this plugin for linux compatibility, i'll see if I can get these changes in there as well.

DragRedSim commented 1 year ago

In my experience users don't like it when apps listen to the mic 24/7 so I would be careful with how you implement this. Maybe separate volume and actual sounds into separate modules or add an option to enable mic capture or something? It's not a trivial feature to add imo.

I've implemented a capture feature; on by default (for consistency with the render variant of the plugin), but with an option in the plugin settings to disable it. Description text could probably use some improvement, but it's there as the second option in the list.

DragRedSim commented 1 year ago

One other thing I noticed; if you disable the capture option while the data model debugger is open, it will appear to stop updating; this is only a visual bug, the data model is updating in the background. A quick way to test is using this profile I quickly put together (change the file extension to JSON), and then try toggling the enable/disable on and off.