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

fix: actually give the larger incoming NGC custom packets via API to the client #2441

Closed
wants to merge 1 commit into from

Conversation

zoff99
Copy link

@zoff99 zoff99 commented Dec 1, 2023

This change is Reviewable

@zoff99 zoff99 requested a review from Green-Sky December 1, 2023 12:59
@zoff99 zoff99 changed the title actually give the larger incoming NGC custom packets via API to the client fix: actually give the larger incoming NGC custom packets via API to the client Dec 1, 2023
@@ -156,6 +156,11 @@ typedef enum Group_Sync_Flags {
GF_STATE = (1 << 2), // 4
} Group_Sync_Flags;

typedef enum Group_CustomPkts_Direction {
Copy link
Member

Choose a reason for hiding this comment

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

I dont think we need an whole enum for 2 states.

Copy link
Author

Choose a reason for hiding this comment

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

makes it more readable that way. with true/false you never know

Copy link
Member

Choose a reason for hiding this comment

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

still looks funky next to bool lossless, hmmm

Copy link
Member

Choose a reason for hiding this comment

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

+1 for enums, make the code much more readable.

@pull-request-attention pull-request-attention bot assigned zoff99 and Green-Sky and unassigned Green-Sky and zoff99 Dec 1, 2023
Copy link
Member

@JFreegman JFreegman left a comment

Choose a reason for hiding this comment

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

Can you give a description for why this fix is necessary?

@@ -156,6 +156,11 @@ typedef enum Group_Sync_Flags {
GF_STATE = (1 << 2), // 4
} Group_Sync_Flags;

typedef enum Group_CustomPkts_Direction {
Copy link
Member

Choose a reason for hiding this comment

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

Why Pkts and not just Packets?

@@ -156,6 +156,11 @@ typedef enum Group_Sync_Flags {
GF_STATE = (1 << 2), // 4
} Group_Sync_Flags;

typedef enum Group_CustomPkts_Direction {
GC_CUSTOMPKTS_SENDING = (1 << 0), // 1
Copy link
Member

Choose a reason for hiding this comment

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

Why are we using bitmasks? There will only ever be two possibilities and never at the same time. I don't mind an enum but the values shouldn't be bit set.

@@ -6057,6 +6068,7 @@ static bool handle_gc_lossless_packet(const GC_Session *c, GC_Chat *chat, const
uint8_t packet_type;
uint64_t message_id;


Copy link
Member

Choose a reason for hiding this comment

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

Remove double empty lines

@@ -4906,11 +4911,17 @@ static int handle_gc_private_message(const GC_Session *c, const GC_Chat *chat, c
}

/** @brief Returns false if a custom packet is too large. */
static bool custom_gc_packet_length_is_valid(uint16_t length, bool lossless)
static bool custom_gc_packet_length_is_valid(uint16_t length, bool lossless, Group_CustomPkts_Direction pkt_direction)
Copy link
Member

Choose a reason for hiding this comment

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

This whole function should be rewritten with less nesting.

@zoff99
Copy link
Author

zoff99 commented Jan 10, 2024

i think this one is obsolete now in favor of #2535

@zoff99 zoff99 closed this Jan 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants