RoomView: Handle joining federated rooms #270
Conversation
This hopefully fixes an issue where joining a federated room via the directory would get stuck at a spinner of doom, due to us not recognising the room in question when it came down the /sync. We now catch the room id in the response from the /join, and use it to match up the room in onRoom. props.roomAlias, props.roomId, and state.room.roomId were somewhat confusing, so I've tried to rationalise them: * props.roomAlias (named thus to stop you assuming it's a room id) is the thing that the parent component uses to identify the room of interest, and can be either an ID or an alias (ie, it replaces props.roomId and props.roomAlias) * Everything that needs a room ID now has to get it from state.room.roomId.
| @@ -1052,8 +1055,7 @@ module.exports = React.createClass({ | |||
| page_element = ( | |||
| <RoomView | |||
| ref="roomView" | |||
| roomId={this.state.currentRoom} | |||
| roomAlias={this.state.currentRoomAlias} | |||
| roomAlias={this.state.currentRoom || this.state.currentRoomAlias} | |||
There was a problem hiding this comment.
Isn't this setting the room alias prop to a room ID? Wouldn't currentRoom always be present if currentRoomAlias is?
Edit: oh, looking at the doc for RoomView this is an alias or ID.
There was a problem hiding this comment.
Yup; see also the PR description.
As for whether currentRoomAlias is ever present without currentRoom: ¯_(ツ)_/¯. The logic in MatrixChat is opaque to me, and I was rather hoping to avoid poking at it with some minimal changes; hence this defensiveness.
|
I'm not super keen on something called roomAlias either being an alias or an ID, although that might mean thinking up a better name for it. roomAddress perhaps (ie. address = any string that can be used to get an identifier)? |
| // peekable. Currently there is a big mess, where at least four | ||
| // different components (RoomView, MatrixChat, RoomDirectory, | ||
| // SlashCommands) have logic for turning aliases into rooms, and each | ||
| // of them do it differently and have different edge cases. |
There was a problem hiding this comment.
Presumably this is safe because we never actually instantiate RoomView with an alias yet? That said I'd probably rather avoid merging things in where the interface is there but not functional yet as it could trip people up easily. Would it make more sense to make RoomView support both aliases and IDs and then eliminate the duplicate alias lookups once that's in place?
There was a problem hiding this comment.
As discussed via PM: this is really just making the existing situation more explicit.
We do instantiate RoomView with an alias (for instance when you type an alias into the directory); but previously in that case the alias would have been stuffed into props.roomId, causing all manner of breakage. In an ideal world, we'd make this better, (and yes, I'm leaning towards RoomView being in charge of resolving aliases to rooms) but I'm hoping to avoid fixing all of the alias->room flows right now.
|
In general I like the idea of RoomView being able to take an alias and do the lookup itself (this is something I was hoping to be able to do) and the rest looks fine. |
|
@dbkr: ptal? |
|
That should confuse me less :) lgtm |
This hopefully fixes an issue where joining a federated room via the directory
would get stuck at a spinner of doom, due to us not recognising the room in
question when it came down the /sync. We now catch the room id in the response
from the /join, and use it to match up the room in onRoom.
props.roomAlias, props.roomId, and state.room.roomId were somewhat confusing,
so I've tried to rationalise them:
thing that the parent component uses to identify the room of interest, and
can be either an ID or an alias (ie, it replaces props.roomId and
props.roomAlias)
Also remove the apparently redundant
onRoomNamehandler. (The room name is displayed by theRoomHeader, which listens to room state changes itself)