[region-editor] Don't prompt for save path if one was provided

[scanner] Fix crash when `pillow` is not available

[general] Cleanup some TODOs

[app] Add link to Discord chat under About menu

Author: Breakthrough

docs/changelog.md

@@ -23,6 +23,11 @@ The UI can be started by running `dvr-scan-app`, and is installed alongside the
  - [feature] Add ability to control video decoder via `input-mode` config option (`opencv`, `pyav`, `moviepy`)
     - Allows switching between `OpenCV` (default), `PyAV`, and `MoviePy` for video decoding
     - Certain backends provide substantial performance benefits, up to 50% in some cases (let us know which one works best!)
+ - [bugfix] Fix crash on headless systems that don't have `pillow` installed
+ - [general] The region editor no longer prompts for a save path if one was already specified via the `-s`/`--save-regions` option
+ - [general] A small rolling log is now kept automatically to assist with debugging
+    - Can be controlled with config file options `save-log`, `max-log-size` (default: 20 kB), `max-log-files` (default: 4)
+    - Path can be found under help entry for `--logfile` by running `dvr-scan --help` or `dvr-scan-app --help`
  - [general] Minimum supported Python version is now 3.9
 
 ----------------------------------------------------------

dvr-scan.cfg

@@ -17,21 +17,6 @@
 # used (it will be listed under the help text for the -c/--config option).
 #
 
-# * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
-#  GENERAL
-# * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
-
-# Show region editor window (-r/--region-editor) before scanning.
-#region-editor = no
-
-# Suppress all console output.
-#quiet-mode = no
-
-# Verbosity of console output (debug, info, warning, error).
-# If set to debug, overrides quiet-mode unless set via command line.
-#verbosity = info
-
-
 # * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
 #  INPUT
 # * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
@@ -50,6 +35,9 @@
 # Number of frames to skip between processing when looking for motion events.
 #frame-skip = 0
 
+# Always show the region editor window (-r/--region-editor) before scanning.
+#region-editor = no
+
 
 # * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
 #  OUTPUT
@@ -170,3 +158,28 @@
 
 # Text background color in the form (R,G,B) or 0xFFFFFF
 #text-bg-color = 0, 0, 0
+
+
+# * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
+#  LOGGING
+# * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
+
+# Suppress all console output. Output can still be saved using
+# the -l/--logfile option.
+#quiet-mode = no
+
+# Verbosity of console output (debug, info, warning, error).
+# If set to debug, overrides quiet-mode unless set via command line.
+#verbosity = info
+
+# Automatically save rolling logs. Useful for bug reports.
+# The path to logs for your system can be found under the help entry for
+# `--logfile` after running `dvr-scan --help` or `dvr-scan-app --help`.
+#save-log = yes
+
+# Max size of a debug log in bytes.
+#max-log-size = 20000
+
+# Max number of debug logs to keep. Old ones are deleted automatically.
+# Disk space usage will never exceed this times debug-log-max-len
+#max-log-files = 4

dvr_scan/__main__.py

@@ -32,6 +32,9 @@ def main():
     if settings is None:
         sys.exit(EXIT_ERROR)
     logger = logging.getLogger("dvr_scan")
+    # TODO(1.7): The logging redirect does not respect the original log level, which is now set to
+    # DEBUG mode for rolling log files. https://github.com/tqdm/tqdm/issues/1272
+    # We might have to just roll our own instead of relying on this one.
     redirect = FakeTqdmLoggingRedirect if settings.get("quiet-mode") else logging_redirect_tqdm
     # TODO: Use Python __debug__ mode instead of hard-coding as config option.
     debug_mode = settings.get("debug")
@@ -48,7 +51,6 @@ def main():
             if debug_mode:
                 raise
         except KeyboardInterrupt:
-            # TODO: This doesn't always work when the GUI is running.
             logger.info("Stopping (interrupt received)...", exc_info=show_traceback)
             if debug_mode:
                 raise

dvr_scan/app/__main__.py

@@ -12,20 +12,16 @@
 import argparse
 import logging
 import sys
-from logging.handlers import RotatingFileHandler
-from pathlib import Path
 from subprocess import CalledProcessError
 
