fix: Image selector anchor - WPB-11605 #2100

samwyndham · 2024-10-29T16:06:53Z

WPB-11605 [iOS] Anchor profile image selector popover

Issue

Currently on the navigation overhaul epic branch the popover UIImagePickerController is not properly anchored as it doesn't have a suitable sourceView set.

This PR:

Removes an unreferenced protocol, SettingsExternalScreenCellDescriptorType, allowing more flexibility when creating SettingsAppearanceCellDescriptor, and thus allowing the sender of the settings screen to be forwarded to the UIImagePickerControllers picker.
Updates ImagePickerManager.showActionSheet(on:completion:) to accept a popoverSourceView which gets set on the popover controller.
Uses a popover controller when displaying UIImagePickerController on any iPad, not just regular size classes as is necessary for apples API.
Does some general clean up to simplify code and remove optional instance variables

Testing

Please test on a real iPad.
Navigate to Setting
Tap Profile Picture
Tap Choose from Library
Photo picker should be well placed
Navigate to profile
Tap on your profile picker
Tap Choose from Library
Photo picker should be well placed

Also try rotating the device to check things are working

NOTE:

After selecting a photo there is a know issue that is not fixed in this PR: https://wearezeta.atlassian.net/browse/WPB-11929

Checklist

Title contains a reference JIRA issue number like [WPB-XXX].
Description is filled and free of optional paragraphs.
Adds/updates automated tests.

samwyndham · 2024-10-29T16:08:32Z

wire-ios/Wire-iOS/Sources/Managers/Image/ImagePickerManager.swift

