Skip to content

Fix "New" marker for the chat #1977

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Gonals wants to merge 14 commits into from
Closed

Fix "New" marker for the chat #1977

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
d6d7efa
fix and revamp new marker
Gonals Mar 19, 2021
74d7b5c
keep the new marker visible
Gonals Mar 22, 2021
7700a2a
correctly place marker when new messages come in
Gonals Mar 22, 2021
79d18ca
merge master
Gonals Mar 22, 2021
575ebe1
fix merge master error
Gonals Mar 22, 2021
6394e39
store the sequence instead of the actioncount
Gonals Mar 23, 2021
2536b82
Merge branch 'master' into alberto-fixNewMarker
Gonals Mar 29, 2021
4ef6b21
refactor timer setting
Gonals Mar 29, 2021
f4b5d79
Fix marker on Mobile
Gonals Mar 30, 2021
5d1cc73
remove log file
Gonals Mar 31, 2021
928a2b8
add conditions
Gonals Mar 31, 2021
0188cb9
Move some code around
Gonals Apr 1, 2021
4e6a172
style
Gonals Apr 1, 2021
fde2830
merge master
Gonals Apr 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 5 additions & 17 deletions src/components/UnreadActionIndicator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,29 +1,17 @@
import React from 'react';
import {Animated, View} from 'react-native';
import PropTypes from 'prop-types';
import styles, {getOpacityStyle} from '../styles/styles';
import {View} from 'react-native';
Comment thread
Gonals marked this conversation as resolved.
import styles from '../styles/styles';
import Text from './Text';

const propTypes = {
// Animated opacity
// eslint-disable-next-line react/forbid-prop-types
animatedOpacity: PropTypes.object.isRequired,
};

