Read receipts details #310

aramsargsyan · 2017-06-21T22:19:26Z

No description provided.

…XKReceiptSendersContainer for displaying the RR Details view controller

into read-receipts-details

morozkin

Feedback

morozkin · 2017-06-22T21:18:49Z

MatrixKit/Views/MXKReceiptSendersContainer.h

+@optional
+
+- (void)didTapReceiptsContainerWithRestClient:(MXRestClient *)restClient RoomMembers:(NSArray *)roomMembers avatars:(NSArray *)avatars recieptDescriptions:(NSArray *)recieptDescriptions;
+


You used generic annotations for ReadReceiptsViewController and i think it's a good idea to add them for MXKRecieptSendersContainerDelegate . Also RoomMembers

About a protocol for a delegate, a good practice is to return the concerned instance as the first parameter:

(void)receiptSendersContainer:(MXKReceiptSendersContainer*)receiptSendersContainer...

then the delegate is able to use the instance's property (you don't have to pass them in parameters), like the REST client here.

BTW after reviewing your PRs, I regret to say that we will not be able to use a delegate at the MXKReceiptSendersContainer level. As we can see in the usage of this delegate in Riot ios (see RoomDataSource file) this is not easy to define the actual delegate. You decided to use [AppDelegate theDelegate].masterTabBarController.currentRoomViewController as delegate but nothing guarantee that the current data source is working for this view controller (there are several room view controller instances in the app, like the one used to display the context of a room message resulting of a search session...)

To sum up, you should remove the definition and the usage of this new delegate. I will expose to you an other solution based on the - (void)cell:(id)cell didRecognizeAction:(NSString*)actionIdentifier userInfo:(NSDictionary *)userInfo; defined in MXKCellRenderingDelegate protocol. Sorry to have missed that point before.

morozkin · 2017-06-22T21:19:50Z

MatrixKit/Views/ReadReceipts/MXKReadReceiptTableViewCell.h

+
+#import <UIKit/UIKit.h>
+#import "MXKImageView.h"
+#import "MXKTableViewCell.h"


MXKImageView can be forward declared

Agree with Denis (because of swift compiler), even if we don't really like forward declaration :(

@aramsargsyan: please apply the forward declaration suggested by morozkin.

morozkin · 2017-06-22T21:21:47Z

MatrixKit/Views/ReadReceipts/MXKReadReceiptTableViewCell.m

+{
+    [super awakeFromNib];
+    [self configureAvatarImageView];
+}


Maybe put code from configureAvatarImageView right into awakeFromNib ? You know, it's just 3 lines of code

giomfo

Commented

Typo: please replace the wrong reciept prefix with receipt.

giomfo · 2017-06-23T09:41:27Z

MatrixKit/Views/MXKReceiptSendersContainer.h

@@ -59,6 +70,16 @@ typedef NS_ENUM(NSInteger, ReadReceiptsAlignment)
 @property (nonatomic) UILabel* moreLabel;

 /**
+ The receipt descriptions to show in the details view controller.
+ */
+@property (nonatomic) NSArray <NSString *> *recieptDescriptions;


I would prefer to store here the original array of read receipts (instead of the formatted dates). The receipt descriptions will be prepared only when the user decides to display the details.

Please replace this property with:
NSArray <MXReceiptData *> *readReceipt;

giomfo · 2017-06-23T09:51:45Z

MatrixKit/Views/MXKReceiptSendersContainer.m

+@interface MXKReceiptSendersContainer ()
+
+@property (nonatomic) NSArray <MXRoomMember *> *roomMembers;
+@property (nonatomic) NSArray <UIImage *> *placeholders;


We should expose this 2 arrays in the header (in readonly mode). This will ease their use at the RR details screen level.

giomfo · 2017-06-23T09:52:57Z

MatrixKit/Views/MXKReceiptSendersContainer.m

@@ -29,12 +38,35 @@ - (instancetype)initWithFrame:(CGRect)frame andRestClient:(MXRestClient*)restcli
        _maxDisplayedAvatars = 3;
        _avatarMargin = 2.0;
        _moreLabel = nil;
+
+        [self addTapGestureRecognizer];


Remove this tap gesture recogniser, the tap will be handled at the cell level

giomfo · 2017-06-23T09:57:56Z

MatrixKit/Views/ReadReceipts/MXKReadReceiptTableViewCell.h

+
+#import <UIKit/UIKit.h>
+#import "MXKImageView.h"
+#import "MXKTableViewCell.h"


Agree with Denis (because of swift compiler), even if we don't really like forward declaration :(

giomfo · 2017-06-23T10:03:56Z

MatrixKit/Views/ReadReceipts/MXKReadReceiptTableViewCell.m

+
+- (void)configureAvatarImageView
+{
+    self.avatarImageView.layer.cornerRadius = CGRectGetWidth(self.avatarImageView.frame)/2;


You have to override - (void)layoutSubviews to set correctly the corner radius. Set this value during awakeFromNib may be too early.

Please use the following code:
`- (void)layoutSubviews
{
[super layoutSubviews];

if (self.avatarImageView) { // Round image view [self.avatarImageView.layer setCornerRadius:self.pictureView.frame.size.width / 2]; self.avatarImageView.clipsToBounds = YES; }

}`

You should move self.avatarImageView.enableInMemoryCache = YES; in awakeFromNib

giomfo · 2017-06-23T11:53:17Z

MatrixKit/Views/ReadReceipts/MXKReadReceiptTableViewCell.xib

@@ -0,0 +1,89 @@
+<?xml version="1.0" encoding="UTF-8"?>


Could you please adjust the cell height by using for example 74 (instead of 180)?
For the design of this cell, you should copy the existing ContactTableViewCell.xib (riot-ios) by considering only the following items:thumbnailView, contactDisplayNameLabel and contactInformationLabel (Do not rename your properties, your naming is fine).
You could not define a specific label with a fix string like the "Read" label. We won't be able to translate it. You will have to use an attributed string for the receiptDescriptionLabel
(Note: remove the white space before ':': "Read: xxx").

giomfo · 2017-06-23T13:28:56Z

MatrixKit/Views/MXKReceiptSendersContainer.h

+/*
+ The MXSession used to format the events in the details view
+ */
+@property (nonatomic) MXSession *mxSession;


We don't need the matrix session here. The receipt description will be created in the RR details view controller.

…ad, fixing the potential issue of sending the wrong Room controller.

…ing to the Contact cell.

…of the added subviews, which have to be removed before reusing the cell. element-hq/element-ios#59

Read Receipts Details - MXKRoomBubbleTableViewCell

giomfo · 2017-06-27T21:49:25Z

MatrixKit/Views/ReadReceipts/MXKReadReceiptTableViewCell.h

+
+#import <UIKit/UIKit.h>
+#import "MXKImageView.h"
+#import "MXKTableViewCell.h"


@aramsargsyan: please apply the forward declaration suggested by morozkin.

giomfo · 2017-06-27T22:23:57Z

MatrixKit/Views/ReadReceipts/MXKReadReceiptTableViewCell.m

+    self.avatarImageView.enableInMemoryCache = YES;
+    NSMutableAttributedString *readString = [[NSMutableAttributedString alloc] initWithAttributedString:self.readLabel.attributedText];
+    [readString setAttributes:@{NSFontAttributeName:[UIFont boldSystemFontOfSize:15]} range:NSMakeRange(0, readString.length)];
+    self.readLabel.attributedText = readString;


I advised you to use an attributed string to display the receipt details in only one UILabel instance (instead of using 2 labels:readLabel and receiptDescriptionLabel).

If you decide to keep this label named readLabel for the constant prefix "Read:", you don't have to use attributed string. And you may define the text font in the xib file.
About the label text value, you have to add a new key-value in the MatrixKit.strings file to handle correctly the translation (for example "receipt_status_read" = "Read:";). Then you use the key to define the label text:
self.readLabel.text = [NSBundle mxk_localizedStringForKey:@"receipt_status_read"].

Personally I would prefer to remove the readLabel object. We should let the owner of the MXKReadReceiptTableViewCell instance set an attributed string in the receiptDescriptionLabel label to include the receipt status ("Read:" or something else). In that case the attributed string will be handled at the application level (in Riot-ios source code for example). CAUTION: the string translation will be handled at application level (see Vector.strings in Riot-ios ressources).

…ceipt description, added in MatrixKit.strings for translation + review changes

…an/matrix-ios-kit into read-receipts-details

giomfo

LGTM, just some few remarks
Don't forget to sign off your PR ;)

giomfo · 2017-06-29T14:15:49Z

MatrixKit/Assets/MatrixKitAssets.bundle/en.lproj/MatrixKit.strings

@@ -270,6 +270,7 @@
 "network_error_not_reachable" = "Please check your network connectivity";
 "user_id_placeholder" = "ex: @bob:homeserver";
 "ssl_homeserver_url" = "Home Server URL: %@";
+"receipt_status_read" = "Read: ";


Please move this key-value at the application level in Vector.strings
You may group it with read_receipts_list key.

giomfo · 2017-06-29T14:23:27Z

MatrixKit/Views/ReadReceipts/MXKReadReceiptTableViewCell.m

+    self.avatarImageView.enableInMemoryCache = YES;
+}
+
+- (void)layoutSubviews {


Please add a new line before starting a new block {}. Like you did for the other blocks

…level. Signed-off-by: Aram Sargsyan aram.sargsyan.1997@gmail.com

aramsargsyan added 4 commits June 21, 2017 22:23

Added ReadReceiptTableViewCell, made the necessary modifications to M…

cd9eed0

…XKReceiptSendersContainer for displaying the RR Details view controller

Modified the Read Receipts cell, added it as a public header

a41ed80

Merge branch 'develop' of https://github.com/aramsargsyan/matrix-ios-kit

f10a410

into read-receipts-details

Minor changes

618bb77

aramsargsyan mentioned this pull request Jun 21, 2017

Chat: Expand read receipts when user taps on it element-hq/element-ios#59

Closed

giomfo self-requested a review June 22, 2017 13:28

morozkin reviewed Jun 22, 2017

View reviewed changes

giomfo reviewed Jun 23, 2017

View reviewed changes

Review changes

905e0ca

giomfo reviewed Jun 23, 2017

View reviewed changes

aramsargsyan and others added 5 commits June 26, 2017 19:32

Removed the ReceiptsContainerDelegate, used didRecognizeAction: inste…

9f2e3ac

…ad, fixing the potential issue of sending the wrong Room controller.

changed the design and size of the MXKReadReceiptTableViewCell accord…

73ea81d

…ing to the Contact cell.

Review changes

a640d91

Read Receipts Details - MXKRoomBubbleTableViewCell: Define the array …

a189c59

…of the added subviews, which have to be removed before reusing the cell. element-hq/element-ios#59

Merge pull request #1 from giomfo/read-receipts-details-gfo

0642832

Read Receipts Details - MXKRoomBubbleTableViewCell

giomfo reviewed Jun 27, 2017

View reviewed changes

aramsargsyan added 2 commits June 29, 2017 15:02

Modified MXKReadReceiptTableViewCell to only use one label for the re…

4ad1186

…ceipt description, added in MatrixKit.strings for translation + review changes

Merge branch 'read-receipts-details' of https://github.com/aramsargsy…

b77d690

…an/matrix-ios-kit into read-receipts-details

giomfo reviewed Jun 29, 2017

View reviewed changes

Removed receipt_status_read from MatrixKit.strings to move it to app …

0640aef

…level. Signed-off-by: Aram Sargsyan aram.sargsyan.1997@gmail.com

giomfo approved these changes Jun 30, 2017

View reviewed changes

giomfo merged commit 97e11d7 into matrix-org:develop Jun 30, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read receipts details #310

Read receipts details #310

aramsargsyan commented Jun 21, 2017

morozkin left a comment

morozkin Jun 22, 2017

giomfo Jun 23, 2017

morozkin Jun 22, 2017

giomfo Jun 23, 2017

giomfo Jun 27, 2017

morozkin Jun 22, 2017

giomfo left a comment

giomfo Jun 23, 2017

giomfo Jun 23, 2017

giomfo Jun 23, 2017

giomfo Jun 23, 2017

giomfo Jun 23, 2017

giomfo Jun 23, 2017

giomfo Jun 23, 2017

giomfo Jun 27, 2017

giomfo Jun 27, 2017

giomfo left a comment

giomfo Jun 29, 2017

giomfo Jun 29, 2017

		@optional

		- (void)didTapReceiptsContainerWithRestClient:(MXRestClient )restClient RoomMembers:(NSArray )roomMembers avatars:(NSArray )avatars recieptDescriptions:(NSArray )recieptDescriptions;

Read receipts details #310

Read receipts details #310

Conversation

aramsargsyan commented Jun 21, 2017

morozkin 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

giomfo 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

giomfo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment