-
-
Notifications
You must be signed in to change notification settings - Fork 672
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 the application binary icon when available #2527
Conversation
6102ee6
to
6bb1ee0
Compare
The Linux test failure doesn't appear to be due to anything I've done - it looks like it's a configuration error on Ubuntu's end. Hopefully it will be resolved quickly... |
6bb1ee0
to
d9e3e4f
Compare
The remaining GTK coverage failure is because of beeware/briefcase-linux-system-template#24; at present, the application binary isn't being used - but the testbed's icon is the Toga icon, so at runtime, there's no difference. Once that PR is merged, the coverage should be completed. |
I still don't understand the rationale for making this depend on Also, it looks like this PR is applying the same fallback logic to all icons within the app's UI, not just the icon of the app itself. This is also a backward incompatible change, and it seems to complicate the situation for no obvious benefit. Right now, if someone complains that their icons all look like a yak, then we've got a pretty good idea that they're passing the wrong path to the icon constructor. With this change, it could be a yak, or a bee, or a custom icon, depending on whether they're in |
I guess this is a reference to your previous comment that "The default icons for running Python apps aren't consistent across platforms (and in some cases, quite bad), and don't give any indicator that the app is a [Python] (much less Toga) app." That's a fair point, but I'd like to find a way to define this that doesn't involve a series of convoluted conditions. If even I'm finding it confusing, then what chance does a user have? Here's the current documentation:
Here's a proposed new wording:
|
core/src/toga/icons.py
Outdated
specified, it will be ignored. | ||
class. This base filename should *not* contain an extension. If an extension | ||
is specified, it will be ignored. If :any:`None`, the application binary will | ||
be used as the source of the icon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Icon(None)
doesn't seem like a very obvious way of referring to the application icon. Also, it causes an inconsistency in all the other APIs that accept icons (Table, Button, etc), because path
and Icon(path)
mean the same thing, while None
and Icon(None)
would be different (no icon and the app icon, respectively).
Instead, how about adding a constant Icon.APP_ICON
, which is defined as:
- If the Python interpreter is embedded in an app, the icon of the application binary (if any platforms don't support this, they should be listed here).
- Otherwise, the same as DEFAULT_ICON.
And then the App.icon documentation could simply refer to that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comparison with Button/Table isn't entirely fair, because a Button or table item can have no icon at all; but an app must have an icon. However, I agree that Icon.APP_ICON
is a more helpful representation to use instead of None
; this also gives us a way to hide a sentinel value so that we can construct APP_ICON
without exposing a public constant or violate the type definition of the path
argument.
Modifies the behaviour of the toga App so that it is not necessary to package app icons with the app sources if code is being packaged as a bundled app.
When packaged as a macOS .app file, or Windows .exe, or a Linux system package, the packaging process involves distributing an icon. This icon is available at runtime, but isn't currently used by Toga. Instead, toga uses the convention that
resources/<app name>.*
will exist as resources that can be loaded.This requires duplicating the icon in two locations in the bundled app - and also generally means that a Windows app will generally include icons in macOS format, as the "sources" version of the icon will usually be common to all platforms.
This PR makes a small change to this behavior.
If an icon is explicitly provided as part of the app configuration, that icon name will be used.
If an icon is not explicitly provided,
resources/<app_name>
will be used.If
resources/<app_name>
cannot be found, andsys.executable
is notpython
,pythonX
, orpythonX.Y
(where X.Y is first 2 items from thesys.version_info
), Toga will load the icon that is associated withsys.executable
. This will be:../share
folder relative tosys.executable
whose app ID matches the current app.The the app icon cannot be found, or
sys.executable
ispython*
, the default Toga icon will be used.Android and iOS will always fall back to the default Toga icon, as the icon isn't visible (or modifiable) at runtime.
Apps that are standalone python scripts should see no change - they will identify
sys.executable
aspython*
, and fall back to the old behaviour.The only notable edge case is if
sys.executable
isn'tpython*
- in those cases, the underlying executable will still have an icon. (In fact - the fact that all executables have icons is the reason we can't usesys.executable
icon as the initial default, because python.exe has an icon on macOS and Windows).Default app icons will not generate a "cannot find icon" warning. An app will always have an icon; a warning is only generated if an icon is explicitly provided, and cannot be found.
As a result, an app no longer needs to include
resources/appname.*
files as part of the app data; the app's icon will be set based on the packaging version of the icon.To facilitate this change, some additional changes to icon handling have been made:
app.icon
is modified at runtime, this will be reflected in the app. It was previously a no-op.native_X
attribute to retrieve specific icon sizes has been replaced with anative(X)
method that will dynamically resize the best icon candidate.PR Checklist: