1698924 - Improve prplIConvChatBuddy documentation

Status: RESOLVED FIXED

(Chat Core :: General, task)

Description

[Patrick Cloke [:clokep]]

The flags and documentation of prplIConvChatBuddy aren't great and are inherited from libpurple.

Comment 1

[Patrick Cloke [:clokep]]

Attachment: Patch v1

• Improves documentation.
• Renames halfOp -> moderator and op -> admin (these names seem to make more sense, but were somewhat arbitrary).
• Fixes a bug (which I definitely fixed in Instantbird a longggggg time ago) that prefers founder to operator. (Since founder > operator.)

Comment 2

[Patrick Cloke [:clokep]]

(which I definitely fixed in Instantbird a longggggg time ago)

Bug 955170, 7 years ago. That makes me very sad.

Comment 3

[Patrick Cloke [:clokep]]

(In reply to Patrick Cloke [:clokep] from comment #2)

(which I definitely fixed in Instantbird a longggggg time ago)

Bug 955170, 7 years ago. That makes me very sad.

Actually it was imported 7 years ago. I fixed it on 2012-10-23 so more like 8.5 years ago. I grow sadder.

Comment 4

[Martin Giger [:freaktechnik]]

Comment on attachment 9209541 [details] [diff] [review]
Patch v1

Review of attachment 9209541 [details] [diff] [review]:
-----------------------------------------------------------------

> but were somewhat arbitrary
Inheriting from libpurple isn't that arbitrary in my book (also, it's the IRC names).

> Fixes a bug
So that's why it never showed the founder icon in IRC channels...

I like that this actually makes it so the language applies to more protocols.

This patch looks good, but sadly has a merge conflict by now.

::: chat/protocols/irc/irc.jsm
@@ +681,5 @@
>      }
>  
>      // If the channel mode is +t, hops and ops can set the topic; otherwise
>      // everyone can.
> +    return !this._modes.has("t") || participant.admin || participant.moderator;

Should the comment above also be updated, or should this still use the IRC terminology for admin/moderator?

::: chat/protocols/matrix/matrix.jsm
@@ +91,4 @@
>    },
>    get founder() {
> +    // TODO Could look at the room state to see who the creator was, but would
> +    //      need to ensure they're also an admin.

I don't think that would make sense, since founder status has (to my knowledge) no significance in matrix.

::: chat/protocols/matrix/matrixCommands.jsm
@@ +44,5 @@
>   *
>   * @param {number} power - Power level to get the name for.
>   * @returns {string} Name of the power level.
>   */
>  function getUserPower(power) {

This function went away with Bug 1347542 so this patch no longer applies :(

Comment 5

[Patrick Cloke [:clokep]]

(In reply to Martin Giger [:freaktechnik] from comment #4)

but were somewhat arbitrary
Inheriting from libpurple isn't that arbitrary in my book (also, it's the
IRC names).

I meant that the new names are somewhat arbitrary!

::: chat/protocols/irc/irc.jsm
@@ +681,5 @@

 }

 // If the channel mode is +t, hops and ops can set the topic; otherwise
 // everyone can.

• return !this._modes.has("t") || participant.admin || participant.moderator;

Should the comment above also be updated, or should this still use the IRC
terminology for admin/moderator?

I think it makes sense for this to still refer to IRC terminology, but I can change it if you'd prefer.

Thanks for the feedback. I split the bugfix out into bug 1698994 since that's a discrete concrete improvement. I'll upload a new patch soon.

Comment 6

[Patrick Cloke [:clokep]]

Attachment: Patch v2

This fixes the merge conflict and plays with the matrix powers a bit to ignore founder.

Comment 7

[Martin Giger [:freaktechnik]]

Comment on attachment 9210364 [details] [diff] [review]
Patch v2

Review of attachment 9210364 [details] [diff] [review]:
-----------------------------------------------------------------

matrix.jsm doesn't apply for me to the c-c tip, I think because of a change at the very end of the diff to GenericMatrixConversation.

More importantly though, the MatrixPowerLevel tests need to be adjusted.

::: chat/components/public/prplIConversation.idl
@@ +131,5 @@
>    readonly attribute AUTF8String buddyIconFilename;
>  
> +  /* The role of the participant in the room. */
> +
> +  /* The participant has no particular power and is a normal member. */

According to the jsProtoHelper implementation it's also not typing, which I'm not sure this comment explains.

Comment 8

[Patrick Cloke [:clokep]]

We chatted a bit more about the noFlags flag and decided it isn't useful, so we're going to remove it while we're here.

Comment 9

[Patrick Cloke [:clokep]]

Attachment: Patch v3

• Unbitrotted.
• Updates the Matrix tests.
• Removes the noFlags flag since it is duplicative.

Comment 10

[Patrick Cloke [:clokep]]

Attachment: Patch v4

Now actually including the removal of noFlags.

Comment 11

[Martin Giger [:freaktechnik]]

Comment on attachment 9211766 [details] [diff] [review]
Patch v4

Review of attachment 9211766 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/841d7a1cb63a
Improve prplIConvChatBuddy documentation. r=freaktechnik

Comment 13

[Magnus Melin [:mkmelin]]

Please note that /* foo / style comments are comments, not documentation. Documentation should use /* foo */ for any tools to pick them up if we say, generate idl documentation.