(In reply to comment #0)
> it would be nice if clicking on the IRC link would open Polari

Indeed, we should do that.

I have a branch here to address this:
https://git.gnome.org/browse/polari/log/?h=wip/bastianilso/irc-url-parsing

Opening a url using "polari irc://irc.gnome.org/%23polari" seem to cause a segmentation fault, though. Something is depending on the main window to be visible, but we don't display the main window until we know whether the URL is valid or not. (If invalid, we just show a notification instead).

It would be great if we could merge this for GNOME 3.20 release. As I am currently working on segfault of invalid URLs, I think if it is fixed then a merge could be possible. 

However, I can't reproduce the above mentioned sefault with invalid URLs and current code base. Even when the main window is hidden, I get the expected result. Clicking on the link above (irc://irc.gnome.org/%23polari), I get a notification as well: "Polari is ready". 

So far, I have observed only one problem. If Polari is not running I will get below log in console (while running iceweasel from terminal) and Polari will not show up (no matter what the URL is):

Gjs-Message: JS LOG: didnt pass check! accounts: 0, args: [object instance proxy GType:GDaemonFile jsobj@0x7f17927d3100 native@0x7f177c00d720]
Gjs-Message: JS LOG: server: irc.mozilla.org
Gjs-Message: JS LOG: room: newbies
Gjs-Message: JS LOG: port: undefined
Gjs-Message: JS LOG: checking if window exists
Gjs-Message: JS LOG: creates main window


Is this expected behavior?

(In reply to Mehdi Sadeghi from comment #3)
> It would be great if we could merge this for GNOME 3.20 release. As I am
> currently working on segfault of invalid URLs, I think if it is fixed then a
> merge could be possible.

I've talked with Bastian offline about this bug - the main issue is that a simply string comparison of the URL to match an account is a bit too simplistic IMHO. For intance, "irc://irc.gnome.org/%23polari", "irc://irc.gimp.net/%23polari", "irc://irc.acc.umu.se/%23polari" etc. all refer to the same channel. There are some plans (and an intern assigned to work on them!) for updating the connection configuration that should help with this issue, but it means irc:// handling is currently blocked on that work.

Florian, all other IRC clients have the same problem. They solve it ignoring this and still supporting web links. It will work often even though it’s not perfect. If a network connection to that host exist, it will be used, otherwise display a UI for adding a new network and channel.

Created attachment 322190 [details] [review]
appNotification: Rename UNDO_TIMEOUT to TIMEOUT

We are about to land a new class for creating
generic notifications so rename UNDO_TIMEOUT to
the more general "TIMEOUT".

Created attachment 322191 [details] [review]
appNotification: Add class CloseNotification

We are about to land IRC URL parsing which error
handling requires using an app notification when
IRC URLs cannot be properly parsed. This adds a
new CloseNotification class which shows a notification
for 7 seconds along with a close button.

Created attachment 322192 [details] [review]
app: open IRC links

Register Polari as handler of IRC URLs and add
support for opening IRC URLs pointing to rooms on
the networks the user have added.

Created attachment 322193 [details] [review]
app: Handle links that dont match existing networks

IRC URLs may point to networks which the user haven't
connected to yet. Add support for matching IRC URLs
against the list of predefined networks. If a match
can't be found, create a custom connection pointing
directly to the URL which the IRC link specifies.

Review of attachment 322190 [details] [review]:

Could just as well be squashed

Review of attachment 322191 [details] [review]:

::: src/appNotifications.js
@@ +70,3 @@
 
+const CloseNotification = new Lang.Class({
+    Name: 'CloseNotification',

That's not a great name - "close notifications" sounds like either a notification that is shown on close, or a notification that can be used to close something.
I don't really like "ClosableNotification", maybe "MessageNotification"? (Kind of analogous to MessageDialog).

Also might be worth generalizing it a bit:
 (1) make the icon optional (either handle undefined, or use a params dict,
     or add the image but only show it when setIcon() is called with an
     appropriate parameter)
 (2) add a addButton(label, callback) method to optional add additional actions

Then this could become the new base class for UndoNotification instead of copying all the code.

@@ +74,3 @@
+    Signals: { closed: {} },
+
+    _init: function(label, icon_name) {

Nit: camelCase in js code

@@ +94,3 @@
+
+    close: function() {
+        this.emit(this._undo ? 'undo' : 'closed');

'undo' doesn't make any sense here (and would in fact throw an error!), and you don't use 'closed' either

Review of attachment 322192 [details] [review]:

::: src/application.js
@@ +175,3 @@
+        });
+
+        let action = this.lookup_action('join-room');

It makes sense to not repeat the lookup for each URL (in the rare case where there are multiple), but there's a good chance that once you get to the code below that uses it, you forgot what actions was looked up. An easy fix would be to rename the variable to joinAction

@@ +177,3 @@
+        let action = this.lookup_action('join-room');
+        uris.forEach(Lang.bind(this, function(uri) {
+            let uriRegEx = /^(irc?:\/\/)([\da-z\.-]+):?(\d+)?\/(?:%23)?([\w \. \+-]*)/;

There's no reason for the regex to be in the loop (or in fact, a variable - could make this const IRC_SCHEMA_REGEX is so)

@@ +180,3 @@
+            let server, port, room;
+            try {
+                [,, server, port, room] = uri.match(uriRegEx);

This is fine as is, but as the anonymous function grows unwieldily long in the next patch, it might make sense to split this out as:

let [success, server, port, room] = this._parseUri(uri);
if (!success)
    return;

@@ +181,3 @@
+            try {
+                [,, server, port, room] = uri.match(uriRegEx);
+                if (!room) throw new Error('No Room');

Style nit: line break after condition

@@ +183,3 @@
+                if (!room) throw new Error('No Room');
+            } catch(e) {
+                let label = _("Could not open the link.");

openURL() in utils.js uses "Failed to open link". Unless we actually provide additional information in case where we are the failed handler, different messages don't make sense.

@@ +187,3 @@
+                                                               'dialog-error-symbolic');
+                this.notificationQueue.addNotification(n);
+                log("Invalid URI");

Left-over log

Review of attachment 322193 [details] [review]:

This one needs quite a bit of cleanup - an anonymous function with ~100 LOC of spaghetti is just a bit much ;-)

Luckily, there are some good opportunities for reorganization ...

In addition to the comments below, there's another issue that's a bit eerie:
When polari is not already running, the parsing is deferred until the accounts-manager is prepared. So far, so good. But parsing now involves finding matches in the list of predefined networks, which NetworksManager loads asynchronously on startup - there's a race here between NetworksManager and AccountsManager, and if the latter finishes first, we won't find a match in an empty list!

Two options I can think of:
 - do something like:
   let quark = Tp.AccountManager.get_feature_quark_core();
   let openWhenReady = Lang.bind(this,
       function() {
           if (!this._accountsMonitor.accountManager.is_prepared(quark) ||
               !this._networksLoaded)
               return;
           this._accountsMonitor.connect('account-manager-prepared', openWhenReady);
           let id = this._networksManager.connect('changed', Lang.bind(this,
               this._networksManager.disconnect(id);
               this._networksLoaded = true;
               openWhenReady();
           }));
           this._openURIs(uris, time);
        });
    openWhenReady();
 - make loading the networks list synchronous, with the excuse that we
   don't show any UI yet which we could block

I'm not a big fan of the code in the first option, and delaying showing the window as in option two isn't great either :-(

::: src/application.js
@@ +178,3 @@
+            if (predefined) {
+                this._networksManager.getServers(a).forEach(function(s) {
+                    servers.push(s.address);

Why not:

let servers;
if (this._networksManager.getAccountIsPredefined(a)) {
    servers = this._networksManager.getServers(a);
} else {
    let params = a.dup_parameters_vardict().deep_unpack();
    servers = [params.server.deep_unpack()];
}

@@ +209,3 @@
+                    if (s == server)
+                        result = true;
+                        return;

Misleading indentation - this is the code that's actually run:
function(s) {
  if (s == server)
      result = true;
  return;
}

Of course, a return statement that doesn't return a value doesn't make sense at the end of a function:

function(s) {
    if (s == server)
        result = true;
}

In fact, you don't have to iterate over all servers to know if *some* entries match, you can write this much more concisely as:

let matches = Object.keys(map).filter(function(a) {
    return map[a].some(s => s == server);
});

(or return map[a].indexOf(server) != -1);)

@@ +217,3 @@
+                action.activate(new GLib.Variant('(ssu)',
+                                [matches[0], '#' + room, time]));
+            } else {

Here's a good opportunity for splitting out some code:

if (matches.length)
    action.activate(...);
else
    this._createAccount(server, port, function(a) {
        if (a)
            action.activate(...);
    });

@@ +221,3 @@
+                let id, name, params;
+                for (let n of networks) {
+                    for (let s of n.servers) {

This is quite cumbersome and freely uses properties that should probably be encapsulated in networksManager.

I'd add something like:

findByServer: function(server) {
    for (let n of networks)
        if (n.servers.indexOf(server) != -1)
            return n.id;
}

to networksManager, then use it like:

let id = this._networksManager.findByServer(server);
let name, params;

if (id) {
    name = this._networksManager.getNetworkName(id);
    params = this._networksManager.getNetworkDetails(id);
} else {
    name = server;
    params = {
        'account': new GLib.Variant('s', GLib.get_user_name()),
        'server': new GLib.Variant('s', server),
        'port': new GLib.Variant('u', port ? port : 6667),
        'use-ssl': new GLib.Variant('b', port == 6697)
    };
}

@@ +222,3 @@
+                for (let n of networks) {
+                    for (let s of n.servers) {
+                        log("checking " + s.address + " vs. " + server);

Left-over log()

@@ +250,3 @@
+                                                  protocol: 'irc',
+                                                  display_name: name });
+                req.set_enabled(true);

You also want:

  if (id)
      req.set_service(id);

to "mark" the account as predefined

::: src/networksManager.js
@@ +94,3 @@
+    },
+
+    getServers: function(account) {

I don't like this too much, but find it hard to pinpoint the reasons. It's just a tad bit odd to have something that reads "get servers for account a" throw an exception when the account doesn't meet a particular condition (namely: is predefined).

One option would be to handle all accounts:

  if (this.getAccountIsPredefined(account))
      return this._lookupNetwork(account.service).servers;

  let params = account.get_parameters_vardict().deep_unpack();
  return [params.server.deep_unpack()];

but it's *also* odd to have the manager of predefined networks deal with non-predefined networks.

So yet another option would be to use the findByServer() method I'm proposing above:
 (1) change the map to a dictionary:
    let params = a.dup_account_parameters_vardict().deep_unpack();
    map[a.get_object_path()] = { server: params.server.deep_unpack(),
                                 service: a.service };
 (2) For matching, do:
    let matchedId = this._networksManager.findByServer(server);
    let matches = Object.keys(map).filter(function(a) {
        return a.server == server || a.service == matchedId;
    });

(if we go with (2), we should pass the matchedId to the createAccount() method to avoid searching twice)

(In reply to Florian Müllner from comment #12)
> @@ +183,3 @@
> +                if (!room) throw new Error('No Room');
> +            } catch(e) {
> +                let label = _("Could not open the link.");
> 
> openURL() in utils.js uses "Failed to open link". Unless we actually provide
> additional information in case where we are the failed handler, different
> messages don't make sense.

On a side note: Reusing an existing string means the patch won't be affected by the upcoming string freeze.

Created attachment 322423 [details] [review]
appNotification: Add class MessageNotification

We are about to land IRC URL parsing which error
handling requires using an app notification when
IRC URLs cannot be properly parsed. This adds a
new MessageNotification class which shows a notification
for 7 seconds along with a close button.

Created attachment 322424 [details] [review]
app: open IRC links

Register Polari as handler of IRC URLs and add
support for opening IRC URLs pointing to rooms on
the networks the user have added.

Created attachment 322425 [details] [review]
app: Handle links that dont match existing networks

IRC URLs may point to networks which the user haven't
connected to yet. Add support for matching IRC URLs
against the list of predefined networks. If a match
can't be found, create a custom connection pointing
directly to the URL which the IRC link specifies.

Review of attachment 322423 [details] [review]:

Regarding the commit message: It's a bit weird to split out a base class from an existing class and not mention it at all ...

::: src/appNotifications.js
@@ +47,1 @@
+        this._box.pack_start(new Gtk.Label({ label: label, hexpand: true,

You should be able to use this._box.add() here[0].

[0] https://git.gnome.org/browse/gtk+/tree/gtk/gtkbox.c#n2586

@@ +63,3 @@
+        if (callback) {
+            button.connect('clicked', Lang.bind(this, callback));
+            button.connect('clicked', Lang.bind(this, this.close));

Mmmh, I'm a bit undecided about having the closing behavior here instead of leaving it to the callback.

But if we go with this:
 - don't connect two signal handlers, use a single one and just call callback() from there
 - is this the right policy? "close if the button does anything, don't do anything otherwise"
   The alternative would be to always close:
   button.connect('clicked', Lang.bind(this,
       function() {
           if (callback)
               callback();
            this.close();
       }));

@@ +66,3 @@
+        }
+
+        this._box.pack_start(button, true, true, 0);

We don't want the button to expand. Also I'd just use add() again (pack_start/pack_end are fairly old API, we've become much more aware of the horrible "multiple-magic-booleans" pattern nowadays)

@@ +73,3 @@
     close: function() {
+        this.parent();
+    }

This is silly. You override a method to change its behavior from the parent, if you don't do that, there's no need to override it in the first place :-)

@@ +81,3 @@
+    Signals: { closed: {}, undo: {} },
+
+    _init: function(label, iconName) {

Not sure an undo notification with an icon makes sense to me.

(Note that "this.parent(label)" should work out just fine - iconName is undefined in that case, so you don't add anything)

Review of attachment 322424 [details] [review]:

::: src/application.js
@@ +16,3 @@
 const MAX_RETRIES = 3;
 
+const IRC_SCHEMA_REGEX = /^(irc?:\/\/)([\da-z\.-]+):?(\d+)?\/(?:%23)?([\w\.\+-]+)/;

This will not match a host IRC.GNOME.ORG - easy fix is to use the i option for case-insensitive matches, but if we do, we'd need to update our server matching as well (otherwise we correctly identify 'IRC.GNOME.ORG' as a server, but fail to match it to an existing account/predefined network). GLib.ascii_strcasecmp() == 0 is probably our best bet there.

@@ +177,3 @@
+        });
+
+        let joinAction = this.lookup_action('join-room');

For what it's worth, I asked for the name change because there were about a hundred lines between setting the variable and using it. We've cut that to a couple of lines (no scrolling required even in a vintage 80x24 terminal), so keeping the generic name would be fine. Of course the more specific name doesn't hurt in any way, so I'm not asking for a change here :-)

Review of attachment 322425 [details] [review]:

There's still the issue of the potential race between loading the predefined networks and preparing the accounts-manager. As discussed on IRC, I'm fine with not addressing it for now, but in case it does become an issue, a comment mentioning it somewhere in the code would be good.

Created attachment 322502 [details] [review]
appNotification: Add class MessageNotification

We are about to land IRC URL parsing which error
handling requires using an app notification when
IRC URLs cannot be properly parsed. This adds a
new MessageNotification class which shows a notification
for 7 seconds along with a close button.

Created attachment 322503 [details] [review]
app: open IRC links

Register Polari as handler of IRC URLs and add
support for opening IRC URLs pointing to rooms on
the networks the user have added.

Created attachment 322504 [details] [review]
app: Handle links that dont match existing networks

IRC URLs may point to networks which the user haven't
connected to yet. Add support for matching IRC URLs
against the list of predefined networks. If a match
can't be found, create a custom connection pointing
directly to the URL which the IRC link specifies.

Review of attachment 322502 [details] [review]:

Two style nits + a long explanation of the reasoning :-)

::: src/appNotifications.js
@@ +61,3 @@
+        let button = new Gtk.Button({ label: label });
+        button.connect('clicked', Lang.bind(this,
+               function() {

Wrong indent

@@ +69,3 @@
+        this._box.add(button);
+
+        this.show_all();

I'd prefer
  button.show();
(or add 'visible: true' to the contruct parameters of the button)

Let's assume that for some reason we want a notification where the icon can change, so we do something like:

  this._image = new Gtk.Image();
  this._box.add(this._image);

in the constructor and add a setIcon() method:

  setIcon: function(iconName) {
      this._image.icon_name = iconName;
      this._image.visible = iconName != null;
  }

With show_all() in addButton(), we'd end up showing the (empty) image when calling addButton() after setIcon(null). So it's generally better to avoid those side effects and only call show_all() on UI you just created.

Review of attachment 322503 [details] [review]:

LGTM

Review of attachment 322504 [details] [review]:

Dito

Created attachment 322507 [details] [review]
appNotification: Add class MessageNotification

We are about to land IRC URL parsing which error
handling requires using an app notification when
IRC URLs cannot be properly parsed. This adds a
new MessageNotification class which shows a notification
for 7 seconds along with a close button.

Attachment 322503 [details] pushed as 44c75c3 - app: open IRC links
Attachment 322504 [details] pushed as 23d68c5 - app: Handle links that dont match existing networks