Fix truncated icon #2489
Fix truncated icon #2489
Conversation
freakboy3742
left a comment
freakboy3742
left a comment
There was a problem hiding this comment.
Thanks for the PR.
Regarding the test failures - the test failure is exactly what it says on the tin. The test is looking for a specific piece of text - and that text doesn't occur in the actual output. It's almost there... but there's a missing period.
However, in this case, I think the implementation isn't in quite the right place. As flagged inline, the current wording is a hard error, not a soft warning. The AppConfig object is a good place for hard errors - i.e., you can't use a config at all unless the version number is valid, for example. In this case, we can proceed - but we need to warn the user that the description will result in potentially unexpected behavior.
I'd suggest a better approach would be to use the finalize_app_config step - that's a step that will be run once, in a context where you have access to the console, so you can output a warning, rather than a hard error.
src/briefcase/config.py
Outdated
| def is_valid_description(description): | ||
| """Determine if the description is not longer than 80 characters.""" | ||
| return len(description) <= 80 |
There was a problem hiding this comment.
As there's only one place where this logic is needed, and the logic is fundamentally pretty simple, factoring this out as a method is slightly overkill.
src/briefcase/config.py
Outdated
| f"The description is {len(self.description)} characters long.\n" | ||
| "Longer descriptions should use the 'long_description' field.\n" | ||
| "On some platforms, descriptions may be truncated to fit platform limitations.", | ||
| ) |
There was a problem hiding this comment.
There's a slight discrepancy here between the message and the way it's handled. The message suggests the content "may" be truncated; however, it will be impossible to reach that point, because any description over 80 characters will raise a hard error.
|
Thanks for the feedback, I shall try and re-implement |
bigsby-exe
force-pushed
the
fix-truncated-icon
branch
from
fbcc044 to
a40adea
Compare
|
@freakboy3742 is this a better implementation? |
The approach is good; however, I'd be inclined to make it a check on the create command that applies for all platforms, rather than something specific to the Windows backend. While it only has immediate consequences for Windows, someone who is writing an app for Linux/macOS/whatever should be warned that future ports to Windows might be a problem. As a heads up - the test failure isn't of your making; overnight, OpenSUSE has made a change to their repository that has broken Linux app support. We're still investigating the cause to see if it's something we need to work around, or if it's a case where they've messed up an update (which is something that happens way more often than it should with OpenSUSE). |
|
The OpenSUSE issue has now been resolved. |
Thanks again for the feedback. That all makes sense so I will try and implement it on create for all platforms. |
3 participants
This PR adds a warning message for descriptions over 80 chars this is due to an issue packaging on Windows where the icon was getting truncated.
This is part of 3 PRs to update both the Windows app template and VisualStudio template to truncate descriptions over 256 chars & add a warning to briefcase
beeware/briefcase-windows-VisualStudio-template#68
beeware/briefcase-windows-app-template#72
This resolves issue: #2465
Unsure if I am doing something wrong but I can't get all the tests to pass even on a clean clone of the main repo.
I have managed to reduce some of the failures by adding tests for the extra checks I have added
PR Checklist: