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.

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

fix: Image selector anchor - WPB-11605 #2100

Open
wants to merge 9 commits into
base: release/cycle-3.114
Choose a base branch
from
61 changes: 31 additions & 30 deletions wire-ios/Wire-iOS/Sources/Managers/Image/ImagePickerManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -33,22 +33,22 @@ extension UIImage {
class ImagePickerManager: NSObject {

// MARK: - Properties
private weak var viewController: UIViewController?
private var sourceType: UIImagePickerController.SourceType?
private var completion: ((UIImage) -> Void)?
private let mediaShareRestrictionManager = MediaShareRestrictionManager(sessionRestriction: ZMUserSession.shared())

// MARK: - Methods
func showActionSheet(on viewController: UIViewController? = UIApplication.shared.topmostViewController(onlyFullScreen: false),
completion: @escaping (UIImage) -> Void) -> UIAlertController {
func showActionSheet(
on viewController: UIViewController? = UIApplication.shared.topmostViewController(onlyFullScreen: false),
popoverSourceView: UIView,
completion: @escaping (UIImage) -> Void
) -> UIAlertController {
self.completion = completion
self.viewController = viewController

let actionSheet = imagePickerAlert()
let actionSheet = imagePickerAlert(popoverSourceView: popoverSourceView, viewController: viewController)
return actionSheet
}

private func imagePickerAlert() -> UIAlertController {
private func imagePickerAlert(popoverSourceView: UIView, viewController: UIViewController?) -> UIAlertController {
typealias Alert = L10n.Localizable.Self.Settings.AccountPictureGroup.Alert
let actionSheet = UIAlertController(title: Alert.title,
message: nil,
Expand All @@ -57,16 +57,26 @@ class ImagePickerManager: NSObject {
// Choose from gallery option, if security flag enabled
if mediaShareRestrictionManager.isPhotoLibraryEnabled {
let galleryAction = UIAlertAction(title: Alert.choosePicture, style: .default) { [weak self] _ in
self?.sourceType = .photoLibrary
self?.getImage(fromSourceType: .photoLibrary)
guard let self, let viewController else { return }

self.getImage(
fromSourceType: .photoLibrary,
viewController: viewController,
popoverSourceView: popoverSourceView
)
}
actionSheet.addAction(galleryAction)
}

// Take photo
let cameraAction = UIAlertAction(title: Alert.takePicture, style: .default) { [weak self] _ in
self?.sourceType = .camera
self?.getImage(fromSourceType: .camera)
guard let self, let viewController else { return }

self.getImage(
fromSourceType: .camera,
viewController: viewController,
popoverSourceView: popoverSourceView
)
}
actionSheet.addAction(cameraAction)

Expand All @@ -76,11 +86,12 @@ class ImagePickerManager: NSObject {
return actionSheet
}

private func getImage(fromSourceType sourceType: UIImagePickerController.SourceType) {
guard UIImagePickerController.isSourceTypeAvailable(sourceType),
let viewController else {
return
}
private func getImage(
fromSourceType sourceType: UIImagePickerController.SourceType,
viewController: UIViewController,
popoverSourceView: UIView
samwyndham marked this conversation as resolved.
Show resolved Hide resolved
) {
guard UIImagePickerController.isSourceTypeAvailable(sourceType) else { return }

let imagePickerController = UIImagePickerController()
imagePickerController.delegate = self
Expand All @@ -96,23 +107,13 @@ class ImagePickerManager: NSObject {
imagePickerController.cameraDevice = .front
imagePickerController.modalTransitionStyle = .coverVertical
case .photoLibrary, .savedPhotosAlbum:
if viewController.isIPadRegular() {
if viewController.isIPad() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

isIPadRegular isn't sufficient. We need to use a popover on any iPad size

Copy link
Contributor

Choose a reason for hiding this comment

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

I think correct would be to check if the split view controller's layout is collapsed, which is true for iPhones and for iPads when the window is resized so that the horizontal size class is compact.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed with @caldrian here. Is it possible to be done in this PR @samwyndham?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But won't that mean that it is possible to present this on iPad without using a popover? Won't that crash?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll see if there is a iPad at work to test this with

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried if it still works with the full-screen requirement disabled
Screenshot 2024-11-11 at 16 22 35
and resized the window so that the layout collapsed.
It looks like it does. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@caldrian You mean you tested there was no crash?

Copy link
Contributor

Choose a reason for hiding this comment

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

No crash and the presentation seems to be correct.

// UIKit will crash if the photo library is not presented using a popoverPresentationController on iPad
// https://developer.apple.com/documentation/uikit/uiimagepickercontroller
imagePickerController.modalPresentationStyle = .popover

if let popoverPresentationController = imagePickerController.popoverPresentationController {
popoverPresentationController.backgroundColor = UIColor.white

// UIKit will crash if the photo library is not presented using a popoverPresentationController
// https://developer.apple.com/documentation/uikit/uiimagepickercontroller
// TODO: [WPB-11605] fix this workaround and choose proper sourceView/sourceRect
popoverPresentationController.sourceView = viewController.view
popoverPresentationController.sourceRect = .init(
origin: .init(
x: viewController.view.safeAreaLayoutGuide.layoutFrame.maxX,
y: viewController.view.safeAreaLayoutGuide.layoutFrame.minY
),
size: .zero
)
popoverPresentationController.sourceView = popoverSourceView
}
}
default:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,13 @@ import WireSyncEngine

final class ProfileImagePickerManager: ImagePickerManager {

func selectProfileImage() -> UIAlertController {
let actionSheet = showActionSheet { image in
guard let jpegData = image.jpegData else {
return
func selectProfileImage(popoverSourceView: UIView) -> UIAlertController {
let actionSheet = showActionSheet(popoverSourceView: popoverSourceView) { image in
guard let jpegData = image.jpegData, let session = ZMUserSession.shared() else { return }

session.enqueue {
session.userProfileImage.updateImage(imageData: jpegData)
}
ZMUserSession.shared()?.enqueue({
ZMUserSession.shared()?.userProfileImage.updateImage(imageData: jpegData)
})
}
return actionSheet
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ extension UITraitEnvironment {
return device.userInterfaceIdiom == .pad && isHorizontalSizeClassRegular
}

func isIPad(device: DeviceAbstraction = DeviceWrapper(device: .current)) -> Bool {
return device.userInterfaceIdiom == .pad
}

func isIPadRegularPortrait(
device: DeviceAbstraction = DeviceWrapper(device: .current)
) -> Bool {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -206,10 +206,10 @@ final class SelfProfileViewController: UIViewController {
@objc private func userDidTapProfileImage(_ sender: UIGestureRecognizer) {
guard userRightInterfaceType.selfUserIsPermitted(to: .editProfilePicture) else { return }

let alertController = profileImagePicker.selectProfileImage()
let imageView = profileHeaderViewController.imageView
let alertController = profileImagePicker.selectProfileImage(popoverSourceView: imageView)
if let popoverPresentationController = alertController.popoverPresentationController {
popoverPresentationController.sourceView = profileHeaderViewController.imageView.superview!
popoverPresentationController.sourceRect = profileHeaderViewController.imageView.frame.insetBy(dx: -4, dy: -4)
popoverPresentationController.sourceView = imageView
}
present(alertController, animated: true)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,14 @@ import UIKit
import WireSettingsUI
import WireSyncEngine

class SettingsAppearanceCellDescriptor: SettingsCellDescriptorType, SettingsExternalScreenCellDescriptorType {
class SettingsAppearanceCellDescriptor: SettingsGroupCellDescriptorType, SettingsCellDescriptorType {
static let cellType: SettingsTableCellProtocol.Type = SettingsAppearanceCell.self

private var text: String
private let presentationStyle: PresentationStyle

weak var viewController: UIViewController?
let presentationAction: () -> (UIViewController?)
let presentationAction: (_ sender: UIView) -> UIViewController?

var identifier: String?
weak var group: SettingsGroupCellDescriptorType?
Expand All @@ -48,7 +48,7 @@ class SettingsAppearanceCellDescriptor: SettingsCellDescriptorType, SettingsExte
text: String,
previewGenerator: PreviewGeneratorType? = .none,
presentationStyle: PresentationStyle,
presentationAction: @escaping () -> (UIViewController?),
presentationAction: @escaping (_ sender: UIView) -> UIViewController?,
settingsCoordinator: AnySettingsCoordinator
) {
self.text = text
Expand Down Expand Up @@ -81,7 +81,7 @@ class SettingsAppearanceCellDescriptor: SettingsCellDescriptorType, SettingsExte
// MARK: - SettingsCellDescriptorType

func select(_ value: SettingsPropertyValue, sender: UIView) {
guard let controllerToShow = generateViewController() else { return }
guard let controllerToShow = generateViewController(sender: sender) else { return }

switch presentationStyle {
case .alert:
Expand All @@ -97,7 +97,7 @@ class SettingsAppearanceCellDescriptor: SettingsCellDescriptorType, SettingsExte
}
}

func generateViewController() -> UIViewController? {
presentationAction()
private func generateViewController(sender: UIView) -> UIViewController? {
presentationAction(sender)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -115,10 +115,6 @@ extension SettingsInternalGroupCellDescriptorType {
}
}

protocol SettingsExternalScreenCellDescriptorType: SettingsGroupCellDescriptorType {
var presentationAction: () -> (UIViewController?) {get}
}

protocol SettingsPropertyCellDescriptorType: SettingsCellDescriptorType {
var settingsProperty: SettingsProperty { get }
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -300,8 +300,8 @@ extension SettingsCellDescriptorFactory {
return .image(image)
}

let presentationAction: () -> (UIViewController?) = {
let actionSheet = profileImagePicker.selectProfileImage()
let presentationAction: (_ sender: UIView) -> UIViewController? = { sender in
let actionSheet = profileImagePicker.selectProfileImage(popoverSourceView: sender)
return actionSheet
}
return SettingsAppearanceCellDescriptor(
Expand Down Expand Up @@ -331,7 +331,7 @@ extension SettingsCellDescriptorFactory {
return SettingsCellPreview.color((selfUser.accentColor ?? .default).uiColor)
}

private func colorElementPresentationAction() -> UIViewController {
private func colorElementPresentationAction(sender: UIView) -> UIViewController {
guard
let selfUser = ZMUser.selfUser(),
let userSession = ZMUserSession.shared()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ enum AccessoryViewMode: Int {
case alwaysHide
}

class SettingsExternalScreenCellDescriptor: SettingsExternalScreenCellDescriptorType, SettingsControllerGeneratorType {
class SettingsExternalScreenCellDescriptor: SettingsGroupCellDescriptorType, SettingsControllerGeneratorType {
static let cellType: SettingsTableCellProtocol.Type = SettingsTableCell.self
var visible: Bool = true
let title: String
Expand Down