Fix join/part collapsing regressions (#553)

* Fix join/part collapsing regressions

* Simplify loop

* Explain e,e

* Explain return null in _renderSummary

* Kill it properly

* Move . to _renderSummary

* Only use the first and last events to decide whether a net change has occured

* Do not sort events by TS before summarising

* fix loop and comment

* remove data-number-events

* Better explanation comment in _renderSummary

* Less tortuous comment
This commit is contained in:
Luke Barnard 2016-11-16 14:42:30 +00:00 committed by GitHub
parent b718f1542c
commit beecbc7cd7
3 changed files with 53 additions and 24 deletions

View file

@ -229,6 +229,7 @@ module.exports = React.createClass({
_getEventTiles: function() { _getEventTiles: function() {
var EventTile = sdk.getComponent('rooms.EventTile'); var EventTile = sdk.getComponent('rooms.EventTile');
var DateSeparator = sdk.getComponent('messages.DateSeparator');
const MemberEventListSummary = sdk.getComponent('views.elements.MemberEventListSummary'); const MemberEventListSummary = sdk.getComponent('views.elements.MemberEventListSummary');
this.eventNodes = {}; this.eventNodes = {};
@ -278,8 +279,8 @@ module.exports = React.createClass({
var isMembershipChange = (e) => var isMembershipChange = (e) =>
e.getType() === 'm.room.member' e.getType() === 'm.room.member'
&& ['join', 'leave'].indexOf(e.event.content.membership) !== -1 && ['join', 'leave'].indexOf(e.getContent().membership) !== -1
&& (!e.event.prev_content || e.event.content.membership !== e.event.prev_content.membership); && (!e.getPrevContent() || e.getContent().membership !== e.getPrevContent().membership);
for (i = 0; i < this.props.events.length; i++) { for (i = 0; i < this.props.events.length; i++) {
var mxEv = this.props.events[i]; var mxEv = this.props.events[i];
@ -294,23 +295,32 @@ module.exports = React.createClass({
// Wrap consecutive member events in a ListSummary // Wrap consecutive member events in a ListSummary
if (isMembershipChange(mxEv)) { if (isMembershipChange(mxEv)) {
let summarisedEvents = [mxEv]; let ts1 = mxEv.getTs();
i++;
for (;i < this.props.events.length; i++) {
let collapsedMxEv = this.props.events[i];
if (!isMembershipChange(collapsedMxEv)) { if (this._wantsDateSeparator(prevEvent, ts1)) {
i--; let dateSeparator = <li key={ts1+'~'}><DateSeparator key={ts1+'~'} ts={ts1}/></li>;
ret.push(dateSeparator);
}
let summarisedEvents = [mxEv];
for (;i + 1 < this.props.events.length; i++) {
let collapsedMxEv = this.props.events[i + 1];
if (!isMembershipChange(collapsedMxEv) ||
this._wantsDateSeparator(this.props.events[i], collapsedMxEv.getTs())) {
break; break;
} }
summarisedEvents.push(collapsedMxEv); summarisedEvents.push(collapsedMxEv);
} }
// At this point, i = this.props.events.length OR i = the index of the last // At this point, i = the index of the last event in the summary sequence
// MembershipChange in a sequence of MembershipChanges
let eventTiles = summarisedEvents.map( let eventTiles = summarisedEvents.map(
(e) => { (e) => {
let ret = this._getTilesForEvent(prevEvent, e); // In order to prevent DateSeparators from appearing in the expanded form
// of MemberEventListSummary, render each member event as if the previous
// one was itself. This way, the timestamp of the previous event === the
// timestamp of the current event, and no DateSeperator is inserted.
let ret = this._getTilesForEvent(e, e);
prevEvent = e; prevEvent = e;
return ret; return ret;
} }

View file

@ -69,9 +69,9 @@ module.exports = React.createClass({
render: function() { render: function() {
var BaseAvatar = sdk.getComponent("avatars.BaseAvatar"); var BaseAvatar = sdk.getComponent("avatars.BaseAvatar");
var {member, onClick, ...otherProps} = this.props; var {member, onClick, viewUserOnClick, ...otherProps} = this.props;
if (this.props.viewUserOnClick) { if (viewUserOnClick) {
onClick = () => { onClick = () => {
dispatcher.dispatch({ dispatcher.dispatch({
action: 'view_user', action: 'view_user',

View file

@ -107,10 +107,20 @@ module.exports = React.createClass({
</span> </span>
); );
} }
// The joinEvents and leaveEvents are representative of the net movement
// per-user, and so it is possible that the total net movement is nil,
// whilst there are some events in the expanded list. If the total net
// movement is nil, then neither joinSummary nor leaveSummary will be
// truthy, so return null.
if (!joinSummary && !leaveSummary) {
return null;
}
return ( return (
<span> <span>
{joinSummary}{joinSummary && leaveSummary?'; ':''} {joinSummary}{joinSummary && leaveSummary?'; ':''}
{leaveSummary} {leaveSummary}.&nbsp;
</span> </span>
); );
}, },
@ -140,15 +150,24 @@ module.exports = React.createClass({
// Reorder events so that joins come before leaves // Reorder events so that joins come before leaves
let eventsToRender = this.props.events; let eventsToRender = this.props.events;
// Filter out those who joined, then left // Create an array of events that are not "cancelled-out" by another
let filteredEvents = eventsToRender.filter( // A join of sender S is cancelled out by a leave of sender S etc.
(e) => { let filteredEvents = [];
return eventsToRender.filter( let senders = new Set(eventsToRender.map((e) => e.getSender()));
(e2) => { senders.forEach(
return e.getSender() === e2.getSender() (userId) => {
&& e.event.content.membership !== e2.event.content.membership; // Only push the last event if it isn't the same membership as the first
}
).length === 0; let userEvents = eventsToRender.filter((e) => {
return e.getSender() === userId;
});
let firstEvent = userEvents[0];
let lastEvent = userEvents[userEvents.length - 1];
if (firstEvent.getContent().membership !== lastEvent.getContent().membership) {
filteredEvents.push(lastEvent);
}
} }
); );
@ -194,7 +213,7 @@ module.exports = React.createClass({
{avatars} {avatars}
</span> </span>
<span className="mx_TextualEvent mx_MemberEventListSummary_summary"> <span className="mx_TextualEvent mx_MemberEventListSummary_summary">
{summary}{joinAndLeft? '. ' + joinAndLeft + ' ' + noun + ' joined and left' : ''} {summary}{joinAndLeft ? joinAndLeft + ' ' + noun + ' joined and left' : ''}
</span>&nbsp; </span>&nbsp;
{toggleButton} {toggleButton}
</div> </div>