Implement a groupchat accept screen. #1074

robinlinden · 2017-09-30T21:18:16Z

I don't remember where I was on this, but it's not done.

This change is

GrayHatter · 2017-10-03T18:32:47Z

Reviewed 3 of 15 files at r1, 26 of 38 files at r2.
Review status: 29 of 47 files reviewed at latest revision, 2 unresolved discussions.

langs/en.h, line 338 at r2 (raw file):

msgid(GROUP_INVITE_FRIEND)
msgstr("%s invites you to the chat")

we don't allow null termination from data that could come from untrusted users.

src/layout/group.c, line 65 at r2 (raw file):

            }

            unsigned w = textwidth(buf, text_length);

this can overrun.

Comments from Reviewable

redmanmale · 2017-10-03T20:16:52Z

Review status: 27 of 47 files reviewed at latest revision, 2 unresolved discussions.

langs/en.h, line 338 at r2 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

we don't allow null termination from data that could come from untrusted users.

Done.

src/layout/group.c, line 65 at r2 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

this can overrun.

Done.

Comments from Reviewable

robinlinden · 2017-10-07T12:18:36Z

Reviewed 8 of 15 files at r1, 32 of 38 files at r2, 7 of 7 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

Comments from Reviewable

robinlinden · 2017-10-07T18:24:36Z

Review status: all files reviewed at latest revision, 3 unresolved discussions.

langs/en.h, line 335 at r3 (raw file):

msgid(GROUP_INVITE)
msgstr("Chat invite")

I'm not a fan of the change from groupchat to chat. Is this something we want to do?

Comments from Reviewable

robinlinden · 2017-10-08T10:30:51Z

Reviewed 18 of 18 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

Comments from Reviewable

robinlinden · 2017-11-05T23:47:36Z

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

src/layout/group_invite.c, line 23 at r4 (raw file):

static void button_group_invite_reject_on_mup(void);

void group_invite_draw(void);

This declaration isn't needed.

Comments from Reviewable

redmanmale · 2017-11-06T10:29:17Z

Review status: 46 of 47 files reviewed at latest revision, 2 unresolved discussions.

src/layout/group_invite.c, line 23 at r4 (raw file):

Previously, robinlinden (Robin Lindén) wrote…

This declaration isn't needed.

Done.

Comments from Reviewable

robinlinden · 2017-11-06T17:36:05Z

Reviewed 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

jhert0 · 2017-12-05T22:22:16Z

Reviewed 8 of 15 files at r1, 18 of 38 files at r2, 2 of 7 files at r3, 18 of 18 files at r4, 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

src/group_invite.c, line 30 at r5 (raw file):

}

static void group_invite_free(const uint8_t invite_id) {

Add a check to make sure invite_id is less than 16

src/group_invite.c, line 123 at r5 (raw file):

}

