Merge pull request #961 from eoyilmaz/960-colorimeter-correction-web-check-finds-nothing-for-apple-displays

[#960] Fix colorimeter correction web-check finding nothing on Apple built-in displays

Author: eoyilmaz

DisplayCAL/colorimeter_correction.py

@@ -191,6 +191,12 @@ def build_web_check_params(worker: Worker) -> dict:
     ``MainFrame.colorimeter_correction_web_handler`` (the ``http_request``
     call itself, and its progress reporting, stay with the caller).
 
+    Uses ``worker.get_display_generic_name()`` (falling back to
+    ``get_display_name()``) for the ``display`` param: the online database is
+    keyed by generic monitor names ("Color LCD"), and for Apple built-in
+    displays ``get_display_name()`` substitutes the machine model id
+    ("MacBookPro18,1"), which the database has no entries for.
+
     Args:
         worker: The running :class:`DisplayCAL.worker.Worker`, used to derive
             the current display/instrument.
@@ -204,7 +210,9 @@ def build_web_check_params(worker: Worker) -> dict:
         "get": True,
         "type": filetype,
         "manufacturer_id": worker.get_display_edid().get("manufacturer_id", ""),
-        "display": worker.get_display_name(False, True) or "Unknown",
+        "display": worker.get_display_generic_name()
+        or worker.get_display_name(False, True)
+        or "Unknown",
         "instrument": worker.get_instrument_name() or "Unknown",
         "json": 1,
     }