@@ -96,23 +107,13 @@ class ImagePickerManager: NSObject {
            imagePickerController.cameraDevice = .front
            imagePickerController.modalTransitionStyle = .coverVertical
        case .photoLibrary, .savedPhotosAlbum:
-            if viewController.isIPadRegular() {
+            if viewController.isIPad() {


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

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.

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

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

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

I tried if it still works with the full-screen requirement disabled

and resized the window so that the layout collapsed.
It looks like it does. 👍

@caldrian You mean you tested there was no crash?

No crash and the presentation seems to be correct.

github-actions · 2024-10-29T16:27:51Z

Test Results

2 files 288 suites 3m 32s ⏱️
1 784 tests 1 784 ✅ 0 💤 0 ❌
1 792 runs 1 792 ✅ 0 💤 0 ❌

Results for commit b013ec3.

♻️ This comment has been updated with latest results.

This abstract type is not referenced and it's conformance limits flexibility fixing this bug

Also ensure a popover is always used for iPad

This variable is not read

wire-ios/Wire-iOS/Sources/Managers/Image/ImagePickerManager.swift

caldrian · 2024-11-04T13:54:59Z

wire-ios/Wire-iOS/Sources/Managers/Image/ImagePickerManager.swift

@@ -96,23 +107,13 @@ class ImagePickerManager: NSObject {
            imagePickerController.cameraDevice = .front
            imagePickerController.modalTransitionStyle = .coverVertical
        case .photoLibrary, .savedPhotosAlbum:
-            if viewController.isIPadRegular() {
+            if viewController.isIPad() {


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.

agisilaos

Left some comments and questions regarding a PopoverPresentationControllerConfiguration and how to check if it's iPad by checking if SplitViewController layout is collapsed.

wire-ios/Wire-iOS/Sources/Managers/Image/ImagePickerManager.swift

agisilaos · 2024-11-04T15:18:25Z

wire-ios/Wire-iOS/Sources/Managers/Image/ImagePickerManager.swift

@@ -96,23 +107,13 @@ class ImagePickerManager: NSObject {
            imagePickerController.cameraDevice = .front
            imagePickerController.modalTransitionStyle = .coverVertical
        case .photoLibrary, .savedPhotosAlbum:
-            if viewController.isIPadRegular() {
+            if viewController.isIPad() {


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

caldrian · 2024-11-11T15:24:33Z

wire-ios/Wire-iOS/Sources/Managers/Image/ImagePickerManager.swift

@@ -96,23 +107,13 @@ class ImagePickerManager: NSObject {
            imagePickerController.cameraDevice = .front
            imagePickerController.modalTransitionStyle = .coverVertical
        case .photoLibrary, .savedPhotosAlbum:
-            if viewController.isIPadRegular() {
+            if viewController.isIPad() {


I tried if it still works with the full-screen requirement disabled

and resized the window so that the layout collapsed.
It looks like it does. 👍

# Conflicts: # wire-ios/Wire-iOS/Sources/Managers/Image/ImagePickerManager.swift # wire-ios/Wire-iOS/Sources/Managers/Image/ProfileImagePickerManager.swift # wire-ios/Wire-iOS/Sources/UserInterface/SelfProfile/SelfProfileViewController.swift # wire-ios/Wire-iOS/Sources/UserInterface/Settings/CellDescriptors/SettingsCellDescriptor.swift # wire-ios/Wire-iOS/Sources/UserInterface/Settings/CellDescriptors/SettingsCellDescriptorFactory+Account.swift

samwyndham requested review from caldrian and agisilaos and removed request for caldrian October 29, 2024 16:06

echoes-hq bot added the echoes: product-roadmap Work aligned with the customer-announced roadmap, targeting a specific release date. label Oct 29, 2024

samwyndham requested a review from caldrian October 29, 2024 16:07

github-actions bot assigned samwyndham Oct 29, 2024

samwyndham commented Oct 29, 2024

View reviewed changes

Base automatically changed from epic/navigation-overhaul-WPB-6647 to develop October 31, 2024 15:04

echoes-hq bot added echoes: unplanned Any work item that isn’t part of the product or technical roadmap. echoes: technical-roadmap/technical-debt Changes intended at mitigating risks labels Oct 31, 2024

samwyndham added 5 commits November 1, 2024 16:47

Delete SettingsExternalScreenCellDescriptorType

c0997c8

This abstract type is not referenced and it's conformance limits flexibility fixing this bug

Add isIPad method

7b5a32e

Pass the source popover source view through to popover controller

13ce71b

Also ensure a popover is always used for iPad

Remove ImagePickerManager.sourceType

fb95b2e

This variable is not read

Remove ImagePickerManager.viewController property

a9a55f9

samwyndham force-pushed the fix/WPB-11605-image-selector-achor branch from 8086ba2 to a9a55f9 Compare November 1, 2024 15:49

caldrian approved these changes Nov 4, 2024

View reviewed changes

agisilaos reviewed Nov 4, 2024

View reviewed changes

caldrian approved these changes Nov 11, 2024

View reviewed changes

samwyndham added 4 commits November 18, 2024 16:07

Merge branch 'develop' into fix/WPB-11605-image-selector-achor

7fd5147

Use PopoverPresentationControllerConfiguration

c6a5c7b

Fix SwiftFormat issue

b013ec3

samwyndham changed the base branch from develop to release/cycle-3.114 November 22, 2024 15:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Image selector anchor - WPB-11605 #2100

fix: Image selector anchor - WPB-11605 #2100

samwyndham commented Oct 29, 2024 •

edited

Loading

samwyndham Oct 29, 2024

caldrian Nov 4, 2024

agisilaos Nov 4, 2024

samwyndham Nov 4, 2024

samwyndham Nov 5, 2024

caldrian Nov 11, 2024

samwyndham Nov 18, 2024

caldrian Nov 22, 2024

github-actions bot commented Oct 29, 2024 •

edited

Loading

caldrian Nov 4, 2024

agisilaos left a comment

agisilaos Nov 4, 2024

caldrian Nov 11, 2024

fix: Image selector anchor - WPB-11605 #2100

Are you sure you want to change the base?

fix: Image selector anchor - WPB-11605 #2100

Conversation

samwyndham commented Oct 29, 2024 • edited Loading

Issue

Testing

NOTE:

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 29, 2024 • edited Loading

Test Results

Choose a reason for hiding this comment

agisilaos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samwyndham commented Oct 29, 2024 •

edited

Loading

github-actions bot commented Oct 29, 2024 •

edited

Loading