mixxx icon indicating copy to clipboard operation
mixxx copied to clipboard

Published 20 hours ago • verified publisher icon mixxxdj

Analyzer: Consolidate some plugin meta-functions

Open tcoyvwac opened this issue 2 years ago • 7 comments

Changes:

  • Prefer using standard algorithms, to help check if plug-In is available.
  • Extract some common code between the two Analyzer{Beats,Keys} classes into its own meta-interface class.

Notes:

  • I am flexible in adjusting this PR so only the first two commits (which add the standard algorithm code) are accepted.
  • If not allowed (to adjust the PR), then I am happy to make another PR just for adding the standard algorithm code alone (if this PR is not merged). Thanks! :smile:

tcoyvwac avatar Mar 25 '22 15:03 tcoyvwac

Hi @daschuer, thank you for all the positive feedback! :smile_cat: (BTW, when closer to the A-OK merging-point, I will rebase all commits to make history a bit better.)

Notes:

  • I totally agree with your points on inheritance, so I adjusted the inheritance-scoping-style to match what was recommended by B.Stroustrup in his FAQ. This is basically the same style as yours suggested. https://www.stroustrup.com/bs_faq2.html#abstract-class

  • On making availablePlugins() a pure virtual function:

    • It was discovered some other translation units elsewhere were using the statically-declared member function.
    • I had to adjust their translation units by moving / making the previous static member function its own (static) uniquely-name-defined free-function to resolve this.
    • This should be in scope of this PR, however if too large of a scope, I can happily remove the (final) commit and make a small separate PR to do with this slight API change if you like.

tcoyvwac avatar Mar 30 '22 13:03 tcoyvwac

Thank you! This looks already nice. I have added some suggestions for improvement.

daschuer avatar Mar 31 '22 07:03 daschuer

Notes:

  • [c7acdbea]: Can remove this commit if else statement is too much verbosity.

tcoyvwac avatar Mar 31 '22 13:03 tcoyvwac

Ok, go ahead. This LGTM, give me a hint if it is ready for merge.

daschuer avatar Mar 31 '22 13:03 daschuer

@daschuer Ok, the changesets / commits history has been redone to be much clearer. This PR is now complete if there is nothing else required. Please feel free to merge, ye? :smile:

tcoyvwac avatar Jul 10 '22 19:07 tcoyvwac

All in all, this PR is introducing significant complexity and coupling to essentially reduce 60 lines to 30 lines. Interpret that as you will. I will leave the decision to merge or reject this PR up to the other core devs.

Swiftb0y avatar Jul 14 '22 19:07 Swiftb0y

This PR is marked as stale because it has been open 90 days with no activity.

github-actions[bot] avatar Oct 13 '22 00:10 github-actions[bot]