Read receipts details #1341

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

No description provided.

…ers. If the user doesn't have an avatar, the placeholder is placed.

…to read-receipts-details

morozkin · 2017-06-22T21:08:27Z

Riot/ViewController/ReadReceiptsViewController.h

+
+@interface ReadReceiptsViewController : UIViewController
+
+ (void)openInViewController:(UIViewController *)viewController withRestClient:(MXRestClient *)restClient withRoomMembers:(NSArray <MXRoomMember *> *)roomMembers placeholders:(NSArray <UIImage *> *)placeholders receiptDescriptions:(NSArray <NSString *> *)receiptDescriptions;


withRestClient and withRoomMembers

morozkin · 2017-06-22T21:09:23Z

Riot/ViewController/ReadReceiptsViewController.h

+ */
+
+#import <UIKit/UIKit.h>
+#import <MatrixKit/MatrixKit.h>


I prefer to use forward declaration rather than import everything

morozkin · 2017-06-22T21:14:16Z

Riot/ViewController/ReadReceiptsViewController.m

+    [self configureViews];
+    [self configureReceiptsTableView];
+    [self addOverlayViewGesture];
+}


I suggest you to put code from this functions into viewDidLoad and separate code with comments. These functions are really short and it looks like an overhead to create function for 2 lines of code.

morozkin

Feedback

giomfo · 2017-06-23T13:46:52Z

Riot/ViewController/ReadReceiptsViewController.h

@@ -19,6 +19,6 @@

 @interface ReadReceiptsViewController : UIViewController

-+ (void)openInViewController:(UIViewController *)viewController withRestClient:(MXRestClient *)restClient withRoomMembers:(NSArray <MXRoomMember *> *)roomMembers placeholders:(NSArray <UIImage *> *)placeholders receiptDescriptions:(NSArray <NSString *> *)receiptDescriptions;
+ (void)openInViewController:(UIViewController *)viewController withRestClient:(MXRestClient *)restClient session:(MXSession *)session withRoomMembers:(NSArray <MXRoomMember *> *)roomMembers placeholders:(NSArray <UIImage *> *)placeholders receipts:(NSArray <MXReceiptData *> *)receipts;


I would prefer split this operation in 2:

one to create and return a new ReadReceiptsViewController object:
+ (instancetype)readReceiptsViewControllerBasedOnReceiptSendersContainer:(MXKReceiptSendersContainer*)receiptSendersContainer andSession:(MXSession *)session;

Note: the REST client will be deduced from the provided session. The receiptSendersContainer instance will provide all the other inputs.

another to handle its display: - (void)showInViewController:(UIViewController*)viewController;

giomfo · 2017-06-23T15:18:51Z

Riot/ViewController/ReadReceiptsViewController.xib

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


Could you please increase the margin of the container view with its superview

giomfo · 2017-06-23T15:21:03Z

Riot/ViewController/ReadReceiptsViewController.m

+        NSString *avatarUrl = self.roomMembers[indexPath.row].avatarUrl;
+        if (self.restClient && avatarUrl)
+        {
+            CGFloat side = CGRectGetWidth(cell.imageView.frame);


You should use cell. avatarImageView instead of cell.imageView, shouldn't you?

giomfo · 2017-06-23T15:36:00Z

Riot/Model/Room/RoomDataSource.m

@@ -148,7 +149,7 @@ - (UITableViewCell *)tableView:(UITableView *)tableView cellForRowAtIndexPath:(N
            // Some vertical whitespaces are added in message text view (see RoomBubbleCellData class) to insert correctly multiple receipts.
            bubbleCell.bubbleOverlayContainer.backgroundColor = [UIColor clearColor];
            bubbleCell.bubbleOverlayContainer.alpha = 1;
-            bubbleCell.bubbleOverlayContainer.userInteractionEnabled = NO;
+            bubbleCell.bubbleOverlayContainer.userInteractionEnabled = YES;


FYI, I agree we have to enable the user interaction here on bubble overlay to catch the tap on RR container.
Unfortunately this change will break the message selection in the room history for the bubbles with RR. Don't worry about this, I will take this in charge.

…content view instead of the overlay container to not disturb tag gesture handling. element-hq#59

Read Receipts Details - RoomDataSource

giomfo · 2017-06-27T22:46:15Z

Riot/Model/Room/RoomDataSource.m

@@ -17,6 +17,7 @@

 #import "RoomDataSource.h"

+#import "AppDelegate.h"


Why do you need to import AppDelegate here?

giomfo · 2017-06-27T22:49:19Z

Riot/ViewController/ReadReceiptsViewController.h

+#import <UIKit/UIKit.h>
+#import <MatrixKit/MatrixKit.h>
+
+@interface ReadReceiptsViewController : UIViewController


Please inherit of MXKViewController instead of UIViewController

giomfo · 2017-06-27T22:53:02Z

Riot/ViewController/ReadReceiptsViewController.h

+
+@interface ReadReceiptsViewController : UIViewController
+
+ (void)openInViewController:(UIViewController *)viewController withRestClient:(MXRestClient *)restClient session:(MXSession *)session withRoomMembers:(NSArray <MXRoomMember *> *)roomMembers placeholders:(NSArray <UIImage *> *)placeholders receipts:(NSArray <MXReceiptData *> *)receipts;


Reduce the number of parameters by passing directly the MXKReceiptSendersContainer instance

giomfo · 2017-06-27T22:56:53Z

Riot/ViewController/ReadReceiptsViewController.m

+- (void)configureViews
+{
+    self.containerView.layer.cornerRadius = 20;
+    self.titleLabel.text = @"Read Receipts List";


Define a new key-value in Vector.strings to handle translation of this title.
Then use the macro:
NSLocalizedStringFromTable(@"xxx", @"Vector", nil), where xxx is your key.

giomfo · 2017-06-27T22:59:11Z

Riot/ViewController/ReadReceiptsViewController.m

+    if (indexPath.row < self.receipts.count)
+    {
+        NSString *receiptDescription = [(MXKEventFormatter*)self.session.roomSummaryUpdateDelegate dateStringFromTimestamp:self.receipts[indexPath.row].ts withTime:YES];
+        cell.receiptDescriptionLabel.text = receiptDescription;


You may use attributed string to include the prefix "Read:" in this description.
(see https://github.com/vector-im/riot-ios/blob/master/Riot/Model/Contact/ContactsDataSource.m#L658 for an example of NSMutableAttributedString use)

…Vector.strings for translation

…an/riot-ios into read-receipts-details

… now sending the container itself

giomfo

LGTM, just few remarks
Please sign off

giomfo · 2017-06-29T14:38:02Z

Riot.xcodeproj/project.pbxproj

@@ -2405,7 +2414,7 @@
 			);
 			runOnlyForDeploymentPostprocessing = 0;
 			shellPath = /bin/sh;
-			shellScript = "diff \"${PODS_PODFILE_DIR_PATH}/Podfile.lock\" \"${PODS_ROOT}/Manifest.lock\" > /dev/null\nif [ $? != 0 ] ; then\n    # print error to STDERR\n    echo \"error: The sandbox is not in sync with the Podfile.lock. Run 'pod install' or update your CocoaPods installation.\" >&2\n    exit 1\nfi\n";
+			shellScript = "diff \"${PODS_ROOT}/../Podfile.lock\" \"${PODS_ROOT}/Manifest.lock\" > /dev/null\nif [ $? != 0 ] ; then\n    # print error to STDERR\n    echo \"error: The sandbox is not in sync with the Podfile.lock. Run 'pod install' or update your CocoaPods installation.\" >&2\n    exit 1\nfi\n";


Please find the reason of this change on shellScript. Which pod version are you using? FYI we use 1.2.1.
Discard this change from your PR if this is simple for you. Don't waste time on this, I can handle it on my side.

giomfo · 2017-06-29T14:40:43Z

Riot/ViewController/ReadReceiptsViewController.m

+
+#pragma mark - Public
+
+ (void)openInViewController:(UIViewController *)viewController fromContainer:(MXKReceiptSendersContainer *)receiptSendersContainer withsession:(MXSession *)session


typo withSession instead of withsession

giomfo · 2017-06-29T14:42:47Z

Riot/ViewController/ReadReceiptsViewController.m

+    }
+    if (indexPath.row < self.receipts.count)
+    {
+        NSString *receiptReadText = [NSBundle mxk_localizedStringForKey:@"receipt_status_read"];


Use NSLocalizedStringFromTable(@"receipt_status_read", @"Vector", nil) when you will have moved the key-value receipt_status_read=xxx in Vector.strings

giomfo · 2017-06-29T14:45:24Z

Riot/ViewController/ReadReceiptsViewController.m

+    {
+        NSString *receiptReadText = [NSBundle mxk_localizedStringForKey:@"receipt_status_read"];
+        NSString *receiptTimeText = [(MXKEventFormatter*)self.session.roomSummaryUpdateDelegate dateStringFromTimestamp:self.receipts[indexPath.row].ts withTime:YES];
+        cell.receiptDescriptionLabel.text = receiptTimeText;


Remove this line, you don't have to set cell.receiptDescriptionLabel.text.

…. Signed-off-by: Aram Sargsyan [email protected]

aramsargsyan added 4 commits June 21, 2017 22:28

Created ReadReceiptsViewController

6243eec

Modified ReadReceiptsViewController to download resized avatars of us…

ab5c0e6

…ers. If the user doesn't have an avatar, the placeholder is placed.

Merge branch 'develop' of https://github.com/aramsargsyan/riot-ios in…

ee47992

…to read-receipts-details

Minor changes

6b9465f

aramsargsyan mentioned this pull request Jun 21, 2017

Chat: Expand read receipts when user taps on it #59

Closed

morozkin reviewed Jun 22, 2017

View reviewed changes

Review changes

f32ccae

giomfo reviewed Jun 23, 2017

View reviewed changes

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

Removed the ReceiptsContainerDelegate, used didRecognizeAction: instead

e688c61

Using Attributed string for read label

6f08b14

Review changes

74455c0

Read Receipts Details - RoomDataSource: Add RR container in the cell …

d736006

…content view instead of the overlay container to not disturb tag gesture handling. element-hq#59

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

6b25f8f

Read Receipts Details - RoomDataSource

giomfo reviewed Jun 27, 2017

View reviewed changes

aramsargsyan added 3 commits June 29, 2017 15:05

Handled the creation of the attributed receipt description, added in …

9bb1897

…Vector.strings for translation

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

ea3ae98

…an/riot-ios into read-receipts-details

Decreased the parameter cound for ReadReceiptsViewController opening,…

e17622b

… now sending the container itself

giomfo reviewed Jun 29, 2017

View reviewed changes

Brought the key receipt_status_read to app level + some minor changes…

f287dc4

…. Signed-off-by: Aram Sargsyan [email protected]

giomfo approved these changes Jun 30, 2017

View reviewed changes

giomfo merged commit 3fac2a0 into element-hq:develop Jun 30, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read receipts details #1341

Read receipts details #1341

aramsargsyan commented Jun 21, 2017

morozkin Jun 22, 2017

morozkin Jun 22, 2017

morozkin Jun 22, 2017

morozkin left a comment

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 Jun 27, 2017

giomfo Jun 27, 2017

giomfo Jun 27, 2017 •

edited

Loading

giomfo left a comment

giomfo Jun 29, 2017

giomfo Jun 29, 2017

giomfo Jun 29, 2017

giomfo Jun 29, 2017


		@interface ReadReceiptsViewController : UIViewController

		+ (void)openInViewController:(UIViewController )viewController withRestClient:(MXRestClient )restClient withRoomMembers:(NSArray <MXRoomMember > )roomMembers placeholders:(NSArray <UIImage > )placeholders receiptDescriptions:(NSArray <NSString > )receiptDescriptions;

		@@ -17,6 +17,7 @@

		#import "RoomDataSource.h"

		#import "AppDelegate.h"


		#pragma mark - Public

		+ (void)openInViewController:(UIViewController )viewController fromContainer:(MXKReceiptSendersContainer )receiptSendersContainer withsession:(MXSession *)session

Read receipts details #1341

Read receipts details #1341

Conversation

aramsargsyan commented Jun 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giomfo Jun 27, 2017 • edited Loading

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

giomfo Jun 27, 2017 •

edited

Loading