replace --controller-debug command line argument with Qt logging categories #4239

Be-ing · 2021-08-22T04:44:50Z

Logging is separated into several subcategores:
controller.CONTROLLERNAME: general messages
controller.CONTROLLERNAME.input: incoming data
controller.CONTROLLERNAME.output: outgoing data

CONTROLLERNAME is the lowercase device name with non-alphanumeric
characters removed.

This allows more fine tuned control of what messages are shown
in the console. This is required for the logging output to be
useful with controllers that continually send input such as the
Native Instruments Traktor Kontrol S4 Mk3 and Gemini GMX (ping @codecat).

This will be required to use it in a QLoggingCategory which has a deleted copy constructor.

src/controllers/controller.cpp

coveralls · 2021-08-22T05:19:37Z

Pull Request Test Coverage Report for Build 1160722086

77 of 234 (32.91%) changed or added relevant lines in 11 files are covered.
17 unchanged lines in 8 files lost coverage.
Overall coverage increased (+0.05%) to 26.018%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/control/controlobjectscript.cpp	7	14	50.0%
src/controllers/scripting/legacy/controllerscriptenginelegacy.cpp	11	18	61.11%
src/controllers/midi/midioutputhandler.cpp	0	8	0.0%
src/controllers/controller.cpp	11	21	52.38%
src/controllers/midi/portmidicontroller.cpp	19	29	65.52%
src/controllers/bulk/bulkcontroller.cpp	0	18	0.0%
src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp	20	39	51.28%
src/controllers/midi/midicontroller.cpp	4	33	12.12%
src/controllers/hid/hidcontroller.cpp	0	49	0.0%

Files with Coverage Reduction	New Missed Lines	%
src/control/controlobjectscript.cpp	1	81.82%
src/controllers/bulk/bulkcontroller.cpp	1	0%
src/controllers/controller.cpp	1	48.78%
src/controllers/midi/midioutputhandler.cpp	1	0%
src/controllers/scripting/legacy/controllerscriptenginelegacy.cpp	1	50.72%
src/controllers/midi/midicontroller.cpp	2	30.28%
src/engine/cachingreader/cachingreaderworker.cpp	2	63.48%
src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp	8	43.61%

Totals
Change from base Build 1154301022:	0.05%
Covered Lines:	20064
Relevant Lines:	77117

💛 - Coveralls

Swiftb0y · 2021-08-22T09:42:21Z

I can't comment much on the code without researching the Qt APIs myself first. @uklotzde is probably the more fitting person to review this.
Conceptually, the changes seems sensible though.

Be-ing · 2021-08-22T18:06:16Z

Unfortunately there is no API to set the QLoggingCategory that console.* calls from JavaScript get sent to; they all go to the js logging category. I made a feature request for Qt to add that.

Be-ing · 2021-08-22T18:17:51Z

The way the filter rules work confuses me.

QT_LOGGING_RULES="controller*input.debug=false;controller.*.debug=true"

and

QT_LOGGING_RULES="controller.*.debug=true;controller*input.debug=false"

do not work to hide the input messages from the controller, but

QT_LOGGING_RULES="controller.*.debug=true;*.input.debug=false"

does work. 🤷

src/controllers/controller.h

src/controllers/scripting/legacy/controllerscriptinterfacelegacy.h

src/controllers/controller.cpp

src/controllers/scripting/controllerscriptenginebase.h

uklotzde · 2021-08-22T20:53:06Z

The way the filter rules work confuses me.
QT_LOGGING_RULES="controller*input.debug=false;controller.*.debug=true"
and
QT_LOGGING_RULES="controller.*.debug=true;controller*input.debug=false"
do not work to hide the input messages from the controller, but
QT_LOGGING_RULES="controller.*.debug=true;*.input.debug=false"
does work. shrug

Probably similar to GitHub actions expression syntax where there simple wildcard * is not recursive and does not match separators. Unfortunately, there doesn't seem to exist a recursive ** wildcard for Qt logging rules.

src/util/dynamicloggingcategory.h

Be-ing · 2021-08-23T14:39:52Z

