Skip to content

Commit

Permalink
Fix invalid state transition when only isLoading is changed
Browse files Browse the repository at this point in the history
  • Loading branch information
dus7 committed Nov 20, 2024
1 parent b8f782f commit 5f65053
Show file tree
Hide file tree
Showing 4 changed files with 138 additions and 9 deletions.
4 changes: 4 additions & 0 deletions DuckDuckGo.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -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 */; };
Expand Down Expand Up @@ -1623,6 +1624,7 @@
6F64AA5E2C49463C00CF4489 /* ShortcutsModel.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ShortcutsModel.swift; sourceTree = "<group>"; };
6F655BE12BAB289E00AC3597 /* DefaultTheme.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = DefaultTheme.swift; sourceTree = "<group>"; };
6F691CC92C4979EC002E9553 /* FavoritesTooltip.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = FavoritesTooltip.swift; sourceTree = "<group>"; };
6F7BACD32CEE084100F561D8 /* OmniBarEqualityCheckTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = OmniBarEqualityCheckTests.swift; sourceTree = "<group>"; };
6F7FB8DF2C660B1A00867DA7 /* NewTabPageFavoritesModelTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = NewTabPageFavoritesModelTests.swift; sourceTree = "<group>"; };
6F7FB8E22C660BF300867DA7 /* DailyPixelFiring.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = DailyPixelFiring.swift; sourceTree = "<group>"; };
6F7FB8E42C66158D00867DA7 /* NewTabPageShortcutsSettingsModelTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = NewTabPageShortcutsSettingsModelTests.swift; sourceTree = "<group>"; };
Expand Down Expand Up @@ -6348,6 +6350,7 @@
isa = PBXGroup;
children = (
6F3529FE2CDCEDF700A59170 /* OmniBarLoadingStateBearerTests.swift */,
6F7BACD32CEE084100F561D8 /* OmniBarEqualityCheckTests.swift */,
BBFF18B02C76448100C48D7D /* QuerySubmittedTests.swift */,
8588026424E4209900C24AB6 /* LargeOmniBarStateTests.swift */,
85F20005221702F7006BB258 /* AddressDisplayHelperTests.swift */,
Expand Down Expand Up @@ -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 */,
Expand Down
14 changes: 9 additions & 5 deletions DuckDuckGo/OmniBar.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
16 changes: 12 additions & 4 deletions DuckDuckGo/OmniBarState.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand Down Expand Up @@ -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 {
Expand Down
113 changes: 113 additions & 0 deletions DuckDuckGoTests/OmniBarEqualityCheckTests.swift
Original file line number Diff line number Diff line change
@@ -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
}
}

0 comments on commit 5f65053

Please sign in to comment.