Skip to content

Improve handling of peers entering and leaving conferences #1267

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

Merged
merged 1 commit into from
Jan 6, 2019

Conversation

zugz
Copy link

@zugz zugz commented Nov 18, 2018

(based on #1266)

  • delete existing peers with same real_pk on adding a peer
  • send freeze packet on quit

This change is Reviewable

@hugbubby
Copy link
Member

Make sure you run astyle so Travis doesn't freak out.

@hugbubby
Copy link
Member

hugbubby commented Nov 18, 2018

You also need to fix the codefactor issues. 3 of them are about there being redundant or unnecessary newlines, one is because a method in this patch is more "complex" than codefactor likes (has too many arguments, return paths, etc.)

@codecov
Copy link

codecov bot commented Nov 22, 2018

Codecov Report

Merging #1267 into master will increase coverage by <.1%.
The diff coverage is 96.5%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #1267     +/-   ##
========================================
+ Coverage      83%     83%   +<.1%     
========================================
  Files          82      82             
  Lines       14928   14957     +29     
========================================
+ Hits        12392   12418     +26     
- Misses       2536    2539      +3
Impacted Files Coverage Δ
toxcore/group.c 78.7% <96.5%> (+0.3%) ⬆️
toxcore/LAN_discovery.c 84.9% <0%> (-1%) ⬇️
toxcore/onion_client.c 95.3% <0%> (-0.7%) ⬇️
toxcore/friend_connection.c 94.6% <0%> (-0.3%) ⬇️
toxav/toxav.c 67.6% <0%> (ø) ⬆️
toxcore/DHT.c 77.8% <0%> (ø) ⬆️
toxav/msi.c 65.6% <0%> (+1.1%) ⬆️

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 e08f631...e5561ad. Read the comment docs.

@anthonybilinski
Copy link

From my own use, I used to reproduce a corruption of the tox savefile frequently when running off tip. Since I switched to this PR branch, I haven't seen it once.

To recover from the corrupt save file, downgrading toxcore to before persistent conferences were introduced, loading then saving, then upgrading to this PR should fix it. Upgrading to this PR directly from a corrupt save file on tip will just fail to load completely.

Copy link

@sudden6 sudden6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

}

static bool delete_frozen(Group_c *g, uint32_t frozen_index)

add nullptr check or comment that g must not be nullptr


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

 * return -1 on failure
 */
static int group_leave(const Group_Chats *g_c, uint32_t groupnumber, bool permanent)

why change the return type? IIRC we're trying to use bool for success/failure return codes


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

 * return -1 on failure
 */
static int group_freeze_peer_send(const Group_Chats *g_c, uint32_t groupnumber, uint16_t peer_num)

use bool for success/error return types?

Copy link
Member

@robinlinden robinlinden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Previously, sudden6 wrote…

add nullptr check or comment that g must not be nullptr

That's fine for functions in the public API, but I think that's unnecessary for things that are only used internally in Toxcore.

@robinlinden robinlinden modified the milestones: v0.2.x, v0.2.9 Jan 3, 2019
@robinlinden robinlinden mentioned this pull request Jan 3, 2019
Copy link

@sudden6 sudden6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm_strong:

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


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

Previously, robinlinden (Robin Lindén) wrote…

That's fine for functions in the public API, but I think that's unnecessary for things that are only used internally in Toxcore.

ok, ignoring this then.

@anthonybilinski
Copy link

#1285 may be related to this change.

* send freeze packet on quit
* delete existing peers with same real_pk on adding a peer
* record actual number of conference peers saved
@iphydf iphydf merged commit e5561ad into TokTok:master Jan 6, 2019
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.

6 participants