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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Keep support for ICO formats by defaulting to PNG if necessary #4343

Merged
merged 10 commits into from Feb 4, 2022
35 changes: 35 additions & 0 deletions e2e/scripts/st_set_page_config_icon.py
@@ -0,0 +1,35 @@
import streamlit as st
import os, fnmatch


def is_assets_favicon_path(path):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Haha this code got complicated, I would just add a comment on the reason why this is happening (we should note that this later can be a e2e_test_util library

if "assets/favicon.ico" in path:
return path


def filter_nones(l):
return list(filter(None, l))


def find(pattern, path):
result = []
for root, dirs, files in os.walk(path):
for name in files:
if fnmatch.fnmatch(name, pattern):
result.append(os.path.join(root, name))
return result


# We want to find the file from 2 directories behind for circle ci
# and for local testing if you are in the e2e/scripts folder
# if you are in the root streamlit directory,
# this script still work but will be slower likely.
paths = find("favicon.ico", "../..")
filtered_paths = filter_nones(list(map(is_assets_favicon_path, paths)))

if len(filtered_paths) != 1:
print("Can't seem to find assets/favicon.ico in the directory you're in.")
else:
st.set_page_config(
page_icon=filtered_paths[0],
)
31 changes: 31 additions & 0 deletions e2e/specs/st_set_page_config_icon.spec.js
@@ -0,0 +1,31 @@
/**
* @license
* Copyright 2018-2022 Streamlit Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

describe("st.set_page_config", () => {
before(() => {
cy.loadApp("http://localhost:3000/");
});

it("sets the page favicon with ico file", () => {
cy.get("link[rel='shortcut icon']")
.should("have.attr", "href")
.should(
"contain",
"92018b2805266c4cb9a98e90c849ce5b5e7ba6d1af423bd7b7c345da.png"
);
});
});
Binary file added frontend/src/assets/favicon.ico
Binary file not shown.
Empty file added lib/diff
Empty file.
3 changes: 3 additions & 0 deletions lib/streamlit/elements/image.py
Expand Up @@ -207,6 +207,9 @@ def _normalize_to_bytes(data, width, output_format):
if output_format.lower() == "auto":
ext = imghdr.what(None, data)
mimetype = mimetypes.guess_type("image.%s" % ext)[0]
# if no other options, attempt to convert
if mimetype is None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I realize I wrote this, but I would just add a comment that suggests that there's no other options, just try to convert it (would require Pillow support).

mimetype = "image/" + format.lower()
else:
mimetype = "image/" + format.lower()

Expand Down