Page MenuHomePhabricator
Differential D7065

[native] Added displaying labels to InlineEngagement
ClosedPublic
Actions

Authored by • kuba on Mar 14 2023, 1:51 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Mar 25, 1:21 PM
Unknown Object (File)
Mon, Mar 17, 7:27 PM
Unknown Object (File)
Mon, Mar 17, 7:27 PM
Unknown Object (File)
Mon, Mar 17, 7:27 PM
Unknown Object (File)
Mon, Mar 17, 7:27 PM
Unknown Object (File)
Mon, Mar 17, 7:27 PM
Unknown Object (File)
Mon, Mar 17, 7:27 PM
Unknown Object (File)
Mon, Mar 17, 7:27 PM
View All Files
Subscribers
ashoat
atul
tomek

Details

Reviewers
michal
inka
ginsu
ashoat
Commits
rCOMMc22513111262: [native] Added displaying labels to InlineEngagement
Summary

Added possibility to label the messages to display ‘Edited' label in next diffs.

Test Plan
  • Run the app.
  • Added the label to every message.
  • Checked if the labels are displayed correctly both with text messages and with images.
  • Checked if Reactions/Threads are also correctly positioned next to the label.
  • Checked if labels in a few messages in a row do not take too much space.
  • Checked displaying on both the left and right side of the chat.
  • Checked displaying on both iOS and Android.
  • Checked displaying on both dark and light mode.

Screenshots (iOS / Android):

  • Light mode:

Screenshot 2023-03-14 at 09.33.12.png (948×1 px, 523 KB)

  • Dark mode:

Screenshot 2023-03-14 at 09.35.55.png (680×1 px, 302 KB)

  • Few messages in a row:

Screenshot 2023-03-14 at 09.50.23.png (664×1 px, 261 KB)

  • Image:

Screenshot 2023-03-14 at 09.35.26.png (856×1 px, 402 KB)

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

• kuba created this revision.Mar 14 2023, 1:51 AM
Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptMar 14 2023, 1:51 AM
• kuba added a parent revision: D7063: [native] Move 'InlineEngagement' styling from 'ComposedMessage' to component.Mar 14 2023, 1:52 AM
• kuba edited the test plan for this revision. (Show Details)Mar 14 2023, 1:54 AM
• kuba added a child revision: D7066: [native] Displaying 'Edit' labels next to edited messages.
Harbormaster completed remote builds in B17311: Diff 23710.Mar 14 2023, 2:07 AM
• kuba requested review of this revision.Mar 14 2023, 2:07 AM
• kuba planned changes to this revision.Mar 21 2023, 10:42 AM

The same comments as in: https://phab.comm.dev/D7017?id=23714#inline-46406

• kuba updated this revision to Diff 23953.Mar 21 2023, 11:16 AM
• kuba edited the test plan for this revision. (Show Details)

Addressed @ginsu feedback from the web version of diff

Harbormaster completed remote builds in B17485: Diff 23953.Mar 21 2023, 11:31 AM
• kuba updated this revision to Diff 23976.Mar 22 2023, 4:40 AM

Added unique keys to the rendered components

Harbormaster completed remote builds in B17508: Diff 23976.Mar 22 2023, 4:56 AM
ashoat added a comment.Mar 22 2023, 2:04 PM

Will this affect the height of the item? Have you made the necessary changes to the height determination code (textMessageItemHeight)?

• kuba updated this revision to Diff 24071.Mar 24 2023, 5:35 AM

Rebase

Harbormaster completed remote builds in B17583: Diff 24071.Mar 24 2023, 5:50 AM
ashoat requested changes to this revision.Mar 24 2023, 11:55 AM
In D7065#212101, @ashoat wrote:

Will this affect the height of the item? Have you made the necessary changes to the height determination code (textMessageItemHeight)?

Requesting changes for my question above

This revision now requires changes to proceed.Mar 24 2023, 11:55 AM
• kuba added a comment.Mar 28 2023, 12:48 AM
In D7065#212932, @ashoat wrote:
In D7065#212101, @ashoat wrote:

Will this affect the height of the item? Have you made the necessary changes to the height determination code (textMessageItemHeight)?

Requesting changes for my question above

Done in https://phab.comm.dev/D7066, because that is where it was used for the first time.

• kuba requested review of this revision.Mar 28 2023, 1:05 AM
ashoat requested changes to this revision.Mar 28 2023, 5:41 AM

Just nits, but requesting changes because some of them are non-obvious and I want to confirm it's implemented right before you land :)

native/chat/inline-engagement.react.js
130–132

We can move this below the !label check to avoid unnecessary computation

142

Use "De Morgan's law"

172

Please always avoid inline logic from within JSX. JSX should be a "leaf node" in the AST

This revision now requires changes to proceed.Mar 28 2023, 5:41 AM
• kuba mentioned this in D7063: [native] Move 'InlineEngagement' styling from 'ComposedMessage' to component.Mar 28 2023, 8:56 AM
• kuba updated this revision to Diff 24289.Mar 28 2023, 9:20 AM
• kuba marked 3 inline comments as done.

Responded to review

Harbormaster completed remote builds in B17749: Diff 24289.Mar 28 2023, 9:35 AM
ashoat accepted this revision.Mar 28 2023, 11:24 AM
This revision is now accepted and ready to land.Mar 28 2023, 11:24 AM
• kuba updated this revision to Diff 24373.Mar 30 2023, 1:27 AM

Created new color in theme for messageLabel

Harbormaster completed remote builds in B17807: Diff 24373.Mar 30 2023, 1:43 AM
• kuba updated this revision to Diff 24374.Mar 30 2023, 1:52 AM

Add explicitly fontSize for label to prevent messageHeight error

Harbormaster completed remote builds in B17808: Diff 24374.Mar 30 2023, 2:07 AM
Closed by commit rCOMMc22513111262: [native] Added displaying labels to InlineEngagement (authored by kosmydel <kukubaczek@gmail.com>). · Explain WhyMar 30 2023, 7:43 AM
This revision was automatically updated to reflect the committed changes.
kosmydel <kukubaczek@gmail.com> added a commit: rCOMMc22513111262: [native] Added displaying labels to InlineEngagement.

Revision Contents
Changeset List

PathSize
native/
chat/
47 lines

Diff 24071

View Options

native/chat/inline-engagement.react.js

