Add native joining of 3p networks to room dir #2379

dbkr · 2016-09-29T14:38:40Z

Use the 3rd party location lookup API to accept third-party locations
in their native form and look up the corresponding portal room for
that location.

Also give the network dropdown some placeholder text.

Fixes #2374
Requires matrix-org/matrix-react-sdk#502
Requires matrix-org/matrix-js-sdk#217

Use the 3rd party location lookup API to accept third-party locations in their native form and look up the corresponding portal room for that location. Also give the network dropdown some placeholder text. Fixes #2374

dbkr · 2016-09-29T14:51:02Z

Tests failing because of js-sdk requirement

richvdh · 2016-09-30T08:32:00Z

src/components/structures/RoomDirectory.js

+        this.nativePatterns = {};
+        if (this.props.config.networks) {
+            for (const network of Object.keys(this.props.config.networks)) {
+                if (this.props.config.networks[network].portalRoomPattern) {


y'know, local variables aren't rationed.

const net = this.props.config.networks[network]; // etc

richvdh

I'm not a fan of this, for reasons discussed in #matrix-core:

It puts knowledge that ought to be in the server in the client (specifically the roomDirectory object in the config).
It hardcodes a list of supported protocols into the client.
It doesn't allow for anyone who isn't on the matrix.org HS to use the bridge functionality - which is an important feature and might change the way all this works.

Most of this, AIUI, is due to Vector making the best of the API as it stands. If it's hard for Vector to use right, then the API probably needs fixing, rather than working around.

richvdh · 2016-09-30T08:48:04Z

src/components/structures/RoomDirectory.js

-        this.showRoomAlias(alias);
+        // If we're on the 'Matrix' network (or all networks),
+        // just show that rooms alias
+        if (this.state.network == null || this.state.network == '_matrix') {


how can we be sure that we don't need to do the 3rd party dance if we are showing all networks?

If we're showing all networks, the user input is just treated as a matrix room ID: we don't do 3rd party network stuff since there'd be no obvious 3rd party network to choose.

richvdh · 2016-09-30T08:50:29Z

src/components/structures/RoomDirectory.js

+
+    _getFieldsForThirdPartyLocation: function(user_input, network) {
+        const getfields_funcs = {
+            irc: (user_input, network) => {


really? we're hard-coding this into vector?

Try to match protocol insance from 'domain' field and use its fields for all but the last field. Assume the last takes the user input.

dbkr · 2016-10-03T14:07:36Z

I've updated this to get the fields by filling in all but the last field from the instance fields and the last from the user input instead of vector explicitly supporting individual protocols.

richvdh · 2016-10-03T14:41:12Z

vector/config.sample.json

-            "irc:freenode": "//matrix.org/_matrix/media/v1/download/matrix.org/DHLHpDDgWNNejFmrewvwEAHX",
-            "irc:mozilla": "//matrix.org/_matrix/media/v1/download/matrix.org/DHLHpDDgWNNejFmrewvwEAHX",
-            "gitter": "//gitter.im/favicon.ico"
+        "networks": {


if we're doing this, it needs some user documentation. What do the different fields mean? which ones are required and which optional?

Yeah. Unsure what the best way to do this is, given the config file is JSON.

a section in the README, maybe?

Works for me

richvdh · 2016-10-03T14:54:34Z

src/components/structures/RoomDirectory.js

@@ -107,10 +126,10 @@ module.exports = React.createClass({
            opts.server = my_server;
        }
        if (this.nextBatch) opts.since = this.nextBatch;
-        if (this.filterString) opts.filter = { generic_search_term: my_filter_string } ;
+        if (this.state.filterString) opts.filter = { generic_search_term: my_filter_string } ;


s/this.state.filterString/my_filter_string/ for clarity here

richvdh · 2016-10-03T14:56:09Z

src/components/structures/RoomDirectory.js

-        this.showRoomAlias(alias);
+        // If we're on the 'Matrix' network (or all networks),
+        // just show that rooms alias
+        if (this.state.network == null || this.state.network == '_matrix') {


how can we be sure that we don't need to do the 3rd party dance if we are showing all networks?

richvdh · 2016-10-03T15:11:40Z

src/components/structures/RoomDirectory.js

+            // If there's only one instance in this protocol, use it
+            // as long as it has no domain (which we assume to mean it's
+            // there is only one possible instance).
+            if (the_instance.fields.domain === undefined && network_info.domain === undefined) {


I'm not sure what domain is, but shouldn't this work if the domains are defined and match?

Yeah, that would just drop into the second case. I was thinking about getting rid of the two cases here and just relying on undefined === undefined catching the first one, but figured it was clearer to leave them as two.

how will it drop into the second case?

Ah, I guess it won't for a single domain. Fixed.

richvdh · 2016-10-03T15:11:50Z

src/components/structures/RoomDirectory.js

+            // as long as it has no domain (which we assume to mean it's
+            // there is only one possible instance).
+            if (the_instance.fields.domain === undefined && network_info.domain === undefined) {
+                instance = this.protocols[network_info.protocol].instances[0];


the_instance

This one should be 'instance' as we've now found the correct alias. I've renamed the variable to make it more obvious, hopefully.

I mean, use the_instance instead of this.protocols[network_info.protocol].instances[0]

Oh, I see. Done.

richvdh · 2016-10-03T15:17:13Z

src/components/views/directory/NetworkDropdown.js

+                    name = network;
+                }
+                if (this.props.config.networks[network].icon) {
+                    icon = <img src={this.props.config.networks[network].icon} />;


no width/height here?

richvdh · 2016-10-03T15:20:37Z

src/components/views/directory/NetworkDropdown.js

@@ -170,8 +170,19 @@ export default class NetworkDropdown extends React.Component {
            icon = <img src="img/network-matrix.svg" width="16" height="16" />;
            span_class = 'mx_NetworkDropdown_menu_network';
        } else {
-            name = this.props.config.networkNames[network];
-            icon = <img src={this.props.config.networkIcons[network]} />;
+            if (this.props.config.networks[network]) {


name is left undefined if this is false? would it make more sense to throw if it is false?

richvdh · 2016-10-03T15:22:10Z

src/components/views/directory/NetworkDropdown.js

@@ -199,6 +210,7 @@ export default class NetworkDropdown extends React.Component {
            </div>;
            current_value = <input type="text" className="mx_NetworkDropdown_networkoption"
                ref={this.collectInputTextBox} onKeyUp={this.onInputKeyUp}
+                placeholder="matrix.org"


don't get why this is?

It's just used here as an example of the name of an HS: hopefully comment clarifies.

richvdh

lgtm modulo typo

richvdh · 2016-10-04T17:37:03Z

README.md

+1. `roomDirectory.networks`: config for each network type. Optional.
+1. `roomDirectory.<network_type>.name`: Human-readable name for the network. Required.
+1. `roomDirectory.<network_type>.protocol`: Protocol as given by the server in
+   `/_matrix/client/unstable/thirdparty/protocolss` response. Required to be able to join


Add native joining of 3p networks to room dir

761600f

Use the 3rd party location lookup API to accept third-party locations in their native form and look up the corresponding portal room for that location. Also give the network dropdown some placeholder text. Fixes #2374

dbkr assigned richvdh Sep 29, 2016

dbkr mentioned this pull request Sep 29, 2016

Move 'show join button' functionality out matrix-org/matrix-react-sdk#502

Merged

richvdh reviewed Sep 30, 2016

View reviewed changes

Use more variables

b71b1b5

richvdh requested changes Sep 30, 2016

View reviewed changes

richvdh assigned dbkr and unassigned richvdh Sep 30, 2016

Different way of getting fields for 3p location

776fe2a

Try to match protocol insance from 'domain' field and use its fields for all but the last field. Assume the last takes the user input.

dbkr assigned richvdh and unassigned dbkr Oct 3, 2016

richvdh requested changes Oct 3, 2016

View reviewed changes

richvdh assigned dbkr and unassigned richvdh Oct 3, 2016

dbkr added 3 commits October 3, 2016 16:30

Don't pop up errors when this API fails for guests

000ca35

PR Freedback

304e5b9

Specify width on icons & comment

b80b08f

dbkr assigned richvdh and unassigned dbkr Oct 4, 2016

richvdh assigned dbkr and unassigned richvdh Oct 4, 2016

dbkr added 3 commits October 4, 2016 15:18

Actually use variable

de9bf4b

For single instance, allow domain present & match

c0a4574

Doc roomDirectory config section

381c900

dbkr assigned richvdh and unassigned dbkr Oct 4, 2016

richvdh approved these changes Oct 4, 2016

View reviewed changes

richvdh removed their assignment Oct 4, 2016

richvdh assigned dbkr Oct 4, 2016

Typo

562b047

dbkr merged commit ea38968 into develop Oct 5, 2016

t3chguy deleted the dbkr/join_3p_location branch May 12, 2022 08:55

Add native joining of 3p networks to room dir #2379

Add native joining of 3p networks to room dir #2379

Conversation

dbkr commented Sep 29, 2016 • edited Loading

dbkr commented Sep 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbkr commented Oct 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbkr commented Sep 29, 2016 •

edited

Loading