*** Bug 773052 has been marked as a duplicate of this bug. ***

i dont know what one would do for custom networks but for predefined networks one way to solve this would be by adding a new field in networks.json so we can define special cases like OFTC, fx:

  {
    "name": "OFTC",
    "id": "oftc",
    "authorder": {"password", "nick"},
    "servers": [
      { "ssl": false, "port": 6667, "address": "irc.oftc.net" }
    ]
  },

and then update the code inside polari-room.c / polari-util.c to be aware of this.

I guess the "real" solution is to support SASL though.

*** Bug 778384 has been marked as a duplicate of this bug. ***

I'm investigating this issue now.

After checking out around 20 of the most popular irc networks i have come to find that most support "/msg nickserv identify password". Even if some supported the current paradigm most do not. The only ones that did not accept the paradigm offered by the patch required a whole separate form of ident generally of "/ident xxx xxx"

I will attach the patch i have suggested to remove the username field from the identify message.

Created attachment 349824 [details] [review]
Proposed fix for bug

see last comment for explanation.

Review of attachment 349824 [details] [review]:

The drawback of this approach is that NickServ identification doesn't work when connecting with a different nick (say, 'nick_'), and users have to revert back to manually identifying. Sorry, but I'm much more interested in working as best as we can with GNOME/Freenode than with some "wide range of irc servers".

Thinking about it, we may be able to do both without an elaborate scheme to determine what parameters a particular bot supports:
Just include the username parameter iff the configured nickname doesn't match the currently used one. This should work in all cases where identification currently works (unlike your patch), but also work for bots that don't understand the username parameter if the nickname matches the configured one (we'd still send a "wrong" identify command if the nickname doesn't match, but then the "correct" command would fail anyway because the nick isn't the registered one).

::: src/lib/polari-room.c
@@ +297,3 @@
   room->priv->ignore_identify = TRUE;
 
+  text = g_strdup_printf ("%s %s %s", command, password); //Updated to not send the username as most irc connections either don't require or will not be able to process with username.

Mismatch between number of format specifiers and parameters - this not only results in a compiler warning, but actually breaks nickserv identification where two parameters are supported.

Also: Try to keep lines at ~80 characters

Sounds like i get to do some more research! Thanks for your suggestion i will look at implementing it and submitting a patch with the proposed idea. I think it is important to remember a large group of users use other irc networks as well so i want to make sure polari is rocking for those users as well (which just happens to include myself :)).

Created attachment 350274 [details] [review]
Proposed fix with method recomended

Here is a patch with the solution as recommended.

Review of attachment 350274 [details] [review]:

In addition to the errors below, please try to provide a proper commit message - https://chris.beams.io/posts/git-commit/ can provide some guidance. Multiple complete sentences on the *subject* line are a complete no-go ...

(Something like:
room: Only identify with nickname when necessary

Many NickServ bots only take a password parameter, which we don't support
as we currently always send the username parameter along to make sure
that identification is still possible when connecting with a nickname
that doesn't match the registered one. 
To address this without regressing for networks where the username is
understood, omit the optional parameter if it matches the currently
used nickname.

::: src/lib/polari-room.c
@@ +297,3 @@
   /* Don't emit ::identify-sent for our own identify message */
   room->priv->ignore_identify = TRUE;
+  if (strcmp(active_username, username)!=1) {

- the check is wrong - we want to know when the two
  nicknames are equal (== 0), not when they differ in
  some specific way
- style nit: space before opening parentheses and around
  operators
- style nit: use GNU-style indentation, omit braces when
  all blocks are single lines

::: src/lib/polari-room.h
@@ +40,3 @@
 void polari_room_send_identify_message_async (PolariRoom          *room,
                                               const char          *command,
+                                              const char          *active_username,

What is the use case for using any string other than the currently active username? If the answer is: "there is none", then the parameter shouldn't exist.

::: src/telepathyClient.js
@@ +311,3 @@
                 let room = this._roomManager.lookupRoomByChannel(channel);
+                let active_username = room.channel.connection.self_contact.alias;
+                log(active_username)

Left-over debug

Created attachment 350520 [details] [review]
Fixed with proposed issues

Corrected the issues you raised, asked on irc about your statement 

"What is the use case for using any string other than the currently active username? If the answer is: "there is none", then the parameter shouldn't exist."

As i don't quite understand what issue you are raising, (forgive my ignorance) however it seems we are in different time zones, and i'm not always on irc, i figured bugzilla would be a better form of communication.

If there is something else that needs fixed please let me know and I will get right to work on it. 
-justyn

(In reply to justyn temme from comment #11)
> Corrected the issues you raised, asked on irc about your statement 
> 
> "What is the use case for using any string other than the currently active
> username? If the answer is: "there is none", then the parameter shouldn't
> exist."

The 'active_username' parameter is awkward, so unless there is a good reason for it, I don't want it. "Getting the active nick is more verbose in C" is *not* a good reason, so unless there's a use case for passing in a different string, the patch should only change the send_identify_message() implementation, not the API.

Created attachment 350576 [details] [review]
Proposed patch

Was able to consolidate the fix to a lot less code than originally submitted. Thanks for the advice i would have to agree with what you are saying. Again if there is another issue just let me know.

Review of attachment 350576 [details] [review]:

::: src/lib/polari-room.c
@@ +300,3 @@
   room->priv->ignore_identify = TRUE;
 
+  if (strcmp (room->priv->self_nick, username) == 0)

No, that's not correct - "self_nick" is the string we use to decide whether a message should be highlighted, not necessarily the active nick:

https://git.gnome.org//browse/polari/tree/src/lib/polari-room.c#n354

For example if the current nick is "somenick__", self_nick will be "somenick".

Created attachment 351073 [details] [review]
patch

Okay, everything is working. If you would rather me get the nick with nested function calls i can, however, I figured displaying each call would make it easier to read. Up to you, just let me know and I will change if needed.

Review of attachment 351073 [details] [review]:

::: src/lib/polari-room.c
@@ +300,3 @@
   room->priv->ignore_identify = TRUE;
 
+  TpConnection *current_connection = tp_channel_get_connection(priv->channel);

Coding style: declarations after statements

@@ +302,3 @@
+  TpConnection *current_connection = tp_channel_get_connection(priv->channel);
+  TpContact *current_contact = tp_connection_get_self_contact(current_connection);
+  char *current_alias = tp_contact_get_alias(current_contact);

const char *

Created attachment 351186 [details]
Userserv error

Sigh, I found a bot that requires the username ... :-(

(In reply to Florian Müllner from comment #17)
> Sigh, I found a bot that requires the username ... :-(

OK, so I think we can support this case as well:

 - when the user identifies herself, also store whether
   the username was included in addition to botname, command,
   username and password

 - then include the username if it was provided originally or
   the currently active one doesn't match the stored one

Now, the settings are handled in the JS code, so we'll need to somehow pass to send_identify() that the username should be included unconditionally - a boolean force_username or similar would do the job, but "magic" booleans generally make for pretty bad API.

So what do you think of the following - split the patch in two, so that:

 1. the username parameter to send_identify() is made optional, and only
    send "<command> <password>" if it's not provided

 2. implement the "currentAlias == savedNickname" logic in the calling JS code

With that, I can easily add a 3rd patch on top to implement the above setting. (Or if you are interested in looking into that last part as well, just tell me and I'll be happy to assist you with that)

well i think the best place to begin is adding the variable use-username
then as you said when it's grabbed simply document the number of args and determine if we need to send username as well.

if we could somehow use method overloading that would avoid the "magic" boolean. however both C and js lack those abilities. C11 has something of the sort with generic functions. not sure if that's the route we need to take

i will get started on the documenting if username was used but would like your input on how to proceed after that.

(In reply to justyn temme from comment #19)
> if we could somehow use method overloading that would avoid the "magic"
> boolean. however both C and js lack those abilities.

Well, my idea would be to pass a username of %NULL if it shouldn't be sent, avoiding the magic boolean :-)

okay before i get started changing code just wanted to make sure my goals aligned with yours. gameplan

js code
when does the username gets stored count number of args


if args = 3

use_username=0

C Code
change C function args to include use_username, 

depending on use username send username or not

(In reply to justyn temme from comment #21)
> js code
> when does the username gets stored count number of args

You don't need to count the number of arguments, if a username was entered, it will be passed on the the JS code, otherwise it will be null:

https://git.gnome.org/browse/polari/tree/src/telepathyClient.js#n552

So

alwaysIncludeUsername = (username != null)


> C Code
> change C function args to include use_username,

That's possible, but I don't like a boolean like that. I prefer allowing %NULL for the username parameter, then have the JS code deal with the logic of whether or not it should be included. Something like:

let savedUsername = settings.get_string('identify-username');
let alwaysIncludeUsername = settings.get_boolean('identify-include-username');

this._requestChannel(() => {
    let includeUsername = alwaysIncludeUsername ||
                          channel.connection.self_contact.alias != username;
    room.send_identify_message_async(command, includeUsername ? username : null, password);
}


As far as history is concerned, I imagine at least three commits:

 - change the C function to accept %NULL
 - omit the username when it matches the currently used one
   (what your current patch does, but moved to JS)
 - add the setting and use it as well, to not regress for bots that
   require the username

Created attachment 351481 [details] [review]
Patches

After applying as you had suggested javascript is alerting me the username param may NOT be null. I have cleared up the issue in the C code but the JS error still remains. Is this do to something in javascript or just my ingorance?

You need to add the (nullable) annotation to mark the parameter as optional, see https://wiki.gnome.org/Projects/GObjectIntrospection/Annotations#GObject-Introspection_annotations.

identify (null) ******** 

is the message it sends when i have added the proper g-introspection comment. it seems i'm going to have to check if the value is null and replace with an empty string, would a better option be in the js code passing "" as the username?

(In reply to justyn temme from comment #25)
> identify (null) ******** 

Yes, you still have to do the "%s %s" vs. "%s %s %s" differentiation from the previous iteration.

Created attachment 351538 [details] [review]
First set of patches for proposed fix

Okay, all that is left to do is to add the setting to check the arguments of the initial nickserv ident. i can add that fairly easy should have a patch uploaded here soon.

Review of attachment 351538 [details] [review]:

(Note: splinter doesn't deal well with more than one patch per attachment, so separate attachments would be appreciated)

In addition to the comments below, both commit messages need work:
 - wrap lines
 - don't reword *what* the code is doing, describe *why* something is done
 - style: *never* start a message with "This commit does ..."
   https://chris.beams.io/posts/git-commit/ has good advice

::: src/telepathyClient.js
@@ +311,2 @@
                 let room = this._roomManager.lookupRoomByChannel(channel);
+                let active_username = room.channel.connection.self_contact.alias;

Style: camelCase

@@ +311,3 @@
                 let room = this._roomManager.lookupRoomByChannel(channel);
+                let active_username = room.channel.connection.self_contact.alias;
+                if (active_username == username) username = null;

Style: separate lines

Also the code isn't immediately obvious, so a small comment would be good

::: src/lib/polari-room.c
@@ +271,3 @@
+/**
+ * polari_room_send_identify_message_async:
+ *  @username: (nullable): Allows username to be passed as NULL

Either skip the doc part, or provide a proper documentation string

@@ +306,3 @@
 
+  if (username == NULL)
+    text = g_strdup_printf ("%s %s", command,password);

Nit: missing space

Oh, and one more nit:
The patches are in the wrong order, you currently pass null to the function before the patch that makes this a valid value.

It would seem we need to add the variable, use_username to overide as you suggested. My thoughts would be to add a key to org.gnome.Polari.gschema.xml. Are these added manually then edited by the code or is there a setter function for adding these variables i'm not aware of?

Hey Florian life is busy so i do not want to press you. I would like to help the project and need a little more info on the idiomatic way to add variables to org.gnome.polari.gschema.xml. When you get time if you could point me in the direction of the right documentation, as i can't seem to find it, i would greatly appreciate it. I owe you a beer

GSettings require every setting to be backed by a schema, so yes, you'll have to add any new one to the xml file. (We should also come up with something better than 'use-username' as well, as a value of false doesn't mean that we don't send it)

Created attachment 354174 [details] [review]
room: Make username parameter to send_identify() optional

Not all NickServ bots understand the username parameter, so the
identify command we send doesn't work at all on those networks.
We will address this by not sending the username unconditionally,
so as a first step make the parameter optional.

(Fixed up patch according to review)

Created attachment 354175 [details] [review]
telepathyClient: Only identify with nickname when necessary

Many NickServ bots only take a password parameter, while we currently
send the username along unconditionally to make sure that identification
is still possible when connecting with a nickname that doesn't match
the registered one.
In order to work both with bots that support an optional username
parameter and those that don't understand the parameter at all,
omit the username when it already matches the currently used nick.

Created attachment 354176 [details] [review]
telepathyClient: Always include username when it's known to work

In addition to NickServ bots that don't support the username parameter
and those that optionally support it, there are also bots that require
it. To support those as well, store whether the user included the
username in the identify command - if it was, we know the parameter
is supported and we can always send it regardless of the currently
used nick.

Attachment 354174 [details] pushed as dc9e903 - room: Make username parameter to send_identify() optional
Attachment 354175 [details] pushed as 03eef02 - telepathyClient: Only identify with nickname when necessary
Attachment 354176 [details] pushed as 87b377a - telepathyClient: Always include username when it's known to work

*** Bug 784217 has been marked as a duplicate of this bug. ***