Skip to content
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.

Sign up for GitHub

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

Bugfix FXIOS-10756 App hangs while opening Tab Tray when there are many tabs open #23474

Merged
merged 7 commits into from
Dec 10, 2024
+28 −27
Merged

Bugfix FXIOS-10756 App hangs while opening Tab Tray when there are many tabs open #23474

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
619de16
Little optimization on tab tray when updating counter image
Nov 19, 2024
c84b9c7
Optimize TabDisplayPanel newState to avoid unnecessary view reload, F…
Nov 28, 2024
c46fc92
Refactor TabDisplayView to pipe collection view scroll after layout
Nov 29, 2024
4752f98
Merge branch 'main' into zazz/bugfix-tab-tray-opening-hangs
Nov 29, 2024
8d6d058
Refactor as per code review
Dec 5, 2024
c3ee9e5
Refactor state to use tabState
Dec 5, 2024
ec62f29
Merge branch 'main' into zazz/bugfix-tab-tray-opening-hangs
Dec 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions firefox-ios/Client/Frontend/Browser/SwipeAnimator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,13 +75,15 @@ extension SwipeAnimator {
// Calculate the edge to calculate distance from
let translation = velocity.x >= 0 ? animatingView.frame.width : -animatingView.frame.width
let timeStep = TimeInterval(abs(translation) / speed)
self.delegate?.swipeAnimator(self)
UIView.animate(
withDuration: timeStep,
animations: {
animatingView.transform = self.transformForTranslation(translation)
animatingView.alpha = self.alphaForDistanceFromCenter(abs(translation))
}, completion: { finished in
}, completion: { [weak self] finished in
if let self {
delegate?.swipeAnimator(self)
}
if finished {
animatingView.alpha = 0
}
Expand Down
16 changes: 8 additions & 8 deletions firefox-ios/Client/Frontend/Browser/Tabs/Views/TabCell.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ class TabCell: UICollectionViewCell, ThemeApplicable, ReusableCell {

private lazy var smallFaviconView: FaviconImageView = .build()
private lazy var favicon: FaviconImageView = .build()
private var headerView = UIVisualEffectView(effect: UIBlurEffect(style: .dark))
private lazy var headerView = UIVisualEffectView(effect: UIBlurEffect(style: .dark))

// MARK: - UI

Expand Down Expand Up @@ -82,8 +82,8 @@ class TabCell: UICollectionViewCell, ThemeApplicable, ReusableCell {

override init(frame: CGRect) {
super.init(frame: frame)
self.animator = SwipeAnimator(animatingView: self)
self.closeButton.addTarget(self, action: #selector(close), for: .touchUpInside)
animator = SwipeAnimator(animatingView: self)
closeButton.addTarget(self, action: #selector(close), for: .touchUpInside)

contentView.addSubview(backgroundHolder)

Expand All @@ -92,14 +92,14 @@ class TabCell: UICollectionViewCell, ThemeApplicable, ReusableCell {

accessibilityCustomActions = [
UIAccessibilityCustomAction(name: .TabTrayCloseAccessibilityCustomAction,
target: self.animator,
target: animator,
selector: #selector(SwipeAnimator.closeWithoutGesture))
]

headerView.translatesAutoresizingMaskIntoConstraints = false
headerView.contentView.addSubview(self.closeButton)
headerView.contentView.addSubview(self.titleText)
headerView.contentView.addSubview(self.favicon)
headerView.contentView.addSubview(closeButton)
headerView.contentView.addSubview(titleText)
headerView.contentView.addSubview(favicon)

setupConstraints()
}
Expand All @@ -112,7 +112,7 @@ class TabCell: UICollectionViewCell, ThemeApplicable, ReusableCell {
self.delegate = delegate

if let swipeAnimatorDelegate = delegate as? SwipeAnimatorDelegate {
self.animator?.delegate = swipeAnimatorDelegate
animator?.delegate = swipeAnimatorDelegate
}

animator?.animateBackToCenter()
Expand Down
7 changes: 4 additions & 3 deletions firefox-ios/Client/Frontend/Browser/Tabs/Views/TabDisplayPanelViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,6 @@ class TabDisplayPanelViewController: UIViewController,
super.viewWillAppear(animated)

if !viewHasAppeared {
tabDisplayView.layoutIfNeeded()
store.dispatch(TabPanelViewAction(panelType: panelType,
windowUUID: windowUUID,
actionType: TabPanelViewActionType.tabPanelWillAppear))
Expand Down Expand Up @@ -162,9 +161,11 @@ class TabDisplayPanelViewController: UIViewController,
}

func newState(state: TabsPanelState) {
if state != tabsState {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would this be cleaner if it was a guard? so, guard tabsState != state else { return } and do the stuff only then? Or does the shouldShowEmptyView need to get called every time?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes i agree i'd be much more cleaner. The thing also for the other states i don't know if comparing the state and applying changes for different state could cause any side effects.
I believe it should be safe to do the comparison at the top since the state should be Equatable what do you think ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, if we're comparing at the top of newState we should be fine, I think, because it's only doing this particular state. And yeah, everything should be Equatable state wise, so we should be safe.

But maybe I didn't understand the question. What do you mean by "also for the other states i don't know if comparing the state and applying changes for different state could cause any side effects"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry Rou i wasn't clear, i was referring to the other comment you wrote. So i'll go also for the TabTrayViewController.newState to have top comparison for the states

tabDisplayView.newState(state: state)
}
shouldShowEmptyView(state.isPrivateTabsEmpty)
tabsState = state
tabDisplayView.newState(state: tabsState)
shouldShowEmptyView(tabsState.isPrivateTabsEmpty)
}

// MARK: EmptyPrivateTabsViewDelegate
Expand Down
21 changes: 8 additions & 13 deletions firefox-ios/Client/Frontend/Browser/Tabs/Views/TabDisplayView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,19 +127,14 @@ class TabDisplayView: UIView,
private func scrollToTab(_ scrollState: TabsPanelState.ScrollState) {
let section: Int = scrollState.isInactiveTabSection ? 0 : 1
let indexPath = IndexPath(row: scrollState.toIndex, section: section)

// We cannot get the visible cells unless all layout operations have finished finished (e.g. after `reloadData()`)
collectionView.layoutIfNeeded()

// Only scroll to the view if it is not visible.
guard !collectionView.indexPathsForFullyVisibleItems.contains(indexPath) else { return }

// Scrolling to an invalid cell will crash the app, so check indexPath first
guard collectionView.isValid(indexPath: indexPath) else { return }

collectionView.scrollToItem(at: indexPath,
at: .centeredVertically,
animated: scrollState.withAnimation)
// Piping this into main thread let the collection view finish its layout process
DispatchQueue.main.async {
guard !self.collectionView.indexPathsForFullyVisibleItems.contains(indexPath) else { return }
guard self.collectionView.isValid(indexPath: indexPath) else { return }
self.collectionView.scrollToItem(at: indexPath,
at: .centeredVertically,
animated: scrollState.withAnimation)
}
}

private func configureDataSource() {
Expand Down
12 changes: 8 additions & 4 deletions firefox-ios/Client/Frontend/Browser/Tabs/Views/TabTrayViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,15 +291,19 @@ class TabTrayViewController: UIViewController,
}

func newState(state: TabTrayState) {
tabTrayState = state
updateTabCountImage(count: state.normalTabsCount)
defer {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to defer the state?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes true we can avoid it. I added it in case in future we want to end the newState method on some if branch, so we don't forget to apply the state, since with this implementation it must be set as last method operation.

if state.shouldDismiss {
  delegate.didFinish()
  // having defer would be nice here so we don't forget applying the state
  return
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so, I'm confused here. We're not doing a check if the states are the same or not? what does defer get us vs setting tabTrayState = state at the beginning, and then updating, given that we're just basically update off state here??

Copy link
Collaborator Author

@FilippoZazzeroni FilippoZazzeroni Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting the state as first thing invalidates the checks because then tabTrayState will be always equal to state. Yes here the only state comparison is done on updateTabCountImage but yes you are right @adudenamedruby. i didn't implement the guard at the top cause i wasn't sure about any side effects it could cause. But i think that is the best idea

tabTrayState = state
}
if state.normalTabsCount != tabTrayState.normalTabsCount {
updateTabCountImage(count: state.normalTabsCount)
}
segmentedControl.selectedSegmentIndex = state.selectedPanel.rawValue

if tabTrayState.shouldDismiss {
if state.shouldDismiss {
delegate?.didFinish()
}

if tabTrayState.showCloseConfirmation {
if state.showCloseConfirmation {
showCloseAllConfirmation()
}

Expand Down