Add PEP nicknames from XEP-0172 #1867

bodqhrohro · 2023-07-13T02:13:48Z

How to test the functionality

Setup telegabber
Switch to the roster window
Change some chat name at the Telegram side
Look if it's changed in the roster

I ran valgrind when using my new feature

==4147910== Memcheck, a memory error detector
==4147910== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
==4147910== Using Valgrind-3.19.0 and LibVEX; rerun with -h for copyright info
==4147910== Command: profanity
==4147910== 
==4147910== 
==4147910== HEAP SUMMARY:
==4147910==     in use at exit: 1,849,573 bytes in 1,554 blocks
==4147910==   total heap usage: 1,551,248 allocs, 1,549,694 frees, 87,668,537 bytes allocated
==4147910== 
==4147910== LEAK SUMMARY:
==4147910==    definitely lost: 1,080 bytes in 15 blocks
==4147910==    indirectly lost: 2,125 bytes in 41 blocks
==4147910==      possibly lost: 832 bytes in 14 blocks
==4147910==    still reachable: 1,842,968 bytes in 1,457 blocks
==4147910==         suppressed: 0 bytes in 0 blocks
==4147910== Rerun with --leak-check=full to see details of leaked memory
==4147910== 
==4147910== For lists of detected and suppressed errors, rerun with: -s
==4147910== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

sjaeckel

👍

Will you also add the possibility to set such a nickname? as FMU now you'd have to use a different client to set it.

==4147910== definitely lost: 1,080 bytes in 15 blocks

where do those lost blocks originate? I'm mostly certain they're not lost in your feature, but still it'd be nice to know. Feel free to attach a logfile.

sjaeckel · 2023-07-13T05:42:45Z

src/xmpp/nickname.c

+        xmpp_stanza_t* event = xmpp_stanza_get_child_by_name_and_ns(stanza, STANZA_NAME_EVENT, STANZA_NS_PUBSUB_EVENT);
+        if (event) {
+            xmpp_stanza_t* items = xmpp_stanza_get_child_by_name(event, STANZA_NAME_ITEMS);
+            if (items) {
+                xmpp_stanza_t* item = xmpp_stanza_get_child_by_name(items, STANZA_NAME_ITEM);
+                if (item) {
+                    xmpp_stanza_t* nick = xmpp_stanza_get_child_by_name_and_ns(item, STANZA_NAME_NICK, STANZA_NS_NICK);


This could be simplified by using xmpp_stanza_get_child_by_path()

Suggested change

xmpp_stanza_t* event = xmpp_stanza_get_child_by_name_and_ns(stanza, STANZA_NAME_EVENT, STANZA_NS_PUBSUB_EVENT);

if (event) {

xmpp_stanza_t* items = xmpp_stanza_get_child_by_name(event, STANZA_NAME_ITEMS);

if (items) {

xmpp_stanza_t* item = xmpp_stanza_get_child_by_name(items, STANZA_NAME_ITEM);

if (item) {

xmpp_stanza_t* nick = xmpp_stanza_get_child_by_name_and_ns(item, STANZA_NAME_NICK, STANZA_NS_NICK);

xmpp_stanza_t* nick = xmpp_stanza_get_child_by_path(stanza, XMPP_STANZA_NAME_IN_NS(STANZA_NAME_EVENT, STANZA_NS_PUBSUB_EVENT), STANZA_NAME_ITEMS, STANZA_NAME_ITEM, XMPP_STANZA_NAME_IN_NS(STANZA_NAME_NICK, STANZA_NS_NICK), NULL);

Nah, nick is NULL this way, and it's non-trivial to debug where exactly does it fail (looks OK to me, huh :/), so I'll probably leave it intact for now.

Indeed, as I forgot that you have to give the full path, i.e. this should work:

Suggested change

xmpp_stanza_t* event = xmpp_stanza_get_child_by_name_and_ns(stanza, STANZA_NAME_EVENT, STANZA_NS_PUBSUB_EVENT);

if (event) {

xmpp_stanza_t* items = xmpp_stanza_get_child_by_name(event, STANZA_NAME_ITEMS);

if (items) {

xmpp_stanza_t* item = xmpp_stanza_get_child_by_name(items, STANZA_NAME_ITEM);

if (item) {

xmpp_stanza_t* nick = xmpp_stanza_get_child_by_name_and_ns(item, STANZA_NAME_NICK, STANZA_NS_NICK);

xmpp_stanza_t* nick = xmpp_stanza_get_child_by_path(stanza, STANZA_NAME_MESSAGE, XMPP_STANZA_NAME_IN_NS(STANZA_NAME_EVENT, STANZA_NS_PUBSUB_EVENT), STANZA_NAME_ITEMS, STANZA_NAME_ITEM, XMPP_STANZA_NAME_IN_NS(STANZA_NAME_NICK, STANZA_NS_NICK), NULL);

bodqhrohro · 2023-07-13T06:20:14Z

Feel free to attach a logfile.

prof1.txt

jubalh

Thanks for your feature!

Regarding CI:

Please check https://github.com/profanity-im/profanity/blob/master/CONTRIBUTING.md on how to run clang format.
You need to add a nickname_pep_subscribe (stub) to the tests. You can grep for avatar_pep_subscribe for example as inspiration

jubalh · 2023-07-13T08:30:43Z

A couple of questions:

How should this play together with /nick (setting your own nickname in a MUC) /roster nick (assigning a nickname to a user in your roster). Should a nick sent by the contact or the nick I assign via /roster nick have a higher priority?

Will you also add the possibility to set such a nickname? as FMU now you'd have to use a different client to set it.

?

jubalh · 2023-07-13T18:10:37Z

From our MUC:

jubalh: pep.: do you support xep0172 in poezio? I would have some questions how to handle it: https://github.com/profanity-im/profanity/pull/1867#issuecomment-1633801969
pep.: I think we do but not really. I'd like to. I started something here about it: https://codeberg.org/joinjabber/support/issues/6
pep.: But for your question,  
pep.: If a nick is set manually in a MUC I'd use that rather than the global set nick
pep.: Same for the roster.
pep.: In general, if a user has done something manually for a more detailed level I'd use that rather than the global setting (172)
jubalh: pep.: I agree. Thanks for your input!

So I think we need to check if the user did set a nickname for person in their roster and only if they didn't use the nick that we get over this XEP. Same for MUC.

Could you add such functionality?

And do you plan to also implement the setting of the nick?

If you need any help or guidance don't hesitate to ask here or contact us in the MUC.

bodqhrohro · 2023-07-13T22:02:09Z

How should this play together with /nick (setting your own nickname in a MUC) /roster nick (assigning a nickname to a user in your roster). Should a nick sent by the contact or the nick I assign via /roster nick have a higher priority?

From the experience with my transport, users have different expectations. Some indeed prefer to set contact nicknames manually, and are frustrated when they are updated automatically. Other users are rather frustrated when they don't see relevant updates of the nickname (especially if they have saved a nickname provided in a subscription request, which in some clients is equal to setting it manually).

I suppose it should be a configuration option then, and the default should be to respect the nickname from the roster first then, as recommended by Modern XMPP.

And do you plan to also implement the setting of the nick?

Maybe, but later. Setting the nickname via PubSub seems to need some server-side support, so I'll need to prepare it for testing first.

jubalh · 2023-07-14T05:27:40Z

I suppose it should be a configuration option then, and the default should be to respect the nickname from the roster first then, as recommended by Modern XMPP.

Yes configuration option would be good.

Taking roster nick in MUC doesn't make much sense to me since it could be that I want to reply to a user and write "John Tolkien: how are you doing?" because in my roster he is named like that. But everyone else sees his nickname as "JRRT" because that's what he set when joining.

Maybe, but later. Setting the nickname via PubSub seems to need some server-side support, so I'll need to prepare it for testing first.

Ok, great.

H3rnand3zzz

Please, fix the nesting, add XEP support information to .doap file and disco (XEP-0030, XEP-0115, if it's recommended/required by the XEP that you are adding).

Thank you for your contribution.

H3rnand3zzz · 2023-07-30T12:57:47Z

src/xmpp/nickname.c

+            if (items) {
+                xmpp_stanza_t* item = xmpp_stanza_get_child_by_name(items, STANZA_NAME_ITEM);
+                if (item) {
+                    xmpp_stanza_t* nick = xmpp_stanza_get_child_by_name_and_ns(item, STANZA_NAME_NICK, STANZA_NS_NICK);


Even if you want to follow this way, your nesting is too deep. Please, follow the best practices https://testing.googleblog.com/2017/06/code-health-reduce-nesting-reduce.html

jubalh · 2023-09-11T10:29:18Z

@bodqhrohro ping :)

bodqhrohro · 2023-09-11T15:06:36Z

I see no real-life use for setting the nickname via PubSub and thus not sure what to do and why.

I forgot that my transport also sends the nickname in presences, and I implemented PubSub later than that specifically for Gajim as Gajim ignores the nickname in presences and uses only nicknames from PubSub. The person who I made this patch for finally implemented their own patch which takes the nickname from presences, as they found it easier; not sure if they intend to merge it upstream at all. Later I discovered that this actually violates the standard, as it specifies that regular non-subscribe presences MUST NOT include the nick.

jubalh · 2023-10-09T07:50:06Z

Do I understand you right that you think your PR is obsolete?

bodqhrohro · 2023-10-09T09:59:46Z

I'm not sure about that, as the other patch violates the XEP, but as of now it Just Works™. PEP is also not a great solution for the task, given that the roster name is anyway the highest priority according to modernxmpp spec, yet it works in Gajim. Slidge implements roster sync via a special privilege, and we probably would go this way eventually too (but that won't work for the users from other servers).

As of now, I'll probably close this PR and reopen it later if we come up with some decision. When more important features are implemented, I'll anyway return to the problem of presence flood, as they consume traffic excessively, and those out-of-spec nickname presences are a part of it.

jubalh · 2023-10-09T14:03:58Z

As of now, I'll probably close this PR and reopen it later if we come up with some decision.

Feel free to reopen or create a new patch when you feel like it!
All improvements are welcome.

sjaeckel requested changes Jul 13, 2023

View reviewed changes

bodqhrohro force-pushed the xep-172-headline branch from 97d1925 to 4d6c797 Compare July 13, 2023 06:33

jubalh self-requested a review July 13, 2023 06:46

jubalh requested changes Jul 13, 2023

View reviewed changes

jubalh assigned bodqhrohro Jul 13, 2023

jubalh added the feature label Jul 13, 2023

Add PEP nicknames from XEP-0172

761932d

bodqhrohro force-pushed the xep-172-headline branch from 4d6c797 to 761932d Compare July 13, 2023 07:24

H3rnand3zzz suggested changes Jul 30, 2023

View reviewed changes

bodqhrohro closed this Oct 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PEP nicknames from XEP-0172 #1867

Add PEP nicknames from XEP-0172 #1867

bodqhrohro commented Jul 13, 2023

sjaeckel left a comment

sjaeckel Jul 13, 2023

bodqhrohro Jul 13, 2023

sjaeckel Jul 30, 2023

bodqhrohro commented Jul 13, 2023

jubalh left a comment

jubalh commented Jul 13, 2023

jubalh commented Jul 13, 2023

bodqhrohro commented Jul 13, 2023

jubalh commented Jul 14, 2023

H3rnand3zzz left a comment

H3rnand3zzz Jul 30, 2023

jubalh commented Sep 11, 2023

bodqhrohro commented Sep 11, 2023

jubalh commented Oct 9, 2023

bodqhrohro commented Oct 9, 2023

jubalh commented Oct 9, 2023

Add PEP nicknames from XEP-0172 #1867

Add PEP nicknames from XEP-0172 #1867

Conversation

bodqhrohro commented Jul 13, 2023

How to test the functionality

I ran valgrind when using my new feature

sjaeckel left a comment

Choose a reason for hiding this comment

sjaeckel Jul 13, 2023

Choose a reason for hiding this comment

bodqhrohro Jul 13, 2023

Choose a reason for hiding this comment

sjaeckel Jul 30, 2023

Choose a reason for hiding this comment

bodqhrohro commented Jul 13, 2023

jubalh left a comment

Choose a reason for hiding this comment

jubalh commented Jul 13, 2023

jubalh commented Jul 13, 2023

bodqhrohro commented Jul 13, 2023

jubalh commented Jul 14, 2023

H3rnand3zzz left a comment

Choose a reason for hiding this comment

H3rnand3zzz Jul 30, 2023

Choose a reason for hiding this comment

jubalh commented Sep 11, 2023

bodqhrohro commented Sep 11, 2023

jubalh commented Oct 9, 2023

bodqhrohro commented Oct 9, 2023

jubalh commented Oct 9, 2023