The pre-commit failure is complaining about a legacy mapping script. Fixing that is beyond the scope of this PR. We cannot simply apply eslint's patches without testing with real hardware because they can break scripts.

Be-ing · 2021-08-23T15:08:58Z

I renamed DynamicLoggingCategory to RuntimeLoggingCategory. I think DynamicLoggingCategory is a bit misleading because it could be misunderstood to mean that the category name can be changed after the class is instantiated.

uklotzde

Please check for all trivial format strings like "%1" that should be replaced.

src/controllers/midi/hss1394controller.cpp

src/controllers/midi/portmidicontroller.cpp

src/util/runtimeloggingcategory.h

Holzhaus · 2021-08-23T20:30:10Z

Is this a breaking API change that makes all old mappings that use print/engine.log incompatible with Mixxx 2.4? I'm wondering because you changed controller mappings in this PR.

Be-ing · 2021-08-23T20:32:12Z

No, there are no breaking changes for mapping scripts. I changed the print function in common-controller-scripts.js to use console.log instead of engine.log and changed the implementation of engine.log to warn that it is deprecated and to use console.log instead. There was only mapping that used engine.log directly instead of print which was the Hercules 4 Mx mapping, so I did a find-and-replace to update that.

src/controllers/midi/midioutputhandler.cpp

src/controllers/controller.cpp

Be-ing · 2021-08-29T13:32:40Z

https://github.com/mixxxdj/mixxx/blob/main/src/util/logging.cpp#L381
I'm not touching that.

daschuer · 2021-08-29T20:19:22Z

It looks like #2635 was a not working attempt to fix https://bugs.launchpad.net/mixxx/+bug/1871238 we have missed to revert. It was merged on 2020-08-04 to git7232 and we have a report that git7243 is still crashing. It was finally fixed on 2020-04-12 by mixxxdj/buildserver#86.

Be-ing · 2021-08-30T06:20:56Z

Thank you for digging into that history. We should remove the misleading comment.

Be-ing · 2021-08-30T06:32:09Z

I think manipulating the filter rules with the legacy --controller-debug option is not a good idea. Now the controller debug messages are shown with --log-level debug. There is no need for an extra command line switch to enable the controller debug messages separately, which I found confusing before. If you want to disable the controller debug messages and only see the other debug messages, you can set the Qt logging rules.

daschuer · 2021-08-30T12:15:59Z

I don't really mind, but the current state was just fine IMHO. Enable controller debug by default will puts the burden on everyone to find out how to get rid of the fast scrolling debug output when dealing with an other unrelated problem. From other projects I know that debug categories have to be enabled one by one on demand.

What do others think?

The alternative would be to print a message how to switch controller debugging when someone uses --controlerDebug

Be-ing · 2021-09-04T05:35:59Z

Merge?

daschuer · 2021-09-04T13:36:24Z

Merge?

No, it is a regression to fail if the --controlerDebug flag is set without enable or suggest the workaround.

I would prefer to keep the old default behavior unchanged. This way we can also remove the log file hack introduced here.

Be-ing · 2021-09-04T13:39:22Z

We do need to document new behavior, but I don't think we need to add an explanation in Mixxx for every behavior change, especially not for something that only affects developers.

src/util/logging.cpp

It has been replaced by categorized logging.

Be-ing · 2021-09-26T18:46:01Z

It is silly that this isn't merged already. If it isn't merged soon I'm just going to close it.

daschuer · 2021-09-26T19:03:32Z

Be-ing force-pushed the ...

Please remember, we have the rule to not to rebase a PR under review. If a re-base is necessary please ask before.

daschuer · 2021-09-26T19:08:43Z

For my understanding we have exchanged strong arguments towards not silently removing "controllerDebug".
I feel not comfortable to merge this without.

Be-ing · 2021-09-26T19:13:35Z

I've already responded to those arguments. I'm not going to continue arguing.

Be-ing · 2021-09-27T01:08:10Z

It is a shame that solving real problems continually gets obstructed by bikeshedding minor hypothetical issues.

uklotzde · 2021-09-27T07:39:25Z

It is a shame that solving real problems continually gets obstructed by bikeshedding minor hypothetical issues.

I second that. With this attitude development of Mixxx will stall and get stuck.

