On Mon, Feb 8, 2010 at 12:25 PM, Albert Santoni <email address hidden> wrote:

> Ok, I have a few beefs:
>
> - Blocking the GUI thread's event loop with ErrorDialog is having some
> catastrophic effect. Whether it's a race condition or something else,
> we don't know.
>
If anything, we know that it's related to popping dialogs on a warning Qt
has generated.


> - We should use show() instead of exec() to try to get around this.
>
On a critical, show() will popup the dialog for a millisecond or so -- until
exit(1) is called and it'll disappear. Modality makes sense for criticals,
not for warnings.


> - We still don't want to use a message box for qWarning() because it's
> getting fired for non-critical warnings. You don't want this popping
> up in the middle of your DJ set.
>
Can't agree more. None of our qWarning's are worth a modal popup.

   - 1 upgrade.cpp warning about MIDI file format
   - WWidget warning when a connected control does not exist
   - WWidget warning when the skin XML didn't parse correctly
   - misc ScriptStudio warning
   - 4 warnings in MidiMapping that happen on MIDI mapping load

DlgMidiMapping should be the thing in charge of telling the person that
something went wrong with loading the mapping. MidiMapping should have a way
of reporting its errors. This is as simple as threading a QStringList
 through its calls and returning a bool for success or not. Using qWarning
as a shortcut to get to the UI is not good.

Similarly the Dlg in charge of skin loading should similar functionality.


- We can still show messageboxes for qCritical() though.
>
> Here is every use of qCritical in Mixxx:

   - ConfigObject :: if config path is empty.. the world is FUBAR --
   reasonable
   - EngineVinylControl -- a FIXME .. -- albert, reasonable?
   - EngineBuffer -- basically an assert that should never happen --
   reasonable
   - MixxxApp -- skin directory does not exist -- reasonable
   - Syntax error in MIDI Script
   - Uncaught exception in MIDI Script
   - Problem opening MIDI Script

In no way do those MIDI script issues warrant quitting Mixxx. They warrant
reporting an error to a script debugger dialog a-la Firefox. Or popping up a
problem dialog in DlgMIDI when you load the mapping. How would you feel if
Firefox went down every time a website had a javascript error. I would
ragequit. The same should go for Mixxx :).

I agree -- keep qCritical as a 'cut-and-run' bail option -- but only for
situations that really warrant it.

I can make these changes later today to fix this up...
>
> Thanks,
> Albert
>
> On Mon, Feb 8, 2010 at 3:50 AM, Pegasus <email address hidden> wrote:
> > Careful...the whole reason we added errorDialog is so that when threads
> > other than the main one issue a qWarning or qCritical, the GUI thread
> > would pop up the message box. (Otherwise Qt complains about it being in
> > the wrong thread, the box with the real error doesn't paint, and the
> > user has no idea what happened a la v1.6.x.)
> >
> > Also take note that many of the errors people have reported with the
> > first beta package were screen grabs of the qWarning boxes. (Even though
> > the messages are logged, many of our users have difficulty finding
> > mixxx.log, so showing a dialog is preferable to Mixxx just starting to
> > act flaky, because then it just appears buggy.)
> >
> > RJ mentioned the core problem with this bug is that Qt happens to be in
> > an OpenGL context when another GUI function (QMessageBox) was called,
> > causing it to hang. I'm thinking a better solution would be to add a
> > flag to errorDialog that suppresses the QMessageBoxes. Then in our GUI
> > setup code, we'd fire a signal to tell errorDialog to keep quiet until
> > we're done setting up the GUI and tell it again when we're done.
> >
> > Thoughts?
> >
> > --
> > QGLShader::link: Fragment shader(s) linked.... Crash on startup
> > https://bugs.launchpad.net/bugs/517373
> > You received this bug notification because you are a member of Mixxx
> > Development Team, which is subscribed to Mixxx.
> >
>
> --
> QGLShader::link: Fragment shader(s) linked.... Crash on startup
> https://bugs.launchpad.net/bugs/517373
> You received this bug notification because you are subscribed to Mixxx.
>