Message bubbles: Text message layout #5383

SBiOSoftWhare · 2022-01-14T10:31:44Z

Two things to make in another PR:

Improve bubble height calculation (needs extra properties on RoomBubbleCellData to get only text position)
Use layout constants instead of copy pasted magic numbers

…vide room timeline cells.

…ntifiers.

…oom timeline cell view.

…e identifier.

…stomize timeline appearance.

…pearance configuration.

…y changes.

github-actions · 2022-01-14T11:07:11Z

📱 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/qx5TjS

…ay for collection view.

…bview.

…s in bubble background.

stefanceriu

Looking great so far 👏 Left some comments inline.

stefanceriu · 2022-01-19T14:21:58Z

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

+
+        } else if let messageTextViewHeight = cell.messageTextView?.frame.height {
+
+            finalBubbleHeight = messageTextViewHeight + extraMargin


These will not build on Xcode 12 because "Cannot convert value of type 'Double' to expected argument type 'CGFloat'" (probably why the CI checks fail)

stefanceriu · 2022-01-19T14:47:15Z

Riot/Modules/MatrixKit/Models/Room/MXKRoomBubbleCellData.h

+
+ @return Last visible component or nil.
+ */
+- (MXKRoomBubbleComponent*)getLastBubbleComponentWithDisplay;


We should probably start annotating nullability in ObjC, no matter how many warnings it generates.

I understand your POV but we have 25k warnings to address on Xcode 13, not sure we will see them :/

I was more thinking about the Swift point of view, expose a nice interface to it without caring about ObjC warnings.

Ok I see. Yes we should do that for sure but we have to carefully inspect each ObjC properties and method to be sure we are using the right nullability flag.

stefanceriu · 2022-01-19T14:54:04Z

Riot/Modules/MatrixKit/Models/Room/MXKRoomBubbleCellData.m

+        for (NSInteger index = 0; index < bubbleComponents.count; index++)
+        {
+            MXKRoomBubbleComponent *component = bubbleComponents[index];
+            if (component.attributedTextMessage)


Perhaps a foreach in reverseObjectEnumerator would be cleaner in this case. Same applies for the getFirstBubbleComponentWithDisplay method.. not reversed of course 😄

for(MXKRoomBubbleComponent *component in bubbleComponents.reverseObjectEnumerator) { if (component.attributedTextMessage) { return component; } }

stefanceriu · 2022-01-19T14:55:30Z

Riot/Modules/Room/Views/BubbleCells/Styles/RoomTimelineCellProviderProtocol.h

+NS_ASSUME_NONNULL_BEGIN
+
+/// Enables to register and provide room timeline cells
+@protocol RoomTimelineCellProviderProtocol <NSObject>


Noice! 👌

stefanceriu · 2022-01-19T15:06:44Z

Riot/Modules/Room/Views/BubbleCells/Styles/Bubble/BubbleRoomTimelineCellProvider.m

+    // Hide sender info and avatar for bubble outgoing messages
+    return @{
+        // Clear
+        @(RoomTimelineCellIdentifierOutgoingTextMessage) : RoomOutgoingTextMsgWithoutSenderInfoBubbleCell.class,


Possibly typo here, should it be RoomOutgoingTextMsgBubbleCell?

In fact for outgoing text messages we are hiding sender infos every time

Ah, got it 👍

stefanceriu · 2022-01-19T15:33:29Z

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

+        let leadingAnchor = messageTextView.leadingAnchor
+        let trailingAnchor = messageTextView.trailingAnchor
+
+        bubbleBackgroundView.updateHeight(messageTextView.frame.height)


Can't we pin this to the cells content somehow so we don't have to manually manage its height?

The problem is that the bubble background should wrap the visible text. And currently the textView is not exactly wrapping the text, especially if you add decorations such as previews and reactions.

stefanceriu · 2022-01-19T15:40:59Z

Riot/Modules/Room/BubbleReactions/BubbleReactionsView.swift

+import UIKit
+
+/// BubbleReactionsView items alignement
+enum BubbleReactionsViewAlignement {


Typo, should be Alignment

stefanceriu · 2022-01-19T15:46:00Z

Riot/Modules/Common/CollectionView/CollectionViewRightAlignFlowLayout.swift

+import UIKit
+
+/// UICollectionViewFlowLayout with right align behavior
+class CollectionViewRightAlignFlowLayout: UICollectionViewFlowLayout {


Feels like we should have one single parameterizable layout instead of two. Layout logic difference between left and right should be small.

Yes sure but the implementation is not the same atm for the two different layout.

stefanceriu · 2022-01-20T07:35:07Z

Riot/Modules/MatrixKit/Views/RoomBubbleList/MXKRoomBubbleTableViewCell.h

@@ -325,4 +325,7 @@ extern NSString *const kMXKRoomBubbleCellUrlItemInteraction;
 */
 - (void)setupViews;

+/// Add temporary subview to `tmpSubviews` property.
+- (void)addTmpSubview:(UIView*)subview;


Adding subviews on cells and handling their frames externally is probably going to create more problems than it fixes. Ideally these should go in some sort of base cell container. I believe this code already existed and was just moved around but what's the long term goal?
Either way, we shouldn't use abbreviations in method names -> addTemporarySubview

I agree that the decoration view and reuse handling can be better managed. It was already existing before as you said.

stefanceriu · 2022-01-20T07:37:49Z

Riot/Modules/Room/Views/BubbleCells/Styles/Bubble/BubbleRoomTimelineCellProvider.m

+#import "RoomOutgoingAttachmentWithoutSenderInfoBubbleCell.h"
+#import "RoomOutgoingAttachmentWithPaginationTitleBubbleCell.h"
+
+@implementation BubbleRoomTimelineCellProvider


Is the plan here to provide different cells classes for bubbles or to continue manually decorating/modifying them based on the layout? I imagine that ideally we would have different views with no manual customization on top but I might be missing something 🤔

Ideally we should not have the RoomCellLayoutUpdaterProtocol that updates existing cell layout. And provide appropriated cells with RoomTimelineCellProvider for each room timeline styles.

Awesome, that's what I though 👍

thibaultamartin · 2022-01-20T08:53:59Z

This might be an opportunity to close #4150 as well?

… getLastBubbleComponentWithDisplay.

SBiOSoftWhare added 26 commits January 13, 2022 16:50

MXRoomViewController: Remove unused cell classes.

c9cecdd

Move MatrixKit text message cells to Riot target.

7ae9b40

MXKRoomBubbleCellData: Add convenient method.

b251123

RoomTimelineStyleIdentifier: Make it available for ObjC.

24c8b8d

Add RoomTimelineCellProviderProtocol that enables to register and pro…

f7fb4c7

…vide room timeline cells.

Add PlainRoomTimelineCellProvider.

3d7a7fa

Add BubbleRoomTimelineCellProvider.

cc829ec

Add RoomTimelineCellProviders to bridging header.

6c76530

Add RoomTimelineCellIdentifier that represents room timeline cell ide…

fca3344

…ntifiers.

Update MatrixKit bridging header.

10fc0b0

Add RoomCellLayoutUpdaterProtocol that enables to setup or update a r…

057ea8d

…oom timeline cell view.

Move RoomBubbleCellLayout.

2642223

RiotSettings: Add convenient method to get current room timeline styl…

b916968

…e identifier.

Add cell layout updater implementation for bubbles.

f90f2fb

Add RoomTimelineStyle that describes a room timeline style used to cu…

88ceb07

…stomize timeline appearance.

Add RoomMessageBubbleBackgroundView.

0878d2e

Add room timeline style for bubbles.

54d5bc5

Add room timeline style for default cell style.

6fcdae5

Add RoomTimelineConfiguration that enables to manage room timeline ap…

5153982

…pearance configuration.

Update MatrixKit text message cells.

b2713ea

SettingsVC: Handle possibility to change room timeline style and appl…

71b4f72

…y changes.

RoomDataSource: Handle extra cell layout update if needed.

893fd5f

RoomVC: Handle cell provider.

d2ba53e

RoomBubbleCellData: Handle event merge configuration.

9b02f65

BubbleRoomCellLayoutUpdater: Update bubble height computation.

7b86a28

Settings: Enable message bubble configuration.

a84b830

SBiOSoftWhare added 3 commits January 18, 2022 14:21

Add CollectionViewRightAlignFlowLayout to have right alignement displ…

14dc04f

…ay for collection view.

BubbleReactionsView: Add alignement option (left or right).

7ff97f1

MXKRoomBubbleTableViewCell: Add convenient method to add temporary su…

55349c6

…bview.

SBiOSoftWhare added 9 commits January 18, 2022 17:51

BubbleRoomTimelineCellDecorator: Show timestamp on every text message…

f1e8e47

…s in bubble background.

RoomDataSource: Update timestamp decoration.

ce56391

BubbleRoomTimelineCellDecorator: Fix CGFloat type issues.

ec0a1ca

Text message bubble cells: Fix call to super.setupViews

34db845

RoomTimelineStyle: Add method to apply selected style.

5b2ae72

PlainRoomTimelineStyle: Handle selected style.

5a01b0b

BubbleRoomTimelineStyle: Handle selected style.

486bd0b

RoomDataSource: Use RoomTimelineStyle to apply cell selection style.

ac821f4

BubbleRoomCellLayoutUpdater: Update bubble height methods.

d1e1a90

SBiOSoftWhare requested a review from amshakal January 19, 2022 11:01

Update changes.

e38fa82

amshakal approved these changes Jan 19, 2022

View reviewed changes

stefanceriu self-requested a review January 19, 2022 14:21

PlainRoomTimelineCellProvider: Fix missing outgoing attachment cells.

9cb43a0

stefanceriu requested changes Jan 20, 2022

View reviewed changes

SBiOSoftWhare added 5 commits January 20, 2022 14:46

BubbleReactionsView: Fix typos.

d2fb2eb

BubbleRoomCellLayoutUpdater: Fix CGFloat type.

8913953

MXKRoomBubbleTableViewCell: Update a method name.

7893dbf

MXKRoomBubbleTableViewCell: Update a method name.

4b2e9bb

MXKRoomBubbleCellData: Improve getFirstBubbleComponentWithDisplay and…

5ae11d1

… getLastBubbleComponentWithDisplay.

stefanceriu self-requested a review January 20, 2022 15:51

stefanceriu approved these changes Jan 20, 2022

View reviewed changes

SBiOSoftWhare added 5 commits January 20, 2022 16:56

RoomTimelineStyle: Conforms to Themable.

f987664

Refactor RoomCellLayoutUpdaterProtocol to RoomCellLayoutUpdater.

83808f6

Refactor RoomTimelineCellProviderProtocol to RoomTimelineCellProvider.

6edacd7

Remane RoomCellLayoutUpdater to RoomCellLayoutUpdating.

003e17a

BuildSettings: Disable room timeline style configuration atm.

de54099

SBiOSoftWhare merged commit 95ee3cf into develop Jan 21, 2022

SBiOSoftWhare deleted the steve/5208_text_msg_layout branch January 21, 2022 15:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Message bubbles: Text message layout #5383

Message bubbles: Text message layout #5383

SBiOSoftWhare commented Jan 14, 2022 •

edited

Loading

github-actions bot commented Jan 14, 2022 •

edited

Loading

stefanceriu left a comment

stefanceriu Jan 19, 2022

stefanceriu Jan 19, 2022

SBiOSoftWhare Jan 20, 2022

stefanceriu Jan 20, 2022

SBiOSoftWhare Jan 20, 2022

stefanceriu Jan 19, 2022

stefanceriu Jan 19, 2022

stefanceriu Jan 19, 2022

SBiOSoftWhare Jan 20, 2022

stefanceriu Jan 20, 2022

stefanceriu Jan 19, 2022

SBiOSoftWhare Jan 20, 2022

stefanceriu Jan 19, 2022

stefanceriu Jan 19, 2022

SBiOSoftWhare Jan 20, 2022

stefanceriu Jan 20, 2022

SBiOSoftWhare Jan 20, 2022

stefanceriu Jan 20, 2022

SBiOSoftWhare Jan 20, 2022

stefanceriu Jan 20, 2022

thibaultamartin commented Jan 20, 2022


		} else if let messageTextViewHeight = cell.messageTextView?.frame.height {

		finalBubbleHeight = messageTextViewHeight + extraMargin

Message bubbles: Text message layout #5383

Message bubbles: Text message layout #5383

Conversation

SBiOSoftWhare commented Jan 14, 2022 • edited Loading

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

stefanceriu 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thibaultamartin commented Jan 20, 2022

SBiOSoftWhare commented Jan 14, 2022 •

edited

Loading

github-actions bot commented Jan 14, 2022 •

edited

Loading