Skip to content
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.

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

Remove the client side filtering from the room dir #2750

Merged
merged 4 commits into from Dec 16, 2016

Conversation

@dbkr
Copy link
Member

dbkr commented Dec 16, 2016

This removes the ability for the client to filter remote room
directories by network, since the /thirdparty/protocols API does
not yet work for remote servers. They would only get the main list
now though anyway, so there is no point in us continuing to support
it.

This removes the ability for the client to filter remote room
directories by network, since the /thirdparty/protocols API does
not yet work for remote servers. They would only get the main list
now though anyway, so there is no point in us continuing to support
it.
opts.third_party_instance_id = this.state.instance_id;
} else if (this.state.network !== '_matrix') {
if (this.state.instanceId) {
opts.third_party_instanceId = this.state.instanceId;

This comment has been minimized.

Copy link
@lukebarnard1

lukebarnard1 Dec 16, 2016

Contributor

I'm not convinced third_party_instanceId is the right format to give to synapse.

This comment has been minimized.

Copy link
@dbkr

dbkr Dec 16, 2016

Author Member

heh, good catch!

@@ -295,12 +284,19 @@ module.exports = React.createClass({
onJoinClick: function(alias) {
// If we're on the 'Matrix' network (or all networks),

This comment has been minimized.

Copy link
@lukebarnard1

lukebarnard1 Dec 16, 2016

Contributor

Comment needs updating

This comment has been minimized.

Copy link
@dbkr

dbkr Dec 16, 2016

Author Member

done

this.showRoomAlias(alias);
} else {
// This is a 3rd party protocol. Let's see if we
// can join it
const fields = this._getFieldsForThirdPartyLocation(alias, this.state.network);
const protocol_name = protocolNameForInstanceId(this.protocols, this.state.instanceId);

This comment has been minimized.

Copy link
@lukebarnard1

lukebarnard1 Dec 16, 2016

Contributor

protocol_name -> protocolName

This comment has been minimized.

Copy link
@dbkr

dbkr Dec 16, 2016

Author Member

done

export function protocolNameForInstanceId(protocols, instance_id) {
if (!instance_id) return null;
for (const proto of Object.keys(protocols)) {
if (!protocols[proto].instances) continue;

This comment has been minimized.

Copy link
@lukebarnard1

lukebarnard1 Dec 16, 2016

Contributor

Should probably be checking whether instances is an array, in case it's an object.

This comment has been minimized.

Copy link
@dbkr

dbkr Dec 16, 2016

Author Member

done

@@ -309,8 +305,7 @@ module.exports = React.createClass({
});
return;
}
const protocol = this._protocolForThirdPartyNetwork(this.state.network);
MatrixClientPeg.get().getThirdpartyLocation(protocol, fields).done((resp) => {
MatrixClientPeg.get().getThirdpartyLocation(protocol_name, fields).done((resp) => {

This comment has been minimized.

Copy link
@lukebarnard1

lukebarnard1 Dec 16, 2016

Contributor

Need to null guard protocol_name.

This comment has been minimized.

Copy link
@dbkr

dbkr Dec 16, 2016

Author Member

done

if (matched_instance === undefined) return null;

// now make an object with the fields specified by that protocol. We
_getFieldsForThirdPartyLocation: function(user_input, protocol, instance) {

This comment has been minimized.

Copy link
@lukebarnard1

lukebarnard1 Dec 16, 2016

Contributor

userInput

This comment has been minimized.

Copy link
@dbkr

dbkr Dec 16, 2016

Author Member

done

// require that the values of all but the last field come from the
// instance. The last is the user input.
const required_fields = this.protocols[network_info.protocol].location_fields;
const required_fields = protocol.location_fields;
if (!required_fields) return null;
const fields = {};
for (let i = 0; i < required_fields.length - 1; ++i) {
const this_field = required_fields[i];

This comment has been minimized.

Copy link
@lukebarnard1

lukebarnard1 Dec 16, 2016

Contributor

thisField

This comment has been minimized.

Copy link
@dbkr

dbkr Dec 16, 2016

Author Member

done

for (const proto of Object.keys(this.props.protocols)) {
if (!this.props.protocols[proto].instances) continue;
for (const instance of this.props.protocols[proto].instances) {
if (!instance.instance_id) continue;

This comment has been minimized.

Copy link
@lukebarnard1

lukebarnard1 Dec 16, 2016

Contributor

This feels like it could use protocolNameForInstanceId

This comment has been minimized.

Copy link
@dbkr

dbkr Dec 16, 2016

Author Member

In this case we're just iterating through the protocols response to make an option for each, rather than getting one particular protocol / instance

icon = <img src={iconPath} />;
}

key = server + '_inst_'+instance.instance_id;

This comment has been minimized.

Copy link
@lukebarnard1

lukebarnard1 Dec 16, 2016

Contributor

'_inst_' + instan...

protocols: {},
config: {},

This comment has been minimized.

Copy link
@lukebarnard1

lukebarnard1 Dec 16, 2016

Contributor

Maybe comment within config to explain the optional servers property

This comment has been minimized.

Copy link
@dbkr

dbkr Dec 16, 2016

Author Member

I've put it in propTypes

@lukebarnard1

This comment has been minimized.

Copy link
Contributor

lukebarnard1 commented Dec 16, 2016

I'm new to this scheme of variable names, could you quickly explain the use of underscores at times vs. use of other casings?

@dbkr

This comment has been minimized.

Copy link
Member Author

dbkr commented Dec 16, 2016

Basically we were a bit inconsistent, which vdh pointed out in his last review, so I'd been trying to make them a bit more consistent and make parameter names camelcase at least. If there anything that looks wrong?

@lukebarnard1

This comment has been minimized.

Copy link
Contributor

lukebarnard1 commented Dec 16, 2016

Well it's just that sometimes variable names are camelcase and some are underscored. One example is handleClicks vs. includeAll

@dbkr

This comment has been minimized.

Copy link
Member Author

dbkr commented Dec 16, 2016

yeah, they generally should be camelcase

@@ -278,7 +267,7 @@ module.exports = React.createClass({
this.filterTimeout = setTimeout(() => {
this.filterTimeout = null;
this.refreshRoomList();
}, 300);
}, 700);

This comment has been minimized.

Copy link
@Kegsay

Kegsay Dec 16, 2016

Contributor

Why the bump?

This comment has been minimized.

Copy link
@dbkr

dbkr Dec 16, 2016

Author Member

700ms still feels pretty responsive, and will help further reduce the number of hits to the API

// If we're on the 'Matrix' network (or all networks),
// just show that rooms alias
if (this.state.network == null || this.state.network == '_matrix') {
// If we don't have a prticular instance id selected, just show that rooms alias

This comment has been minimized.

Copy link
@Kegsay

Kegsay Dec 16, 2016

Contributor

prticular

@Kegsay

This comment has been minimized.

Copy link
Contributor

Kegsay commented Dec 16, 2016

Probably LGTM - It's hard to adequately review this without knowing the context of what it was like before, and given the expedited need for this PR to land, assuming it works, LGTM.

@dbkr

This comment has been minimized.

Copy link
Member Author

dbkr commented Dec 16, 2016

Yay!

@dbkr dbkr merged commit 09f79b9 into develop Dec 16, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.