From 1dd45f442434b6ac769a6b0494d552c3bde623fa Mon Sep 17 00:00:00 2001
From: Blake Harnden <32446120+bharnden@users.noreply.github.com>
Date: Sun, 3 May 2020 12:42:56 -0700
Subject: [PATCH] pygui cleaned up error display by creating top level app
 methods for displaying exceptions and errors, logging exceptions, and making
 sure they work for background tasks

---
 daemon/core/gui/app.py                        | 16 ++++++++++
 daemon/core/gui/coreclient.py                 | 31 +++++++++----------
 .../core/gui/dialogs/configserviceconfig.py   | 30 +++++++-----------
 daemon/core/gui/dialogs/emaneconfig.py        |  3 +-
 .../core/gui/{errors.py => dialogs/error.py}  | 20 ------------
 daemon/core/gui/dialogs/mobilityconfig.py     |  3 +-
 daemon/core/gui/dialogs/mobilityplayer.py     |  7 ++---
 daemon/core/gui/dialogs/serviceconfig.py      |  5 ++-
 daemon/core/gui/dialogs/sessionoptions.py     |  5 ++-
 daemon/core/gui/dialogs/sessions.py           |  3 +-
 daemon/core/gui/dialogs/wlanconfig.py         |  5 ++-
 daemon/core/gui/graph/node.py                 |  3 +-
 daemon/core/gui/task.py                       |  5 +--
 daemon/core/gui/toolbar.py                    |  3 +-
 14 files changed, 57 insertions(+), 82 deletions(-)
 rename daemon/core/gui/{errors.py => dialogs/error.py} (70%)

diff --git a/daemon/core/gui/app.py b/daemon/core/gui/app.py
index e797f1de..0f37b370 100644
--- a/daemon/core/gui/app.py
+++ b/daemon/core/gui/app.py
@@ -1,11 +1,15 @@
+import logging
 import math
 import time
 import tkinter as tk
 from tkinter import font, ttk
 from tkinter.ttk import Progressbar
 
+import grpc
+
 from core.gui import appconfig, themes
 from core.gui.coreclient import CoreClient
+from core.gui.dialogs.error import ErrorDialog
 from core.gui.graph.graph import CanvasGraph
 from core.gui.images import ImageEnum, Images
 from core.gui.menubar import Menubar
@@ -138,6 +142,18 @@ class Application(ttk.Frame):
         message = f"Task ran for {total:.3f} seconds"
         self.statusbar.set_status(message)
 
+    def show_grpc_exception(self, title: str, e: grpc.RpcError) -> None:
+        logging.exception("app grpc exception", exc_info=e)
+        message = e.details()
+        self.show_error(title, message)
+
+    def show_exception(self, title: str, e: Exception) -> None:
+        logging.exception("app exception", exc_info=e)
+        self.show_error(title, str(e))
+
+    def show_error(self, title: str, message: str) -> None:
+        self.after(0, lambda: ErrorDialog(self, self, title, message).show())
+
     def on_closing(self):
         self.menubar.prompt_save_running_session(True)
 
diff --git a/daemon/core/gui/coreclient.py b/daemon/core/gui/coreclient.py
index fc4fc64f..86329591 100644
--- a/daemon/core/gui/coreclient.py
+++ b/daemon/core/gui/coreclient.py
@@ -16,9 +16,9 @@ from core.api.grpc.mobility_pb2 import MobilityConfig
 from core.api.grpc.services_pb2 import NodeServiceData, ServiceConfig, ServiceFileConfig
 from core.api.grpc.wlan_pb2 import WlanConfig
 from core.gui import appconfig
+from core.gui.dialogs.error import ErrorDialog
 from core.gui.dialogs.mobilityplayer import MobilityPlayer
 from core.gui.dialogs.sessions import SessionsDialog
-from core.gui.errors import show_grpc_error
 from core.gui.graph import tags
 from core.gui.graph.edges import CanvasEdge
 from core.gui.graph.node import CanvasNode
