Error-reporting: improve UI/display behaviour

Assess how critical the error is, and based on this, show a popup
instead of the dialog in some cases. Add commands to allow delayed
display of the main dialog, and stepping through multiple error
reports in the dialog.

Also add a new error category for shader errors, since these are
always emitted from the render thread and we can’t easily attribute
them to an aircraft, scenery or core feature.

src/Main/ErrorReporter.cxx

@@ -74,10 +74,13 @@ enum class Aggregation {
     MultiPlayer,
     Unknown,     ///< error coudln't be attributed more specifcially
     OutOfMemory, ///< seperate category to give it a custom message
-    Traffic
+    Traffic,
+    ShadersEffects
 };
 
 // these should correspond to simgear::ErrorCode enum
+// they map to translateable strings in fgdata/Translations/sys.xml
+
 string_list static_errorIds = {
     "error-missing-shader",
     "error-loading-texture",
@@ -116,7 +119,8 @@ string_list static_categoryIds = {
     "error-category-multiplayer",
     "error-category-unknown",
     "error-category-out-of-memory",
-    "error-category-traffic"};
+    "error-category-traffic",
+    "error-category-shaders"};
 
 class RecentLogCallback : public simgear::LogCallback
 {
@@ -221,6 +225,7 @@ public:
 
         bool haveShownToUser = false;
         OccurrenceVec errors;
+        bool isCritical = false;
 
         bool addOccurence(const ErrorOcurrence& err);
     };
@@ -241,7 +246,6 @@ public:
 
     void collectError(simgear::LoadFailure type, simgear::ErrorCode code, const std::string& details, const sg_location& location)
     {
-        SG_LOG(SG_GENERAL, SG_WARN, "Error:" << static_errorTypeIds.at(static_cast<int>(type)) << " from " << static_errorIds.at(static_cast<int>(code)) << "::" << details << "\n\t" << location.asString());
         ErrorOcurrence occurrence{code, type, details, location, time(nullptr)};
 
         // snapshot the top of the context stacks into our occurence data
@@ -255,9 +259,24 @@ public:
         auto it = getAggregateForOccurence(occurrence);
 
         // add to the occurence, if it's not a duplicate
-        if (it->addOccurence(occurrence)) {
+        if (!it->addOccurence(occurrence)) {
+            return; // duplicate, nothing else to do
+        }
+
+        // log it once we know it's not a duplicate
+        SG_LOG(SG_GENERAL, SG_WARN, "Error:" << static_errorTypeIds.at(static_cast<int>(type)) << " from " << static_errorIds.at(static_cast<int>(code)) << "::" << details << "\n\t" << location.asString());
+
         it->lastErrorTime.stamp();
         _reportsDirty = true;
+
+        const auto ty = it->type;
+        // decide if it's a critical error or not
+        if ((ty == Aggregation::OutOfMemory) || (ty == Aggregation::MainAircraft) || (ty == Aggregation::InputDevice)) {
+            it->isCritical = true;
+        }
+
+        if (code == simgear::ErrorCode::LoadEffectsShaders) {
+            //   it->isCritical = true;
         }
     }
 
@@ -330,8 +349,13 @@ public:
         });
         assert(it != _aggregated.end());
         _activeReportIndex = std::distance(_aggregated.begin(), it);
+        _displayNode->setBoolValue("have-next", _activeReportIndex < (_aggregated.size() - 1));
+    }
 
-        flightgear::sentryReportUserError(static_categoryIds.at(catId), detailsTextStream.str());
+    void sendReportToSentry(AggregateReport& report)
+    {
+        const int catId = static_cast<int>(report.type);
+        flightgear::sentryReportUserError(static_categoryIds.at(catId), _displayNode->getStringValue());
     }
 
     void writeReportToStream(const AggregateReport& report, std::ostream& os) const;
@@ -341,6 +365,7 @@ public:
 
     bool dismissReportCommand(const SGPropertyNode* args, SGPropertyNode*);
     bool saveReportCommand(const SGPropertyNode* args, SGPropertyNode*);
+    bool showErrorReportCommand(const SGPropertyNode* args, SGPropertyNode*);
 };
 
 auto ErrorReporter::ErrorReporterPrivate::getAggregateForOccurence(const ErrorReporter::ErrorReporterPrivate::ErrorOcurrence& oc)
@@ -427,6 +452,13 @@ auto ErrorReporter::ErrorReporterPrivate::getAggregateForOccurence(const ErrorRe
         return getAggregate(Aggregation::FGData);
     }
 
+    // becuase we report shader errors from the main thread, they don't
+    // get attributed. Collect them into their own category, which also
+    // means we can display a more specific message
+    if (oc.code == simgear::ErrorCode::LoadEffectsShaders) {
+        return getAggregate(Aggregation::ShadersEffects);
+    }
+
     return getAggregate(Aggregation::Unknown);
 }
 
