Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make group callbacks stateless #146

Merged
merged 1 commit into from
Sep 21, 2016

Conversation

JFreegman
Copy link
Member

@JFreegman JFreegman commented Sep 18, 2016


This change is NOT reviewable. Use Github reviews instead.

@sudden6
Copy link

sudden6 commented Sep 18, 2016

Review status: 0 of 10 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


toxcore/Messenger.c, line 821 at r1 (raw file):

    m->friendlist[friendnumber].statusmessage_length = length;
    return 0;group_invite)(m, i

huh, what's that?


Comments from Reviewable

@JFreegman JFreegman force-pushed the toktok-stateless-group branch from 5db5d9c to d05545d Compare September 18, 2016 19:05
@JFreegman
Copy link
Member Author

toxcore/Messenger.c, line 821 at r1 (raw file):

Previously, sudden6 wrote…

huh, what's that?

Think I must've accidentally ctrl+v'd before I pushed.

Comments from Reviewable

@GrayHatter
Copy link

Reviewed 7 of 10 files at r1, 2 of 2 files at r2.
Review status: 9 of 10 files reviewed at latest revision, 3 unresolved discussions.


toxcore/group.c, line 455 at r2 (raw file):

    add_to_closest(g_c, groupnumber, real_pk, temp_pk);

    if (do_gc_callback && g_c->group_namelistchange) {

Do we want a comment here explaining that this isn't the desired behavior?


toxcore/group.c, line 459 at r2 (raw file):

    }

    if (g->peer_on_join) {

does this need to check do_gc_callback?


toxcore/Messenger.h, line 257 at r2 (raw file):

    void *group_chat_object; /* Set by new_groupchats()*/
    void (*group_invite)(struct Messenger *m, uint32_t, const uint8_t *, uint16_t);
    void (*group_message)(struct Messenger *m, uint32_t, const uint8_t *, uint16_t);

Where was this moved? Or why was it here?


Comments from Reviewable

@JFreegman
Copy link
Member Author

Review status: 9 of 10 files reviewed at latest revision, 3 unresolved discussions.


toxcore/group.c, line 455 at r2 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

Do we want a comment here explaining that this isn't the desired behavior?

I'm not sure what you mean. This behaviour is desired - we want to control when the callback is triggered.

toxcore/group.c, line 459 at r2 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

does this need to check do_gc_callback?

That callback doesn't take a userdata object so it should be safe. It and the other two callbacks inside the Group_C struct are set by ToxAV, rather than by the public API. I'm not sure what disabling them would do, but it's something we might want to look into outside of this PR.

toxcore/Messenger.h, line 257 at r2 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

Where was this moved? Or why was it here?

That was always there. It's needed in Messenger because there's no group yet when you receive an invite packet.

Comments from Reviewable

@JFreegman
Copy link
Member Author

toxcore/Messenger.h, line 257 at r2 (raw file):

Previously, JFreegman wrote…

That was always there. It's needed in Messenger because there's no group yet when you receive an invite packet.

Oh you meant group_message, for some reason reviewable hid that line from your quote. I have no idea what it was for, but it wasn't being used and seemed out of place so I removed it.

Comments from Reviewable

@GrayHatter
Copy link

:lgtm:


Reviewed 1 of 10 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


toxcore/group.c, line 455 at r2 (raw file):

Previously, JFreegman wrote…

I'm not sure what you mean. This behaviour is desired - we want to control when the callback is triggered.

no I mean the desired behavior is not having to make this check because tox_iterate() is supposed to be the only function sending callbacks.

toxcore/Messenger.h, line 257 at r2 (raw file):

Previously, JFreegman wrote…

Oh you meant group_message, for some reason reviewable hid that line from your quote. I have no idea what it was for, but it wasn't being used and seemed out of place so I removed it.

LGTM

That's what I figured, just wanted to make sure.


Comments from Reviewable

@JFreegman JFreegman force-pushed the toktok-stateless-group branch from d05545d to f04814b Compare September 19, 2016 15:31
@JFreegman
Copy link
Member Author

Review status: 9 of 10 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


toxcore/group.c, line 455 at r2 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

no I mean the desired behavior is not having to make this check because tox_iterate() is supposed to be the only function sending callbacks.

Done.

Comments from Reviewable

@GrayHatter
Copy link

:lgtm:


Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@nurupo
Copy link
Member

nurupo commented Sep 19, 2016

Reviewed 8 of 10 files at r1, 1 of 2 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@nurupo
Copy link
Member

nurupo commented Sep 19, 2016

:lgtm_strong:


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@JFreegman JFreegman force-pushed the toktok-stateless-group branch from f04814b to dd2965a Compare September 21, 2016 19:34
@iphydf iphydf merged commit dd2965a into TokTok:master Sep 21, 2016
@iphydf iphydf modified the milestone: v0.0.1 Nov 5, 2016
@JFreegman JFreegman deleted the toktok-stateless-group branch May 3, 2020 03:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants