-
-
Notifications
You must be signed in to change notification settings - Fork 832
Control currently viewed event via RoomViewStore #1058
Conversation
Fix for element-hq/element-web#4224 Due to the way `MatrixChat` does a state update when the `view_room` dispatch fires and a second update when `RoomViewStore` sends an update, the current event ID and room ID were becoming out of sync. The solution devised was to have the event ID managed by the `RoomViewStore` itself and do any defaulting there (for when we revisit a room that we saved scroll state for previously). This required a few changes: - The addition of `update_scroll_state` in `RoomViewStore` allows the `RoomView` to save scroll state for a room before swapping to another one. Previously the caching of scroll state was done in `RoomView`. - The `view_room` dispatch now accepts an `event_id`, which dictates which event is supposed to be scrolled to in the `MessagePanel` when a new room is viewed. It also accepts `event_offset`, but currently, this isn't passed in by a dispatch in the app, but it is clobbered when loading the default position when an `event_id` isn't specified. Finally, `highlighted` was added to distinguish whether the initial event being scrolled to is also highlighted. This flag is also used by `viewRoom` in `MatrixChat` in order to decide whether to `notifyNewScreen` with the specified `event_id`.
Conflicts: src/components/structures/MatrixChat.js
@@ -618,40 +618,21 @@ module.exports = React.createClass({ | |||
this.focusComposer = true; | |||
|
|||
const newState = { | |||
initialEventId: roomInfo.event_id, | |||
highlightedEventId: roomInfo.event_id, | |||
initialEventPixelOffset: undefined, | |||
page_type: PageTypes.RoomView, | |||
thirdPartyInvite: roomInfo.third_party_invite, | |||
roomOobData: roomInfo.oob_data, | |||
currentRoomAlias: roomInfo.room_alias, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is redundant now?
@@ -680,7 +661,7 @@ module.exports = React.createClass({ | |||
} | |||
} | |||
|
|||
if (roomInfo.event_id) { | |||
if (roomInfo.event_id && roomInfo.highlighted) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add this to the jsdoc please
@@ -1137,6 +1118,7 @@ module.exports = React.createClass({ | |||
const payload = { | |||
action: 'view_room', | |||
event_id: eventId, | |||
highlighted: Boolean(eventId), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment to explain what's going on here would be helpful
isEventHighlighted: RoomViewStore.isEventHighlighted(), | ||
}; | ||
|
||
if (this.state.eventId !== newState.eventId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment please
@@ -598,6 +578,14 @@ module.exports = React.createClass({ | |||
}); | |||
}, | |||
|
|||
_updateScrollMap(roomId) { | |||
dis.dispatch({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need some sort of check that roomId isn't null/undef
src/stores/RoomViewStore.js
Outdated
if (!payload.event_id) { | ||
const roomScrollState = this._state.scrollStateMap[payload.room_id]; | ||
if (roomScrollState) { | ||
payload.event_id = roomScrollState.focussedEvent; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please don't write to payload
src/stores/RoomViewStore.js
Outdated
@@ -27,12 +27,32 @@ const INITIAL_STATE = { | |||
joinError: null, | |||
// The room ID of the room | |||
roomId: null, | |||
// The event being viewed | |||
eventId: null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggest calling this something like initialEventId
src/stores/RoomViewStore.js
Outdated
// The event being viewed | ||
eventId: null, | ||
// The offset to display the event at (see scrollStateMap) | ||
eventPixelOffset: null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto?
src/stores/RoomViewStore.js
Outdated
eventId: null, | ||
// The offset to display the event at (see scrollStateMap) | ||
eventPixelOffset: null, | ||
// Whether to highlight the event |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the initial event
src/stores/RoomViewStore.js
Outdated
@@ -176,6 +229,18 @@ class RoomViewStore extends Store { | |||
return this._state.roomId; | |||
} | |||
|
|||
getEventId() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comments here might be helpful?
…update from RoomViewStore
Otherwise we pointlessly assign the null key to something
@@ -607,6 +607,7 @@ module.exports = React.createClass({ | |||
// @param {boolean=} roomInfo.show_settings Makes RoomView show the room settings dialog. | |||
// @param {string=} roomInfo.event_id ID of the event in this room to show: this will cause a switch to the | |||
// context of that particular event. | |||
// @param {boolean=} roomInfo.highlighted If true, add event_id to the hash of the URL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
surely it means way more than that? like, the event will also be highlighted in the view.
Bear in mind this is documenting the parameters on the view_room more than the parameters to the function - the docs just got left behind when view_room started going to more places :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should document things that happen elsewhere. If someone wants to know what the view_room
dispatch does, they should look for all the things that receive it (namely, RoomViewStore).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not convinced by that. The view_room
dispatch is an API, like any other. You don't refuse to document your methods and say "look at these other methods which I call". I want to know: what does it mean, conceptually, to set the 'highlighted' param in a view_room dispatch. I don't think at its heart it means 'add the event_id' to the location bar.
There's a (very strong) argument that this is not the place for that documentation, but we have nowhere else right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, but I feel that someone is going to get confused when the documentation for this function doesn't match what the function does.
@@ -1118,6 +1118,7 @@ module.exports = React.createClass({ | |||
const payload = { | |||
action: 'view_room', | |||
event_id: eventId, | |||
// If an event ID is set (truthy), mark it as highlighted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, now it has a comment, but I'm not sure I'm any the wiser - the comment says what the code is doing (which I could see for myself), but not why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So that it is highlighted in the MessagePanel?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I'm after something like
// if there is an event_id in the URL, tell the room view to highlight it
@@ -94,6 +94,13 @@ module.exports = React.createClass({ | |||
roomLoading: true, | |||
peekLoading: false, | |||
|
|||
// The event to be scrolled to initially | |||
eventId: null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be initial* now I think
src/stores/RoomViewStore.js
Outdated
// Whether to highlight the event | ||
isEventHighlighted: false, | ||
|
||
// The event to scroll to initially |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you clarify 'initially' here: I think you mean 'when the room is first viewed' or sth?
src/stores/RoomViewStore.js
Outdated
const newState = { | ||
roomId: payload.room_id, | ||
initialEventId: payload.event_id, | ||
initialEventPixelOffset: payload.event_offset, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is payload.event_offset
ever set? I don't think so. Better to use an explicit undefined
here.
src/stores/RoomViewStore.js
Outdated
getRoomId() { | ||
return this._state.roomId; | ||
} | ||
|
||
getEventId() { | ||
return this._state.eventId; | ||
// The event to scroll to initially |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"initially"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
// If an event ID is set (truthy), mark it as highlighted | ||
// If an event ID is given in the URL hash, notify RoomViewStore to mark | ||
// it as highlighted, which will propagate to RoomView and highlight the | ||
// associated EventTile. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perfect ⭐
Fix for element-hq/element-web#4224
Due to the way
MatrixChat
does a state update when theview_room
dispatch fires and a second update whenRoomViewStore
sends an update, the current event ID and room ID were becoming out of sync. The solution devised was to have the event ID managed by theRoomViewStore
itself and do any defaulting there (for when we revisit a room that we saved scroll state for previously).This required a few changes:
update_scroll_state
inRoomViewStore
allows theRoomView
to save scroll state for a room before swapping to another one. Previously the caching of scroll state was done inRoomView
.view_room
dispatch now accepts anevent_id
, which dictates which event is supposed to be scrolled to in theMessagePanel
when a new room is viewed. It also acceptsevent_offset
, but currently, this isn't passed in by a dispatch in the app, but it is clobbered when loading the default position when anevent_id
isn't specified. Finally,highlighted
was added to distinguish whether the initial event being scrolled to is also highlighted. This flag is also used byviewRoom
inMatrixChat
in order to decide whether tonotifyNewScreen
with the specifiedevent_id
.