@@ -519,7 +551,6 @@ bool ErrorReporter::ErrorReporterPrivate::dismissReportCommand(const SGPropertyN
 
     // clear any values underneath displayNode?
 
-
     _nextShowTimeout.stamp();
     _reportsDirty = true; // set this so we check for another report to present
     _activeReportIndex = -1;
@@ -527,6 +558,39 @@ bool ErrorReporter::ErrorReporterPrivate::dismissReportCommand(const SGPropertyN
     return true;
 }
 
+bool ErrorReporter::ErrorReporterPrivate::showErrorReportCommand(const SGPropertyNode* args, SGPropertyNode*)
+{
+    std::lock_guard<std::mutex> g(_lock);
+
+    if (_aggregated.empty()) {
+        return false;
+    }
+
+    if (args->getBoolValue("next")) {
+        _activeReportIndex++;
+        if (_activeReportIndex >= _aggregated.size()) {
+            return false;
+        }
+    } else if (args->hasChild("index")) {
+        _activeReportIndex = args->getIntValue("index");
+        if ((_activeReportIndex < 0) || (_activeReportIndex >= _aggregated.size())) {
+            return false;
+        }
+    } else {
+        _activeReportIndex = 0;
+    }
+
+    auto& report = _aggregated.at(_activeReportIndex);
+    presentErrorToUser(report);
+
+    auto gui = globals->get_subsystem<NewGUI>();
+    if (!gui->getDialog("error-report")) {
+        gui->showDialog("error-report");
+    }
+
+    return true;
+}
+
 bool ErrorReporter::ErrorReporterPrivate::saveReportCommand(const SGPropertyNode* args, SGPropertyNode*)
 {
     if (_activeReportIndex < 0) {
@@ -667,7 +731,8 @@ void ErrorReporter::preinit()
 
 void ErrorReporter::init()
 {
-    if (!d->_enabledNode) {
+    const auto developerMode = false; // fgGetBool("sim/developer-mode");
+    if (developerMode || !d->_enabledNode) {
         SG_LOG(SG_GENERAL, SG_INFO, "Error reporting disabled");
         simgear::setFailureCallback(simgear::FailureCallback());
         simgear::setErrorContextCallback(simgear::ContextCallback());
@@ -677,6 +742,7 @@ void ErrorReporter::init()
 
     globals->get_commands()->addCommand("dismiss-error-report", d.get(), &ErrorReporterPrivate::dismissReportCommand);
     globals->get_commands()->addCommand("save-error-report-data", d.get(), &ErrorReporterPrivate::saveReportCommand);
+    globals->get_commands()->addCommand("show-error-report", d.get(), &ErrorReporterPrivate::showErrorReportCommand);
 
     // cache these values here
     d->_fgdataPathPrefix = globals->get_fg_root().utf8Str();
@@ -686,6 +752,7 @@ void ErrorReporter::init()
 void ErrorReporter::update(double dt)
 {
     bool showDialog = false;
+    bool showPopup = false;
     bool havePendingReports = false;
 
     // beginning of locked section
@@ -731,7 +798,13 @@ void ErrorReporter::update(double dt)
             const auto ageSec = (n - report.lastErrorTime).toSecs();
             if (ageSec > NoNewErrorsTimeout) {
                 d->presentErrorToUser(report);
+                if (report.isCritical) {
                     showDialog = true;
+                } else {
+                    showPopup = true;
+                }
+
+                d->sendReportToSentry(report);
 
                 // if we show one report, don't consider any others for now
                 break;
@@ -745,8 +818,9 @@ void ErrorReporter::update(double dt)
         }
     } // end of locked section
 
-    if (showDialog && flightgear::isHeadlessMode()) {
+    if (flightgear::isHeadlessMode()) {
         showDialog = false;
+        showPopup = false;
     }
 
     // do not call into another subsystem with our lock held,
@@ -754,7 +828,6 @@ void ErrorReporter::update(double dt)
     if (showDialog) {
         auto gui = globals->get_subsystem<NewGUI>();
         gui->showDialog("error-report");
-
         // this needs a bit more thought, disabling for the now
 #if 0
         // pause the sim when showing the popup
@@ -762,6 +835,9 @@ void ErrorReporter::update(double dt)
         pauseArgs->setBoolValue("force-pause", true);
         globals->get_commands()->execute("do_pause", pauseArgs);
 #endif
+    } else if (showPopup) {
+        SGPropertyNode_ptr popupArgs(new SGPropertyNode);
+        globals->get_commands()->execute("show-error-notification-popup", popupArgs, nullptr);
     }
 }
 
@@ -770,6 +846,7 @@ void ErrorReporter::shutdown()
     if (d->_enabledNode) {
         globals->get_commands()->removeCommand("dismiss-error-report");
         globals->get_commands()->removeCommand("save-error-report-data");
+        globals->get_commands()->removeCommand("show-error-report");
         sglog().removeCallback(d->_logCallback.get());
     }
 }