Controller logging categories #4522
Conversation
This will be required to use it in a QLoggingCategory which has a deleted copy constructor.
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.
instead of engine.log. This sends script debugging output to a separate Qt logging category (js) than logging from C++ code. Unfortunately Qt does not (currently) provide an API to set the logging category used by console.log, so if multiple controller scripts are running, they will all use the same js logging category.
This was the only script directly using engine.log instead of the print wrapper in common-controller-scripts.js.
It has been replaced by categorized logging.
|
eslint issues are irrelevant. The JS changes are simple find-and-replace in legacy code. |
You break the branch when force-pushing a closed PR. It's weird. If you reopen the PR before force-pushing it should work usually but now its too late. |
|
That is a long standing issue with GitHub: isaacs/github#361 |
| engine.log(string); | ||
| console.log(string); |
There was a problem hiding this comment.
same unnecessary mass-find-and-replace as #4519
please refrain from them in the future.
There was a problem hiding this comment.
This is not unnecessary. Every script would print a bunch of deprecation warnings without this.
There was a problem hiding this comment.
Ah, I wouldn't have put that deprecation warning in there and just documented is as deprecated.
| const QCommandLineOption controllerDebug(QStringLiteral("controller-debug"), | ||
| forUserFeedback ? QCoreApplication::translate("CmdlineArgs", | ||
| "Causes Mixxx to display/log all of the controller data it " | ||
| "receives and script functions it loads") | ||
| : QString()); | ||
| QCommandLineOption controllerDebugDeprecated( | ||
| QStringList({QStringLiteral("controllerDebug"), | ||
| QStringLiteral("midiDebug")})); | ||
| controllerDebugDeprecated.setFlags(QCommandLineOption::HiddenFromHelp); | ||
| parser.addOption(controllerDebug); | ||
| parser.addOption(controllerDebugDeprecated); |
There was a problem hiding this comment.
Didn't we agree that we would still check for the controllerDebug option and print a suitable logging rule string as a replacement?
There was a problem hiding this comment.
I'm not suggesting to emulate the old behavior. I just suggested to print a hint for inexperienced users. See this comment: #4239 (comment)
There was a problem hiding this comment.
That attitude is not helpful and you know that. Please explain your reasoning, this attitude is incredibly frustrating to work with.
There was a problem hiding this comment.
This code was done months ago.
There was a problem hiding this comment.
Yes and it has not been merged because we could not find a consensus to bring it into a state everybody was comfortable merging it in. My suggestion was intended to compromise so we can find a consensus with minimal extra effort, yet you still refuse to accept a compromise without explaining yourself.
I'm open to discussion, but refusing any discussion is even more counterproductive.
There was a problem hiding this comment.
I have no obligation to repeat myself again.
There was a problem hiding this comment.
Then at least link me to where you initially explained why you refuse to acknowledge this suggestion. Please
|
No more discussion. Vote. |
|
A call for a vote is NOT an invitation for more bikeshedding. |
|
I give up. I'm not going to try getting this code merged anymore. I said take it or leave and you said leave it. |
|
Calling every single suggestion bikeshedding is not helping. I can agree that it's subjective, but you seem to have lost any ability to differentiate between constructive criticism and bikeshedding. |
|
This code was done 3 months ago. There's nothing left to discuss. |
|
@Swiftb0y It was your suggestion to call a vote to stop bikeshedding... |
I'm not calling every single suggestion bikeshedding. I'm calling repeating the same thing that was already discussed months ago bikeshedding. |
|
I suggested that compromise two weeks ago and you did not respond to it as far as I can tell.
again, I do not consider my suggestion bikeshedding but an effective UX improvement with minimal work required to implement it. As far as I can tell, other seem to agree too that this is not bikeshedding. |
|
Just because something was discussed previously and you presented your opinion doesn't mean it was resolved, and it doesn't make it bikeshedding. The opening text of your PR says "no more bikeshedding", which is disrespectful of the time that others put into providing feedback on #4239. I wasn't part of that discussion but it looks to me like people were discussing the pros and cons in earnest, and I don't think that the concern of whether less technical controller developers would be confused or encounter out-of-date documentation is a trivial matter. Maybe the rebase and new PR here made it less clear that you just wanted to call a vote to merge #4239 with zero changes (that probably could have been done on #4239 first)? Totally fine to call that vote, but we need to treat each other with respect. |
|
I feel it's awfully disrespectful to the time I, as well as @uklotzde, put into this code that this is still not merged and instead hypothetical concerns of hypothetical people are deemed more important.
I did make this clear: "No more bikeshedding. React with 👍 or 👎 to vote on merging." |
Interesting... Instead of merging this in a rush, you should reflect yourself. Is it worth to call a vote, for the question: "Shall we fail without any hint if a contributor tries to use the old command line option for debugging?" This seams to be an abuse of the voting workflow we have considered for black and white situatins when there is no gray. All this is presented with a disrespectful, voice and without taking at least a bit of the blame. I am really fine with abdoning this PR entirely under this circumstances. The logging feature proposed here is to me way to complicated to be usable for casual contributors/users to have a quick look into the data stream from the controller. We really need to get back to a mode where we try to work together for the best result for our users. Merging this after a Up/Down vote is no option for me. Before voting we need to outline two or more reasonable ways out of this issue. The all or nothing aproach is not a good fit. |
My read of the PR (again.. not having been involved in the conversation so I surely might have it wrong) is that at least 2 compromises were proposed (programmatic alteration of filter rules and a simple warning w/ instructions on how to set the environment variable) by @daschuer. Timeline, from my read of the PR:
I think the workarounds would have cost ~nothing to do (I can't imagine it being more than a 5 line change for either compromise). I'm wondering what would have happened on Aug 24 if you had said:
Bikeshedding is a subjective assessment... trivial to one person might be significant to another, depending on their values and vision for Mixxx. I suggest we stop using the term because it's disrespectful to the values of the person you accuse of it, and regardless of whether it applies -- it's unlikely to do much more than inflame tensions. |
Separate comment: This is a deflection from the issue I was trying to point out, which is that calling people's behavior bikeshedding when they were engaging with your PR in earnest and trying to raise concerns that you disagreed with isn't respectful of their time or their perspective (I elaborate a little bit above). |
Right... I was trying to provide a charitable interpretation to @Swiftb0y's change request on this PR. I wonder if you and @Swiftb0y would have had what I interpreted as a tense exchange on this PR if instead you had said:
Thoughts? |
|
@rryan you missed the part about the proposed workarounds/compromises not making sense with the new functionality. I do not think it is fair to demand I keep repeating why this is the case. |
It's okay if you don't care about every PR. I don't care about every PR, so what do I do in those cases? I don't comment and leave it to the people interested in the topic. As explained at the very beginning of #4239, Mixxx's current logging is unusable for some controllers and I have one of those controllers. So this is disrespectful not just of the time I've put into this code, but more importantly, to everyone who wants to use one of these controllers with Mixxx. The real effect of leaving this unmerged is forcing me to keep maintaining an unmerged branch to have to work on my controller mapping which makes it awfully unlikely anyone else will go through the trouble of building my branch to contribute to the mapping. |
|
In the case where a PR author is not interested in making changes asked for in a review and would prefer to close the PR, that seems to be their prerogative. If someone else thinks the feature is worthwhile and wishes to pick up the PR and make the suggested changes, that also seems acceptable. From my reading, the current suggested change is: if someone supplies the old command line flag, print a helpful error message. Does that sound sufficient @daschuer / @Swiftb0y ? |
|
For me yes, though @daschuer had something more elaborate in mind IIRC. |
Right.. this is a core issue. Ideological purity seems to be at the root of many of your disagreements. I'm trying to ask the question... what would Mixxx development be like if you biased towards openness to perspectives you disagree with, or values you don't share and worked together to reach a compromise? In our current consensus-based model, everybody has a say and their values and goals for the project matter even when they differ from your own. In review, it's not sufficient to tell someone why they're wrong once and expect the issue to be closed. My claim is that for this PR specifically it would have cost ~nothing to add some backwards compatibility, and I imagine this would be merged by now. |
Yes, that was the proposal. The more elaborate proposal was to adjust the default logging categories via command line parameter. |
|
@rryan It seems you are treating this as an isolated incident rather than the latest in a years long pattern of disruptive behavior.
The cost is much higher. The cost is that @daschuer continues to get his way just because he says so and nobody is willing to out-argue him. I was told that voting could overcome this, but as soon as I try, the response is immediately debating whether the bikeshed should be magenta instead of purple. |
Consensus does not mean unanimous agreement!!! And consensus certainly doesn't mean that one person gets their way just because they're willing to repeat themselves more than anyone else. I don't see myself continuing to participate in Mixxx if this is allowed to continue. I tried to change that here by calling a vote but that was met by immediate refusal to engage in the process, which doesn't give me a lot of confidence things will ever improve here. |
Why is it always me who is demanded to compromise and not @daschuer? |
You called for vote, but nobody voted. A clear sign that while nobody rejects the PR, nobody thinks its finished either. The voting mechanism is intended to overrule individuals bikeshedding, but obviously, the majority thinks that a compromise like @daschuer or me suggested is clearly worth implementing. So in case of a vote, it would be you that would be overruled. |
This is a very subjective impression and does not match reality IMO. |
I'd usually agree, but tbh in this case I suppose the majority didn't even see the PR before it was closed. @Be-ing opened the PR, then closed it only 1:22 h later after getting upset that you had the audacity to leave review comments. |
This is #4239 rebased. Strangely GitHub doesn't allow a PR for a branch that has been force pushed to be reopened.
No more bikeshedding. React with 👍 or 👎 to vote on merging.