const UnreadActionIndicator = props => (
<Animated.View style={[
styles.unreadIndicatorContainer,
getOpacityStyle(props.animatedOpacity),
]}
>
const UnreadActionIndicator = () => (
<View style={styles.unreadIndicatorContainer}>
<View style={styles.unreadIndicatorLine} />
<Text style={styles.unreadIndicatorText}>
NEW
</Text>
</Animated.View>
</View>
);

UnreadActionIndicator.propTypes = propTypes;
UnreadActionIndicator.displayName = 'UnreadActionIndicator';

export default UnreadActionIndicator;
6 changes: 6 additions & 0 deletions src/pages/home/report/ReportActionItem.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import PopoverWithMeasuredContent from '../../../components/PopoverWithMeasuredC
import ReportActionItemSingle from './ReportActionItemSingle';
import ReportActionItemGrouped from './ReportActionItemGrouped';
import ReportActionContextMenu from './ReportActionContextMenu';
import UnreadActionIndicator from '../../../components/UnreadActionIndicator';

const propTypes = {
// The ID of the report this action is on.
Expand All @@ -27,6 +28,9 @@ const propTypes = {
// Should the comment have the appearance of being grouped with the previous comment?
displayAsGroup: PropTypes.bool.isRequired,

// Should we display the new indicator on top of the comment?
displayNewIndicator: PropTypes.bool.isRequired,

/* --- Onyx Props --- */
// List of betas for the current user.
betas: PropTypes.arrayOf(PropTypes.string),
Expand Down Expand Up @@ -58,6 +62,7 @@ class ReportActionItem extends Component {
shouldComponentUpdate(nextProps, nextState) {
return this.state.isPopoverVisible !== nextState.isPopoverVisible
|| this.props.displayAsGroup !== nextProps.displayAsGroup
|| (this.props.displayNewIndicator !== nextProps.displayNewIndicator)
|| !_.isEqual(this.props.action, nextProps.action);
}

Expand Down Expand Up @@ -109,6 +114,7 @@ class ReportActionItem extends Component {
<Hoverable>
{hovered => (
<View>
{this.props.displayNewIndicator && <UnreadActionIndicator />}
<View style={getReportActionItemStyle(hovered)}>
{!this.props.displayAsGroup
? <ReportActionItemSingle action={this.props.action} />
Expand Down
72 changes: 32 additions & 40 deletions src/pages/home/report/ReportActionsView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import React from 'react';
import {
Animated,
View,
Keyboard,
AppState,
Expand All @@ -11,7 +10,6 @@ import _ from 'underscore';
import lodashGet from 'lodash/get';
import {withOnyx} from 'react-native-onyx';
import Text from '../../../components/Text';
import UnreadActionIndicator from '../../../components/UnreadActionIndicator';
import {
fetchActions,
updateLastReadActionID,
Expand Down Expand Up @@ -69,16 +67,10 @@ class ReportActionsView extends React.Component {
this.recordMaxAction = this.recordMaxAction.bind(this);
this.onVisibilityChange = this.onVisibilityChange.bind(this);
this.loadMoreChats = this.loadMoreChats.bind(this);
this.scheduledRecordMaxAction = this.scheduledRecordMaxAction.bind(this);
this.sortedReportActions = [];
this.timers = [];
this.unreadIndicatorOpacity = new Animated.Value(1);

// Helper variable that keeps track of the unread action count before it updates to zero
this.unreadActionCount = 0;

// Helper variable that prevents the unread indicator to show up for new messages
// received while the report is still active
this.shouldShowUnreadActionIndicator = true;
this.unreadTimer = null;
this.newMessageMarkerPosition = -1;

@marcaaron marcaaron Mar 26, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why are we setting this to -1?

@Gonals Gonals Mar 29, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Just initializing it to a non-valid position so we know is has yet to be set. I could initialize it to null, probably, if you like that better.

@marcaaron marcaaron Mar 31, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmm... I'm not entirely sold we need this variable yet. But if we are making -1 have a special meaning then maybe we should use a constant like NON_VALID_POSITION.

@Gonals Gonals Mar 31, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm ok with that. If we keep the variable, we can do that :)


this.state = {
isLoadingMoreChats: false,
Expand Down Expand Up @@ -127,10 +119,9 @@ class ReportActionsView extends React.Component {
this.scrollToListBottom();
}

// When the last action changes, wait three seconds, then record the max action
// This will make the unread indicator go away if you receive comments in the same chat you're looking at
Comment thread
marcaaron marked this conversation as resolved.
if (Visibility.isVisible()) {
this.timers.push(setTimeout(this.recordMaxAction, 3000));
// If we are adding a new action while the chat is open, record the max action immediately
if (!this.unreadTimer && Visibility.isVisible()) {
this.scheduledRecordMaxAction();
}
}
}
Expand All @@ -142,7 +133,10 @@ class ReportActionsView extends React.Component {

AppState.removeEventListener('change', this.onVisibilityChange);

_.each(this.timers, timer => clearTimeout(timer));
if (this.unreadTimer) {
clearTimeout(this.unreadTimer);
this.unreadTimer = null;
}
unsubscribeFromReportChannel(this.props.reportID);
}

Expand All @@ -151,33 +145,21 @@ class ReportActionsView extends React.Component {
*/
onVisibilityChange() {
if (Visibility.isVisible()) {
this.timers.push(setTimeout(this.recordMaxAction, 3000));
this.unreadTimer = setTimeout(this.scheduledRecordMaxAction, 3000);
}
}

/**
* Checks if the unreadActionIndicator should be shown.
* If it does, starts a timeout for the fading out animation and creates
* a flag to not show it again if the report is still open
* Sets the max action after a set delay
*/
setUpUnreadActionIndicator() {
if (!this.shouldShowUnreadActionIndicator) {
return;
}

this.unreadActionCount = this.props.report.unreadActionCount;

if (this.unreadActionCount > 0) {
this.unreadIndicatorOpacity = new Animated.Value(1);
this.timers.push(setTimeout(() => {
Animated.timing(this.unreadIndicatorOpacity, {
toValue: 0,
useNativeDriver: false,
}).start();
}, 3000));
scheduledRecordMaxAction() {
// Always cancel the existing timer

@marcaaron marcaaron Apr 1, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What happens if we do not cancel this timer ?

@Gonals Gonals Apr 1, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Related to the answer above: I get rid of it so I could check for existence accurately (There is no "is there a timer running?" function)

if (this.unreadTimer) {
clearTimeout(this.unreadTimer);
this.unreadTimer = null;
}

this.shouldShowUnreadActionIndicator = false;
this.recordMaxAction();
}

/**
Expand All @@ -192,6 +174,12 @@ class ReportActionsView extends React.Component {

// Fetch the new set of actions
fetchActions(this.props.reportID);
this.newMessageMarkerPosition = -1;

// Wait three seconds, then record the max action
if (Visibility.isVisible()) {
this.unreadTimer = setTimeout(this.scheduledRecordMaxAction, 3000);
}
}

/**
Expand Down Expand Up @@ -335,15 +323,14 @@ class ReportActionsView extends React.Component {
// <InvertedFlatList /> are implemented on native and web/desktop which leads to
// the unread indicator on native to render below the message instead of above it.
<View>
{this.unreadActionCount > 0 && index === this.unreadActionCount - 1 && (
<UnreadActionIndicator animatedOpacity={this.unreadIndicatorOpacity} />
)}
<ReportActionItem
reportID={this.props.reportID}
action={item.action}
displayAsGroup={this.isConsecutiveActionMadeByPreviousActor(index)}
onLayout={onLayout}
needsLayoutCalculation={needsLayoutCalculation}
displayNewIndicator={this.newMessageMarkerPosition > 0
&& item.action.sequenceNumber === this.newMessageMarkerPosition}
/>
</View>
);
Expand All @@ -364,7 +351,12 @@ class ReportActionsView extends React.Component {
);
}

this.setUpUnreadActionIndicator();
if (this.newMessageMarkerPosition < 0) {
this.newMessageMarkerPosition = this.props.report.unreadActionCount === 0

@marcaaron marcaaron Mar 26, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder if we really need this variable, but correct me if this sounds wrong...

Since the newMessageMarkerPosition can always be inferred by looking at props we can instead make a method that calculates the marker position + tells us if a current sequenceNumber matches? I think it would be cleaner to call a method in renderItem that does this so that the logic isn't all over the place.

I think that will eliminate the need to "reset" this variable back to -1, but haven't tested this out.

@Gonals Gonals Mar 29, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

newMessageMarkerPosition can't really be always inferred by looking at props, right? Once we update the unreadActionCount (which gets updated when we update the last action), we'll lose that info.
I've been trying to get rid of this variable, but nothing I could think of really worked (for a "permanent" marker), so I'm down for any suggestions.

@marcaaron marcaaron Mar 31, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It seems there are really only two things we need to know, yet we are expressing it in a single variable (and also updating it in a render method). This makes everything a little harder to understand IMO. Hopefully this helps think about the problem in a different way.

We need to know...

  1. Whether we should show a new message marker at all. something like shouldShowNewMessageMarker stored in state could tell us this. When would we need to hide and show this marker?
  2. If we should show the new message marker what sequenceNumber should we show it at? The easier question, as this can always be known by looking at the props like we are already doing here.

@Gonals Gonals Mar 31, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

  1. Yep, we can totally have that piece of data in a different variable or in the state if we want.
  2. Nope. That's the issue I've been fighting with. this.props.report.unreadActionCount gets updated to zero after the 3 seconds timer ends. At that point, if we haven't saved the sequence in a variable, we lose it. When a new message comes in and the component gets rerendered, we'll lose the marker.

@marcaaron marcaaron Apr 1, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry for all the back and forth here. I'm having a hard time understanding things, but I think turning a corner on a real suggestion. 😅

we can totally have that piece of data in a different variable or in the state if we want

Ok I think actually such a variable is not necessary, but it's an interesting exercise to think about when it might be true.

I think it'd be true when our unreadActionCount > 0 (you've got that part), but we need to use the unread count when the report view mounts (set in the constructor) or we switch to a new report.

This is where things get unclear for me...

this.props.report.unreadActionCount gets updated to zero after the 3 seconds timer ends. At that point, if we haven't saved the sequence in a variable, we lose it. When a new message comes in and the component gets rerendered, we'll lose the marker.

Right now we are setting this to -1 when the reportID changes and then updating it in the render() method when it's less than 0. This seems like kind of an anti-pattern because we're using -1 as a "signal" to the render() method that it needs to save this reference. That's not really the job of the render method and we should be able to save this reference when the component mounts or when the reportID changes.

So we should be able to end up with something like:

  • Set this.initialUnreadActionCount in componentDidMount and when we switch to a new report (reset())
  • Do the same checks we already have in renderItem()

I think that would clean things up a bit. Does that sound like it might work?

@Gonals Gonals Apr 1, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Haha, no worries! I also thought this would be simpler than it ended up being. This didn't work for a couple of reasons:

  • I can't get rid of the -1 check (unless we use a different variable):
    When we swap reports, componentDidUpdate is actually called twice. Once with prevProps.reportID !== newProps.reportID, which is when we call reset, and once more afterwards to actually update the report contents.
    On the first call, we still don't have the correct unreadActionCount, so I can't set the variable at that point. We need to wait till the second round, so I still need to check that "set this only once" flag (the -1, in this case).
  • I still tried moving the logic to componentDidUpdate so it wouldn't be in render, but since this change does't trigger a rerender, even though the variable gets set correctly, the marker doesn't show up until something else happens. Maybe I could force a redraw, but we probably want to avoid that. 😢

@marcaaron marcaaron Apr 2, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ok, thanks for explaining. It seems clear what is happening here is that there is one update for the reportID then another update when withOnyx re-fetches anything with keys depending on props (so reportActions and report in this case).

Seems like we have two options...

  1. Implement this workaround for now (maybe try to add a lot of comments to explain why we need to do what we're doing)
  2. Fix Onyx so that we don't run into this situation again in the future

What would you prefer to do?

My expectation is that if a key depends on a prop and the prop it depends on changes we should check to see if we can also batch additional updates together instead of causing an extra re-render. I'm going to start looking into a solution on the Onyx side regardless and if we want to hold off for that, cool.

Otherwise, we can go ahead with the workaround, but here's what I'd suggest to make everything a little clearer...

  1. In reset() set a flag on an instance variable called this.didReportIDChange to true
  2. In componentDidUpdate() check to see if this is equal to true
  3. Create a method called this.getInitialUnreadCount() and use is to initialize a variable in state this.state.initialUnreadActionCount in the constructor
  4. In componentDidUpdate() check for this.didReportIDChange and use getInitialUnreadCount() to update the value of this.state.initialUnreadActionCount and set this.didReportIDChange back to false.

This will "force" an update. But I can't say whether that is a problem or not. I'd think maybe not.

? this.props.report.unreadActionCount
: _.size(this.props.reportActions) - this.props.report.unreadActionCount;
}

this.updateSortedReportActions();
return (
<InvertedFlatList
Expand Down
3 changes: 2 additions & 1 deletion src/styles/styles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -833,7 +833,7 @@ const styles = {
left: 0,
top: 0,
bottom: 0,
zIndex: 2,
zIndex: variables.zIndexTop,
},

navigationModalOverlay: {
Expand Down Expand Up @@ -1154,6 +1154,7 @@ const styles = {
paddingHorizontal: 20,
flexDirection: 'row',
alignItems: 'center',
zIndex: variables.zIndexMiddle,
},

unreadIndicatorLine: {
Expand Down
2 changes: 2 additions & 0 deletions src/styles/variables.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,5 +23,7 @@ export default {
safeInsertPercentage: 0.7,
sideBarWidth: 375,
pdfPageMaxWidth: 992,
zIndexMiddle: 50,
zIndexTop: 999,
tooltipzIndex: 10050,
};