Emtpy fields in General tab

[Chocobo1]

Please provide the following information

qBittorrent version and Operating System

qbt master branch

If on linux, libtorrent-rasterbar and Qt version

libtorrent RC_1_2 branch

What is the problem

The "creation date" field and comment field under General tab are empty.

What is the expected behavior

Not empty.

Steps to reproduce

1. Load a torrent with non-empty "creation date" field and comment field. Now the fields are displayed correctly.
2. Restart qbt
3. Now the fields are empty.

Extra info(if any)

I suspect this is due to the change in #11104. The 2 fields are not saved to fastresume file and the new loading mechanism doesn't load from .torrent anymore, hence the fields are lost.

Screenshot: #11406 (comment)

[Chocobo1]

Also ping @arvidn.
This is how we load fastresume when using libtorrent 1.2:

qBittorrent/src/base/bittorrent/session.cpp

Lines 1940 to 1945 in 12c127b

	lt::error_code ec;
	p = lt::read_resume_data(fastresumeData, ec);
	// load from .torrent file when fastresume doesn't contain the required `info` dict
	if (!p.ti || !p.ti->is_valid())
	p.ti = torrentInfo.nativeInfo();

AFAIK there is no easy way to put back the "creation date" field and comment field to libtorrent, I hope you don't suggest we need to track these fields ourselves. :(

[glassez]

@Chocobo1, as far as I understand this problem, we need data from both places (1. metadata from fastresume since it contains modified data and 2. original torrent file since it contains some data that isn't included in fastresume), but we can only use one of them, right?
Well, at a first glance we can try to use one of the following solutions:

1. Don't store metadata in fastresume (use previous way) but modify associated .torrent file when some affected data is changed.
2. Store metadata in fastresume (use new way) but load associated .torrent file as well and merge both metadata.

[glassez]

@Chocobo1, @sledgehammer999, shouldn't we fix this issue until v4.2 final release?

[sledgehammer999]

I was intending to open a bug on libtorrent. Quick thoughts:

1. I want to return to our old way of saving torrent/fastresume separately.
2. Propose solutions (not tested at all):
   a) Have a special value saved for each key when it is truly empty (vs non-initialized). eg textual NULL. I don't know the loading function of libtorrent but maybe this bad value is enough to choke it and make not load any eg trackers
   b) Introduce extra keys in the fastresume. eg for the trackers key there should be an extra key has_trackers.

[glassez]

@sledgehammer999, sorry, I'm not sure you (or me?) understand the problem correctly...
The problems is libtorrent doesn't store some changed data as normal fastresume field (like it does for other kind of data) but as part of "info" (i.e. metadata) field. From the other side "info" field doesn't contain all torrent metadata (e.g. comment). Since we can pass to "add_torrent_params" only one "torrent_info" (either loaded from .torrent file or from fastresume "info" field) we lose either one part of data or other one.
@Chocobo1, please correct me if I'm wrong.

I was intending to open a bug on libtorrent.

Of course we can ask @arvidn to fix libtorrent (if he admits it's a problem) to store all changed data consistently (like, e.g., file names: torrent metadata contains original names and fastresume data changed ones). Then we can just use our previous way (load .torrent + .fastresume).
Or we can ask @arvidn to store complete torrent metadata in fastresume so we can use new way (have only one resume file per torrent).
But even if it is fixed there, it will take time, so we will either have to postpone the v4.2 release again or use the old libtorrent version as the main one.

[glassez]

... or try to fix it at qBittorrent level.

[sledgehammer999]

sorry, I'm not sure you (or me?) understand the problem correctly

This became a problem after we switched to saving the info object into the fastresume. A switch to faciliatate a fix to another problem: When user deleted all trackers, the relevant field in the fastresume is empty. But during loading libtorrent thinks that it is uninitialized and loads the trackers from the torrent_info blob (which we provided separately) thus defeating the users's deletion.

[Chocobo1]

Personally I would like libtorrent to handle it. IMO it doesn't make sense that comment (or any other) field becomes empty when following the recommended libtorrent API usage (no need for .torrent file when resuming torrents).

... or try to fix it at qBittorrent level.

The first idea came to my mind is that when loading the initial (first-time encounter) .torrent file, we track/store comment string ourselves into fastresume file (using "qbt-comment" key). This way we can still follow the "no .torrent file" trend (if this trend make sense).

[arvidn]

Or we can ask @arvidn to store complete torrent metadata in fastresume so we can use new way (have only one resume file per torrent).

It's possible to include the info-dict of the .torrent into the fastresume, which is sufficient to restore the torrent in the session. But maybe by "complete" you also mean the fields outside the info dict.

(I haven't read through this whole thread, so I may be missing some important detail).

My understanding is that new way of using add_torrent_params (in libtorrent master) as the resume data solves these problems, is that right?

[sledgehammer999]

@arvidn I haven't looked at master. But here are the 2 separate problems that indirectly link to each other:

1. For years qbittorrent loaded the fastresume and .torrent separately. It was discovered that in RC_1_2, if some fields(eg trackers) in the fastresume are empty then libtorrent can't differentiate between actually empty vs uninitialized. So if a user has deleted all trackers, libtorrent ends up loading the trackers from the .torrent instead of loading nothing.

2. As a workaround to the above we opted to save the info into the fastresume (via relevant flag) and never load it separately. (personally I don't like this method). This method only saves the info object into the fastresume and not other secondary fields like "comment" or "created by". So upon restarting the client only the fastresume is loaded and those fields are empty in the gui.

Personally I would like to see a fix for number 1 in RC_1_2? Maybe introduce add_torrent_params2 struct to keep ABI compatibility and rework the read_resume_data() function to recognize between empty and uninitialized fields?
PS: I imaging the add_torrent_params2 maybe use a std::optional<std::vector> trackers; to differentiate, and internally either save a special value or have extra keys in the fastresume (eg has_trackers).

[glassez]

internally either save a special value or have extra keys in the fastresume

I think it isn't necessary (unless empty list isn't available to be stored in bencoded form). std::optional in add_torrent_params should be enough.

[glassez]

My understanding is that new way of using add_torrent_params (in libtorrent master)

It's different from RC_1_2?

[glassez]

But maybe by "complete" you also mean the fields outside the info dict.

Yes. .torrent file has some other fields which should be restored as well.

[sledgehammer999]

(unless empty list isn't available to be stored in bencoded form)

From my limited knowledge, I don't think you can differentiate between empty/uninitialized in bencoded form.

std::optional in add_torrent_params should be enough.

Are you sure? Won't this break ABI/API backwards compatibility in the RC_1_2 branch? (That's why I said to introduce a new struct).

[glassez]

Are you sure? Won't this break ABI/API backwards compatibility in the RC_1_2 branch?

I meant only that you don't need any pseudo values in fastresume.
You're right about ABI compatibility.

From my limited knowledge, I don't think you can differentiate between empty/uninitialized in bencoded form.

Some list can exist, but be empty, or not exist at all. Isn't that so?

But it seems that we do not need it at all...
If we have optional field in add_torrent_params (and libtorrent correctly handle it) then it always should has value (e.g. either empty or non-empty tracker list) when we create it from fastresume data so it overrides corresponding value from torrent file. When we create add_torrent_params from scratch this field has no value so corresponding value from torrent file is used.

[glassez]

You're right about ABI compatibility.

It seems like the second way (restoring torrent without .torrent file) can be fixed in libtorrent without breaking ABI.

[glassez]

If we have optional field in add_torrent_params (and libtorrent correctly handle it) then it always should has value (e.g. either empty or non-empty tracker list) when we create it from fastresume data so it overrides corresponding value from torrent file.

Even if storing unchanged trackers in fastresume isn't optimal it's not a problem. As I said above it is possible to handle it correctly:

1. Default add_torrent_params has empty (null) optionals so fields from .torrent file are used.
2. When fastresume data has no corresponding values add_torrent_params has empty (null) optionals so fields from .torrent file are still used.
3. When fastresume data has corresponding values (even empty lists/strings) optionals in add_torrent_params has values so they're used instead of fields from .torrent file.

[arvidn]

This is supposed to work in RC_1_2. There is a flag in add_torrent_params::flags. If override_trackers is set, then add_torrent_params::trackers is taken as initialized, whether it's empty or not, and the trackers in the .torrent file are ignored. If the flag is not set, add_torrent_params::trackers are added to the trackers from the .torrent file.

I will review this logic to make sure it works as it's supposed to, especially read_resume_data() and write_resume_data()

[arvidn]

oh, right. But the actual issue isn't the trackers, it's that everything else from the .torrent, not part of the info-dict, is lost when saving resume data.

[arvidn]

it would be easy to store and load "comment", "created by" and "creation date" in the resume data, in a backwards compatible way in RC_1_2. Are there any other fields I should store while I'm at it?

[sledgehammer999]

If override_trackers is set, then add_torrent_params::trackers is taken as initialized, whether it's empty or not, and the trackers in the .torrent file are ignored. If the flag is not set, add_torrent_params::trackers are added to the trackers from the .torrent file.

(I may be mistaken) How are we supposed to know to set or not the override flag, when the user hasn't changed the trackers at all? IIRC in that case no trackers' list is saved in the fastresume and the one provided by the .torrent file should be used.

[arvidn]

(I may be mistaken) How are we supposed to know to set or not the override flag, when the user hasn't changed the trackers at all?

I think this should be set by libtorrent. If I understand correctly, it would be safe for read_resume_data() to always set the override_trackers flag, if the trackers field exists. The only case when the trackers in add_torrent_params is when a torrent is added for the first time, when there is no resume data. In that case the override_trackers flag won't be set.

[arvidn]

@sledgehammer999 I think this would solve the resume data issue in this ticket, and also comment, creation date and created-by. Would you mind giving it a try?

arvidn/libtorrent#4112

[glassez]

Are there any other fields I should store while I'm at it?

@arvidn, if you want to provide convenient/reliable way of restoring torrents using only fastresume data you need to store all the torrent metadata (except maybe the fields that aren't handled by libtorrent at all).

[arvidn]

yeah, I think these were the only missing ones. web seeds and trackers are already saved

[sledgehammer999]

Fixed by arvidn/libtorrent#4112