Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use dark brand logos for update entities in dark mode #22448

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

karwosts
Copy link
Contributor

Proposed change

Load dark brand images for update entities in dark mode.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue or discussion:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Copy link
Member

@piitaya piitaya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we listen for prefers-color-scheme to update the image when the system color mode change?

const mql = matchMedia("(prefers-color-scheme: dark)");

@silamon
Copy link
Contributor

silamon commented Oct 21, 2024

A bit of an alternative solution, using a regex to get the BrandsOptions from a given URL, so it can be modified and passed into brandsUrl again:

diff --git a/src/util/brands-url.ts b/src/util/brands-url.ts
index e5afa9532..51621acea 100644
--- a/src/util/brands-url.ts
+++ b/src/util/brands-url.ts
@@ -1,6 +1,8 @@
+type BrandIconType = "icon" | "logo" | "icon@2x" | "logo@2x";
+
 export interface BrandsOptions {
   domain: string;
-  type: "icon" | "logo" | "icon@2x" | "logo@2x";
+  type: BrandIconType;
   useFallback?: boolean;
   darkOptimized?: boolean;
   brand?: boolean;
@@ -13,6 +15,30 @@ export interface HardwareBrandsOptions {
   darkOptimized?: boolean;
 }
 
+const regex =
+  // eslint-disable-next-line no-useless-escape
+  /https:\/\/brands\.home-assistant\.io\/(_\/)?([^\/]+)\/(dark_)?((?:icon|logo)(?:@2x)?)\.png/;
+
+export const parseBrandsUrl = (url: string): BrandsOptions | undefined => {
+  const match = url.match(regex);
+  const allowedValues: BrandIconType[] = ["icon", "logo", "icon@2x", "logo@2x"];
+
+  if (!match) {
+    return undefined;
+  }
+
+  if (!allowedValues.includes(match[4] as BrandIconType)) {
+    return undefined;
+  }
+
+  return {
+    useFallback: match[1] !== undefined,
+    domain: match[2],
+    darkOptimized: match[3] !== undefined,
+    type: match[4] as BrandIconType,
+  };
+};
+
 export const brandsUrl = (options: BrandsOptions): string =>
   `https://brands.home-assistant.io/${options.brand ? "brands/" : ""}${
     options.useFallback ? "_/" : ""
diff --git a/test/util/generate-brands-url-spec.ts b/test/util/generate-brands-url-spec.ts
index 68d8df107..2995d2c5b 100644
--- a/test/util/generate-brands-url-spec.ts
+++ b/test/util/generate-brands-url-spec.ts
@@ -1,5 +1,5 @@
 import { assert } from "chai";
-import { brandsUrl } from "../../src/util/brands-url";
+import { brandsUrl, parseBrandsUrl } from "../../src/util/brands-url";
 
 describe("Generate brands Url", () => {
   it("Generate logo brands url for cloud component without fallback", () => {
@@ -39,3 +39,79 @@ describe("Generate brands Url", () => {
     );
   });
 });
+
+describe("parseBrandsUrl", () => {
+  it("Returns valid brand options for cloud logo without fallback", () => {
+    const url = "https://brands.home-assistant.io/cloud/logo.png";
+    const parsed = parseBrandsUrl(url);
+
+    assert.deepStrictEqual(parsed, {
+      useFallback: false,
+      domain: "cloud",
+      type: "logo",
+      darkOptimized: false,
+    });
+  });
+
+  it("Returns valid brand options for cloud icon with fallback", () => {
+    const url = "https://brands.home-assistant.io/_/cloud/icon.png";
+    const parsed = parseBrandsUrl(url);
+
+    assert.deepStrictEqual(parsed, {
+      useFallback: true,
+      domain: "cloud",
+      type: "icon",
+      darkOptimized: false,
+    });
+  });
+
+  it("Returns valid brand options for icon@2x without fallback", () => {
+    const url = "https://brands.home-assistant.io/cloud/icon@2x.png";
+    const parsed = parseBrandsUrl(url);
+
+    assert.deepStrictEqual(parsed, {
+      useFallback: false,
+      domain: "cloud",
+      type: "icon@2x",
+      darkOptimized: false,
+    });
+  });
+
+  it("Returns undefined for invalid type", () => {
+    const url = "https://brands.home-assistant.io/cloud/invalid.png";
+    const parsed = parseBrandsUrl(url);
+
+    assert.isUndefined(parsed);
+  });
+
+  it("Returns undefined for non-matching URL", () => {
+    const url = "https://example.com/other-path";
+    const parsed = parseBrandsUrl(url);
+
+    assert.isUndefined(parsed);
+  });
+
+  it("Returns valid brand options for logo@2x with fallback and dark optimization", () => {
+    const url = "https://brands.home-assistant.io/_/cloud/dark_logo@2x.png";
+    const parsed = parseBrandsUrl(url);
+
+    assert.deepStrictEqual(parsed, {
+      useFallback: true,
+      domain: "cloud",
+      type: "logo@2x",
+      darkOptimized: true,
+    });
+  });
+
+  it("Returns valid brand options for dark icon without fallback", () => {
+    const url = "https://brands.home-assistant.io/cloud/dark_icon.png";
+    const parsed = parseBrandsUrl(url);
+
+    assert.deepStrictEqual(parsed, {
+      useFallback: false,
+      domain: "cloud",
+      type: "icon",
+      darkOptimized: true,
+    });
+  });
+});

I'm not against the code you propose, but we're depending much more to fix this if they change the entity_picture url using the string indexes.
After all, we're fixing this problem here because the update entity doesn't provide the integration of which it provides an update for.

@bramkragten
Copy link
Member

Should we even provide an entity picture from core? Why don't we just create a brand image on the fly for update entities when there is no entity picture?

@silamon
Copy link
Contributor

silamon commented Nov 13, 2024

Should we even provide an entity picture from core? Why don't we just create a brand image on the fly for update entities when there is no entity picture?

If I recall correctly, update entities don't have the brand or domain set anywhere they are providing an update for. The domain is part of the URL and can be parsed from the URL only last time I looked into this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants