Message bubbles: Layout for media (part 2) #5461

SBiOSoftWhare · 2022-01-31T10:00:41Z

Included:

Voice message cells
Location sharing cells
File without thumbnail cells
Selected sticker cell
Remove bubble for emotes
Timestamp inside file thumbnail

Screenshots:

…d outgoing voice message cells.

…ers.

…ght constraints.

…d outgoing location cells.

…aTag value.

…nvenient message type property from MXEvent.

…bnail.

…timestamp.

github-actions · 2022-01-31T10:33:19Z

📱 Scan the QR code below to install the build for this PR.
🔒 This build is for internal testing purpose. Only devices listed in the ad-hoc provisioning profile can install Element Alpha.

If you can't scan the QR code you can install the build via this link: https://i.diawi.com/jMuQ3D

…right display for timestamp.

pixlwave

More bubbles! Comments inline :)

pixlwave · 2022-02-01T10:40:17Z

Podfile

@@ -87,6 +87,7 @@ abstract_target 'RiotPods' do
    import_SwiftUI_pods

    pod 'DGCollectionViewLeftAlignFlowLayout', '~> 1.0.4'
+    pod 'UICollectionViewRightAlignedLayout', '~> 0.0.3'


Do we need to add the license to third_party_licenses.html?

Good point. In fact UICollectionViewRightAlignedLayout does not work with one item (#5469). I'll maybe remove it but I can add the license in between.

pixlwave · 2022-02-01T10:43:13Z

Riot/Categories/MXEvent.swift

+            return nil
+        }
+        return MXMessageType(identifier: messageTypeString)
+    }


pixlwave · 2022-02-01T11:36:30Z

Riot/Modules/Room/Views/BubbleCells/Styles/Bubble/BubbleRoomCellLayoutUpdater.swift