-from platformdirs import user_log_path
 from scenedetect import VideoOpenFailure
 from scenedetect.platform import FakeTqdmLoggingRedirect, logging_redirect_tqdm
 
 import dvr_scan
 from dvr_scan import get_license_info
 from dvr_scan.app.application import Application
 from dvr_scan.config import CHOICE_MAP, USER_CONFIG_FILE_PATH, ConfigLoadFailure, ConfigRegistry
-from dvr_scan.platform import LOG_FORMAT_ROLLING_LOGS, attach_log_handler
-from dvr_scan.shared import ScanSettings, init_logging
+from dvr_scan.shared import ScanSettings, init_logging, logfile_path, setup_logger
 from dvr_scan.shared.cli import VERSION_STRING, LicenseAction, VersionAction, string_type_check
 
 logger = logging.getLogger("dvr_scan")
@@ -34,9 +30,7 @@
 EXIT_SUCCESS: int = 0
 EXIT_ERROR: int = 1
 
-# Keep the last 16 KiB of logfiles automatically
-MAX_LOGFILE_SIZE_KB = 16384
-MAX_LOGFILE_BACKUPS = 4
+LOGFILE_PATH = logfile_path(logfile_name="dvr-scan-app.log")
 
 
 def get_cli_parser():
@@ -71,7 +65,7 @@ def get_cli_parser():
         metavar="type",
         type=string_type_check(CHOICE_MAP["verbosity"], False, "type"),
         help=(
-            "Amount of verbosity to use for log output. Must be one of: %s."
+            "Verbosity type to use for log output. Must be one of: %s."
             % (", ".join(CHOICE_MAP["verbosity"]),)
         ),
     )
@@ -81,8 +75,8 @@ def get_cli_parser():
         metavar="file",
         type=str,
         help=(
-            "Path to log file for writing application output. If FILE already exists, the program"
-            " output will be appended to the existing contents."
+            "Appends application output to file. If file does not exist it will be created. "
+            f"Debug log path: {LOGFILE_PATH}"
         ),
     )
 
@@ -109,45 +103,28 @@ def get_cli_parser():
     return parser
 
 
-def init_debug_logger() -> logging.Handler:
-    """Initialize rolling debug logger."""
-    folder = user_log_path("DVR-Scan", False)
-    folder.mkdir(parents=True, exist_ok=True)
-    handler = RotatingFileHandler(
-        folder / Path("dvr-scan.app.log"),
-        maxBytes=MAX_LOGFILE_SIZE_KB * 1024,
-        backupCount=MAX_LOGFILE_BACKUPS,
-    )
-    handler.setLevel(logging.DEBUG)
-    handler.setFormatter(logging.Formatter(fmt=LOG_FORMAT_ROLLING_LOGS))
-    return handler
-
-
 # TODO: There's a lot of duplicated code here between the CLI and GUI. See if we can combine some
 # of the handling of config file loading and exceptions to be consistent between the two.
 #
 # It would also be nice if both commands took the same set of arguments. Can probably re-use the
 # existing CLI parser.
 def main():
     """Parse command line options and load config file settings."""
+    # We defer printing the debug log until we know where to put it.
     init_log = []
-    debug_log_handler = init_debug_logger()
-    config_load_error = None
-    failed_to_load_config = False
+    failed_to_load_config = True
     config = ConfigRegistry()
+    config_load_error = None
     # Create debug log handler and try to load the user config file.
     try:
         user_config = ConfigRegistry()
         user_config.load()
         config = user_config
     except ConfigLoadFailure as ex:
         config_load_error = ex
-
     # Parse CLI args, override config if an override was specified on the command line.
     try:
         args = get_cli_parser().parse_args()
-        # TODO: Add a UI element somewhere (e.g. in the about window) that indicates to the user
-        # where the log files are being written.
         init_logging(args, config)
         init_log += [(logging.INFO, "DVR-Scan Application %s" % dvr_scan.__version__)]
         if config_load_error and not hasattr(args, "config"):
@@ -158,28 +135,38 @@ def main():
             init_logging(args, config_setting)
             config = config_setting
         init_log += config.consume_init_log()
+        if config.config_dict:
+            logger.debug("Loaded configuration:\n%s", str(config.config_dict))
+        logger.debug("Program arguments:\n%s", str(args))
+        settings = ScanSettings(args=args, config=config)
+        if settings.get("save-log"):
+            setup_logger(
+                logfile_path=LOGFILE_PATH,
+                max_size_bytes=settings.get("max-log-size"),
+                max_files=settings.get("max-log-files"),
+            )
+        failed_to_load_config = False
     except ConfigLoadFailure as ex:
         init_log += ex.init_log
         if ex.reason is not None:
             init_log += [(logging.ERROR, "Error: %s" % str(ex.reason).replace("\t", "  "))]
         failed_to_load_config = True
         config_load_error = ex
     finally:
-        attach_log_handler(debug_log_handler)
         for log_level, log_str in init_log:
             logger.log(log_level, log_str)
-        if failed_to_load_config:
-            logger.critical("Failed to load config file.")
-            logger.debug("Error loading config file:", exc_info=config_load_error)
-            # Intentionally suppress the exception in release mode since we've already logged the
-            # failure reason to the user above. We can now exit with an error code.
-            raise SystemExit(1)
-
-    if config.config_dict:
-        logger.debug("Loaded configuration:\n%s", str(config.config_dict))
-
-    logger.debug("Program arguments:\n%s", str(args))
-    settings = ScanSettings(args=args, config=config)
+
+    if failed_to_load_config:
+        logger.critical("Failed to load config file.")
+        logger.debug("Error loading config file:", exc_info=config_load_error)
+        # Intentionally suppress the exception in release mode since we've already logged the
+        # failure reason to the user above. We can now exit with an error code.
+        raise SystemExit(1)
+
+    # TODO(1.7): The logging redirect does not respect the original log level, which is now set to
+    # DEBUG mode for rolling log files. https://github.com/tqdm/tqdm/issues/1272
+    # We can just remove the use of a context manager here and install our own hooks into the
+    # loggers instead as a follow-up action.
     redirect = FakeTqdmLoggingRedirect if settings.get("quiet-mode") else logging_redirect_tqdm
     show_traceback = getattr(logging, settings.get("verbosity").upper()) == logging.DEBUG
     # TODO: Use Python __debug__ mode instead of hard-coding as config option.

dvr_scan/app/application.py

@@ -54,9 +54,6 @@
 MAX_THRESHOLD = 255.0
 MAX_DOWNSCALE_FACTOR = 128
 NO_REGIONS_SPECIFIED_TEXT = "No Region(s) Specified"
-
-# TODO: Remove this and use the "debug" setting instead.
-SUPPRESS_EXCEPTIONS = False
 EXPAND_HORIZONTAL = tk.EW
 
 logger = getLogger("dvr_scan")
@@ -1296,15 +1293,17 @@ def __init__(self, settings: ScanSettings, initial_videos: ty.List[str]):
 
         self._create_menubar()
 
-        if not SUPPRESS_EXCEPTIONS:
+        # Make sure we don't suppress exceptions in debug mode.
+        # TODO: This should probably use the logger in release mode rather than the default from Tk.
+        if settings.get("debug"):
 
             def error_handler(*args):
                 raise
 
             self._root.report_callback_exception = error_handler
 
         # Initialize UI state from config.
-        self._initialize_settings(settings)
+        self._initialize(settings)
         for path in initial_videos:
             self._input_area._add_video(path)
 
@@ -1366,6 +1365,11 @@ def _create_menubar(self):
             command=lambda: webbrowser.open_new_tab("www.dvr-scan.com/guide"),
             underline=0,
         )
+        help_menu.add_command(
+            label="Join Discord Chat",
+            command=lambda: webbrowser.open_new_tab("https://discord.gg/69kf6f2Exb"),
+            underline=5,
+        )
         # TODO: Add window to show log messages and copy them to clipboard or save to a logfile.
         # help_menu.add_command(label="Debug Log", underline=0, state=tk.DISABLED)
         help_menu.add_separator()
@@ -1475,9 +1479,9 @@ def _reset_config(self, program_default: bool = False):
 
     def _reload_config(self, config: ConfigRegistry):
         """Reinitialize UI from another config."""
-        self._initialize_settings(ScanSettings(args=self._settings._args, config=config))
+        self._initialize(ScanSettings(args=self._settings._args, config=config))
 
-    def _initialize_settings(self, settings: ScanSettings):
+    def _initialize(self, settings: ScanSettings):
         """Initialize UI from both UI command-line arguments and config file."""
         logger.debug("initializing UI state from settings")
         # Store copy of settings internally.