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

Add support for uploading files from memory + bug fixes #168

Open
wants to merge 17 commits into
base: develop
Choose a base branch
from

Conversation

MacDue
Copy link
Contributor

@MacDue MacDue commented Jan 5, 2020

This PR allows for uploading files directly from memory (a feature I needed).

uploadFile(Snowflake<Channel> channelID, uint8_t* buffer, size_t buffer_len, std::string message, std::string filename)

Note!
This PR changes the cpr repo to https://github.com/MacDue/cpr !
This is due to a bug with cpr where uploading files larger than 16,000bytes would corrupt the file -- which I repaired in my fork. Issue described here: libcpr/cpr#342

@MacDue MacDue changed the title Add support for uploading files from memory. Add support for uploading files from memory + bug fixes Jan 24, 2020
@MacDue
Copy link
Contributor Author

MacDue commented Jan 24, 2020

  • fixes for: C++20 support, missing includes, crashes in the error handler

@MacDue MacDue requested a review from yourWaifu January 24, 2020 11:58
@yourWaifu
Copy link
Owner

yourWaifu commented Apr 9, 2020

Can you separate the bug fixes into different pull request?

@yourWaifu
Copy link
Owner

Hello, are you there, I would be happy to accept bug fixes. However I want to consider the new feature separately.

@MacDue
Copy link
Contributor Author

MacDue commented Apr 17, 2020

Not got around to it yet. It'll try to sort it out soon.

@yourWaifu
Copy link
Owner

I've added a number of you bug fixes, please fix the merge conflicts

@MacDue
Copy link
Contributor Author

MacDue commented Jun 5, 2020

@yourWaifu the only merge conflict is the version of CPR I use (since I had to use a custom one to fix an upload bug), they recently merged a fix for that, but it looks like the version you use is still before that. I'll try switching the new official version soon & make sure that works (this branch is kind of changes for my bot -- not a super neat PR).

It currently has these changes:

  • Uploading files from memory a SleepyDiscord::Buffer (with a shared_ptr to the data that allows it to work safely with SleepyDiscord::Async)
  • Making Server::unavailable have three states (NOT_PRESENT, FALSE, TRUE) as detecting if it's not present can be used to tell if you're joining a new server or just reconnecting to a server.
  • Previously merged Async fixes
  • Updating asio to master (which fixed random high CPU bugs)

@yourWaifu
Copy link
Owner

I can't merge when there are merge conflicts, maybe try doing a git rebase.

@MacDue
Copy link
Contributor Author

MacDue commented Jun 5, 2020

@yourWaifu I fixed it for you. I'm leaving the CPR version as you had it as I've yet to try the new version & don't want to link to my fork here. Though I suggest you look into updating it soon.

@yourWaifu
Copy link
Owner

I have updated cpr, I took a look at supporting buffer uploads but I found a problem. You Discord expects a filename if you want to attach it to an embed. CPR doesn't look like it has a way to set the filename of a buffer that I know of.

@yourWaifu
Copy link
Owner

oops nevermind my last statement about buffers and filenames, I didn't look at the libcurl docs hard enough

@MacDue
Copy link
Contributor Author

MacDue commented Sep 26, 2020

You don't have to do it in libcurl, cpr::Buffer is a thing & it just takes the buffer data + a name.

@MacDue
Copy link
Contributor Author

MacDue commented Sep 26, 2020

(the constructor of cpr::Buffer changed a bit since this PR, the name now has to be a std::string like: cpr::Buffer(data, data + m.buffer.length, std::string(m.name));)

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.

2 participants