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.

Sign up for GitHub

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

Jump to bottom

Add ability to get resume data synchronously #7869

Open
wants to merge 1 commit into
base: RC_2_0
Choose a base branch
from
Open

Add ability to get resume data synchronously #7869

wants to merge 1 commit into from
+27 −0

Conversation

glassez
Copy link
Contributor

@glassez glassez commented Feb 14, 2025 •
edited
Loading

No description provided.

@glassez
Copy link
Contributor Author

glassez commented Feb 14, 2025

@arvidn
I don't know if you're notified of new Issues/PRs immediately, or if you review them from time to time, so I preferred to call you explicitly.
I would like to draw your attention to #7855 and get at least a general feedback on it as early as possible. I don't want to be a "train lagger" if it could have been approved, but it was noticed too late. I could contribute to its implementation myself, but I don't want to start work without knowing if it has a chance to be merged.

@glassez glassez closed this Feb 14, 2025
@arvidn
Copy link
Owner

arvidn commented Feb 15, 2025

I think this PR looks good. It would be nice to expose it to python, add a test to exercise it and I don't think you need the static empty add_torrent_params object, since you're returning it by value. (had the return value been by reference on the other hand, you would have needed it).

@glassez glassez reopened this Feb 15, 2025
@glassez
Copy link
Contributor Author

glassez commented Feb 15, 2025

Probing the ground in the direction of solving #7855...

Unfortunately, I got my cards mixed up in a hurry. These changes are not really an attempt to solve #7855, but just a quick workaround for some problems related to generating torrent file as mentioned in #7854.

glassez
glassez commented Feb 15, 2025
View reviewed changes
include/libtorrent/torrent_handle.hpp
@@ -844,6 +845,8 @@ namespace aux {
bool need_save_resume_data() const;
bool need_save_resume_data(resume_data_flags_t flags) const;

add_torrent_params get_resume_data(resume_data_flags_t flags = {}) const;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@arvidn
Does it make sense to support flags in this function? The only flag that makes sense when applied to it is save_info_dict. But the point is that for some time now it only assign a pointer to add_torrent_params. What could be the problem of doing this unconditionally? The caller could just clear it if it is not needed. (IMO, save_info_dict could be even deprecated at all.)

@glassez glassez marked this pull request as ready for review February 15, 2025 14:38
@glassez
Copy link
Contributor Author

glassez commented Feb 15, 2025

I don't think you need the static empty add_torrent_params object, since you're returning it by value. (had the return value been by reference on the other hand, you would have needed it).

Ok, changed.

It would be nice to expose it to python

I don't know anything about it at all. At least where is it?

add a test to exercise it

I could add something similar to save_resume_data(), but I found a lot of occurrences of it in the test sources. I doubt what exactly I should do about it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@glassez @arvidn