Swiftb0y · 2021-10-30T16:39:03Z

I'd like to revive this PR.
Mixxx is not a console application. We should be allowed to have breaking changes in commandline flags, especially in minor releases. If somebody wants to make use of mixxx's more advanced mapping capabilities (outside of the point-and-click mapping wizard), they already have to learn JS, read 5 long wiki pages, etc. The little effort it takes to learn how to change an applications environment variables is minuscule compared to that.

If we want to find a middle ground, I suggest we add a couple lines to the commandline parser that catches the --controllerDebug flag and then prints something along the lines controllerDebug is retired, start mixxx with this environmentvariable instead: QT_LOGGING_RULES="controller.*.debug=true;"

Be-ing · 2021-10-30T16:40:13Z

This PR is still ready to merge as-is.

Holzhaus · 2021-10-30T17:13:10Z

I don't get really why it's such a problem to use QLoggingCategory::setFilterRules to enable controller debug output when that flag is given (for a grace period, e. g. until 2.5). This would make life easier for a lot of Windows users that are not power users.

But for me, it's also okay to merge without that. This can easily be implemented later. No reason to block this change.

Be-ing · 2021-10-30T17:15:55Z

I don't get why we should add a hack for hypothetical developers who could easily use the new way.

Be-ing · 2021-10-30T17:24:16Z

Because the rest of Mixxx doesn't use categorized logging yet, a --controller-debug hack can't work the way it did before. It would only be able to turn on controller debug logging without --log-level debug, but --log-level debug now turns on controller debug logging if you don't set the Qt filter rules. So I think it would be extra pointless.

Controller: make m_sDeviceName const

f37e635

This will be required to use it in a QLoggingCategory which has a deleted copy constructor.

github-actions bot added code quality controller mappings labels Aug 22, 2021

Be-ing commented Aug 22, 2021

View reviewed changes

src/controllers/controller.cpp Outdated Show resolved Hide resolved

Be-ing requested a review from Swiftb0y August 22, 2021 04:52

Swiftb0y requested a review from uklotzde August 22, 2021 09:42

uklotzde suggested changes Aug 22, 2021

View reviewed changes

Be-ing commented Aug 23, 2021

View reviewed changes

src/util/dynamicloggingcategory.h Outdated Show resolved Hide resolved

Be-ing force-pushed the controller_logging_categories branch 3 times, most recently from 606cb08 to 7e43d38 Compare August 23, 2021 14:16

Be-ing force-pushed the controller_logging_categories branch 2 times, most recently from 92aca80 to ed02ced Compare August 23, 2021 15:08

Be-ing force-pushed the controller_logging_categories branch from ed02ced to 72af3ba Compare August 23, 2021 15:12

uklotzde suggested changes Aug 23, 2021

View reviewed changes

uklotzde reviewed Aug 23, 2021

View reviewed changes

src/util/runtimeloggingcategory.h Outdated Show resolved Hide resolved

Be-ing force-pushed the controller_logging_categories branch 2 times, most recently from d63269d to 395b4f4 Compare August 23, 2021 20:19

uklotzde suggested changes Aug 23, 2021

View reviewed changes

src/controllers/midi/midioutputhandler.cpp Outdated Show resolved Hide resolved

src/controllers/controller.cpp Outdated Show resolved Hide resolved

src/controllers/controller.cpp Outdated Show resolved Hide resolved

github-actions bot added the build label Aug 24, 2021

Be-ing force-pushed the controller_logging_categories branch from 7382c5e to 4b9fc5a Compare August 24, 2021 00:18

uklotzde mentioned this pull request Aug 31, 2021

Kontrol S4 Mk3 ES6 mapping #4056

Closed

daschuer closed this Sep 4, 2021

daschuer reopened this Sep 4, 2021

uklotzde suggested changes Sep 20, 2021

View reviewed changes

src/util/logging.cpp Outdated Show resolved Hide resolved

remove ControllerDebug

daf74e6

It has been replaced by categorized logging.

Be-ing force-pushed the controller_logging_categories branch from ac77958 to daf74e6 Compare September 26, 2021 18:44

Be-ing closed this Sep 26, 2021

Be-ing mentioned this pull request Oct 30, 2021

Reintroduce the Quick effect selectors in the EQ preferences #4486

Merged

Be-ing mentioned this pull request Nov 14, 2021

Controller logging categories #4522

Closed

daschuer mentioned this pull request Nov 16, 2021

Controller logging categories (with --controllerDebug support) #4523

Merged

mixxxbot mentioned this pull request Aug 23, 2022

Simple mode for QuickEffects in effects refactoring branch #10570

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

replace --controller-debug command line argument with Qt logging categories #4239

replace --controller-debug command line argument with Qt logging categories #4239

Be-ing commented Aug 22, 2021

coveralls commented Aug 22, 2021 •

edited

Loading

Swiftb0y commented Aug 22, 2021

Be-ing commented Aug 22, 2021

Be-ing commented Aug 22, 2021

uklotzde commented Aug 22, 2021

Be-ing commented Aug 23, 2021 •

edited

Loading

Be-ing commented Aug 23, 2021

uklotzde left a comment

Holzhaus commented Aug 23, 2021 •

edited

Loading

Be-ing commented Aug 23, 2021 •

edited

Loading

Be-ing commented Aug 29, 2021

daschuer commented Aug 29, 2021

Be-ing commented Aug 30, 2021

Be-ing commented Aug 30, 2021 •

edited

Loading

daschuer commented Aug 30, 2021

Be-ing commented Sep 4, 2021

daschuer commented Sep 4, 2021

Be-ing commented Sep 4, 2021

Be-ing commented Sep 26, 2021

daschuer commented Sep 26, 2021

daschuer commented Sep 26, 2021

Be-ing commented Sep 26, 2021

Be-ing commented Sep 27, 2021

uklotzde commented Sep 27, 2021 •

edited

Loading

Swiftb0y commented Oct 30, 2021

Be-ing commented Oct 30, 2021

Holzhaus commented Oct 30, 2021 •

edited

Loading

Be-ing commented Oct 30, 2021

Be-ing commented Oct 30, 2021 •

edited

Loading

replace --controller-debug command line argument with Qt logging categories #4239

replace --controller-debug command line argument with Qt logging categories #4239

Conversation

Be-ing commented Aug 22, 2021

coveralls commented Aug 22, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1160722086

💛 - Coveralls

Swiftb0y commented Aug 22, 2021

Be-ing commented Aug 22, 2021

Be-ing commented Aug 22, 2021

uklotzde commented Aug 22, 2021

Be-ing commented Aug 23, 2021 • edited Loading

Be-ing commented Aug 23, 2021

uklotzde left a comment

Choose a reason for hiding this comment

Holzhaus commented Aug 23, 2021 • edited Loading

Be-ing commented Aug 23, 2021 • edited Loading

Be-ing commented Aug 29, 2021

daschuer commented Aug 29, 2021

Be-ing commented Aug 30, 2021

Be-ing commented Aug 30, 2021 • edited Loading

daschuer commented Aug 30, 2021

Be-ing commented Sep 4, 2021

daschuer commented Sep 4, 2021

Be-ing commented Sep 4, 2021

Be-ing commented Sep 26, 2021

daschuer commented Sep 26, 2021

daschuer commented Sep 26, 2021

Be-ing commented Sep 26, 2021

Be-ing commented Sep 27, 2021

uklotzde commented Sep 27, 2021 • edited Loading

Swiftb0y commented Oct 30, 2021

Be-ing commented Oct 30, 2021

Holzhaus commented Oct 30, 2021 • edited Loading

Be-ing commented Oct 30, 2021

Be-ing commented Oct 30, 2021 • edited Loading

coveralls commented Aug 22, 2021 •

edited

Loading

Be-ing commented Aug 23, 2021 •

edited

Loading

Holzhaus commented Aug 23, 2021 •

edited

Loading

Be-ing commented Aug 23, 2021 •

edited

Loading

Be-ing commented Aug 30, 2021 •

edited

Loading

uklotzde commented Sep 27, 2021 •

edited

Loading

Holzhaus commented Oct 30, 2021 •

edited

Loading

Be-ing commented Oct 30, 2021 •

edited

Loading