Fix #1767: Allow custom favicon.png by renaming defaults and adding o…#2799
Fix #1767: Allow custom favicon.png by renaming defaults and adding o…#2799arryansh5 wants to merge 5 commits into
Conversation
…ding override detection Problem: Custom favicon named 'favicon.png' was not displayed due to naming conflict with bundled default favicon. Solution: Implemented two complementary fixes: 1. Renamed default favicons from favicon.png/ico to taipy-favicon.png/ico 2. Added override detection in Flask server to prioritize custom favicon.png Changes: - Renamed frontend/taipy-gui/public/favicon.* to taipy-favicon.* - Updated HTML templates to reference renamed default favicons - Updated _DEFAULT_FAVICON_URL constant in gui.py - Added override logic in server.py to intercept favicon.png/ico requests - Custom favicons with standard names now take priority over defaults Result: Users can now use favicon='favicon.png' successfully
There was a problem hiding this comment.
Pull request overview
This pull request fixes issue #1767 where custom favicons named favicon.png or favicon.ico were not displayed due to naming conflicts with bundled default favicons. The solution implements a dual approach: renaming the default favicon files and adding override detection logic to intercept standard favicon requests.
Changes:
- Renamed bundled default favicons from
favicon.png/icototaipy-favicon.png/icoto eliminate naming conflicts - Added override detection in the Flask route handler to intercept requests for
favicon.pngandfavicon.icowhen custom favicons are configured - Updated all references to point to the renamed default favicon files
Reviewed changes
Copilot reviewed 3 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
taipy/gui/servers/flask/server.py |
Added override detection logic in my_index() route handler to intercept favicon requests and serve custom files via content accessor when configured |
taipy/gui/gui.py |
Updated _DEFAULT_FAVICON_URL constant to reference the renamed taipy-favicon.png file in the external assets repository |
frontend/taipy-gui/public/index.html |
Updated favicon link references from favicon.png to taipy-favicon.png |
frontend/taipy-gui/public/taipy-favicon.png |
Renamed default PNG favicon file (from favicon.png) |
frontend/taipy-gui/public/taipy-favicon.ico |
Renamed default ICO favicon file (from favicon.ico) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| __DOWNLOAD_ACTION = "__Taipy__download_csv" | ||
| __DOWNLOAD_DELETE_ACTION = "__Taipy__download_delete_csv" | ||
| _DEFAULT_FAVICON_URL = "https://raw.githubusercontent.com/Avaiga/taipy-assets/develop/favicon.png" | ||
| _DEFAULT_FAVICON_URL = "https://raw.githubusercontent.com/Avaiga/taipy-assets/develop/taipy-favicon.png" |
There was a problem hiding this comment.
The external default favicon URL has been updated to point to "taipy-favicon.png" in the taipy-assets repository. Please verify that this file exists at the specified URL (https://raw.githubusercontent.com/Avaiga/taipy-assets/develop/taipy-favicon.png) to ensure the fallback default favicon works correctly when no custom favicon is specified.
| if custom_page_resource is not None: | ||
| return custom_page_resource | ||
|
|
||
| # Solution 4: Override detection for custom favicon |
There was a problem hiding this comment.
The comment "Solution 4: Override detection for custom favicon" references an implementation approach number that may not be meaningful to future maintainers. Consider updating to a more descriptive comment like "Override default favicon with custom favicon if configured".
| # Solution 4: Override detection for custom favicon | |
| # Override default favicon with custom favicon if configured |
| parts = path.split("/") | ||
| file_name = parts[-1] |
There was a problem hiding this comment.
Unused variable: file_name is assigned but never used. These lines should be removed.
| parts = path.split("/") | |
| file_name = parts[-1] |
| from pathlib import Path | ||
| # Check if custom favicon filename matches the requested path | ||
| custom_path = Path(custom_favicon) |
There was a problem hiding this comment.
Unnecessary import: The pathlib module is already imported at the top of the file (line 17). Instead of importing Path from pathlib here, use pathlib.Path directly to avoid redundant imports.
| from pathlib import Path | |
| # Check if custom favicon filename matches the requested path | |
| custom_path = Path(custom_favicon) | |
| # Check if custom favicon filename matches the requested path | |
| custom_path = pathlib.Path(custom_favicon) |
| # Solution 4: Override detection for custom favicon | ||
| if path in ["favicon.png", "favicon.ico"]: | ||
| custom_favicon = self._gui._get_config("favicon") # type: ignore[attr-defined] | ||
| if custom_favicon: | ||
| from pathlib import Path | ||
| # Check if custom favicon filename matches the requested path | ||
| custom_path = Path(custom_favicon) | ||
| if custom_path.name == path: | ||
| # Serve custom favicon via content accessor | ||
| parts = path.split("/") | ||
| file_name = parts[-1] | ||
| # Get the mapped URL from content accessor | ||
| url = self._gui._get_content("__taipy_favicon", custom_favicon, True) # type: ignore[attr-defined] | ||
| # Extract the path after _CONTENT_ROOT | ||
| if isinstance(url, str) and url.startswith(f"/{self._gui._CONTENT_ROOT}/"): # type: ignore[attr-defined] | ||
| content_path = url[len(f"/{self._gui._CONTENT_ROOT}/"):] # type: ignore[attr-defined] | ||
| return self._gui._serve_content(content_path) # type: ignore[attr-defined] |
There was a problem hiding this comment.
Missing test coverage: The new override detection logic for custom favicons in the my_index route handler is not covered by tests. Consider adding tests that verify:
- A custom favicon with the name "favicon.png" is correctly served instead of the default
- A custom favicon with the name "favicon.ico" is correctly served instead of the default
- When no custom favicon is configured, the default favicon files are served correctly
- Updated comment for better clarity - Removed unused variables (parts, file_name) - Fixed redundant pathlib.Path import
The taipy-assets repo still uses 'favicon.png' not 'taipy-favicon.png'
There was a problem hiding this comment.
What if custom favicon is set to taipy-favicon.png ?
FredLL-Avaiga
left a comment
There was a problem hiding this comment.
can you fix the linter issues ?
What if custom favicon is set to taipy-favicon.png ?
Okay I will fix it |
|
Hello @arryansh5, We appreciate your help. You have been automatically unassigned, but if you are still working on that issue, I can re-assign it to you without any trouble. Just tell me, please. Thx, |
Fixes #1767
Summary
Resolves the issue where custom favicons named
favicon.pngorfavicon.icowere not displayed due to naming conflicts with bundled default favicons. Implements a dual-approach solution that renames default favicons and adds override detection for standard favicon filenames.Changes
frontend/taipy-gui/public/favicon.png→taipy-favicon.pngfrontend/taipy-gui/public/favicon.ico→taipy-favicon.icofrontend/taipy-gui/public/index.htmlto referencetaipy-favicon.pngtaipy/gui/webapp/index.htmlto referencetaipy-favicon.pngtaipy/gui/gui.py- Changed_DEFAULT_FAVICON_URLconstant to usetaipy-favicon.pngtaipy/gui/servers/flask/server.py- Added override detection logic inmy_index()route handler to interceptfavicon.pngandfavicon.icorequestsProblem Solved
When users specified
run(favicon="favicon.png"), the Flask server always served the bundled default favicon from the webapp folder instead of the user's custom file. This occurred because:webapp/firstfavicon.png)This implementation uses two complementary solutions:
taipy-favicon.png/icoUsers can now use standard favicon filenames without any issues: