conferences saving by zugz · Pull Request #1156 · TokTok/c-toxcore

zugz · 2018-09-09T21:31:19Z

This change is

codecov · 2018-09-25T19:53:00Z

Codecov Report

Merging #1156 into master will increase coverage by <.1%.
The diff coverage is 98.6%.

@@           Coverage Diff            @@
##           master   #1156     +/-   ##
========================================
+ Coverage    83.2%   83.3%   +<.1%     
========================================
  Files          82      82             
  Lines       14685   14827    +142     
========================================
+ Hits        12225   12354    +129     
- Misses       2460    2473     +13

Impacted Files	Coverage Δ
toxcore/tox.c	`66.7% <100%> (ø)`	⬆️
auto_tests/conference_test.c	`99.2% <100%> (+0.1%)`	⬆️
toxcore/friend_connection.c	`94.6% <100%> (ø)`	⬆️
toxcore/group.c	`78.8% <98.3%> (+2.2%)`	⬆️
toxav/msi.c	`64.2% <0%> (-4.2%)`	⬇️
toxcore/LAN_discovery.c	`83% <0%> (-2.9%)`	⬇️
toxav/toxav.c	`67.4% <0%> (-2.6%)`	⬇️
toxcore/onion_client.c	`95.3% <0%> (-0.7%)`	⬇️
toxcore/Messenger.c	`86.8% <0%> (-0.2%)`	⬇️
... and 4 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 caca350...9770880. Read the comment docs.

sudden6

Reviewed 13 of 13 files at r2, 4 of 4 files at r3, 6 of 6 files at r4, 2 of 2 files at r5, 1 of 1 files at r6, 1 of 1 files at r7, 1 of 1 files at r8, 4 of 4 files at r9, 1 of 1 files at r10, 3 of 3 files at r11, 3 of 3 files at r12, 2 of 2 files at r13, 1 of 1 files at r14, 1 of 1 files at r15, 1 of 1 files at r16.
Reviewable status: 0 of 1 approvals obtained (waiting on @iphydf and @zugz)

toxcore/group.h, line 461 at r4 (raw file):

 *
 * @param status Result of loading section is stored here if the section is handled.
 * @return true iff section handled.

document rest of the parameters?

toxcore/group.c, line 2937 at r13 (raw file):

    uint32_t len = SAVED_CONF_SIZE_CONSTANT + g->title_len;

    bool found_self = false;

why do we need this flag? is id_equal that expensive to call? If you want to keep this code I'd expect a comment explaining the reason for the existence of this flag.

toxcore/Messenger.c, line 3263 at r4 (raw file):

bool messenger_load_state_section(Messenger *m, const uint8_t *data, uint32_t length, uint16_t type,
                                  State_Load_Status *status)
{

add assertions for non-null parameters m, data, status?

toxcore/state.c, line 108 at r3 (raw file):

void host_to_lendian_bytes16(uint8_t *dest, uint16_t num)
{

I'm not sure if we want to document preconditions for functions, but I would add an assert(dest != nullptr); here

toxcore/state.c, line 116 at r3 (raw file):

void lendian_bytes_to_host16(uint16_t *dest, const uint8_t *lendian)
{

same

toxcore/tox.c, line 607 at r6 (raw file):

{
    if (data == nullptr) {
        return;

add a log message, because loading to a nullptr was probably not intended?

zugz

Reviewed 1 of 7 files at r17.
Reviewable status: 0 of 1 approvals obtained (waiting on @iphydf, @zugz, and @sudden6)

toxcore/group.h, line 461 at r4 (raw file):

Previously, sudden6 wrote…

document rest of the parameters?

Done.

toxcore/group.c, line 2937 at r13 (raw file):

Previously, sudden6 wrote…

why do we need this flag? is id_equal that expensive to call? If you want to keep this code I'd expect a comment explaining the reason for the existence of this flag.

Done.

toxcore/tox.c, line 607 at r6 (raw file):

Previously, sudden6 wrote…

add a log message, because loading to a nullptr was probably not intended?

This behaviour on null is documented in the API, so we'd best leave it.

toxcore/Messenger.c, line 3263 at r4 (raw file):

Previously, sudden6 wrote…

add assertions for non-null parameters m, data, status?

Generally we don't do this in non-API functions in toxcore. I'd rather conform to that style.

toxcore/state.c, line 108 at r3 (raw file):

Previously, sudden6 wrote…

I'm not sure if we want to document preconditions for functions, but I would add an assert(dest != nullptr); here

As above.

toxcore/state.c, line 116 at r3 (raw file):

Previously, sudden6 wrote…

same

As above.

sudden6

Reviewed 2 of 7 files at r17, 1 of 2 files at r19, 1 of 1 files at r21, 1 of 2 files at r23, 1 of 1 files at r24.
Reviewable status: 0 of 1 approvals obtained (waiting on @iphydf)

toxcore/tox.c, line 607 at r6 (raw file):

Previously, zugz (zugz) wrote…

This behaviour on null is documented in the API, so we'd best leave it.

ok

toxcore/Messenger.c, line 3263 at r4 (raw file):

Previously, zugz (zugz) wrote…

Generally we don't do this in non-API functions in toxcore. I'd rather conform to that style.

ok then

toxcore/state.c, line 108 at r3 (raw file):

Previously, zugz (zugz) wrote…

As above.

ok

toxcore/state.c, line 116 at r3 (raw file):

Previously, zugz (zugz) wrote…

As above.

ok

sudden6 · 2018-10-30T18:04:49Z

Are these CI fails because of this PR or did they exist before?

zugz · 2018-10-30T20:38:46Z

* Tuesday, 2018-10-30 at 11:13 -0700 - sudden6 <notifications@github.com>:

Are these CI fails because of this PR or did they exist before?

The tsan failure is a long-standing problem. The Cirrus failure was due to a complaint about this PR (it didn't like a variable being used only for an assertion) which I've now fixed. At the time of writing it's failing again, but that's just because the tcp test intermittently fails (another long-standing problem).

sudden6

then

Reviewed 5 of 7 files at r17.
Reviewable status: 0 of 1 approvals obtained (waiting on @iphydf)

This change creates groups on startup of Core. We need this once TokTok/c-toxcore#1156 is merged to load existing groups.

* Prepare for TokTok/c-toxcore#1156 to be merged * Restore known groupchats from chatlist

sudden6 · 2018-11-24T12:37:41Z

@iphydf Your lgtm seems to block this PR, what is your current state with this?

iphydf · 2018-11-28T21:25:19Z

Use `backticks` for symbol names.

iphydf · 2018-11-28T21:25:47Z

Add "." at the end (or remove it from all the others).

iphydf · 2018-11-28T21:26:27Z

Remove extra () around returned expression.

* add global friend_connection status callback, used for group rejoining * stop leaving groups on killing tox

iphydf added this to the v0.2.x milestone Sep 9, 2018

zugz force-pushed the minpgcSaving branch from 80be701 to c51126a Compare September 10, 2018 18:42

zugz force-pushed the minpgcSaving branch 2 times, most recently from 0f10df1 to faf64f4 Compare September 25, 2018 19:52

iphydf reviewed Sep 25, 2018

View reviewed changes

Comment thread toxcore/group.c Outdated

zugz force-pushed the minpgcSaving branch 6 times, most recently from ad35c68 to 4ff673e Compare September 28, 2018 17:02

zugz force-pushed the minpgcSaving branch from 4ff673e to 84e7857 Compare October 10, 2018 19:17

zugz force-pushed the minpgcSaving branch from 84e7857 to cdecaf2 Compare October 18, 2018 19:37

zugz changed the title ~~WIP: conferences saving~~ conferences saving Oct 18, 2018

zoff99 mentioned this pull request Oct 24, 2018

Proposal: multidevice and (partial) offline messages #1238

Closed

sudden6 requested changes Oct 28, 2018

View reviewed changes

zugz force-pushed the minpgcSaving branch from 5b4856e to 501d422 Compare October 28, 2018 14:23

zugz commented Oct 28, 2018

View reviewed changes

sudden6 reviewed Oct 28, 2018

View reviewed changes

zugz force-pushed the minpgcSaving branch from 501d422 to dcd55b8 Compare October 30, 2018 20:14

sudden6 approved these changes Oct 30, 2018

View reviewed changes

sudden6 added a commit to sudden6/qTox that referenced this pull request Oct 30, 2018

feat(core): prepare qTox for groupchat saving

c22cbfe

This change creates groups on startup of Core. We need this once TokTok/c-toxcore#1156 is merged to load existing groups.

sudden6 mentioned this pull request Oct 30, 2018

feat(core): prepare qTox for groupchat saving qTox/qTox#5417

Merged

1 task

iphydf mentioned this pull request Nov 4, 2018

Proposal: multidevice and (partial) offline messages #1259

Open

sudden6 added a commit to sudden6/qTox that referenced this pull request Nov 17, 2018

feat(core): prepare qTox for groupchat saving

ce76e08

This change creates groups on startup of Core. We need this once TokTok/c-toxcore#1156 is merged to load existing groups.

zugz mentioned this pull request Nov 18, 2018

Expose offline conference peers in API #1266

Merged

sudden6 added a commit to sudden6/qTox that referenced this pull request Nov 21, 2018

feat(core): prepare qTox for groupchat saving

9f07ca9

This change creates groups on startup of Core. We need this once TokTok/c-toxcore#1156 is merged to load existing groups.

sudden6 added a commit to sudden6/qTox that referenced this pull request Nov 22, 2018

feat(core): prepare qTox for groupchat saving

a82eb6f

This change creates groups on startup of Core. We need this once TokTok/c-toxcore#1156 is merged to load existing groups.

naxuroqa added a commit to naxuroqa/Venom that referenced this pull request Nov 23, 2018

VENOM-443: Prepare for persistent groupchats

af1e490

* Prepare for TokTok/c-toxcore#1156 to be merged * Restore known groupchats from chatlist

naxuroqa mentioned this pull request Nov 23, 2018

VENOM-443: Prepare for persistent groupchats naxuroqa/Venom#443

Merged

iphydf requested changes Nov 28, 2018

View reviewed changes

iphydf approved these changes Nov 29, 2018

View reviewed changes

Comment thread toxcore/group.c Outdated

Comment thread toxcore/group.c Outdated

Implement conferences saving

9770880

* add global friend_connection status callback, used for group rejoining * stop leaving groups on killing tox

zugz force-pushed the minpgcSaving branch from 60c219c to 9770880 Compare November 29, 2018 20:57

zugz merged commit 9770880 into TokTok:master Nov 29, 2018

zugz deleted the minpgcSaving branch November 29, 2018 21:12

anthonybilinski mentioned this pull request Dec 6, 2018

Preserve group chats after restart/disconnect (persistent) & notify on new messages qTox/qTox#896

Closed

robinlinden modified the milestones: v0.2.x, v0.2.9 Jan 3, 2019

Uh oh!

Conversation

zugz commented Sep 9, 2018 • edited by iphydf Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Sep 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

sudden6 left a comment

Choose a reason for hiding this comment

Uh oh!

zugz left a comment

Choose a reason for hiding this comment

Uh oh!

sudden6 left a comment

Choose a reason for hiding this comment

Uh oh!

sudden6 commented Oct 30, 2018

Uh oh!

zugz commented Oct 30, 2018 via email

Uh oh!

sudden6 left a comment

Choose a reason for hiding this comment

Uh oh!

sudden6 commented Nov 24, 2018

Uh oh!

iphydf Nov 28, 2018

Choose a reason for hiding this comment

Uh oh!

zugz Nov 29, 2018

Choose a reason for hiding this comment

Uh oh!

iphydf Nov 28, 2018

Choose a reason for hiding this comment

Uh oh!

zugz Nov 29, 2018

Choose a reason for hiding this comment

Uh oh!

iphydf Nov 28, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

zugz commented Sep 9, 2018 •

edited by iphydf

Loading

codecov Bot commented Sep 25, 2018 •

edited

Loading