Skip to content

Comments

[NEW] Add recording, uploading activity indicators#3243

Closed
sumukhah wants to merge 74 commits intoRocketChat:developfrom
sumukhah:activity-indicator
Closed

[NEW] Add recording, uploading activity indicators#3243
sumukhah wants to merge 74 commits intoRocketChat:developfrom
sumukhah:activity-indicator

Conversation

@sumukhah
Copy link
Contributor

@sumukhah sumukhah commented Jun 28, 2021

Setup #222392 on server.

Recording, Uploading activities

Screen.Recording.2021-11-22.at.23.00.01.mov

Thread activities

Screen.Recording.2021-11-22.at.23.01.23.mov

Proposed changes

Issue(s)

How to test or reproduce

First of all, you need to check which version type of the server you are on. If you are in the servers with a version equal to or greater than 4.0.0 the indicators will appear as demonstrated in the video, and will happen for rooms and threads. Adding to this, the test plan is:

  • You should be logged with 2 different accounts into different devices.
  • Then go to the same room, with both users
  • And when you are typing/recording/uploading with one account, should appear the same indicator to the other user
  • Besides, if you are uploading a bigger file and in parallel is typing, the other user should read that: you are uploading, you are typing
  • And vice versa too

Although, if you are on a server with a version less than 4.0.0 the indicators should occur just into rooms and only for typing

Screenshots

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • Improvement (non-breaking change which improves a current function)
  • New feature (non-breaking change which adds functionality)
  • Documentation update (if none of the other choices apply)

Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works (if applicable)
  • I have added necessary documentation (if applicable)
  • Any dependent changes have been merged and published in downstream modules

Further comments

web app integration

#222327

@sumukhah sumukhah changed the title [NEW] Add thread activity indicators [NEW] Add recording, uploading activity indicators Jun 28, 2021
@sumukhah sumukhah force-pushed the activity-indicator branch from 5c71f29 to 2faf9b2 Compare June 28, 2021 15:04
@lgtm-com
Copy link

lgtm-com bot commented Jun 28, 2021

This pull request introduces 1 alert when merging 2faf9b2 into a6ded95 - view on LGTM.com

new alerts:

  • 1 for Expression has no effect

Comment on lines 102 to 105
if (this.actionTimeouts?.[roomId]) {
clearTimeout(this.actionTimeouts[roomId]);
delete this.actionTimeouts[roomId];
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this the correct way?

Comment on lines 27 to 29
if (id in activityRenews && status === true) {
yield delay(2000);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it a correct way of limiting server requests?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@diegolmello I think only you or your team can answer here, I don't have that knowledge

const { user } = login;
const name = UI_Use_Real_Name ? user.name : user.username;
return this.methodCall('stream-notify-room', `${ room }/typing`, name, typing);
const stream = Use_New_Activity_Indicators ? 'user-activity' : 'typing';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should emit both if you accept my comment on the other PR

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can make the backend control this and emit the old event as well if the setting is enabled. Something to check

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you need to check the server version and only emit the new event when the connected server is the version containing this improvement. We still don't know when it will be released but you can keep a specific number and make a note in the PR's description to update it when we have the right info.

return <ActivityIndicator action='uploading' users={users} />;
}

return null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return null;
return;

Copy link
Contributor Author

@sumukhah sumukhah Aug 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it is a component. 'return;' is not returning the "null", getting an error with eslint.

Comment on lines 27 to 29
if (id in activityRenews && status === true) {
yield delay(2000);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@diegolmello I think only you or your team can answer here, I don't have that knowledge

sumukhah and others added 4 commits August 4, 2021 16:21
Co-authored-by: Rodrigo Nascimento <rodrigoknascimento@gmail.com>
Handle both versions of activities, code optimization
@rodrigok
Copy link
Member

I've changed the event format, need to update this PR

@sumukhah
Copy link
Contributor Author

okay I'll update it very soon

@sumukhah sumukhah force-pushed the activity-indicator branch 2 times, most recently from ec37259 to 39bdbb8 Compare September 25, 2021 14:38
@lgtm-com
Copy link

lgtm-com bot commented Sep 30, 2021

This pull request introduces 5 alerts when merging b7792c9 into 5bb5d54 - view on LGTM.com

new alerts:

  • 4 for Unused variable, import, function or class
  • 1 for Invocation of non-function

};
}

export function clearUserActivity(username, rid, tmid) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing activity

case USERS_ACTIVITY.ADD:
if (state[roomId]) {
const obj = state[roomId];
delete obj[username];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lgtm-com
Copy link

lgtm-com bot commented Dec 21, 2021

This pull request introduces 4 alerts when merging 603dd06 into b75e192 - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class
  • 2 for Invocation of non-function

@lgtm-com
Copy link

lgtm-com bot commented Dec 21, 2021

This pull request introduces 5 alerts when merging 6913f1a into b75e192 - view on LGTM.com

new alerts:

  • 3 for Unused variable, import, function or class
  • 2 for Invocation of non-function

@lgtm-com
Copy link

lgtm-com bot commented Dec 22, 2021

This pull request introduces 5 alerts when merging e490d94 into b75e192 - view on LGTM.com

new alerts:

  • 3 for Unused variable, import, function or class
  • 2 for Invocation of non-function

@diegolmello diegolmello closed this Feb 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants