fix: Removed duplicated alert system. - WPB-23037#4492
fix: Removed duplicated alert system. - WPB-23037#4492JaCaLlaSchwarz wants to merge 21 commits intodevelopfrom
Conversation
netbe
left a comment
There was a problem hiding this comment.
Left a couple comments, it's going towards the right direction but still need some changes before approval.
I would expect conversation snapshot test to change too somehow
wire-ios-request-strategy/Sources/Payloads/Processing/ConversationEventPayloadProcessor.swift
Show resolved
Hide resolved
...Domain/Sources/WireDomain/Repositories/Conversations/LocalStore/ConversationLocalStore.swift
Show resolved
Hide resolved
| connectionViewController = UserConnectionViewController(userSession: userSession, user: otherParticipant) | ||
| headerView = connectionViewController?.view | ||
| } else { | ||
| noUserConnectionViewController = NoUserConnectionViewController(userSession: userSession) |
There was a problem hiding this comment.
suggestion: I feel like the name of this view NoUserConnectionViewController is a bit misleading, as it's the header for groups or channels.
maybe DefaultConversationHeaderViewController, feel free to challenge this name ;)
|
|
||
| let connectionOrOneOnOne = conversation.conversationType == .connection || conversation | ||
| .conversationType == .oneOnOne | ||
| let connectionOrOneOnOne = [.connection, .oneOnOne /* , .group */ ] |
There was a problem hiding this comment.
suggestion: revert, this change is not necessary
| await context.perform { [self] in | ||
| if isInitialFetch { | ||
| // we just got a new conversation, we display new conversation header | ||
| localConversation.appendNewConversationSystemMessage( |
There was a problem hiding this comment.
issue: not inserting the new conversationsystemMessage is not enough (+ could be avoided). For current conversations the double system message would still show.
I suggest to not return a cell on the conversationContentViewController for the newConversation system message
Test Results1 876 tests 1 825 ✅ 2m 14s ⏱️ For more details on these failures, see this check. Results for commit ad9b1be. ♻️ This comment has been updated with latest results. Summary: workflow run #23837176590 |
90c22a2 to
ae6308f
Compare
|
…ithub.com:wireapp/wire-ios into fix/remove_duplicated_warning_message_WPB23037_v2
|
FYI I'll wait for the issues @netbe raised to be addressed before review. |
netbe
left a comment
There was a problem hiding this comment.
still some issues are there, please reply to my comments before re-asking for review
| cells.append(AnyConversationMessageCellDescription(startedConversationCell)) | ||
|
|
||
| // Only display invite user cell for team members | ||
| if selfUser.isTeamMember, | ||
| conversation.selfCanAddUsers(selfUser: selfUser), | ||
| conversation.isOpenGroup { | ||
| cells.append( | ||
| AnyConversationMessageCellDescription( | ||
| GuestsAllowedCellDescription(isChannel: conversation.isChannel) | ||
| ) | ||
| ) | ||
| } | ||
|
|
||
| if conversation.isWireDriveEnabled { | ||
| let fileCollaborationCell = ConversationFileCollaborationSystemMessageCellDescription() | ||
| cells.append(AnyConversationMessageCellDescription(fileCollaborationCell)) |
There was a problem hiding this comment.
thought: I wonder if all the cells here should move to header
There was a problem hiding this comment.
issue: there seems to be a git lfs issue here, try:
git lfs migrate import --no-rewrite --yes wire-ios/Wire-iOS Tests/ReferenceImages/StartedConversationCellTests/testThatItRendersNewConversationCellStartedFromSelfWithOneParticipantAndWithoutName.414-0.png
| let selfProfileUIBuilder: SelfProfileViewControllerBuilderProtocol | ||
| let conversationCreationRepository: any ConversationCreationRepositoryProtocol | ||
| var connectionViewController: UserConnectionViewController? | ||
| var defaultConversationHeaderViewController: DefaultConversationHeaderViewController? |
There was a problem hiding this comment.
| var defaultConversationHeaderViewController: DefaultConversationHeaderViewController? | |
| var defaultConversationHeaderViewController: DefaultConversationHeaderViewController? |
…testThatItRendersNewConversationCellStartedFromSelfWithOneParticipantAndWithoutName.414-0.png: convert to Git LFS
…ithub.com:wireapp/wire-ios into fix/remove_duplicated_warning_message_WPB23037_v2
|
samwyndham
left a comment
There was a problem hiding this comment.
Nice work. I've left a few questions questions/change requests regarding some UIKit points.
|
|
||
| final class DefaultConversationHeaderView: UIView { | ||
|
|
||
| private static var correlationFormatter: AddressBookCorrelationFormatter = .init( |
There was a problem hiding this comment.
question: Is this used anywhere?
| guestWarningContainer.leadingAnchor.constraint(equalTo: leadingAnchor, constant: 0), | ||
| guestWarningContainer.trailingAnchor.constraint(equalTo: trailingAnchor, constant: 0), |
There was a problem hiding this comment.
suggestion: Sorry I'm forgetting UIKit :). Is the constant needed? I think if you remove the constant parameter it will be the same (like what you are doing with the top and bottom anchors)
| ) | ||
|
|
||
| private let guestWarningView = GuestAccountWarningView() | ||
| private let guestWarningContainer = UIView() |
There was a problem hiding this comment.
question: What is the purpose of guestWarningContainer. Can't guestWarningView be added directly to self as DefaultConversationHeaderView is also a view.
|
|
||
| fileprivate var defaultConversationHeaderView: DefaultConversationHeaderView! | ||
|
|
||
| let userSession: ZMUserSession |
There was a problem hiding this comment.
question: How is this property used?
|
|
||
| final class DefaultConversationHeaderViewController: UIViewController { | ||
|
|
||
| fileprivate var defaultConversationHeaderView: DefaultConversationHeaderView! |
There was a problem hiding this comment.
suggestion: This property isn't accessed so it is not needed. Instead you can do:
override func loadView() {
view = DefaultConversationHeaderView()
}| defaultConversationHeaderViewController = DefaultConversationHeaderViewController(userSession: userSession) | ||
| headerView = defaultConversationHeaderViewController?.view |
There was a problem hiding this comment.
question: What is the need for the existence of DefaultConversationHeaderViewController? This view controller doesn't seem to do anything. My guess is we can delete DefaultConversationHeaderViewController and just use DefaultConversationHeaderView directly.




Issue
Duplicated Alert warning message

Testing
Checklist
[WPB-XXX].UI accessibility checklist
If your PR includes UI changes, please utilize this checklist: