Skip to content

Refactor FXIOS-7301 - Remove 2 closure_body_length violations from MainMenuMiddleware.swift (Firefox) and InternalTelemetrySettingsView.swift (Focus) #26242

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.

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

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions .swiftlint.yml
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,8 @@ line_length:
ignores_interpolated_strings: true

closure_body_length:
warning: 43
error: 43
warning: 42
error: 42

file_header:
required_string: |
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,65 +54,117 @@ final class MainMenuMiddleware {
guard let action = action as? MainMenuAction else { return }
let isHomepage = action.telemetryInfo?.isHomepage ?? false

switch action.actionType {
case MainMenuActionType.tapNavigateToDestination:
self.handleTapNavigateToDestinationAction(action: action, isHomepage: isHomepage)
if self.handleMainMenuActionType(action: action, isHomepage: isHomepage) { return }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we change this to instead of checking if the method handles the action, to check for the action type, like we did in other cases.

if let action = action as? GeneralBrowserAction {
 handleGeneralBrowserAction()
}

With this we can avoid also returning a boolean to check if the action was handled. What do you think @ionixjunior let me know if it makes sense what i said, thanks 😃

if self.handleGeneralBrowserActionType(action: action) { return }
if self.handleMainMenuDetailsActionType(action: action, isHomepage: isHomepage) { return }
}

private func handleMainMenuActionType(action: MainMenuAction, isHomepage: Bool) -> Bool {
guard let mainMenuActionType = action.actionType as? MainMenuActionType else {
return false
}

case MainMenuActionType.tapShowDetailsView:
self.handleTapShowDetailsViewAction(action: action, isHomepage: isHomepage)
switch mainMenuActionType {
case .tapNavigateToDestination:
handleTapNavigateToDestinationAction(action: action, isHomepage: isHomepage)
return true

case MainMenuActionType.tapToggleUserAgent:
self.handleTapToggleUserAgentAction(action: action, isHomepage: isHomepage)
case .tapShowDetailsView:
handleTapShowDetailsViewAction(action: action, isHomepage: isHomepage)
return true

case MainMenuActionType.tapCloseMenu:
self.telemetry.closeButtonTapped(isHomepage: isHomepage)
case .tapToggleUserAgent:
handleTapToggleUserAgentAction(action: action, isHomepage: isHomepage)
return true

case GeneralBrowserActionType.showReaderMode:
self.handleShowReaderModeAction(action: action)
case .tapCloseMenu:
telemetry.closeButtonTapped(isHomepage: isHomepage)
return true

case MainMenuActionType.didInstantiateView:
self.handleDidInstantiateViewAction(action: action)
case .didInstantiateView:
handleDidInstantiateViewAction(action: action)
return true

case MainMenuActionType.viewDidLoad:
self.handleViewDidLoadAction(action: action)
case .viewDidLoad:
handleViewDidLoadAction(action: action)
return true

case MainMenuActionType.menuDismissed:
self.telemetry.menuDismissed(isHomepage: isHomepage)
case .menuDismissed:
telemetry.menuDismissed(isHomepage: isHomepage)
return true

case MainMenuDetailsActionType.tapZoom:
self.telemetry.toolsSubmenuOptionTapped(with: isHomepage, and: TelemetryAction.zoom)
default:
return false
}
}

case MainMenuDetailsActionType.tapReportBrokenSite:
self.telemetry.toolsSubmenuOptionTapped(with: isHomepage, and: TelemetryAction.reportBrokenSite)
private func handleGeneralBrowserActionType(action: MainMenuAction) -> Bool {
guard let generalBrowserActionType = action.actionType as? GeneralBrowserActionType else {
return false
}

case MainMenuDetailsActionType.tapAddToBookmarks:
self.telemetry.saveSubmenuOptionTapped(with: isHomepage, and: TelemetryAction.bookmarkThisPage)
switch generalBrowserActionType {
case .showReaderMode:
handleShowReaderModeAction(action: action)
return true

case MainMenuDetailsActionType.tapEditBookmark:
self.telemetry.saveSubmenuOptionTapped(with: isHomepage, and: TelemetryAction.editBookmark)
default:
return false
}
}

private func handleMainMenuDetailsActionType(action: MainMenuAction, isHomepage: Bool) -> Bool {
guard let mainMenuDetailsActionType = action.actionType as? MainMenuDetailsActionType else {
return false
}

switch mainMenuDetailsActionType {
case .tapZoom:
telemetry.toolsSubmenuOptionTapped(with: isHomepage, and: TelemetryAction.zoom)
return true

case .tapReportBrokenSite:
telemetry.toolsSubmenuOptionTapped(with: isHomepage, and: TelemetryAction.reportBrokenSite)
return true

case .tapAddToBookmarks:
telemetry.saveSubmenuOptionTapped(with: isHomepage, and: TelemetryAction.bookmarkThisPage)
return true

case .tapEditBookmark:
telemetry.saveSubmenuOptionTapped(with: isHomepage, and: TelemetryAction.editBookmark)
return true

case MainMenuDetailsActionType.tapAddToShortcuts:
self.telemetry.saveSubmenuOptionTapped(with: isHomepage, and: TelemetryAction.addToShortcuts)
case .tapAddToShortcuts:
telemetry.saveSubmenuOptionTapped(with: isHomepage, and: TelemetryAction.addToShortcuts)
return true

case MainMenuDetailsActionType.tapRemoveFromShortcuts:
self.telemetry.saveSubmenuOptionTapped(with: isHomepage, and: TelemetryAction.removeFromShortcuts)
case .tapRemoveFromShortcuts:
telemetry.saveSubmenuOptionTapped(with: isHomepage, and: TelemetryAction.removeFromShortcuts)
return true

case MainMenuDetailsActionType.tapAddToReadingList:
self.telemetry.saveSubmenuOptionTapped(with: isHomepage, and: TelemetryAction.saveToReadingList)
case .tapAddToReadingList:
telemetry.saveSubmenuOptionTapped(with: isHomepage, and: TelemetryAction.saveToReadingList)
return true

case MainMenuDetailsActionType.tapRemoveFromReadingList:
self.telemetry.saveSubmenuOptionTapped(with: isHomepage, and: TelemetryAction.removeFromReadingList)
case .tapRemoveFromReadingList:
telemetry.saveSubmenuOptionTapped(with: isHomepage, and: TelemetryAction.removeFromReadingList)
return true

case MainMenuDetailsActionType.tapToggleNightMode:
self.handleTapToggleNightModeAction(action: action, isHomepage: isHomepage)
case .tapToggleNightMode:
handleTapToggleNightModeAction(action: action, isHomepage: isHomepage)
return true

case MainMenuDetailsActionType.tapBackToMainMenu:
self.handleTapBackToMainMenuAction(action: action, isHomepage: isHomepage)
case .tapBackToMainMenu:
handleTapBackToMainMenuAction(action: action, isHomepage: isHomepage)
return true

case MainMenuDetailsActionType.tapDismissView:
self.telemetry.closeButtonTapped(isHomepage: isHomepage)
case .tapDismissView:
telemetry.closeButtonTapped(isHomepage: isHomepage)
return true

default: break
@unknown default:
return false
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,57 +51,69 @@ extension InternalTelemetrySettingsView: View {
var body: some View {
Form {
if #available(iOS 14, *) {
SwiftUI.Section(header: Text(verbatim: "Logging")) {
Toggle(isOn: $internalSettings.gleanLogPingsToConsole) {
VStack(alignment: .leading) {
Text(verbatim: "Log Pings to Console")
}
}.onChange(of: internalSettings.gleanLogPingsToConsole, perform: changeLogPingsToConsole)
loggingSection

debugViewSection

eventPingsSection
} else {
Text(verbatim: "Internal Telemetry Settings are only available on iOS 14 and newer.")
}
}.navigationBarTitle(Text(verbatim: "Telemetry"))
}

private var loggingSection: some View {
return SwiftUI.Section(header: Text(verbatim: "Logging")) {
Toggle(isOn: $internalSettings.gleanLogPingsToConsole) {
VStack(alignment: .leading) {
Text(verbatim: "Log Pings to Console")
}
}.onChange(of: internalSettings.gleanLogPingsToConsole, perform: changeLogPingsToConsole)
}
}

SwiftUI.Section(header: Text(verbatim: "Debug View")) {
Toggle(isOn: $internalSettings.gleanEnableDebugView) {
VStack(alignment: .leading) {
Text(verbatim: "Enable Debug View")
Text(verbatim: "Requires app restart").font(.caption)
}
}.disabled(internalSettings.gleanDebugViewTag.isEmpty)

VStack(alignment: .leading) {
TextField("Debug View Tag", text: $internalSettings.gleanDebugViewTag)
.onChange(of: internalSettings.gleanDebugViewTag, perform: changeDebugViewTag)
}

Button(action: { UIApplication.shared.open(GleanDebugViewURL) }) {
Text(verbatim: "Open Debug View (In Default Browser)")
}

Button(action: { UIPasteboard.general.url = GleanDebugViewURL }) {
Text(verbatim: "Copy Debug View Link")
}
private var debugViewSection: some View {
return SwiftUI.Section(header: Text(verbatim: "Debug View")) {
Toggle(isOn: $internalSettings.gleanEnableDebugView) {
VStack(alignment: .leading) {
Text(verbatim: "Enable Debug View")
Text(verbatim: "Requires app restart").font(.caption)
}
}.disabled(internalSettings.gleanDebugViewTag.isEmpty)

VStack(alignment: .leading) {
TextField("Debug View Tag", text: $internalSettings.gleanDebugViewTag)
.onChange(of: internalSettings.gleanDebugViewTag, perform: changeDebugViewTag)
}

Button(action: { UIApplication.shared.open(GleanDebugViewURL) }) {
Text(verbatim: "Open Debug View (In Default Browser)")
}

SwiftUI.Section {
Button(action: { sendPendingEventPings() }) {
Text(verbatim: "Send Pending Event Pings")
}
Button(action: { UIPasteboard.general.url = GleanDebugViewURL }) {
Text(verbatim: "Copy Debug View Link")
}
}
}

Button(action: { sendPendingBaselinePings() }) {
Text(verbatim: "Send Baseline Event Pings")
}
private var eventPingsSection: some View {
return SwiftUI.Section {
Button(action: { sendPendingEventPings() }) {
Text(verbatim: "Send Pending Event Pings")
}

Button(action: { sendPendingMetricsPings() }) {
Text(verbatim: "Send Metrics Event Pings")
}
Button(action: { sendPendingBaselinePings() }) {
Text(verbatim: "Send Baseline Event Pings")
}

Button(action: { sendPendingDeletionRequestPings() }) {
Text(verbatim: "Send Deletion Request Event Pings")
}
}
} else {
Text(verbatim: "Internal Telemetry Settings are only available on iOS 14 and newer.")
Button(action: { sendPendingMetricsPings() }) {
Text(verbatim: "Send Metrics Event Pings")
}
}.navigationBarTitle(Text(verbatim: "Telemetry"))

Button(action: { sendPendingDeletionRequestPings() }) {
Text(verbatim: "Send Deletion Request Event Pings")
}
}
}
}

Expand Down