@@ -343,7 +343,7 @@ class CoreClient:
             response = self.client.get_session_metadata(self.session_id)
             self.parse_metadata(response.config)
         except grpc.RpcError as e:
-            self.app.after(0, show_grpc_error, e, self.app, self.app)
+            self.app.show_grpc_exception("Join Session Error", e)
 
         # update ui to represent current state
         self.app.after(0, self.app.joined_session_update)
@@ -426,21 +426,16 @@ class CoreClient:
             )
             self.join_session(response.session_id, query_location=False)
         except grpc.RpcError as e:
-            self.app.after(0, show_grpc_error, e, self.app, self.app)
+            self.app.show_grpc_exception("New Session Error", e)
 
-    def delete_session(self, session_id: int = None, parent_frame=None):
+    def delete_session(self, session_id: int = None):
         if session_id is None:
             session_id = self.session_id
         try:
             response = self.client.delete_session(session_id)
             logging.info("deleted session(%s), Result: %s", session_id, response)
         except grpc.RpcError as e:
-            # use the right master widget so the error dialog displays
-            # right on top of it
-            master = self.app
-            if parent_frame:
-                master = parent_frame
-            self.app.after(0, show_grpc_error, e, master, self.app)
+            self.app.show_grpc_exception("Delete Session Error", e)
 
     def setup(self):
         """
@@ -472,7 +467,9 @@ class CoreClient:
                 dialog = SessionsDialog(self.app, self.app, True)
                 dialog.show()
         except grpc.RpcError as e:
-            show_grpc_error(e, self.app, self.app)
+            logging.exception("core setup error")
+            dialog = ErrorDialog(self.app, self.app, "Setup Error", e.details())
+            dialog.show()
             self.app.close()
 
     def edit_node(self, core_node: core_pb2.Node):
@@ -481,7 +478,7 @@ class CoreClient:
                 self.session_id, core_node.id, core_node.position, source=GUI_SOURCE
             )
         except grpc.RpcError as e:
-            self.app.after(0, show_grpc_error, e, self.app, self.app)
+            self.app.show_grpc_exception("Edit Node Error", e)
 
     def start_session(self) -> core_pb2.StartSessionResponse:
         self.interfaces_manager.reset_mac()
@@ -532,7 +529,7 @@ class CoreClient:
             if response.result:
                 self.set_metadata()
         except grpc.RpcError as e:
-            self.app.after(0, show_grpc_error, e, self.app, self.app)
+            self.app.show_grpc_exception("Start Session Error", e)
         return response
 
     def stop_session(self, session_id: int = None) -> core_pb2.StartSessionResponse:
@@ -543,7 +540,7 @@ class CoreClient:
             response = self.client.stop_session(session_id)
             logging.info("stopped session(%s), result: %s", session_id, response)
         except grpc.RpcError as e:
-            self.app.after(0, show_grpc_error, e, self.app, self.app)
+            self.app.show_grpc_exception("Stop Session Error", e)
         return response
 
     def show_mobility_players(self):
@@ -597,7 +594,7 @@ class CoreClient:
             logging.info("launching terminal %s", cmd)
             os.system(cmd)
         except grpc.RpcError as e:
-            self.app.after(0, show_grpc_error, e, self.app, self.app)
+            self.app.show_grpc_exception("Node Terminal Error", e)
 
     def save_xml(self, file_path: str):
         """
@@ -610,7 +607,7 @@ class CoreClient:
             response = self.client.save_xml(self.session_id, file_path)
             logging.info("saved xml file %s, result: %s", file_path, response)
         except grpc.RpcError as e:
-            self.app.after(0, show_grpc_error, e, self.app, self.app)
+            self.app.show_grpc_exception("Save XML Error", e)
 
     def open_xml(self, file_path: str):
         """
@@ -621,7 +618,7 @@ class CoreClient:
             logging.info("open xml file %s, response: %s", file_path, response)
             self.join_session(response.session_id)
         except grpc.RpcError as e:
-            self.app.after(0, show_grpc_error, e, self.app, self.app)
+            self.app.show_grpc_exception("Open XML Error", e)
 
     def get_node_service(self, node_id: int, service_name: str) -> NodeServiceData:
         response = self.client.get_node_service(self.session_id, node_id, service_name)
diff --git a/daemon/core/gui/dialogs/configserviceconfig.py b/daemon/core/gui/dialogs/configserviceconfig.py
index 45ea3a76..aaf67869 100644
--- a/daemon/core/gui/dialogs/configserviceconfig.py
+++ b/daemon/core/gui/dialogs/configserviceconfig.py
@@ -10,7 +10,6 @@ import grpc
 
 from core.api.grpc.services_pb2 import ServiceValidationMode
 from core.gui.dialogs.dialog import Dialog
-from core.gui.errors import show_grpc_error
 from core.gui.themes import FRAME_PAD, PADX, PADY
 from core.gui.widgets import CodeText, ConfigFrame, ListboxScroll
 
@@ -116,8 +115,8 @@ class ConfigServiceConfigDialog(Dialog):
                 self.modified_files.add(file)
                 self.temp_service_files[file] = data
         except grpc.RpcError as e:
+            self.app.show_grpc_exception("Get Config Service Error", e)
             self.has_error = True
-            show_grpc_error(e, self.app, self.app)
 
     def draw(self):
         self.top.columnconfigure(0, weight=1)
@@ -323,22 +322,17 @@ class ConfigServiceConfigDialog(Dialog):
             self.destroy()
             return
 
-        try:
-            service_config = self.canvas_node.config_service_configs.setdefault(
-                self.service_name, {}
-            )
-            if self.config_frame:
-                self.config_frame.parse_config()
-                service_config["config"] = {
-                    x.name: x.value for x in self.config.values()
-                }
-            templates_config = service_config.setdefault("templates", {})
-            for file in self.modified_files:
-                templates_config[file] = self.temp_service_files[file]
-            all_current = current_listbox.get(0, tk.END)
-            current_listbox.itemconfig(all_current.index(self.service_name), bg="green")
-        except grpc.RpcError as e:
-            show_grpc_error(e, self.top, self.app)
+        service_config = self.canvas_node.config_service_configs.setdefault(
+            self.service_name, {}
+        )
+        if self.config_frame:
+            self.config_frame.parse_config()
+            service_config["config"] = {x.name: x.value for x in self.config.values()}
+        templates_config = service_config.setdefault("templates", {})
+        for file in self.modified_files:
+            templates_config[file] = self.temp_service_files[file]
+        all_current = current_listbox.get(0, tk.END)
+        current_listbox.itemconfig(all_current.index(self.service_name), bg="green")
         self.destroy()
 
     def handle_template_changed(self, event: tk.Event):
diff --git a/daemon/core/gui/dialogs/emaneconfig.py b/daemon/core/gui/dialogs/emaneconfig.py
index a7835751..8914dc2c 100644
--- a/daemon/core/gui/dialogs/emaneconfig.py
+++ b/daemon/core/gui/dialogs/emaneconfig.py
@@ -9,7 +9,6 @@ from typing import TYPE_CHECKING, Any
 import grpc
 
 from core.gui.dialogs.dialog import Dialog
-from core.gui.errors import show_grpc_error
 from core.gui.images import ImageEnum, Images
 from core.gui.themes import PADX, PADY
 from core.gui.widgets import ConfigFrame
@@ -78,7 +77,7 @@ class EmaneModelDialog(Dialog):
                 )
             self.draw()
         except grpc.RpcError as e:
-            show_grpc_error(e, self.app, self.app)
+            self.app.show_grpc_exception("Get EMANE Config Error", e)
             self.has_error = True
             self.destroy()
 
diff --git a/daemon/core/gui/errors.py b/daemon/core/gui/dialogs/error.py
similarity index 70%
rename from daemon/core/gui/errors.py
rename to daemon/core/gui/dialogs/error.py
index b11e684b..3703b533 100644
--- a/daemon/core/gui/errors.py
+++ b/daemon/core/gui/dialogs/error.py
@@ -1,8 +1,6 @@
 from tkinter import ttk
 from typing import TYPE_CHECKING
 
-import grpc
-
 from core.gui.dialogs.dialog import Dialog
 from core.gui.images import ImageEnum, Images
 from core.gui.themes import FRAME_PAD, PADX, PADY
@@ -41,21 +39,3 @@ class ErrorDialog(Dialog):
 
         button = ttk.Button(self.top, text="Close", command=lambda: self.destroy())
         button.grid(sticky="ew")
-
-
-def show_exception(app: "Application", title: str, exception: Exception) -> None:
-    dialog = ErrorDialog(app, app, title, str(exception))
-    dialog.show()
-
-
-def show_grpc_error(e: grpc.RpcError, master, app: "Application") -> None:
-    title = [x.capitalize() for x in e.code().name.lower().split("_")]
-    title = " ".join(title)
-    title = f"GRPC {title}"
-    dialog = ErrorDialog(master, app, title, e.details())
-    dialog.show()
-
-
-def show_error(app: "Application", title: str, message: str) -> None:
-    dialog = ErrorDialog(app, app, title, message)
-    dialog.show()
diff --git a/daemon/core/gui/dialogs/mobilityconfig.py b/daemon/core/gui/dialogs/mobilityconfig.py
index 2222e06f..b4a6a163 100644
--- a/daemon/core/gui/dialogs/mobilityconfig.py
+++ b/daemon/core/gui/dialogs/mobilityconfig.py
@@ -7,7 +7,6 @@ from typing import TYPE_CHECKING
 import grpc
 
 from core.gui.dialogs.dialog import Dialog
-from core.gui.errors import show_grpc_error
 from core.gui.themes import PADX, PADY
 from core.gui.widgets import ConfigFrame
 
@@ -33,8 +32,8 @@ class MobilityConfigDialog(Dialog):
                 self.config = self.app.core.get_mobility_config(self.node.id)
             self.draw()
         except grpc.RpcError as e:
+            self.app.show_grpc_exception("Get Mobility Config Error", e)
             self.has_error = True
-            show_grpc_error(e, self.app, self.app)
             self.destroy()
 
     def draw(self):
diff --git a/daemon/core/gui/dialogs/mobilityplayer.py b/daemon/core/gui/dialogs/mobilityplayer.py
index 6b7b7869..e822aa4b 100644
--- a/daemon/core/gui/dialogs/mobilityplayer.py
+++ b/daemon/core/gui/dialogs/mobilityplayer.py
@@ -6,7 +6,6 @@ import grpc
 
 from core.api.grpc.mobility_pb2 import MobilityAction
 from core.gui.dialogs.dialog import Dialog
-from core.gui.errors import show_grpc_error
 from core.gui.images import ImageEnum, Images
 from core.gui.themes import PADX, PADY
 
@@ -154,7 +153,7 @@ class MobilityPlayerDialog(Dialog):
                 session_id, self.node.id, MobilityAction.START
             )
         except grpc.RpcError as e:
-            show_grpc_error(e, self.top, self.app)
+            self.app.show_grpc_exception("Mobility Error", e)
 
     def click_pause(self):
         self.set_pause()
@@ -164,7 +163,7 @@ class MobilityPlayerDialog(Dialog):
                 session_id, self.node.id, MobilityAction.PAUSE
             )
         except grpc.RpcError as e:
-            show_grpc_error(e, self.top, self.app)
+            self.app.show_grpc_exception("Mobility Error", e)
 
     def click_stop(self):
         self.set_stop()
@@ -174,4 +173,4 @@ class MobilityPlayerDialog(Dialog):
                 session_id, self.node.id, MobilityAction.STOP
             )
         except grpc.RpcError as e:
-            show_grpc_error(e, self.top, self.app)
+            self.app.show_grpc_exception("Mobility Error", e)
diff --git a/daemon/core/gui/dialogs/serviceconfig.py b/daemon/core/gui/dialogs/serviceconfig.py
index 8fc85394..79e8871f 100644
--- a/daemon/core/gui/dialogs/serviceconfig.py
+++ b/daemon/core/gui/dialogs/serviceconfig.py
@@ -9,7 +9,6 @@ import grpc
 from core.api.grpc.services_pb2 import ServiceValidationMode
 from core.gui.dialogs.copyserviceconfig import CopyServiceConfigDialog
 from core.gui.dialogs.dialog import Dialog
-from core.gui.errors import show_grpc_error
 from core.gui.images import ImageEnum, Images
 from core.gui.themes import FRAME_PAD, PADX, PADY
 from core.gui.widgets import CodeText, ListboxScroll
@@ -119,8 +118,8 @@ class ServiceConfigDialog(Dialog):
             for file, data in file_configs.items():
                 self.temp_service_files[file] = data
         except grpc.RpcError as e:
+            self.app.show_grpc_exception("Get Node Service Error", e)
             self.has_error = True
-            show_grpc_error(e, self.master, self.app)
 
     def draw(self):
         self.top.columnconfigure(0, weight=1)
@@ -484,7 +483,7 @@ class ServiceConfigDialog(Dialog):
                 )
             self.current_service_color("green")
         except grpc.RpcError as e:
-            show_grpc_error(e, self.top, self.app)
+            self.app.show_grpc_exception("Save Service Config Error", e)
         self.destroy()
 
     def display_service_file_data(self, event: tk.Event):
diff --git a/daemon/core/gui/dialogs/sessionoptions.py b/daemon/core/gui/dialogs/sessionoptions.py
index c042eef4..b2c5ede5 100644
--- a/daemon/core/gui/dialogs/sessionoptions.py
+++ b/daemon/core/gui/dialogs/sessionoptions.py
@@ -5,7 +5,6 @@ from typing import TYPE_CHECKING
 import grpc
 
 from core.gui.dialogs.dialog import Dialog
-from core.gui.errors import show_grpc_error
 from core.gui.themes import PADX, PADY
 from core.gui.widgets import ConfigFrame
 
@@ -28,8 +27,8 @@ class SessionOptionsDialog(Dialog):
             response = self.app.core.client.get_session_options(session_id)
             return response.config
         except grpc.RpcError as e:
+            self.app.show_grpc_exception("Get Session Options Error", e)
             self.has_error = True
-            show_grpc_error(e, self.app, self.app)
             self.destroy()
 
     def draw(self):
@@ -56,5 +55,5 @@ class SessionOptionsDialog(Dialog):
             response = self.app.core.client.set_session_options(session_id, config)
             logging.info("saved session config: %s", response)
         except grpc.RpcError as e:
-            show_grpc_error(e, self.top, self.app)
+            self.app.show_grpc_exception("Set Session Options Error", e)
         self.destroy()
diff --git a/daemon/core/gui/dialogs/sessions.py b/daemon/core/gui/dialogs/sessions.py
index 79a153f7..251f74b0 100644
--- a/daemon/core/gui/dialogs/sessions.py
+++ b/daemon/core/gui/dialogs/sessions.py
@@ -7,7 +7,6 @@ import grpc
 
 from core.api.grpc import core_pb2
 from core.gui.dialogs.dialog import Dialog
-from core.gui.errors import show_grpc_error
 from core.gui.images import ImageEnum, Images
 from core.gui.task import ProgressTask
 from core.gui.themes import PADX, PADY
@@ -37,7 +36,7 @@ class SessionsDialog(Dialog):
             logging.info("sessions: %s", response)
             return response.sessions
         except grpc.RpcError as e:
-            show_grpc_error(e, self.app, self.app)
+            self.app.show_grpc_exception("Get Sessions Error", e)
             self.destroy()
 
     def draw(self) -> None:
diff --git a/daemon/core/gui/dialogs/wlanconfig.py b/daemon/core/gui/dialogs/wlanconfig.py
index d5d0c673..5096cd0f 100644
--- a/daemon/core/gui/dialogs/wlanconfig.py
+++ b/daemon/core/gui/dialogs/wlanconfig.py
@@ -4,7 +4,6 @@ from typing import TYPE_CHECKING
 import grpc
 
 from core.gui.dialogs.dialog import Dialog
-from core.gui.errors import show_grpc_error
 from core.gui.themes import PADX, PADY
 from core.gui.widgets import ConfigFrame
 
@@ -21,7 +20,7 @@ class WlanConfigDialog(Dialog):
         self, master: "Application", app: "Application", canvas_node: "CanvasNode"
     ):
         super().__init__(
-            master, app, f"{canvas_node.core_node.name} Wlan Configuration"
+            master, app, f"{canvas_node.core_node.name} WLAN Configuration"
         )
         self.canvas_node = canvas_node
         self.node = canvas_node.core_node
@@ -38,7 +37,7 @@ class WlanConfigDialog(Dialog):
             self.init_draw_range()
             self.draw()
         except grpc.RpcError as e:
-            show_grpc_error(e, self.app, self.app)
+            self.app.show_grpc_exception("WLAN Config Error", e)
             self.has_error = True
             self.destroy()
 
diff --git a/daemon/core/gui/graph/node.py b/daemon/core/gui/graph/node.py
index 758dbd26..c90be311 100644
--- a/daemon/core/gui/graph/node.py
+++ b/daemon/core/gui/graph/node.py
@@ -14,7 +14,6 @@ from core.gui.dialogs.nodeconfig import NodeConfigDialog
 from core.gui.dialogs.nodeconfigservice import NodeConfigServiceDialog
 from core.gui.dialogs.nodeservice import NodeServiceDialog
 from core.gui.dialogs.wlanconfig import WlanConfigDialog
-from core.gui.errors import show_grpc_error
 from core.gui.graph import tags
 from core.gui.graph.edges import CanvasEdge
 from core.gui.graph.tooltip import CanvasTooltip
@@ -180,7 +179,7 @@ class CanvasNode:
                 output = self.app.core.run(self.core_node.id)
                 self.tooltip.text.set(output)
             except grpc.RpcError as e:
-                show_grpc_error(e, self.app, self.app)
+                self.app.show_grpc_exception("Observer Error", e)
 
     def on_leave(self, event: tk.Event):
         self.tooltip.on_leave(event)
diff --git a/daemon/core/gui/task.py b/daemon/core/gui/task.py
index c88a0151..05855945 100644
--- a/daemon/core/gui/task.py
+++ b/daemon/core/gui/task.py
@@ -2,8 +2,6 @@ import logging
 import threading
 from typing import Any, Callable, Tuple
 
-from core.gui.errors import show_exception
-
 
 class ProgressTask:
     def __init__(
@@ -33,7 +31,6 @@ class ProgressTask:
                 self.app.after(0, self.callback, *values)
         except Exception as e:
             logging.exception("progress task exception")
-            args = (self.app, "Task Error", e)
-            self.app.after(0, show_exception, *args)
+            self.app.show_exception("Task Error", e)
         finally:
             self.app.after(0, self.app.progress_task_complete)
diff --git a/daemon/core/gui/toolbar.py b/daemon/core/gui/toolbar.py
index 374a4eaf..c735707a 100644
--- a/daemon/core/gui/toolbar.py
+++ b/daemon/core/gui/toolbar.py
@@ -9,7 +9,6 @@ from core.api.grpc import core_pb2
 from core.gui.dialogs.customnodes import CustomNodesDialog
 from core.gui.dialogs.marker import MarkerDialog
 from core.gui.dialogs.runtool import RunToolDialog
-from core.gui.errors import show_error
 from core.gui.graph.enums import GraphMode
 from core.gui.graph.shapeutils import ShapeType, is_marker
 from core.gui.images import ImageEnum, Images
@@ -273,7 +272,7 @@ class Toolbar(ttk.Frame):
             self.app.core.show_mobility_players()
         else:
             message = "\n".join(response.exceptions)
-            show_error(self.app, "Start Session Error", message)
+            self.app.show_error("Start Session Error", message)
 
     def set_runtime(self):
         self.runtime_frame.tkraise()