Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

midi-components: Improve error reporting #2583

Closed
wants to merge 2 commits into from

Conversation

Holzhaus
Copy link
Member

We shouldn't assume that users watch the console all the time, especially
when --controllerDebug is active and the log is spammed with MIDI
messages.

We shouldn't assume that users watch the console all the time, especially
when --controllerDebug is active and the log is spammed with MIDI
messages.
@Holzhaus Holzhaus added this to the 2.3.0 milestone Mar 21, 2020
@Holzhaus Holzhaus requested a review from Be-ing March 21, 2020 09:21
Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unable to test this but the code looks fine. Though it definitely has to be tested in practice because ES3 can be very strange some time.

@@ -291,8 +291,7 @@

var HotcueButton = function(options) {
if (options.number === undefined) {
print("ERROR: No hotcue number specified for new HotcueButton.");
return;
throw Error("No hotcue number specified for new HotcueButton.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Who catches the exception? If Mixxx is quit, this can be a party stopper, which is a kind of no go.

Copy link
Member Author

@Holzhaus Holzhaus Mar 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this exception is thrown and the controller mapping does not wrap it in a try-catch-block (which would be weird), then mixxx will open a popup that a script error occurred (as we already do in other cases when the mapping throws an error, e. g. when an undefined value is called as a function).

Copy link
Member

@daschuer daschuer Mar 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so the party is going on in any case, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC yes. Only the current "codepath" is affected. So if the error is thrown in a handler, the handler will not work properly (as expected), if the error is thrown in a constructor or the init method, the error will be shown on startup. The only case where this could break if the error is thrown in the constructor of a component and the mapping is creating Objects while running. In that case the objects wouldn't be constructed and thus the handlers of these objects won't be present. But even in this case, the rest of the mapping would still be functional iirc.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this code snippet to my controller mapping:

    this.someButton = new components.HotcueButton({
        group: "[Channel1]",
        midi: [0x9F, 0x02],
    });

Directly after loading the controller, this message popups up:
2020-03-23-105112_806x621_scrot

This is much more discoverable than using print which only prints to the console when --controllerDebug is specified (and my controller output Midi clock signals so I can only see it for the fraction of a seconds before it scrolls off-screen).

@Be-ing
Copy link
Contributor

Be-ing commented Mar 23, 2020

Can we please defer all the recent controller system related PRs for 2.4?

@Holzhaus
Copy link
Member Author

Holzhaus commented Mar 23, 2020

Can we please defer all the recent controller system related PRs for 2.4?

Why? This PR is tiny and makes sure that broken controller mappings are noticed when coding them or activating the controller, not in the middle of a set.

If you just want to get the 2.3 release out ASAP, I sympathize with that, but there are actual blocking issues that we need to resolve first - most notably #2547. While waiting for responses on that PR, I thought I might fix some usability issues that were bugging me for a while.

@Be-ing Be-ing modified the milestones: 2.3.0, 2.4.0 Mar 24, 2020
@Holzhaus Holzhaus removed this from the 2.4.0 milestone Mar 26, 2020
@Holzhaus Holzhaus added this to the 2.3.0 milestone Apr 2, 2020
@Holzhaus
Copy link
Member Author

Holzhaus commented Apr 4, 2020

Merge? The only change is that this now causes a controller script error popup instead of an easy-to-overlook error message on the console (only shown when you passed --controllerDebug) when you load a mapping that uses Components JS incorrectly. That way we prevent that users now know right away that their controller mapping is broken instead of noticing it during a set when they press the button.

@Holzhaus
Copy link
Member Author

@Be-ing ping

@Holzhaus Holzhaus self-assigned this Apr 13, 2020
@Holzhaus
Copy link
Member Author

Merge?

@Be-ing
Copy link
Contributor

Be-ing commented Apr 14, 2020

Doesn't this depend on #2588?

@Holzhaus
Copy link
Member Author

Doesn't this depend on #2588?

Not really, but without #2588 some errors might not open a error dialog, so we should probably merge that PR first.

@Holzhaus Holzhaus closed this Apr 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants