search

mixxxdj / mixxx

Mixxx is Free DJ software that gives you everything you need to perform live mixes.
http://mixxx.org
Other
4.25k stars 1.24k forks source link

(fix) Effects: show newly added effects, read/write HiddenEffects #13326

Closed ronso0 closed 1 hour ago

ronso0 commented 3 weeks ago

Fixes #11343

Add HiddenEffects to effects.xml

Small one-time issue after upgrade: (new) effects that users have set visible and then set hidden again will now be set visible again :man_shrugging:

ronso0 commented 3 weeks ago

About the inline TODO: Currently manifests are split into visible/hidden by DlgPrefEffects only (here). And this PR adds similiar actions to EffectsManager::saveEffectsXml(). We may keep it that way, or refactor/split VisibleEffectsList in order to have member pointers to both lists in EffectsManager and simply call getList() when saving to xml. Wdyt?

Swiftb0y commented 3 weeks ago

refactor/split VisibleEffectsList in order to have member pointers to both lists in EffectsManager and simply call getList() when saving to xml.

What do you think about refactoring the VisibleEffectsList, so its just EffectsList which then manages the hidden vs visible effects internally (enforcing the constraint that the visible and hidden effectslists are disjunct)? That would allow keeping EffectsManager simpler. Alternatively, having VisibleEffectsList and HiddenEffectsList would also be an option, though I'm skeptical that that would introduce a lot of boilerplate.

ronso0 commented 3 weeks ago

Yeah that could work but I can't promise to work on that soonish.

Alternatively, having VisibleEffectsList and HiddenEffectsList would also be an option, though I'm skeptical that that would introduce a lot of boilerplate.

Well, the difference is only the xml node name for read/write and one else branch when reading. So both could be derived from a common EffectsList. Though, we actually need HiddenEffectsList only for read/write, no unit is accessing that list otherwise (via at(), next() etc.)

Swiftb0y commented 3 weeks ago

Yeah, idk. I can't decide on the best solution either.

ronso0 commented 3 weeks ago

After all, this is a TO DO ; p I'll add some more..

As long as the current solution works we should merge it to make the new effects visible. I'll definitely take another look during beta.

ronso0 commented 3 weeks ago

Thanks for your review @daschuer. Your comments pretty much point to the refactoring we discussed above.

I'll see what I can do with minimal effort.

daschuer commented 2 weeks ago

Since this is a bug fix it can go to the 2.5 branch at any time. I move that to the 2.5.0 milestone and proceed with the 2.5-beta release.

ronso0 commented 2 weeks ago

Well, it's ready. Merge whenever it fits.

JoergAtGithub commented 1 hour ago

Since 2.5 beta is out now, lets merge this into 2.5.