From 5f65053a9d83f6a8f6019cfba15691633fbec922 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mariusz=20=C5=9Apiewak?= Date: Wed, 20 Nov 2024 13:29:26 +0100 Subject: [PATCH] Fix invalid state transition when only isLoading is changed --- DuckDuckGo.xcodeproj/project.pbxproj | 4 + DuckDuckGo/OmniBar.swift | 14 ++- DuckDuckGo/OmniBarState.swift | 16 ++- .../OmniBarEqualityCheckTests.swift | 113 ++++++++++++++++++ 4 files changed, 138 insertions(+), 9 deletions(-) create mode 100644 DuckDuckGoTests/OmniBarEqualityCheckTests.swift diff --git a/DuckDuckGo.xcodeproj/project.pbxproj b/DuckDuckGo.xcodeproj/project.pbxproj index 3506b3cf5f..234210beb9 100644 --- a/DuckDuckGo.xcodeproj/project.pbxproj +++ b/DuckDuckGo.xcodeproj/project.pbxproj @@ -315,6 +315,7 @@ 6F64AA5F2C49463C00CF4489 /* ShortcutsModel.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6F64AA5E2C49463C00CF4489 /* ShortcutsModel.swift */; }; 6F655BE22BAB289E00AC3597 /* DefaultTheme.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6F655BE12BAB289E00AC3597 /* DefaultTheme.swift */; }; 6F691CCA2C4979EC002E9553 /* FavoritesTooltip.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6F691CC92C4979EC002E9553 /* FavoritesTooltip.swift */; }; + 6F7BACD42CEE084B00F561D8 /* OmniBarEqualityCheckTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6F7BACD32CEE084100F561D8 /* OmniBarEqualityCheckTests.swift */; }; 6F7FB8E12C660B3E00867DA7 /* NewTabPageFavoritesModelTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6F7FB8DF2C660B1A00867DA7 /* NewTabPageFavoritesModelTests.swift */; }; 6F7FB8E32C660BF300867DA7 /* DailyPixelFiring.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6F7FB8E22C660BF300867DA7 /* DailyPixelFiring.swift */; }; 6F7FB8E52C66158D00867DA7 /* NewTabPageShortcutsSettingsModelTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6F7FB8E42C66158D00867DA7 /* NewTabPageShortcutsSettingsModelTests.swift */; }; @@ -1623,6 +1624,7 @@ 6F64AA5E2C49463C00CF4489 /* ShortcutsModel.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ShortcutsModel.swift; sourceTree = ""; }; 6F655BE12BAB289E00AC3597 /* DefaultTheme.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = DefaultTheme.swift; sourceTree = ""; }; 6F691CC92C4979EC002E9553 /* FavoritesTooltip.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = FavoritesTooltip.swift; sourceTree = ""; }; + 6F7BACD32CEE084100F561D8 /* OmniBarEqualityCheckTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = OmniBarEqualityCheckTests.swift; sourceTree = ""; }; 6F7FB8DF2C660B1A00867DA7 /* NewTabPageFavoritesModelTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = NewTabPageFavoritesModelTests.swift; sourceTree = ""; }; 6F7FB8E22C660BF300867DA7 /* DailyPixelFiring.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = DailyPixelFiring.swift; sourceTree = ""; }; 6F7FB8E42C66158D00867DA7 /* NewTabPageShortcutsSettingsModelTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = NewTabPageShortcutsSettingsModelTests.swift; sourceTree = ""; }; @@ -6348,6 +6350,7 @@ isa = PBXGroup; children = ( 6F3529FE2CDCEDF700A59170 /* OmniBarLoadingStateBearerTests.swift */, + 6F7BACD32CEE084100F561D8 /* OmniBarEqualityCheckTests.swift */, BBFF18B02C76448100C48D7D /* QuerySubmittedTests.swift */, 8588026424E4209900C24AB6 /* LargeOmniBarStateTests.swift */, 85F20005221702F7006BB258 /* AddressDisplayHelperTests.swift */, @@ -8090,6 +8093,7 @@ 85D2187224BF24F2004373D2 /* NotFoundCachingDownloaderTests.swift in Sources */, C1935A242C89CC6D001AD72D /* AutofillHeaderViewFactoryTests.swift in Sources */, C111B26927F579EF006558B1 /* BookmarkOrFolderTests.swift in Sources */, + 6F7BACD42CEE084B00F561D8 /* OmniBarEqualityCheckTests.swift in Sources */, 6F7FB8E72C66197E00867DA7 /* NewTabPageSectionsSettingsModelTests.swift in Sources */, 851CD674244D7E6000331B98 /* UserDefaultsExtension.swift in Sources */, 569437362BE5160600C0881B /* SyncSettingsViewControllerErrorTests.swift in Sources */, diff --git a/DuckDuckGo/OmniBar.swift b/DuckDuckGo/OmniBar.swift index 91a1214bd4..fda9757981 100644 --- a/DuckDuckGo/OmniBar.swift +++ b/DuckDuckGo/OmniBar.swift @@ -360,15 +360,19 @@ class OmniBar: UIView { } fileprivate func refreshState(_ newState: any OmniBarState) { - if !newState.isEquivalent(to: state) { + if state.requiresUpdate(transitioningInto: newState) { Logger.general.debug("OmniBar entering \(newState.description) from \(self.state.description)") - if newState.clearTextOnStart { - clear() + + if state.isDifferentState(than: newState) { + if newState.clearTextOnStart { + clear() + } + cancelAllAnimations() } + state = newState - cancelAllAnimations() } - + searchFieldContainer.adjustTextFieldOffset(for: state) setVisibility(privacyInfoContainer, hidden: !state.showPrivacyIcon) diff --git a/DuckDuckGo/OmniBarState.swift b/DuckDuckGo/OmniBarState.swift index 3e486652c5..6dbba113b3 100644 --- a/DuckDuckGo/OmniBarState.swift +++ b/DuckDuckGo/OmniBarState.swift @@ -29,7 +29,7 @@ protocol OmniBarState: CustomStringConvertible { var showForwardButton: Bool { get } var showBookmarksButton: Bool { get } var showShareButton: Bool { get } - + var clearTextOnStart: Bool { get } var allowsTrackersAnimation: Bool { get } var showSearchLoupe: Bool { get } @@ -61,12 +61,20 @@ protocol OmniBarState: CustomStringConvertible { func withLoading() -> Self func withoutLoading() -> Self - func isEquivalent(to other: OmniBarState) -> Bool + func requiresUpdate(transitioningInto other: OmniBarState) -> Bool + func isDifferentState(than other: OmniBarState) -> Bool } extension OmniBarState { - func isEquivalent(to other: OmniBarState) -> Bool { - name == other.name && isLoading == other.isLoading + /// Returns if new state requires UI update + func requiresUpdate(transitioningInto other: OmniBarState) -> Bool { + name != other.name || isLoading != other.isLoading + } + + /// Checks whether the state type is different. + /// If `true` it may require transitioning to a different appearance and/or cancelling pending animations. + func isDifferentState(than other: OmniBarState) -> Bool { + name != other.name } var description: String { diff --git a/DuckDuckGoTests/OmniBarEqualityCheckTests.swift b/DuckDuckGoTests/OmniBarEqualityCheckTests.swift new file mode 100644 index 0000000000..c10e468259 --- /dev/null +++ b/DuckDuckGoTests/OmniBarEqualityCheckTests.swift @@ -0,0 +1,113 @@ +// +// OmniBarEqualityCheckTests.swift +// DuckDuckGo +// +// Copyright © 2024 DuckDuckGo. All rights reserved. +// +// 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. +// + +import XCTest +@testable import DuckDuckGo + +final class OmniBarEqualityCheckTests: XCTestCase { + func testRequiresUpdateChecksForIsLoading() { + let loadingOmniBarState = DummyOmniBarState(isLoading: true) + let notLoadingOmniBarState = DummyOmniBarState(isLoading: false) + + XCTAssertTrue(loadingOmniBarState.requiresUpdate(transitioningInto: notLoadingOmniBarState)) + } + + func testRequiresUpdateChecksForName() { + let fooOmniBarState = DummyOmniBarState(name: "foo") + let barOmniBarState = DummyOmniBarState(name: "bar") + + XCTAssertTrue(fooOmniBarState.requiresUpdate(transitioningInto: barOmniBarState)) + } + + func testIsDifferentStateChecksForName() { + let fooOmniBarState = DummyOmniBarState(name: "foo") + let barOmniBarState = DummyOmniBarState(name: "bar") + + XCTAssertTrue(fooOmniBarState.isDifferentState(than: barOmniBarState)) + } + + func testIsDifferentStateIgnoresOtherProperties() { + let fooOmniBarState = DummyOmniBarState() + var barOmniBarState = DummyOmniBarState() + + barOmniBarState.hasLargeWidth = !fooOmniBarState.hasLargeWidth + barOmniBarState.showBackButton = !fooOmniBarState.showBackButton + barOmniBarState.showForwardButton = !fooOmniBarState.showForwardButton + barOmniBarState.showBookmarksButton = !fooOmniBarState.showBookmarksButton + barOmniBarState.showShareButton = !fooOmniBarState.showShareButton + barOmniBarState.clearTextOnStart = !fooOmniBarState.clearTextOnStart + barOmniBarState.allowsTrackersAnimation = !fooOmniBarState.allowsTrackersAnimation + barOmniBarState.showSearchLoupe = !fooOmniBarState.showSearchLoupe + barOmniBarState.showCancel = !fooOmniBarState.showCancel + barOmniBarState.showPrivacyIcon = !fooOmniBarState.showPrivacyIcon + barOmniBarState.showBackground = !fooOmniBarState.showBackground + barOmniBarState.showClear = !fooOmniBarState.showClear + barOmniBarState.showRefresh = !fooOmniBarState.showRefresh + barOmniBarState.showMenu = !fooOmniBarState.showMenu + barOmniBarState.showSettings = !fooOmniBarState.showSettings + barOmniBarState.showVoiceSearch = !fooOmniBarState.showVoiceSearch + barOmniBarState.showAbort = !fooOmniBarState.showAbort + + XCTAssertFalse(fooOmniBarState.isDifferentState(than: barOmniBarState)) + } +} + +private struct DummyOmniBarState: OmniBarState, OmniBarLoadingBearerStateCreating { + var name: String + var isLoading: Bool + var voiceSearchHelper: VoiceSearchHelperProtocol + + var hasLargeWidth = false + var showBackButton = false + var showForwardButton = false + var showBookmarksButton = false + var showShareButton = false + var clearTextOnStart = false + var allowsTrackersAnimation = false + var showSearchLoupe = false + var showCancel = false + var showPrivacyIcon = false + var showBackground = false + var showClear = false + var showRefresh = false + var showMenu = false + var showSettings = false + var showVoiceSearch = false + var showAbort = false + + var onEditingStoppedState: OmniBarState { DummyOmniBarState() } + var onEditingStartedState: OmniBarState { DummyOmniBarState() } + var onTextClearedState: OmniBarState { DummyOmniBarState() } + var onTextEnteredState: OmniBarState { DummyOmniBarState() } + var onBrowsingStartedState: OmniBarState { DummyOmniBarState() } + var onBrowsingStoppedState: OmniBarState { DummyOmniBarState() } + var onEnterPhoneState: OmniBarState { DummyOmniBarState() } + var onEnterPadState: OmniBarState { DummyOmniBarState() } + var onReloadState: OmniBarState { DummyOmniBarState() } + + init(voiceSearchHelper: VoiceSearchHelperProtocol, isLoading: Bool) { + self.init(isLoading: isLoading, voiceSearchHelper: voiceSearchHelper) + } + + init(name: String = "DummyOmniBarState", isLoading: Bool = false, voiceSearchHelper: VoiceSearchHelperProtocol = MockVoiceSearchHelper()) { + self.name = name + self.isLoading = isLoading + self.voiceSearchHelper = voiceSearchHelper + } +}