Repository navigation
Conversation
The package had no tests, so fixes could not be proven by a test that fails on the old code and passes on the new one. The shared helper renders a view into pixel data, which lets view-level behavior be compared without a host app.
The setter of `!binding` wrote the new value unchanged into the original binding, so toggling a control bound to `!$isOff` stored the opposite of what the user chose.
The method used the .jpeg2000 file type, so callers expecting a JPEG received a JPEG 2000 file that many consumers (browsers, upload APIs) cannot read.
CustomSymbolConvertible allows metadata after `:` or `*` (rotation and repeat count used by HPicker and VPicker) and documents that it is ignored when creating images. Image(convertible:) and Label(convertible:) passed the raw name though, so such symbols showed no icon at all, also in the CaseIterable Picker initializer and SideTabView that build on them.
The style always rendered the content in the secondary style, so `.horizontal(muteContent: false)` had no effect.
When a download returned no data or data that is not an image, the completion handler returned early and isLoading stayed true, so the progress view spun forever. The state after a download is now decided by a small pure function that ends loading in both cases.
SwiftUI calls updateUIView/updateNSView on every state change of the surrounding view. WebView loaded its URL in there each time, which threw away pages the user had navigated to and reset the scroll position. The coordinator now remembers the loaded URL and also keeps its parent current, so a changed onOpenExternalURL closure is used.
CaseIterable only requires AllCases to be a Collection, so a type with a non-Array AllCases crashed the CaseIterable initializers with a failed `as! [T]` cast. Array(T.allCases) works for every collection.
"None" in SearchableGridPicker and "Done" in the macOS sheet of MultiSelector were looked up in the app's main bundle, so they stayed English although the package's catalog has them in 37 languages. They now use bundle: .module. The empty search result showed "No emojis match", which is wrong when the picker lists SF Symbols, and was not localized either. It now uses the system's ContentUnavailableView.search(text:), which is available wherever SearchableGridPicker is and uses Apple's own wording in every language. The two catalog keys it replaces are removed as unused.
The native SwiftPM build system of Xcode 26 copies Localizable.xcstrings without compiling it into .lproj folders, so the new localization tests found no languages on the runner. The swiftbuild build system compiles the catalog the way Xcode does.
toHex read the components from cgColor, which is nil for dynamic colors like .primary or .systemBlue and has only two components for colors in a gray color space, so it returned nil for all of them. It now reads sRGB components through UIColor/NSColor, which resolves dynamic colors against the current appearance, and clamps wide-gamut components to the sRGB range hex can represent.
change(.opacity, …) converted the color to HLC and back just to set the opacity, which shifted the RGB components by up to about 1e-6 (e.g. green 0 became 1.7e-6). Opacity is the same in every color space, so it now goes through the RGB path. The never reached .opacity branches of the HSB and HLC helpers are removed.
highlightMatchingTokenizedPrefixes searched a folded copy of the string and applied the found range to the original. Folding changes the length of characters like "é" or full-width letters, so with "Café Olé" and the search "ole" it highlighted " Ol" instead of "Olé". It now searches the original string with the folding as comparison options, passes the locale on to the tokenization, and drops the @available attribute that only repeated the package's minimum versions.
The documentation said the model "exists in the database", but the property only checks whether it was inserted into a ModelContext, which is true before the context is saved. It now says so and points to persistentModelID.isTemporary for the saved state.
The initializer used HandyRegex, which HandySwift deprecated, and joined the parts with Text's + operator, which is deprecated starting with iOS 26. It now uses a Swift Regex literal and interpolates the parts into one text. The rendering stays the same, which tests compare pixel by pixel against texts concatenated by hand.
The four #warning directives were notes for the package itself, but every app depending on HandySwiftUI saw them as build warnings.
CachedAsyncImage and LimitedTextField used onChange(of:perform:), which is deprecated starting with iOS 17 and from the start on visionOS, so every visionOS build warned. Its replacement needs iOS 17, above the package minimum, so a small internal helper picks the variant that fits the running OS.
Jeehut
changed the title
After a failed download the view was back in its initial state, which looks exactly like a load that never started. The loading now lives in a small loader object with explicit idle, loading, loaded and failed phases: a failure shows a neutral placeholder and is not retried automatically, only when the view appears again or the URL changes (.task(id:), which also cancels the download when the view disappears). The tests run the loader itself with stubbed downloads.
The tests now call the representable's update method with a web view that records load requests, so they cover the loaded-URL bookkeeping and the delegate and parent updates, not only the comparison. The suite runs on the main actor, which removes the isolation warning.
The localization test hard-coded 37 languages. It now reads the languages from the catalog file and compares them with the compiled bundle, so adding a language needs no test change.
Text(format:) looks up its "%@%@…" key in the module bundle on purpose, which a comment now states so that the key never gets a catalog entry. toHex resolves dynamic colors against the traits current at the time of the call, which outside of trait-aware callbacks are usually the default ones; the docs now say so instead of "current appearance of the app".
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
This PR fixes bugs in existing helpers: eight from a first pass, plus a second set listed under "Further fixes". It also adds the package's first test target (Swift Testing), and every fix comes with a test that fails on the code from
mainand passes on this branch. One commit per fix.mainprefix func !onBinding<Bool>$0unchanged, soToggle(isOn: !$isOff)stored the opposite of the user's choice!$0NSImage.jpegData(compressionQuality:).jpeg2000.jpegImage(convertible:),Label(convertible:)symbolName, including the:angle*countmetadata the protocol documents as ignored, which is not a valid symbol name. Also affected theCaseIterablePickerinitializer andSideTabViewsymbolSystemNamelikeCustomLabelConvertible.labelalready didHorizontalLabeledContentStylemuteContentwas ignored; content was always.secondary.secondaryonly whenmuteContentistrueCachedAsyncImageLoaderwith explicitidle/loading/loaded/failedphases. A failure shows a neutralphotoplaceholder and is not retried automatically, only when the view appears again or the URL changes (.task(id:), which also cancels the download on disappear)WebViewupdateUIView/updateNSView, i.e. on any state change of the parent view, which reset navigation and scroll positionparent, so a newonOpenExternalURLclosure takes effectHPicker,VPickerT.allCases as! [T]crashed forCaseIterabletypes whoseAllCasesis not anArrayArray(T.allCases)SearchableGridPicker,MultiSelectorbundle: .module(both keys already had all 37 translations). The empty state is nowContentUnavailableView.search(text:)(iOS 17, macOS 14, tvOS 17, watchOS 10, the same availability asSearchableGridPicker), which is neutral about the option type and uses Apple's wording in every language. The two catalog keys it replaces ("No matches", "No emojis match '%@'") are removed as unusedFor 4 and 8, rendered pixels are compared via
ImageRenderer. 5 and 6 are tested through their real code paths (see "Review follow-up").How this was verified
To produce the red runs, a script reverted exactly the fixed lines to their
mainbehavior: for 5 and 6, the extracted functions returned what the old code did ("stay loading" on failure, "always load"). Tests were then run withswift test.Red (fixes reverted,
swift test --skip PickerTests):The tests for 5 and 6 and the language count in this run were later replaced by tests of the real code paths, see "Review follow-up".
The passing tests in the red run cover behavior that was already right on
main(muted content, successful downloads, first load and URL change). They guard the fixes against regressions. The two catalog tests are green on both sides because the keys were never missing; only thebundle:argument was. The 36 issues per localization test are every catalog language excepten-GB, whose value equals the English source.Red for the picker fix (separate run, since the old code crashes the test process):
Green (this branch):
The localization tests check that the text no longer renders as the English source under each language's locale. They do not compare pixels with the expected translation, because localized texts pick language-specific glyphs (Chinese, Bulgarian) that a verbatim reference text does not get. That made an exact comparison fail for those languages even with the right string.
Builds:
swift build(macOS) andxcodebuildfor generic iOS, tvOS and visionOS. Their deduplicated diagnostics (file + message) were identical tomain's: 8, 12, 11 and 14 entries, no new warnings. The test target compiles without warnings. watchOS fails to build onmainalready ('UIApplication' is unavailable in watchOS,GroupBox, the semanticColorstatics, …), and this branch neither fixes nor adds to that. The string catalog check of the internal localization tooling exited 0 with 20 keys × 37 languages and nothing open.Further fixes
mainColor.toHex(alpha:)cgColor, which isnilfor dynamic colors (.primary,.systemBlue, …) and has two components in a gray color space, so it returnednilfor bothUIColor/NSColor, which resolve dynamic colors against the current appearance, and clamps wide-gamut values to sRGB. The doc comment now says the result has no leading#Color.change(.opacity, …).opacitycases of the HSB and HLC helpers were never reachedhighlightMatchingTokenizedPrefixes" Ol"localeon to the tokenization, and drops the@availablethat only repeated the package minimumPersistentModel.isPersistedtrueright afterinsert(_:)persistentModelID.isTemporary(OS 27) for the saved stateText(format:partialStyling:)HandyRegexand Text's+, which is deprecated starting with iOS 26Text. Rendering is unchangedCachedAsyncImage,LimitedTextFieldonChange(of:perform:)warned on every visionOS build (deprecated from visionOS 1.0)onValueChange(of:perform:)uses the two-parameteronChangewhere available#warnings (ViewExt,CachedAsyncImage,Emoji,SFSymbol)// TODO:commentsNot changed: the
@StateinPulsatingButtonStyleis not a bug. A probe style with a@Stateflag that is set inonAppearwas hosted in anNSHostingView, andmakeBodywas called again with the updated value (rendered flags: [false, true]), so SwiftUI installs the state of button styles.How the further fixes were verified
Red (the sources of this second set reverted to the previous commit of this branch, tests kept):
The five
TextFormatTestspass on the old and the new implementation on purpose: they compareText(format:)pixel by pixel with texts concatenated by hand, and the old implementation passing them shows the rendering did not change. That they can fail at all was checked by temporarily dropping the styling of paired tags, which madepairedTags,unknownAndMismatchedTagsStayLiteralandtextWithPercentSignsfail.Green, the whole suite on this branch:
Builds: compared with
main, the deduplicated diagnostics now only lose entries. On macOS, iOS and tvOS the twoHandyRegexdeprecations and the four#warnings are gone; visionOS also loses the twoonChange(of:perform:)deprecations. The two remaining warnings on every platform are the unused throwingTasks inAsyncButtonandWebView, which this PR does not touch:Combined check:
mainwith this PR, #6 and #8 merged locally builds for watchOS, iOS and visionOS and passes all 34 tests of the combined suites.Review follow-up
CachedAsyncImage: the first fix ended the loading state on failure, but that state then looked exactly like "never started". Failure is now its own phase (see the table).CachedAsyncImageTestsrun the realLoaderwith stubbed downloads: a thrown error and non-image data both end in.failed, and a successful load ends in.loadedand is served from the cache on the next load (one download for two loads).WebView:WebViewTestscall the representable's realupdate(_:coordinator:)with aWKWebViewsubclass that records load requests. Three updates with the same URL load once, and a URL change loads the new URL once and updates the coordinator'sparent. The suite runs on the main actor, and the test target compiles without warnings. The tests have to writeHandySwiftUI.WebViewbecause they import WebKit, the same ambiguity Deprecate WebView and add an environment-aware Color.rgbo(in:) #6 documents.Localizable.xcstringsand compared with the compiled bundle, instead of a hard-coded 37.Text(format:)has a comment that its%@%@…key is looked up in the module bundle on purpose and must never get a catalog entry.toHexnow says that dynamic colors resolve against the traits current at the time of the call (UITraitCollection.current, usually default traits outside of trait-aware callbacks;NSAppearance.currentDrawing()on macOS), and points to resolving in the view's environment first.Red runs with the OLD code restored.
update(_:coordinator:)got back the exact body ofmain'supdateNSView(load, then set the delegate). The loader gotmain's failure behavior (return without changing state, i.e. stay loading) and, in a second run, the previous commit's behavior (back to idle):CI
The workflow now runs
swift buildandswift testwith--build-system swiftbuild. On the macos-26 runner (Xcode 26), the native build system copiesLocalizable.xcstringswithout compiling it into.lprojfolders, so the localization tests found no languages there; swiftbuild compiles the catalog the way Xcode does.