uint32_t group_invite_get_friend_id(const uint8_t invite_id) {

Add a check to make sure invite_id is less than 16

Comments from Reviewable

im-grey · 2017-12-06T00:38:46Z

Thank you for all your hard work.

redmanmale · 2017-12-06T17:06:36Z

Review status: 32 of 48 files reviewed at latest revision, 3 unresolved discussions.

src/group_invite.c, line 30 at r5 (raw file):

Previously, endoffile78 (Endoffile) wrote…

Add a check to make sure invite_id is less than 16

Done.

src/group_invite.c, line 123 at r5 (raw file):

Previously, endoffile78 (Endoffile) wrote…

Add a check to make sure invite_id is less than 16

Done.

Comments from Reviewable

jhert0 · 2017-12-06T17:14:03Z

Reviewed 14 of 14 files at r6, 2 of 2 files at r7.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

GrayHatter · 2017-12-19T19:41:11Z

"TOUCH ALL THE FILES" -- @robinlinden probably

round 1 review

Reviewed 2 of 15 files at r1, 13 of 18 files at r4, 8 of 14 files at r6, 1 of 2 files at r7.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

langs/en.h, line 335 at r3 (raw file):

Previously, robinlinden (Robin Lindén) wrote…

I'm not a fan of the change from groupchat to chat. Is this something we want to do?

Not in this pull, but we should change everything to conference. Per the toxcore change.

Then groups, and conferences can live side by side. Both with a different feature set.

langs/i18n_decls.h, line 107 at r2 (raw file):

    STR_GROUP_MESSAGE_INVITE,
    STR_GROUP_MESSAGE_JOIN,
    STR_GROUP_MESSAGE_CHANGE_NAME,

Do we want to add a suffix for strings that required additional params?

@robinlinden

src/ui.c, line 314 at r7 (raw file):

    /* Group Settings */
    CREATE_EDIT(group_topic, 10, 95, -10, 24);
    CREATE_DROPDOWN(notify_groupchats, 10, 150, 24, 100);

We don't use this macro anymore. Move it to the UI file.

src/ui.c, line 344 at r7 (raw file):

    CREATE_BUTTON(deny_deletion,    110, MAIN_TOP + 40, _BM_SBUTTON_WIDTH, _BM_SBUTTON_HEIGHT);

    group_invite_draw();

NTS: what is this?

Comments from Reviewable

redmanmale · 2017-12-19T20:49:37Z

Review status: 46 of 48 files reviewed at latest revision, 5 unresolved discussions.

src/ui.c, line 314 at r7 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

We don't use this macro anymore. Move it to the UI file.

This whole commit moved to #1079.

Comments from Reviewable

jhert0 · 2018-01-18T18:30:41Z

Reviewed 2 of 2 files at r8.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

Comments from Reviewable

GrayHatter · 2018-01-24T20:02:46Z

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

src/layout/group.c, line 65 at r2 (raw file):

Previously, redmanmale (redmanmale) wrote…

Done.

Sorry, this is gonna take me some time to review. This being safe depends on how each platform searches string length, + where/how each func counts bytes, vs utf8-chars.

First glance your code looks correct, but I need to make sure all the other functions are being used correctly as well.

Comments from Reviewable

redmanmale · 2018-01-24T21:18:53Z

Reviewed 3 of 15 files at r1, 13 of 38 files at r2, 1 of 7 files at r3, 13 of 18 files at r4, 1 of 1 files at r5, 13 of 14 files at r6, 2 of 2 files at r7, 2 of 2 files at r8.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

src/layout/group.c, line 65 at r2 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

Sorry, this is gonna take me some time to review. This being safe depends on how each platform searches string length, + where/how each func counts bytes, vs utf8-chars.

First glance your code looks correct, but I need to make sure all the other functions are being used correctly as well.

OK, but I did'n touch this at all.

Comments from Reviewable

GrayHatter · 2018-01-24T22:08:17Z

@redmanmale You're right, but I can't LGTM a section that might be wrong. Hence the apology, through no fault of your own, I have to make you wait for code that's likely fine.

redmanmale · 2018-02-10T11:37:42Z

Review status: 40 of 47 files reviewed at latest revision, 7 unresolved discussions, some commit checks broke.

src/messages.c, line 194 at r8 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

what is this section trying to do?

Getting current group to scroll its content; if you're asking why I touched this at all it's because old code (flist_get_groupchat() == get_group(m->id)) was causing a SEGV and I fixed it.

src/layout/group_invite.c, line 27 at r8 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

how often is .update called?

On every UI event; but we do this thing (update = button_setcolors_success) pretty often.

src/layout/group_invite.c, line 88 at r8 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

add the positions to the structs above, and this doesn't' need to exist. (The goal is to remove this MACRO)

Done.

src/ui.c, line 344 at r7 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

NTS: what is this?

Done. Moved to UI definition.

Comments from Reviewable

GrayHatter · 2018-02-25T07:02:44Z

Reviewed 1 of 14 files at r6, 3 of 4 files at r9, 3 of 5 files at r10.
Review status: 45 of 47 files reviewed at latest revision, 5 unresolved discussions, some commit checks broke.

src/layout/group_invite.c, line 27 at r8 (raw file):

Previously, redmanmale (redmanmale) wrote…

On every UI event; but we do this thing (update = button_setcolors_success) pretty often.

LGTM but I'd also like to see // TODO: OPTIMIZE comments when we're doing something the easy way that is a good target for micro optimization.

Comments from Reviewable

redmanmale · 2018-06-16T12:54:31Z

src/layout/group_invite.c, line 27 at r8 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

LGTM but I'd also like to see // TODO: OPTIMIZE comments when we're doing something the easy way that is a good target for micro optimization.

Done.

Comments from Reviewable

jhert0 · 2018-06-18T04:19:01Z

Comments from Reviewable

redmanmale · 2018-09-12T14:05:00Z

If no one mind it, I'd like to merge it. Are there any problems?

robinlinden

Reviewed 2 of 14 files at r6, 1 of 2 files at r7, 1 of 4 files at r9, 2 of 5 files at r10, 17 of 17 files at r11.
Reviewable status: 3 open discussions remaining, 1 assignee LGTMs are missing (waiting on @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, and @undefined)

ghost · 2018-09-12T20:13:53Z

I want to take a look, too. And push translations for Polish, too. And German.

src/command_funcs.c

    if (f != NULL && f->online) {
+        size_t msg_length = UTOX_FRIEND_NAME_LENGTH(f) + SLEN(GROUP_MESSAGE_INVITE);


src/flist.c

+            drawalpha(group_bitmap, avatar_x, y + ROSTER_AVATAR_TOP, default_w, default_w,
+                      selected_item == i ? COLOR_MAIN_TEXT : COLOR_LIST_TEXT);
+
+            size_t msg_length = SLEN(GROUP_INVITE) + 4;


src/flist.c

@@ -895,6 +941,14 @@ GROUPCHAT *flist_get_groupchat(void) {
    return NULL;
 }

+uint8_t flist_get_group_invite_id(void) {
+    if (flist_get_type() == ITEM_GROUP_INVITE) {


src/flist.c

                    if (f->online) {
+                        size_t msg_length = UTOX_FRIEND_NAME_LENGTH(f) + SLEN(GROUP_MESSAGE_INVITE);


src/friend.c

-
-    size_t title_length = snprintf((char *)title, UTOX_FRIEND_NAME_LENGTH(f) + 25, "uTox new message from %.*s",
-                                   (int)UTOX_FRIEND_NAME_LENGTH(f), UTOX_FRIEND_NAME(f));
+    size_t title_length = UTOX_FRIEND_NAME_LENGTH(f) + SLEN(FRIEND_MESSAGE_NEW);


src/groups.c

-        memcpy(old, peer->name, peer->name_length);
-        size_t size = snprintf(msg, TOX_MAX_NAME_LENGTH, "<- has changed their name from %.*s",
-                               peer->name_length, old);
+        size_t size = peer->name_length + SLEN(GROUP_MESSAGE_CHANGE_NAME);


src/groups.c

-    char title[g->name_length + 25];
-
-    size_t title_length = snprintf(title, g->name_length + 25, "uTox new message in %.*s", g->name_length, g->name);
+    size_t title_length = g->name_length + SLEN(GROUP_MESSAGE_NEW);


src/layout/group.c

@@ -56,8 +56,14 @@ static void draw_group(int x, int UNUSED(y), int UNUSED(w), int UNUSED(height))
        GROUP_PEER *peer = g->peer[i];

        if (peer && peer->name_length) {
-            char buf[TOX_MAX_NAME_LENGTH];
-            int  text_length = snprintf((char *)buf, TOX_MAX_NAME_LENGTH, "%.*s, ", (int)peer->name_length, peer->name);
+            uint8_t text_length = peer->name_length + 2;


src/layout/group_invite.c

+    setcolor(COLOR_MAIN_TEXT);
+    setfont(FONT_SELF_NAME);
+
+    size_t label_length = SLEN(GROUP_INVITE) + 4;


src/utox.c

@@ -632,7 +641,7 @@ void utox_message_dispatch(UTOX_MSG utox_msg_id, uint16_t param1, uint16_t param
                g->source[param2] = g->source[g->peer_count];
            }

-            g->topic_length = snprintf((char *)g->topic, sizeof(g->topic), "%u users in chat", g->peer_count);
+            g->topic_length = snprintf((char *)g->topic, sizeof(g->topic), S(GROUP_STATUS), g->peer_count);


src/utox.c

@@ -653,7 +662,7 @@ void utox_message_dispatch(UTOX_MSG utox_msg_id, uint16_t param1, uint16_t param
                return;
            }

-            g->topic_length = snprintf((char *)g->topic, sizeof(g->topic), "%u users in chat", g->peer_count);
+            g->topic_length = snprintf((char *)g->topic, sizeof(g->topic), S(GROUP_STATUS), g->peer_count);


src/group_invite.c

+        return false;
+    }
+
+    uint32_t group_id = UINT32_MAX;


ghost · 2018-09-16T19:17:28Z

I don't like the current screen.

When invited, the groupchat invitation appears on the friend list in a practically unnoticable fashion, even when you learned that it appears there, it is really hard to discover. It should be immediately obvious.
The screen is inconsistent with other screens. It should probably look like the delete-friend screen; bold font and the buttons immediately below and next to each other. The buttons being at opposing ends of the window is not so cool. And the buttons being at the bottom of the window, with the query being at the top, is not that cool, either.
The ignore button is shorter than its text.

ghost · 2018-09-16T20:03:02Z

Also, there should be a setting for auto-accepting group invitations.

Correctly working code is not an exception in uTox.

ghost · 2018-09-16T21:03:53Z

Next I'm translating to German and Polish.

Not yet…

Translate: FRIEND_MESSAGE_NEW GROUP_STATUS GROUP_INVITE GROUP_INVITE_FRIEND GROUP_MESSAGE_INVITE GROUP_MESSAGE_JOIN GROUP_MESSAGE_CHANGE_NAME GROUP_MESSAGE_QUIT GROUP_MESSAGE_NEW

Translate: FRIEND_MESSAGE_NEW GROUP_STATUS GROUP_INVITE GROUP_INVITE_FRIEND GROUP_MESSAGE_INVITE GROUP_MESSAGE_JOIN GROUP_MESSAGE_CHANGE_NAME GROUP_MESSAGE_QUIT GROUP_MESSAGE_NEW Change: GROUP_JOIN_AUDIO LEAVE_GROUP

ghost · 2018-09-17T11:08:40Z

@cebe, you can please look at my translation.

I am not sure about LEAVE_GROUP. I find "Gruppe verlassen" better, but then it's not consistent with GROUP_CREATE, if that is even so important. I find just "Gruppe" to be better, shorter, more natural, easier to read.

robinlinden added the --WIP-- label Sep 30, 2017

robinlinden assigned redmanmale Sep 30, 2017

redmanmale force-pushed the group-accept-screen branch from 28cc526 to c4ca090 Compare October 3, 2017 16:52

redmanmale removed the --WIP-- label Oct 3, 2017

redmanmale mentioned this pull request Oct 3, 2017

Group chat settings #1077

Closed

robinlinden added this to the v0.17.0 milestone Oct 8, 2017

robinlinden mentioned this pull request Oct 14, 2017

Add group accept screen. robinlinden/uToxx#51

Closed

redmanmale mentioned this pull request Oct 30, 2017

Group chats logging #1079

Open

redmanmale force-pushed the group-accept-screen branch from 56b05a3 to da27885 Compare December 6, 2017 17:05

redmanmale assigned GrayHatter and unassigned redmanmale Dec 6, 2017

redmanmale force-pushed the group-accept-screen branch from da27885 to f520dcd Compare December 19, 2017 20:47

redmanmale modified the milestones: v0.17.0, v0.18.0 Apr 18, 2018

redmanmale added 2 commits June 16, 2018 15:41

Merge from develop

31cd4d7

Add todo optimize comment

af2c1a1

redmanmale added 2 commits June 18, 2018 00:05

Resolve lang conflicts; fix AV API callback; remove obsolete var

513fa91

Resolve lang conflicts again

cd4b15d

robinlinden commented Sep 12, 2018

View reviewed changes

ghost suggested changes Sep 12, 2018

View reviewed changes

ghost reviewed Sep 12, 2018

View reviewed changes

src/flist.c Outdated

@@ -895,6 +941,14 @@ GROUPCHAT *flist_get_groupchat(void) {

return NULL;

}

uint8_t flist_get_group_invite_id(void) {

if (flist_get_type() == ITEM_GROUP_INVITE) {

This comment was marked as abuse.

Sign in to view

ghost suggested changes Sep 12, 2018

View reviewed changes

ghost reviewed Sep 12, 2018

View reviewed changes

src/group_invite.c Outdated

return false;

}

uint32_t group_id = UINT32_MAX;

This comment was marked as abuse.

Sign in to view

Fix wrong usage of snprintf()

90007c2

Invert some if-conditions

5da3d51

Correctly working code is not an exception in uTox.

ghost force-pushed the group-accept-screen branch from 5bea998 to 5da3d51 Compare September 16, 2018 20:47

Minor change for clarity

f75fdfd

ghost added the --WIP-- label Sep 16, 2018

ghost previously approved these changes Sep 16, 2018

View reviewed changes

avoidr added 2 commits September 17, 2018 12:45

Update pl.h

a3d6d4f

Translate: FRIEND_MESSAGE_NEW GROUP_STATUS GROUP_INVITE GROUP_INVITE_FRIEND GROUP_MESSAGE_INVITE GROUP_MESSAGE_JOIN GROUP_MESSAGE_CHANGE_NAME GROUP_MESSAGE_QUIT GROUP_MESSAGE_NEW

Update de.h

6426ab1

Translate: FRIEND_MESSAGE_NEW GROUP_STATUS GROUP_INVITE GROUP_INVITE_FRIEND GROUP_MESSAGE_INVITE GROUP_MESSAGE_JOIN GROUP_MESSAGE_CHANGE_NAME GROUP_MESSAGE_QUIT GROUP_MESSAGE_NEW Change: GROUP_JOIN_AUDIO LEAVE_GROUP

		if (f != NULL && f->online) {
		size_t msg_length = UTOX_FRIEND_NAME_LENGTH(f) + SLEN(GROUP_MESSAGE_INVITE);

		if (f->online) {
		size_t msg_length = UTOX_FRIEND_NAME_LENGTH(f) + SLEN(GROUP_MESSAGE_INVITE);

Implement a groupchat accept screen. #1074

Are you sure you want to change the base?

Implement a groupchat accept screen. #1074

Conversation

robinlinden commented Sep 30, 2017 • edited by GrayHatter

GrayHatter commented Oct 3, 2017

redmanmale commented Oct 3, 2017

robinlinden commented Oct 7, 2017

robinlinden commented Oct 7, 2017

robinlinden commented Oct 8, 2017

robinlinden commented Nov 5, 2017

redmanmale commented Nov 6, 2017

robinlinden commented Nov 6, 2017

jhert0 commented Dec 5, 2017

im-grey commented Dec 6, 2017

redmanmale commented Dec 6, 2017

jhert0 commented Dec 6, 2017

GrayHatter commented Dec 19, 2017

redmanmale commented Dec 19, 2017

jhert0 commented Jan 18, 2018

GrayHatter commented Jan 24, 2018

redmanmale commented Jan 24, 2018

GrayHatter commented Jan 24, 2018

redmanmale commented Feb 10, 2018

GrayHatter commented Feb 25, 2018

redmanmale commented Jun 16, 2018

jhert0 commented Jun 18, 2018

redmanmale commented Sep 12, 2018

robinlinden left a comment

Choose a reason for hiding this comment

ghost commented Sep 12, 2018 • edited by ghost

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

ghost commented Sep 16, 2018

ghost commented Sep 16, 2018

ghost commented Sep 16, 2018

ghost commented Sep 17, 2018 • edited by ghost

robinlinden commented Sep 30, 2017 •

edited by GrayHatter

ghost commented Sep 12, 2018 •

edited by ghost

ghost commented Sep 17, 2018 •

edited by ghost