@@ -41,7 +41,7 @@ class BubbleRoomCellLayoutUpdater: RoomCellLayoutUpdating {

    func updateLayoutIfNeeded(for cell: MXKRoomBubbleTableViewCell, andCellData cellData: MXKRoomBubbleCellData) {

-        if cellData.isSenderCurrentUser {
+        if cellData.isIncoming == false {


Super unimportant, but changing to if cellData.isIncoming { and flipping the 2 blocks around would be simpler to read.

Yes true I didn't flip when I refactor this.

pixlwave · 2022-02-01T11:42:30Z

Riot/Modules/Room/Views/BubbleCells/Styles/Bubble/BubbleRoomCellLayoutUpdater.swift

+    private func setupIncomingFileAttachViewMargins(for cell: MXKRoomBubbleTableViewCell) {
+
+        guard let attachmentView = cell.attachmentView,
+              cell.attachViewLeadingConstraint == nil || cell.attachViewLeadingConstraint.isActive == false else {


A general comment here, switching between attachment… and the attach… shorthand terminology might lead to some confusion to anyone coming to this new. Would be good if we could use attachment… everywhere.

Totally agree. I admit that I didn't changed prefixes and use existing ones. But I can refactor and use attachment prefix for readability.

pixlwave · 2022-02-01T11:43:51Z

Riot/Modules/Room/Views/BubbleCells/Styles/Bubble/BubbleRoomCellLayoutUpdater.swift

+
+        let contentView = cell.contentView
+
+        // TODO: Use constants


Is this for a future update?

Edit: Having finished the review, I'm pretty sure this is for later 👍

Yes using proper constraints should be done in a specific PR at the end: #5409

pixlwave · 2022-02-01T12:27:36Z

Riot/Modules/Room/Views/BubbleCells/Styles/Bubble/BubbleRoomTimelineCellDecorator.swift

+        guard let bubbleComponents = cellData.bubbleComponents,
+              componentIndex < bubbleComponents.count else {
+                  return nil
+              }


The indentation of the return and closing brace seems off to me, but I would normally put else { on a new line, so not sure if this is what Xcode does anyway.

Yes it depends on people but we can use the same rules. On my side I prefer line return for else of the guard, to see quickly the return without reading all the conditions of the guard.

pixlwave · 2022-02-01T12:39:19Z

...lls/Styles/Bubble/Cells/FileWithoutThumbnail/Common/FileWithoutThumbnailBaseBubbleCell.swift

+    override func update(theme: Theme) {
+        super.update(theme: theme)
+    }


This can probably be removed?

Yes for sure

pixlwave · 2022-02-01T12:47:16Z

...yles/Bubble/Cells/FileWithoutThumbnail/Incoming/FileWithoutThumbnailIncomingBubbleCell.swift

+        let messageViewMarginRight: CGFloat = 80
+        let messageLeftMargin: CGFloat = 48


Unimportant, but it would be nice if these names had the same format. I'm also wondering if we should be saying leading and trailing when defining such variables or adding IBOutlets for constraints?

Yes I'll remove local constant and use proper global constants in a future PR (#5409)

pixlwave · 2022-02-01T12:49:40Z

...oom/Views/BubbleCells/Styles/Bubble/Cells/Location/Incoming/LocationIncomingBubbleCell.swift

+        let messageViewMarginRight: CGFloat = 80
+        let messageLeftMargin: CGFloat = 48


Same comment on the name format.

pixlwave · 2022-02-01T12:50:20Z

...s/BubbleCells/Styles/Bubble/Cells/VoiceMessage/Incoming/VoiceMessageIncomingBubbleCell.swift

+        let messageViewMarginRight: CGFloat = 80
+        let messageLeftMargin: CGFloat = 48
+        let playbackViewRightMargin: CGFloat = 40


And again :)

pixlwave

Looks good to me! 1 small comment with a typo I just noticed.

Riot/Modules/Room/Views/BubbleCells/Styles/Bubble/BubbleRoomTimelineCellDecorator.swift

…melineCellDecorator.swift Co-authored-by: Doug <6060466+pixlwave@users.noreply.github.com>

SBiOSoftWhare added 30 commits January 25, 2022 14:51

RoomTimelineCellIdentifier: Add specific indentifiers for incoming an…

c51e468

…d outgoing voice message cells.

PlainRoomTimelineCellProvider: Handle new voice message cell identifi…

4f70e93

…ers.

RoomVC: Handle new voice message cell identifiers.

edc21e4

Message bubble: Add extension on theme for incoming and outgoing colors.

45a465b

BubbleRoomCellLayoutUpdater: Update with nex theme colors.

e1b656f

BaseBubbleCell: Expose current theme.

f03aac4

BubbleCellContentView: Add outlets for inner content view left and ri…

b0689b4

…ght constraints.

VoiceMessagePlaybackView: Expose background color.

cd6cacc

Message bubbles: Implement incoming voice message cells.

ca1c749

Message bubbles: Implement outgoing voice message cells.

6cfd4ef

BubbleRoomTimelineCellProvider: Handle voice message cells.

b196a72

BubbleCellContentView: Add outlets for inner content view left and ri…

a6e9797

…ght constraints.

RoomTimelineCellIdentifier: Add specific indentifiers for incoming an…

b9771de

…d outgoing location cells.

RoomVC: Handle new voice location cell identifiers.

3d8b7c8

PlainRoomTimelineCellProvider: Handle new location cell identifiers.

de4e81f

Message bubbles: Implement incoming location cells.

45fdf01

Message bubbles: Implement outgoing location cells.

1cbff67

BubbleRoomTimelineCellProvider: Handle location cells.

5da015c

Message bubbles: Improve voice message cells implementation.

92c660e

MXKRoomBubbleCellData: Add convenient method to get RoomBubbleCellDat…

b5b4b82

…aTag value.

BubbleRoomTimelineCellDecorator: Show timestamp for location sharing.

7071bc0

MXKRoomBubbleCellData: Remove redundant isSenderCurrentUser method.

ae8d73d

MXKAttachment: Improve MXKAttachmentType definition.

33eef99

VoiceMessage: Update MXKAttachmentType usage.

557e93c

MXEvent: Add convenient message type property.

41af0a6

BubbleRoomCellLayoutUpdater & BubbleRoomTimelineCellDecorator: Use co…

d307f3d

…nvenient message type property from MXEvent.

BubbleRoomCellLayoutUpdater: Disable bubble bg for emotes.

fbd2d80

PlainRoomTimelineCellDecorator: Expose a Theme property.

e825423

BubbleRoomTimelineCellDecorator: Handle timestamp for media with thum…

f4807bc

…bnail.

VoiceMessagePlaybackView: Expose stackViewTrailingContraint.

198b91e

SBiOSoftWhare added 8 commits January 31, 2022 10:48

RoomViewController: Handle file without thumbnail cell identifiers.

5468b38

Create file without thumbnail content cell and base cell.

4b20435

Create file without thumbnail iincoming cells.

637c7f5

Add assets for file without thumbnail cells.

d8e9137

Create file without thumbnail outgoing cells.

90dae6b

BubbleRoomTimelineCellProvider: Handle file without thumbnail cells.

96a52aa

BubbleRoomTimelineCellDecorator: Handle file without thumbnail cells …

6dd08ec

…timestamp.

PollBubbleCell: Fix method override.

b9e9dd4

SBiOSoftWhare added 7 commits January 31, 2022 12:11

RoomSelectedStickerBubbleCell: Handle bubble outgoing case.

b99c413

BubbleRoomCellLayoutUpdater: Fix incoming selected sticker cell.

256c204

BuildSettings: Enable temporary message bubble setting.

0b2c447

BubbleRoomTimelineCellDecorator: Handle timestamp for stickers.

bd4db16

MXKRoomBubbleTableViewCell: Add possiblity to choose between left or …

26f4aee

…right display for timestamp.

Message bubbles: Use new background colors.

435f765

Update file without thumbnail cell content view layout.

af07028

SBiOSoftWhare requested a review from amshakal January 31, 2022 16:57

FileWithoutThumbnailBaseBubbleCell: Fix nullability crash.

6cddedd

pixlwave reviewed Feb 1, 2022

View reviewed changes

SBiOSoftWhare added 5 commits February 1, 2022 14:22

Flip cellData.isIncoming usage.

d067718

BubbleRoomTimelineCellDecorator: Update a guard let indentation.

763fa53

FileWithoutThumbnailBaseBubbleCell: Remove useless method.

0cd2391

VoiceMessagePlaybackView: Update background view color property name.

363f160

Update third party licenses.

e5df35d

pixlwave approved these changes Feb 1, 2022

View reviewed changes

Riot/Modules/Room/Views/BubbleCells/Styles/Bubble/BubbleRoomTimelineCellDecorator.swift Outdated Show resolved Hide resolved

SBiOSoftWhare and others added 3 commits February 1, 2022 15:24

Update Riot/Modules/Room/Views/BubbleCells/Styles/Bubble/BubbleRoomTi…

ed86baf

…melineCellDecorator.swift Co-authored-by: Doug <6060466+pixlwave@users.noreply.github.com>

BuildSettings: Disable message bubble setting atm.

48eee0e

BubbleRoomTimelineCellDecorator: Fix typos.

8f414c6

SBiOSoftWhare merged commit 5ff5a70 into develop Feb 1, 2022

SBiOSoftWhare deleted the steve/5209_media_bubbles_2 branch February 1, 2022 16:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Message bubbles: Layout for media (part 2) #5461

Message bubbles: Layout for media (part 2) #5461

SBiOSoftWhare commented Jan 31, 2022 •

edited

Loading

github-actions bot commented Jan 31, 2022 •

edited

Loading

pixlwave left a comment

pixlwave Feb 1, 2022

SBiOSoftWhare Feb 1, 2022

pixlwave Feb 1, 2022

pixlwave Feb 1, 2022

SBiOSoftWhare Feb 1, 2022

pixlwave Feb 1, 2022

SBiOSoftWhare Feb 1, 2022

pixlwave Feb 1, 2022 •

edited

Loading

SBiOSoftWhare Feb 1, 2022

pixlwave Feb 1, 2022

SBiOSoftWhare Feb 1, 2022

pixlwave Feb 1, 2022

SBiOSoftWhare Feb 1, 2022

pixlwave Feb 1, 2022

SBiOSoftWhare Feb 1, 2022

pixlwave Feb 1, 2022

pixlwave Feb 1, 2022

pixlwave left a comment

		let messageViewMarginRight: CGFloat = 80
		let messageLeftMargin: CGFloat = 48

Message bubbles: Layout for media (part 2) #5461

Message bubbles: Layout for media (part 2) #5461

Conversation

SBiOSoftWhare commented Jan 31, 2022 • edited Loading

github-actions bot commented Jan 31, 2022 • edited Loading

pixlwave left a 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

Choose a reason for hiding this comment

pixlwave Feb 1, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pixlwave left a comment

Choose a reason for hiding this comment

SBiOSoftWhare commented Jan 31, 2022 •

edited

Loading

github-actions bot commented Jan 31, 2022 •

edited

Loading

pixlwave Feb 1, 2022 •

edited

Loading