Expose offline conference peers in API #1266

zugz · 2018-11-18T17:27:06Z

(based on #1156)

This change is

sudden6

Reviewed 3 of 12 files at r1.
Reviewable status: 0 of 1 approvals obtained (waiting on @zugz)

toxcore/group.h, line 248 at r1 (raw file):

int del_groupchat(Group_Chats *g_c, uint32_t groupnumber);

/* Copy the public key of (frozen) peernumber who is in groupnumber to pk.

Took me quite some time to understand the meaning of (frozen) in conjunction with the function parameter, maybe add an explanation for this concept? Or maybe switch to (frozen/peer) number? maybe there are even better ways to name this.

toxcore/group.c, line 1090 at r1 (raw file):

    Group_Peer *list = frozen ? g->frozen : g->group;
    uint32_t num = frozen ? g->numfrozen : g->numpeers;

both can be const

toxcore/group.c, line 1115 at r1 (raw file):

    Group_Peer *list = frozen ? g->frozen : g->group;
    uint32_t num = frozen ? g->numfrozen : g->numpeers;

same

toxcore/group.c, line 1144 at r1 (raw file):

    Group_Peer *list = frozen ? g->frozen : g->group;
    uint32_t num = frozen ? g->numfrozen : g->numpeers;

same

toxcore/group.c, line 1168 at r1 (raw file):

                             uint64_t *last_active)
{
    Group_c *g = get_group_c(g_c, groupnumber);

const?

toxcore/group.c, line 1201 at r1 (raw file):

    }

    uint32_t num = frozen ? g->numfrozen : g->numpeers;

const

toxcore/group.c, line 1640 at r1 (raw file):

bool group_leave(const Group_Chats *g_c, uint32_t groupnumber)
{
    Group_c *g = get_group_c(g_c, groupnumber);

const

toxcore/tox.c, line 1536 at r1 (raw file):

{
    Messenger *m = tox->m;
    group_leave(m->conferences_object, conference_number);

check return value? if check not needed, why do we have a return value at all?

toxcore/tox.c, line 1650 at r1 (raw file):

{
    const Messenger *m = tox->m;
    int ret = group_number_peers(m->conferences_object, conference_number, true);

const? (similar cases down here)

toxcore/tox.api.h, line 2324 at r1 (raw file):

  }

  namespace offline_peer {

I'm a bit torn on the naming here, on one hand we call offline peers frozen internally but not externally which is a bit inconsistent, on the other hand frozen is not very descriptive for developers not familiar with Tox. What are your thoughts here?

sudden6 · 2018-11-29T14:35:19Z

So after save and load, the group numbers will be sequential. It's
arguable, but I think on balance this is better than preserving the
group numbers. Note that for the protocol it makes no difference - the
peer connections will go down, and so peers will update the group
numbers they have for us when we reconnect to that in our online packet.

Just to be 100% sure, this doesn't mean that after saving a running Tox instance all group numbers are invalid, right? Asking because qTox saves the *.tox on some actions but doesn't invalidate the group numbers.

zugz · 2018-11-29T19:38:02Z

* Thursday, 2018-11-29 at 06:35 -0800 - sudden6 <[email protected]>:

> So after save and load, the group numbers will be sequential. Just to be 100% sure, this doesn't mean that after saving a running Tox instance all group numbers are invalid, right? Asking because qTox saves the *.tox on some actions but doesn't invalidate the group numbers.

That's fine. It's only on loading the saved data, which is necessarily in a new tox instance, that the group numbers will change.

codecov · 2018-11-29T19:58:45Z

Codecov Report

Merging #1266 into master will decrease coverage by 0.2%.
The diff coverage is 37.6%.

@@           Coverage Diff            @@
##           master   #1266     +/-   ##
========================================
- Coverage    83.3%   83.1%   -0.3%     
========================================
  Files          82      82             
  Lines       14869   14928     +59     
========================================
+ Hits        12394   12407     +13     
- Misses       2475    2521     +46

Impacted Files	Coverage Δ
toxcore/state.c	`66.6% <100%> (+2.5%)`	⬆️
toxcore/group.c	`78.3% <64.1%> (-0.5%)`	⬇️
toxcore/tox.c	`63.6% <9.3%> (-3.1%)`	⬇️
toxcore/LAN_discovery.c	`83% <0%> (-2.9%)`	⬇️
toxcore/friend_connection.c	`94.9% <0%> (-1.2%)`	⬇️
toxcore/Messenger.c	`86.8% <0%> (-0.2%)`	⬇️
toxcore/net_crypto.c	`94.9% <0%> (ø)`	⬆️
toxcore/DHT.c	`77.8% <0%> (ø)`	⬆️
toxav/msi.c	`64.8% <0%> (+0.2%)`	⬆️
toxav/toxav.c	`68.1% <0%> (+0.4%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ebf3a82...a122ee9. Read the comment docs.

sudden6 · 2018-11-30T13:05:23Z

ok, that answers my questions.

zugz

Reviewable status: 0 of 1 approvals obtained (waiting on @sudden6)

toxcore/group.h, line 248 at r1 (raw file):

Previously, sudden6 wrote…

Took me quite some time to understand the meaning of (frozen) in conjunction with the function parameter, maybe add an explanation for this concept? Or maybe switch to (frozen/peer) number? maybe there are even better ways to name this.

How about "(possibly frozen) peernumber"?

toxcore/group.c, line 1090 at r1 (raw file):

Previously, sudden6 wrote…

both can be const

done

toxcore/group.c, line 1115 at r1 (raw file):

Previously, sudden6 wrote…

same

Done.

toxcore/group.c, line 1144 at r1 (raw file):

Previously, sudden6 wrote…

same

Done.

toxcore/group.c, line 1168 at r1 (raw file):

Previously, sudden6 wrote…

const?

Made it a TODO, since it's more appropriate for a separate PR.

toxcore/group.c, line 1201 at r1 (raw file):

Previously, sudden6 wrote…

const

Done.

toxcore/group.c, line 1640 at r1 (raw file):

Previously, sudden6 wrote…

const

as above

toxcore/tox.c, line 1536 at r1 (raw file):

Previously, sudden6 wrote…

check return value? if check not needed, why do we have a return value at all?

if we get an error here, we also get an error in del_groupchat on the next line. Checking both would make the code harder to read. So I think it's best to leave it as is. It still makes sense to have group_leave return bool, for consistency and because we might want to check it in some context in the future.

toxcore/tox.c, line 1650 at r1 (raw file):

Previously, sudden6 wrote…

const? (similar cases down here)

Again, made it a TODO

toxcore/tox.api.h, line 2324 at r1 (raw file):

Previously, sudden6 wrote…

I'm a bit torn on the naming here, on one hand we call offline peers frozen internally but not externally which is a bit inconsistent, on the other hand frozen is not very descriptive for developers not familiar with Tox. What are your thoughts here?

My thinking was that "offline" would be clearer to users of the library, but "frozen" is more descriptive of what's actually going on internally. So I kept "frozen" for tox-internal use, but made in "offline" in the API. If you really think it's too confusing, we could use "offline" everywhere. But in the context of the groups internals, this sounds like it should be about connections going down, which it isn't, and more generally there's the risk of confusion with the entirely separate concept of friend_connections being offline.

sudden6

Reviewed 1 of 12 files at r1, 1 of 4 files at r2.
Reviewable status: 0 of 1 approvals obtained (waiting on @zugz)

toxcore/group.h, line 248 at r1 (raw file):

Previously, zugz (zugz) wrote…

How about "(possibly frozen) peernumber"?

now I'm unsure how this API works exactly, if frozen is false, it will only return online peers, if frozen is true it will return online and frozen or frozen only?

toxcore/tox.c, line 1536 at r1 (raw file):

Previously, zugz (zugz) wrote…

if we get an error here, we also get an error in del_groupchat on the next line. Checking both would make the code harder to read. So I think it's best to leave it as is. It still makes sense to have group_leave return bool, for consistency and because we might want to check it in some context in the future.

after a quick glance I could not find another use of group_leave maybe merge it into `del_groupchat´ to avoid this problem and simplify the code?

toxcore/tox.api.h, line 2324 at r1 (raw file):

Previously, zugz (zugz) wrote…

My thinking was that "offline" would be clearer to users of the library, but "frozen" is more descriptive of what's actually going on internally. So I kept "frozen" for tox-internal use, but made in "offline" in the API. If you really think it's too confusing, we could use "offline" everywhere. But in the context of the groups internals, this sounds like it should be about connections going down, which it isn't, and more generally there's the risk of confusion with the entirely separate concept of friend_connections being offline.

After sleeping a few nights over this, I fully agree with your PoV. Externally it would be pretty confusing for user of the library to talk about frozen peers.

sudden6

Reviewed 1 of 12 files at r1.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @zugz)

zugz

Reviewable status: 0 of 1 approvals obtained (waiting on @sudden6)

toxcore/group.h, line 248 at r1 (raw file):

Previously, sudden6 wrote…

now I'm unsure how this API works exactly, if frozen is false, it will only return online peers, if frozen is true it will return online and frozen or frozen only?

Frozen only. How about "(frozen, if frozen is true) peernumber"? I hope that's clear?

toxcore/tox.c, line 1536 at r1 (raw file):

Previously, sudden6 wrote…

after a quick glance I could not find another use of group_leave maybe merge it into `del_groupchat´ to avoid this problem and simplify the code?

It's used elsewhere too in #1267.

sudden6 · 2018-12-02T09:09:31Z

toxcore/group.h, line 248 at r1 (raw file):

Previously, zugz (zugz) wrote…

Frozen only. How about "(frozen, if frozen is true) peernumber"? I hope that's clear?

yeah, that's pretty clear

sudden6 · 2018-12-02T09:12:51Z

toxcore/tox.c, line 1536 at r1 (raw file):

Previously, zugz (zugz) wrote…

It's used elsewhere too in #1267.

Also there it's immediately followed by a call to del_groupchat

zugz · 2018-12-02T09:31:10Z

* Sunday, 2018-12-02 at 01:12 -0800 - sudden6 <[email protected]>:

___ *[toxcore/tox.c, line 1536 at r1](https://reviewable.io/reviews/toktok/c-toxcore/1266#-LSUsFZr7ZFUZznBrUbr:-LSiBA0f4UqgCGmm9H6Y:b-9i49jt) ([raw file](https://github.com/toktok/c-toxcore/blob/4052ebfd11deea5ae5e3af131f089860386766d4/toxcore/tox.c#L1536)):* <details><summary><i>Previously, zugz (zugz) wrote…</i></summary><blockquote> It's used elsewhere too in #1267. </blockquote></details> Also there it's immediately followed by a call to `del_groupchat`

Yes, but to combine them you'd need to pass a bool to del_groupchat, which doesn't seem right. Deleting is deleting, leaving is something separate. Right?

zugz · 2018-12-02T10:43:15Z

* Sunday, 2018-12-02 at 01:31 -0800 - zugz <[email protected]>:

* Sunday, 2018-12-02 at 01:12 -0800 - sudden6 ***@***.***>: >___ >*[toxcore/tox.c, line 1536 at r1](https://reviewable.io/reviews/toktok/c-toxcore/1266#-LSUsFZr7ZFUZznBrUbr:-LSiBA0f4UqgCGmm9H6Y:b-9i49jt) ([raw file](https://github.com/toktok/c-toxcore/blob/4052ebfd11deea5ae5e3af131f089860386766d4/toxcore/tox.c#L1536)):* ><details><summary><i>Previously, zugz (zugz) wrote…</i></summary><blockquote> > >It's used elsewhere too in #1267. ></blockquote></details> > >Also there it's immediately followed by a call to `del_groupchat` Yes, but to combine them you'd need to pass a bool to del_groupchat, which doesn't seem right. Deleting is deleting, leaving is something separate. Right?

On second thoughts, you're right. Doing it all in one function does indeed seem neatest. I've just pushed a commit doing it. (P.S. sorry to have broken out of reviewables... feel free to revert back to it)

sudden6

Reviewed 3 of 3 files at r3.
Reviewable status: 0 of 1 approvals obtained

iphydf · 2019-01-03T22:15:18Z

toxcore/tox.api.h

+    /**
+     * Error codes for peer info queries.
+     */
+    error for query {


error for peer.query can be reused (I hope that there is a syntax for that that works).

zugz mentioned this pull request Nov 18, 2018

Improve handling of peers entering and leaving conferences #1267

Merged

iphydf added this to the v0.2.x milestone Nov 22, 2018

sudden6 requested changes Nov 29, 2018

View reviewed changes

zugz commented Dec 1, 2018

View reviewed changes

zugz force-pushed the confFrozenAPI branch from 2ae0f65 to bc8e0a8 Compare December 1, 2018 16:06

sudden6 requested changes Dec 1, 2018

View reviewed changes

sudden6 reviewed Dec 1, 2018

View reviewed changes

zugz commented Dec 1, 2018

View reviewed changes

zugz force-pushed the confFrozenAPI branch from bf21600 to 3279c8a Compare December 2, 2018 10:44

sudden6 approved these changes Dec 2, 2018

View reviewed changes

zugz force-pushed the confFrozenAPI branch 2 times, most recently from 012ca95 to 155bfd1 Compare December 2, 2018 15:50

iphydf force-pushed the confFrozenAPI branch 3 times, most recently from 2117bd8 to 388942d Compare January 3, 2019 11:28

iphydf requested changes Jan 3, 2019

View reviewed changes

zugz force-pushed the confFrozenAPI branch from 388942d to b88eefe Compare January 3, 2019 22:30

iphydf approved these changes Jan 5, 2019

View reviewed changes

Expose offline conference peers in API

a122ee9

iphydf force-pushed the confFrozenAPI branch from b88eefe to a122ee9 Compare January 5, 2019 15:01

iphydf merged commit a122ee9 into TokTok:master Jan 5, 2019

iphydf removed this from the v0.2.x milestone Jan 6, 2019

iphydf added this to the v0.2.9 milestone Jan 6, 2019

nurupo mentioned this pull request Jan 28, 2019

Persistent conference's offline peer list always grows and never decreases #1306

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose offline conference peers in API #1266

Expose offline conference peers in API #1266

zugz commented Nov 18, 2018 •

edited by iphydf

Loading

sudden6 left a comment

sudden6 commented Nov 29, 2018

zugz commented Nov 29, 2018 via email

codecov bot commented Nov 29, 2018 •

edited

Loading

sudden6 commented Nov 30, 2018

zugz left a comment

sudden6 left a comment

sudden6 left a comment

zugz left a comment

sudden6 commented Dec 2, 2018

sudden6 commented Dec 2, 2018

zugz commented Dec 2, 2018 via email

zugz commented Dec 2, 2018 via email

sudden6 left a comment

iphydf Jan 3, 2019

Expose offline conference peers in API #1266

Expose offline conference peers in API #1266

Conversation

zugz commented Nov 18, 2018 • edited by iphydf Loading

sudden6 left a comment

Choose a reason for hiding this comment

sudden6 commented Nov 29, 2018

zugz commented Nov 29, 2018 via email

codecov bot commented Nov 29, 2018 • edited Loading

Codecov Report

sudden6 commented Nov 30, 2018

zugz left a comment

Choose a reason for hiding this comment

sudden6 left a comment

Choose a reason for hiding this comment

sudden6 left a comment

Choose a reason for hiding this comment

zugz left a comment

Choose a reason for hiding this comment

sudden6 commented Dec 2, 2018

sudden6 commented Dec 2, 2018

zugz commented Dec 2, 2018 via email

zugz commented Dec 2, 2018 via email

sudden6 left a comment

Choose a reason for hiding this comment

iphydf Jan 3, 2019

Choose a reason for hiding this comment

zugz commented Nov 18, 2018 •

edited by iphydf

Loading

codecov bot commented Nov 29, 2018 •

edited

Loading