@@ -1570,7 +1578,14 @@ def resolve_colorimeter_correction_selection(
     if ccmx_cfg[0] == "AUTO":
         if len(ccmx_cfg) < 2:
             ccmx_cfg.append("")
-        display_name = worker.get_display_name(False, True, False)
+        # Local CCMX/CCSS files are keyed by their own generic DISPLAY field
+        # ("Color LCD"), never by an Apple machine model id, so matching
+        # must use the generic name too (see get_display_generic_name()'s
+        # docstring / build_web_check_params, which need the same fix for
+        # the same reason).
+        display_name = worker.get_display_generic_name() or worker.get_display_name(
+            False, True, False
+        )
         if worker.instrument_supports_ccss():
             # Prefer CCSS
             ccmx_cfg[1] = catalog.mapping.get(f"\0{display_name}", "")

DisplayCAL/worker.py

@@ -2613,6 +2613,7 @@ def __init__(self, owner=None):
         self.display_edid = []
         self.display_manufacturers = []
         self.display_names = []
+        self.display_generic_names = []
         self.display_rects = []
         self.displays = []
         self.instruments = []
@@ -4569,6 +4570,7 @@ def clear_argyll_info(self):
         self.display_edid = []
         self.display_manufacturers = []
         self.display_names = []
+        self.display_generic_names = []
         self.display_rects = []
         self.displays = []
         self.instruments = []
@@ -6456,6 +6458,7 @@ def enumerate_displays_and_ports(
                 self.display_edid = []
                 self.display_manufacturers = []
                 self.display_names = []
+                self.display_generic_names = []
                 if sys.platform == "win32":
                     # The ordering will work as long
                     # as Argyll continues using
@@ -6475,10 +6478,19 @@ def enumerate_displays_and_ports(
                             display_manufacturer = "Google"
                         self.display_manufacturers.append(display_manufacturer)
                         self.display_names.append(display.split(":", 1)[1].strip())
+                        self.display_generic_names.append(
+                            display.split(":", 1)[1].strip()
+                        )
                         continue
                     display_name = split_display_name(display)
                     # Make sure we have nice descriptions
                     desc = []
+                    # Generic (non-model-id-overridden) description, e.g. for
+                    # matching against the online colorimeter correction
+                    # database, which is keyed by generic monitor names
+                    # ("Color LCD") rather than the machine model id
+                    # ("MacBookPro18,1").
+                    generic_desc = []
                     if sys.platform == "win32" and i < len(monitors):
                         # Get monitor description using win32api
                         device = util_win.get_active_display_device(
@@ -6502,6 +6514,7 @@ def enumerate_displays_and_ports(
                         )
                         if is_primary:
                             display += " [PRIMARY]"
+                    original_display = display
                     # Get monitor descriptions from EDID
                     try:
                         # Important: display_name must be given for get_edid
@@ -6524,6 +6537,7 @@ def enumerate_displays_and_ports(
                             "monitor_name",
                             edid.get("ascii", str(edid["product_id"] or "")),
                         )
+                        generic_monitor = monitor
                         if (
                             monitor in ("Color LCD", "iMac")
                             and edid["manufacturer_id"] == "APP"
@@ -6538,6 +6552,10 @@ def enumerate_displays_and_ports(
                                 edid["monitor_name"] = monitor
                         if monitor and monitor not in "".join(desc):
                             desc = [monitor]
+                        if generic_monitor and generic_monitor not in "".join(
+                            generic_desc
+                        ):
+                            generic_desc = [generic_monitor]
                     else:
                         manufacturer = []
                         if sys.platform == "darwin" and i < len(self.display_rects):
@@ -6546,6 +6564,7 @@ def enumerate_displays_and_ports(
                                 rect.width, rect.height
                             )
                             if sp_name:
+                                generic_desc = [sp_name]
                                 if sp_name in ("Color LCD", "iMac"):
                                     model_id = get_model_id()
                                     if model_id:
@@ -6555,34 +6574,48 @@ def enumerate_displays_and_ports(
                         # Only replace the description if it not already
                         # contains the monitor model
                         display = " @".join([" ".join(desc), display.split("@")[-1]])
+                    if generic_desc and generic_desc[-1] not in original_display:
+                        generic_display = " @".join(
+                            [" ".join(generic_desc), original_display.split("@")[-1]]
+                        )
+                    else:
+                        generic_display = original_display
                     displays[i] = display
                     self.display_manufacturers.append(" ".join(manufacturer))
                     self.display_names.append(split_display_name(display))
+                    self.display_generic_names.append(
+                        split_display_name(generic_display)
+                    )
                 if self.argyll_version >= [1, 4, 0]:
                     displays.append("Web @ localhost")
                     self.display_edid.append({})
                     self.display_manufacturers.append("")
                     self.display_names.append("Web")
+                    self.display_generic_names.append("Web")
                 if self.argyll_version >= [1, 6, 0]:
                     displays.append("madVR")
                     self.display_edid.append({})
                     self.display_manufacturers.append("")
                     self.display_names.append("madVR")
+                    self.display_generic_names.append("madVR")
                 # Prisma (via DisplayCAL)
                 displays.append("Prisma")
                 self.display_edid.append({})
                 self.display_manufacturers.append("Q, Inc")
                 self.display_names.append("Prisma")
+                self.display_generic_names.append("Prisma")
                 # Resolve
                 displays.append("Resolve")
                 self.display_edid.append({})
                 self.display_manufacturers.append("DaVinci")
                 self.display_names.append("Resolve")
+                self.display_generic_names.append("Resolve")
                 # Untethered
                 displays.append("Untethered")
                 self.display_edid.append({})
                 self.display_manufacturers.append("")
                 self.display_names.append("Untethered")
+                self.display_generic_names.append("Untethered")
                 # -
                 self.displays = displays
                 setcfg("displays", displays)
@@ -9795,6 +9828,25 @@ def get_display_name(
             return " ".join(display)
         return ""
 
+    def get_display_generic_name(self):
+        """Return the generic (non-model-id-overridden) name of the current display.
+
+        Unlike :meth:`get_display_name`, this never substitutes an Apple
+        machine model id (e.g. "MacBookPro18,1") for a built-in display's
+        generic monitor name (e.g. "Color LCD"), which makes it suitable for
+        matching against the online colorimeter correction database (see
+        :func:`DisplayCAL.colorimeter_correction.build_web_check_params`) -
+        that database is keyed by generic monitor/model names, not by the
+        machine-specific model id.
+
+        Returns:
+            str: The display's generic name, or "" if unavailable.
+        """
+        n = getcfg("display.number") - 1
+        if 0 <= n < len(self.display_generic_names):
+            return self.display_generic_names[n]
+        return ""
+
     def get_display_name_short(self, prepend_manufacturer=False, prefer_edid=False):
         """Return shortened name of configured display (if possible).
 

tests/test_colorimeter_correction.py

@@ -8,10 +8,22 @@
 slice). No display or QApplication is needed.
 """
 
+import os
 from hashlib import md5
 from unittest.mock import MagicMock
 
 from DisplayCAL import colorimeter_correction as cc
+from DisplayCAL import config
+
+# A CCSS fixture whose DISPLAY field ("DELL UP2516D") matches a display's
+# *generic* name, never a machine-specific model id - used to prove Auto
+# resolution matches against the generic name, not the raw display name.
+_CCSS_FIXTURE = os.path.join(
+    os.path.dirname(__file__),
+    "data",
+    "icc",
+    "Dell, DELL UP2516D (i1 Pro 2) 08.2020.ccss",
+)
 
 # A minimal CCMX-shaped blob with the DISPLAY line the injector anchors on.
 BASE = b'CCMX\n\nDESCRIPTOR "test"\nDISPLAY "LCD Monitor"\nCOLOR_REP "XYZ"\n'
@@ -185,6 +197,7 @@ def test_uses_worker_state(self):
         worker = MagicMock()
         worker.instrument_supports_ccss.return_value = True
         worker.get_display_edid.return_value = {"manufacturer_id": "DEL"}
+        worker.get_display_generic_name.return_value = "Dell U2413"
         worker.get_display_name.return_value = "Dell U2413"
         worker.get_instrument_name.return_value = "i1 Pro"
         params = cc.build_web_check_params(worker)
@@ -197,10 +210,21 @@ def test_uses_worker_state(self):
             "json": 1,
         }
 
+    def test_falls_back_to_display_name_when_no_generic_name(self):
+        worker = MagicMock()
+        worker.instrument_supports_ccss.return_value = True
+        worker.get_display_edid.return_value = {"manufacturer_id": "APP"}
+        worker.get_display_generic_name.return_value = ""
+        worker.get_display_name.return_value = "MacBookPro18,1"
+        worker.get_instrument_name.return_value = "i1 DisplayPro"
+        params = cc.build_web_check_params(worker)
+        assert params["display"] == "MacBookPro18,1"
+
     def test_falls_back_to_ccmx_only_and_unknown(self):
         worker = MagicMock()
         worker.instrument_supports_ccss.return_value = False
         worker.get_display_edid.return_value = {}
+        worker.get_display_generic_name.return_value = ""
         worker.get_display_name.return_value = None
         worker.get_instrument_name.return_value = None
         params = cc.build_web_check_params(worker)
@@ -209,6 +233,56 @@ def test_falls_back_to_ccmx_only_and_unknown(self):
         assert params["instrument"] == "Unknown"
 
 
+class TestResolveColorimeterCorrectionSelectionAuto:
+    """"Auto" must match local CCMX/CCSS files by generic display name.
+
+    Regression test: local corrections are keyed by their own DISPLAY field
+    (a generic monitor name, e.g. "DELL UP2516D"), never by a machine model
+    id, so "Auto" resolution has to prefer ``get_display_generic_name()``
+    the same way ``build_web_check_params`` does - otherwise, on a display
+    where ``get_display_name()`` diverges from the generic name (e.g. an
+    Apple built-in display, see ``get_display_generic_name``'s docstring),
+    "Auto" can never find a matching correction even when one is present on
+    disk.
+    """
+
+    def _worker(self):
+        worker = MagicMock()
+        worker.instrument_supports_ccss.return_value = True
+        worker.instrument_can_use_ccxx.return_value = True
+        worker.get_instrument_name.return_value = "i1 Pro 2"
+        worker.get_instrument_measurement_modes.return_value = {"auto": None}
+        # Deliberately wrong/overridden, mirroring get_display_name()'s
+        # Apple model-id substitution - Auto must not use this value.
+        worker.get_display_name.return_value = "MacBookPro18,1"
+        worker.get_display_generic_name.return_value = "DELL UP2516D"
+        return worker
+
+    def _catalog(self):
+        catalog = cc.ColorimeterCorrectionCatalog()
+        catalog.cached_paths = [_CCSS_FIXTURE]
+        return catalog
+
+    def test_auto_resolves_using_generic_display_name(self):
+        config.setcfg("measurement_mode", "auto")
+        config.setcfg("colorimeter_correction_matrix_file", "AUTO:")
+        result = cc.resolve_colorimeter_correction_selection(
+            self._catalog(), self._worker()
+        )
+        assert result.use_ccmx
+        assert result.ccmx[1] == _CCSS_FIXTURE
+        assert result.items[1] != cc.lang.getstr("auto")
+
+    def test_auto_stays_none_when_generic_name_does_not_match(self):
+        config.setcfg("measurement_mode", "auto")
+        config.setcfg("colorimeter_correction_matrix_file", "AUTO:")
+        worker = self._worker()
+        worker.get_display_generic_name.return_value = "Some Other Display"
+        result = cc.resolve_colorimeter_correction_selection(self._catalog(), worker)
+        assert not result.use_ccmx
+        assert result.ccmx[1] == ""
+
+
 class TestValidateUploadOriginator:
     def test_argyll_originator_accepted(self):
         assert cc.validate_upload_originator(