#include "DatabaseManagerBase.h" #include "DatabaseManagerBase.h"
#include "Item.h" #include "Item.h"
#include <aws/core/utils/Outcome.h> #include <aws/core/utils/Outcome.h>
#include <aws/dynamodb/model/BatchWriteItemRequest.h>
#include <aws/dynamodb/model/BatchWriteItemResult.h>
#include <aws/dynamodb/model/DeleteItemRequest.h> #include <aws/dynamodb/model/DeleteItemRequest.h>
#include <iostream> #include <iostream>
namespace comm { namespace comm {
namespace network { namespace network {
namespace database { namespace database {
Show All 23 Linesvoid DatabaseManagerBase::innerRemoveItem(const Item &item) {
const Aws::DynamoDB::Model::DeleteItemOutcome &outcome = const Aws::DynamoDB::Model::DeleteItemOutcome &outcome =
getDynamoDBClient()->DeleteItem(request); getDynamoDBClient()->DeleteItem(request);
if (!outcome.IsSuccess()) { if (!outcome.IsSuccess()) {
throw std::runtime_error(outcome.GetError().GetMessage()); throw std::runtime_error(outcome.GetError().GetMessage());
} }
} }
void DatabaseManagerBase::innerBatchWriteItem(
const std::string &tableName,
std::vector<Aws::DynamoDB::Model::WriteRequest> &writeRequests) {
// Split write requests to chunks by MAX_DYNAMODB_BATCH_ITEMS size and write
// them by batch
Aws::DynamoDB::Model::BatchWriteItemOutcome outcome;
std::vector<Aws::DynamoDB::Model::WriteRequest> writeRequestsChunk;
std::vector<Aws::DynamoDB::Model::WriteRequest>::iterator chunkPositionStart,
chunkPositionEnd;
for (size_t i = 0; i < writeRequests.size(); i += MAX_DYNAMODB_BATCH_ITEMS) {
chunkPositionStart = writeRequests.begin() + i;
chunkPositionEnd = writeRequests.begin() +
std::min(writeRequests.size(), i + MAX_DYNAMODB_BATCH_ITEMS);
writeRequestsChunk = std::vector<Aws::DynamoDB::Model::WriteRequest>(
chunkPositionStart, chunkPositionEnd);
tomekUnsubmitted
Done
Inline Actions
Aws::DynamoDB::Model::BatchWriteItemOutcome outcome;
- std::vector<Aws::DynamoDB::Model::WriteRequest> writeRequestsChunk;
- std::vector<Aws::DynamoDB::Model::WriteRequest>::iterator chunkPositionStart,
- chunkPositionEnd;
for (size_t i = 0; i < writeRequests.size(); i += MAX_DYNAMODB_BATCH_ITEMS) {
- chunkPositionStart = writeRequests.begin() + i;
- chunkPositionEnd = writeRequests.begin() +
+ auto chunkPositionStart = writeRequests.begin() + i;
+ auto chunkPositionEnd = writeRequests.begin() +
std::min(writeRequests.size(), i + MAX_DYNAMODB_BATCH_ITEMS);
- writeRequestsChunk = std::vector<Aws::DynamoDB::Model::WriteRequest>(
+ auto writeRequestsChunk = std::vector<Aws::DynamoDB::Model::WriteRequest>(
chunkPositionStart, chunkPositionEnd);
Aws::DynamoDB::Model::BatchWriteItemRequest writeBatchRequest;

You can simplify the code by avoiding reusing the variables and using auto

tomek: You can simplify the code by avoiding reusing the variables and using `auto`
maxAuthorUnsubmitted
Done
Inline Actions

You can simplify the code by avoiding reusing the variables and using auto

I'm not a big fan of using auto where the type can be declared explicitly. Also, reusing the same variables is a part of loop optimization here (we don't create new ones inside the loop and reuse existing allocated memory for it).

max: > You can simplify the code by avoiding reusing the variables and using `auto` I'm not a big…
tomekUnsubmitted
Not Done
Inline Actions

Also, reusing the same variables is a part of loop optimization here

Have you measured the performance difference? I don't think this type of premature optimization is beneficial - if the variable is used only locally, it is easier to reason about it: you don't have to think about its initial value and about possible usages after the loop. Also, having a variable defined inside a loop might enable some optimization (I'm guessing now). Currently, we only reuse the name - new variables are constructed in every iteration.

So overall, we have a lot more important performance issues and saving a couple of machine instructions wouldn't be noticeable. On the other hand, from readability point of view, this is a noticeable difference - you are certain that the variable is used only where it is valid.

tomek: > Also, reusing the same variables is a part of loop optimization here Have you measured the…
Aws::DynamoDB::Model::BatchWriteItemRequest writeBatchRequest;
writeBatchRequest.AddRequestItems(tableName, writeRequestsChunk);
outcome = getDynamoDBClient()->BatchWriteItem(writeBatchRequest);
if (!outcome.IsSuccess()) {
throw std::runtime_error(outcome.GetError().GetMessage());
}
tomekUnsubmitted
Done
Inline Actions
writeBatchRequest.AddRequestItems(tableName, writeRequestsChunk);
- outcome = getDynamoDBClient()->BatchWriteItem(writeBatchRequest);
+ auto outcome = getDynamoDBClient()->BatchWriteItem(writeBatchRequest);
if (!outcome.IsSuccess()) {
throw std::runtime_error(outcome.GetError().GetMessage());
}
}
}

outcome isn't used outside of this loop so we can declare it here

tomek: `outcome` isn't used outside of this loop so we can declare it here
maxAuthorUnsubmitted
Done
Inline Actions

outcome isn't used outside of this loop so we can declare it here

Same as above. I'm sure we should not use auto, where the type can explicitly be used, and allocating and reusing this variable here, is an optimized way from my perspective.

max: > `outcome` isn't used outside of this loop so we can declare it here Same as above. I'm sure…
}
}
} // namespace database } // namespace database
} // namespace network } // namespace network
} // namespace comm } // namespace comm