libtego, libtego_ui: implemented file transfer UI #61
Conversation
src/libtego/include/tego/tego.h
Outdated
| typedef uint32_t tego_file_transfer_id_t; | ||
| // struct for file hash | ||
| typedef struct tego_file_hash tego_file_hash_t; | ||
| // intger type for file size |
morganava
force-pushed
the
file_transfer_ui_review
branch
2 times, most recently
from
934b364 to
aeac594
Compare
| { | ||
| tego_file_transfer_response_accept, // proceed with a file transfer | ||
| tego_file_transfer_response_reject, // reject the file transfer | ||
| } tego_file_transfer_response_t; |
There was a problem hiding this comment.
these should be swapped, since protobuf treats it as 0 or 1 (i.e. true or false), and it's a bit counter intuitive treating true as false and vice versa.
There was a problem hiding this comment.
Hmm, not sure I agree. The response is an enum rather than a tego_bool_t in case we ended up needing different responses in the future (tego_file_transfer_response_maybe_later, tego_file_transfer_response_never_stop_asking, etc). I don't think it really matters, especially since this enum never actually makes its way down to the protofbuf layer.
src/libtego_ui/libtego_callbacks.cpp
Outdated
| tego_file_transfer_id_t id, | ||
| char const* fileName, | ||
| size_t fileNameLength, | ||
| uint64_t fileSize, |
There was a problem hiding this comment.
this should be tego_file_size_t to match the definition in tego.h (typedef void (*tego_file_transfer_request_received_callback_t))
There was a problem hiding this comment.
Gah I thought I'd managed to find the last of these, will fix.
src/libtego_ui/libtego_callbacks.cpp
Outdated
| tego_file_transfer_id_t id, | ||
| tego_file_transfer_direction_t direction, | ||
| uint64_t bytesComplete, | ||
| uint64_t bytesTotal) |
There was a problem hiding this comment.
these should both be changed to tego_file_size_t to match the declaration in tego.h (typedef void (*tego_file_transfer_progress_callback_t)
| required uint32 file_id = 1; | ||
| required uint64 file_size = 2; | ||
| required string name = 3; | ||
| required bytes file_hash = 4; |
There was a problem hiding this comment.
Should these not be optional? Consider at some point we decide to make file_id's 64 bit - all old clients will fail to send files to newer clients. Also, if at any point we need to move to protobuf3, all fields become optional, so once again old clients will not work properly with newer clients.
There was a problem hiding this comment.
See reasoning for removal in protobuf3: protocolbuffers/protobuf#2497 (comment)
There was a problem hiding this comment.
That actually makes a lot of sense. My reasoning had been, why bother making all of these verbose checks on the existence of params, when we can just mark them required and let protbuf handle the check for us. This will be a bit of effort to reimplement, but I'll try and get to it today.
morganava
force-pushed
the
file_transfer_ui_review
branch
from
aeac594 to
c365198
Compare
|
Ok made the suggested changes, and squashed to review branch |
Squashed down my file transfer UI dev branch to a single commit for review.