Provide v1 and v2 infohashes in UI

[glassez]

No description provided.

[Chocobo1]

I'll look at the UI later.

[thalieht]

I didn't test v2 torrents (no access to libt v2) and i didn't test the %I %J %K parameters. Also the Copy sub-menu was not updated in WebUI.

[FranciscoPombal]

@glassez

Tested this on Windows 10 just now.

• General feedback: the General tab should either show a Torrent ID field, or at least show the first 40 characters of the Info Hash v2 field in a slightly different color than the latter 24 characters - because the font is not monospaced, it can be difficult to count exactly 40 characters when looking at the Info Hash v2 to get the torrent ID. Or maybe even consider separating the last 24 characters with some small spacing.

I tested with some of libtorrent's test torrents. Some things worked as expected, but I found a ton of bugs:

• https://github.com/arvidn/libtorrent/blob/RC_2_0/test/test_torrents/v2.torrent seems to work, but there are no peers to download it from, so the download can't be completed.
• https://github.com/arvidn/libtorrent/blob/RC_2_0/test/test_torrents/v2_only.torrent: gets added, but does not show in the transfer list. It is still accounted for in the sidebar though.
• https://github.com/arvidn/libtorrent/blob/RC_2_0/test/test_torrents/v2_invalid_filename.torrent gets added without error, but again does not show up in the transfer list. It probably should have raised an error though.
• https://github.com/arvidn/libtorrent/blob/RC_2_0/test/test_torrents/v2_invalid_root_hash.torrent does not get added and does not raise an error message dialog, but a warning is printed silently to the execution log. Additionally, if it is added a 2nd time, a dialog says it is "already queued for processing".
• https://github.com/arvidn/libtorrent/blob/RC_2_0/test/test_torrents/v2_hybrid.torrent
  • gets added, shows up in the transfer list and downloads successfully (there are actual peers seeding it).
  • In the torrent addition dialog, the infohash shown is the torrent ID, not the v1 or v2 hashes. The torrent addition dialog should show the v1, v2 and torrent ID fields for such torrents.
  • The .pad directory is selected for download by default. The download can be completed successfully with it deselected. However, regardless of whether this is deselected or not before starting the download, qBittorrent will hang, pegging 1 CPU core, after starting a force recheck. In either case, the Total Size and Size columns in the transfer list differ by the size of the .pad directory. This might be a bug in libtorrent, not sure.
• https://github.com/arvidn/libtorrent/blob/RC_2_0/test/test_torrents/v2_symlinks.torrent crashes qBittorrent.

The remaining v2_* test torrents I did not mention explicitly in this list either error out with a modal dialog as expected, or trigger one of the mentioned bugs (albeit probably for different reasons, of course).

I did not test the "run external script" changes at all.

Copying the ID, hashes, and magnet link from the context menu works fine. The v2 hash and torrent ID options should probably be greyed out for v1-only torrents though, as should the v1 hash option for v2-only.

[FranciscoPombal]

@glassez Note: my comment concerns the previous push (014d37f), not the most recent commit which came in as I was writing the comment: f9c562e.

[glassez]

General remark
The Torrent ID is the internal torrent ID in qBittorrent. Although it matches the torrent info hash in some cases (this is done for ease of implementation), but not in general. There is no such thing in BitTorrent protocol. I purposely put it into a separate entity in order to abstract from the changes in the infohashes (made, and future). The user should not identify it with the info hash in the general case.

Note, BitTorrent v2 is generally a draft, and we can not yet be sure what it will be in the end. Now there is a complete mess caused by an attempt to have some compatibility with existing trackers, etc., which is why we have to deal with the so-called reduced sha-256 info hashes of v2 torrents, which are used as a compatible info hash. I hope this will change by the final version of the protocol and trackers will be required to understand native info hashes.

So if, given the above, you want to see the Torrent ID in General tab, then I will add it.

[glassez]

I tested with some of libtorrent's test torrents. Some things worked as expected, but I found a ton of bugs:

Please create separate Issue for it. We really need to handle such cases before release qBittorrent based on libtorrent-v2. Dunno can we produce first beta release in current state.

[FranciscoPombal]

@glassez

General remark
The Torrent ID is the internal torrent ID in qBittorrent. Although it matches the torrent info hash in some cases (this is done for ease of implementation), but not in general. There is no such thing in BitTorrent protocol. I purposely put it into a separate entity in order to abstract from the changes in the infohashes (made, and future). The user should not identify it with the info hash in the general case.

The "torrent ID" is not just some internal thing in qBittorrent. It is the 20-byte truncated v2 hash for v2-only and v2-byvrid torrents, and the normal 20-byte info hash for v1-only torrents. It stands on its own as a separate entity, which the protocol uses in some cases (https://www.bittorrent.org/beps/bep_0052.html):

For some uses as torrent identifier it [the 256-bit v2 info hash] is truncated to 20 bytes.

It only matches the info hash in case of v1-only torrents.

Note, BitTorrent v2 is generally a draft, and we can not yet be sure what it will be in the end. Now there is a complete mess caused by an attempt to have some compatibility with existing trackers, etc., which is why we have to deal with the so-called reduced sha-256 info hashes of v2 torrents, which are used as a compatible info hash. I hope this will change by the final version of the protocol and trackers will be required to understand native info hashes.

I think BitTorrent v2 is pretty much final and we should operate under that assumption. It says "draft" in the BEP, but actually almost no BEP has been formally finalized (or even exited the "draft" stage), even though they have been in very widespread use for years: https://www.bittorrent.org/beps/bep_0000.html.

I could be wrong though, and maybe there are still some significant changes planned. Let's see what @arvidn thinks about this.

As far as I understand it from the spec, it was decided that for both v2-only torrents and v2 hybrid torrents, the contents of the info_hash parameter in the peer protocol, tracker protocol, and DHT message protocol would be the 20-byte truncated v2 info hash, so that the length would be the same as for info hashes of standard v1 torrents. This means that in BitTorrent V2, the contents of the info_hash parameter is really the "torrent ID". This was done in the name of backwards compatibility, time will tell if it was a wise decision.

So if, given the above, you want to see the Torrent ID in General tab, then I will add it.

I would like an easy way to see it, but I don't think we should have a separate field for it, because it would be too redundant. After all, in the case of v1-only torrents it is the same as the v1 info hash, and in the case of v2-only or hybrid torrents it is the first 40 characters of the v2 info hash.

That's why I suggested adding a small space between the first 40 characters and the last 24 in the v2 info hash field.

Something like this:

Info hash V1: 27cddc614137b2e88cca39d940678de70fec042e
Info hash V2: 27cddc614137b2e88cca39d940678de70fec042e deadbeefdeadbeefbeadbeef

Or maybe make the last 24 characters a slightly different color (e.g. if normal text is black, these last characters could be grey instead).

[glassez]

As far as I understand it from the spec, it was decided that for both v2-only torrents and v2 hybrid torrents, the contents of the info_hash parameter in the peer protocol, tracker protocol, and DHT message protocol would be the 20-byte truncated v2 info hash, so that the length would be the same as for info hashes of standard v1 torrents.

If sha256 won't be used to identify torrents, then what's the point in it? After all, trackers will be able to take into account the same number of torrents as in the first BitTorrent version (in fact, even less, because some of the identifiers are occupied by real sha1 hashes of torrents of the first version). What is the benefit of calculating a more complex hash sum if we use its truncated value only? We only expend more energy and create confusion.
@arvidn, can you clarify the trends in changing the BitTorrent protocol?

[glassez]

There is indeed such a concept in the protocol. From the spec:

info_hash (...) For some uses as torrent identifier it is truncated to 20 bytes. (...)

In other words (taking into account the context of the whole BEP): "each torrent has a torrent identifier, 20 bytes in length.

I still am not sure that this is some kind of stable concept of the protocol, and not some temporary phenomenon. I would prefer to treat qBittorrent Torrent ID as an internal identifier after all, so that as few things as possible depend on its real value, and we would not have to change a bunch of code again when this changes in the protocol. But if you (and not just you) insist that it has a direct correspondence with what we have in the current draft of the protocol, well, please, let's show it in the UI as Torrent ID.

[FranciscoPombal]

@glassez

To be honest, I did not want to take on these changes, because there is a lot of uncertainty and subjectivity here. I fear that it will become completely mired in controversy.

I don't see why, I think the spec is quite clear. But I understand that you don't think it is as finalized as we'd like it to be.

So let's do this. I ask everyone who wants to speak on the following points, but not just how you imagine it, but only if you can describe a plausible use case (why it may actually be needed and how would you use it):

These questions are somewhat confusing since you make a distinction between the "reduced sha256" and the "Torrent ID". I will assume the point of view of the BEP when answering, as in #15097 (comment).

1. Info hashes (including reduced sha256) and Torrent ID in General tab. Which of them and how should be displayed for a particular version of the torrent (including hybrid one) and why do you really need each of them.

From our discussion in the previous comments, I think this is fine as it is in this PR: For v1 torrents, v2 hash displays as N/A and vice-versa. For hybrid torrents, both hashes are shown. If you did not have aesthetic objections to it, I suggested using a monospaced font for displaying the infohashes, so that a user can get the 20-byte torrent identifier from v2 or hybrid torrents at a glance. But I won't insist with this, since the user can easily copy the identifier from the context menu.

Use case: users often want to easily check info hashes/identifiers to compare against other resources (e.g. a hash on a webpage).

2. Info hashes (including reduced sha256) and Torrent ID in Copy menu. What options should we provide in case of single/multiple torrents of same version (including hybrid ones) and in case of multiple torrents of different versions (including hybrid ones) and how would you use each of proposed options.

I'm not sure what you mean by "What options should we provide in case of single/multiple torrents of same version". The only change I would make to this PR regarding the copying from the context menu is the one mentioned in my review: #15097 (comment)

3. Info hashes (including reduced sha256) and Torrent ID as parameters in "Run external program" feature. Note that this is the simplest question, because we are required to substitute the values for all the specified parameters, so we just need to substitute some placeholder for the unavailable one.

No comment from me on this one.

4. Info hashes (including reduced sha256) and Torrent ID in Add torrent dialog. Personally, I don't see the point of seeing neither the reduced sha256 hash (only full) nor Torrent ID (which in general doesn't exist yet, since the torrent hasn't been added yet) here.

Following the same logic as for my answer to 1.: For v1 torrents, v2 hash displays as N/A and vice-versa. For hybrid torrents, both hashes are shown.

[glassez]

@FranciscoPombal
Ok, let us follow "torrent identifier" from BEP (although I fear that this could cause us problems if it changes in the near future).
Then:

For v1 torrents, v2 hash displays as N/A and vice-versa. For hybrid torrents, both hashes are shown. [...] the user can easily copy the identifier from the context menu.

👍

Info hashes (including reduced sha256) and Torrent ID as parameters in "Run external program" feature. Note that this is the simplest question, because we are required to substitute the values for all the specified parameters, so we just need to substitute some placeholder for the unavailable one.

👍
We'll just provide 3 params like it's currently implemented in this PR.
Although I would preserve previously existed one for Torrent ID instead of v1 infohash since existing scripts may rely on it for each torrent.

Following the same logic as for my answer to 1.: For v1 torrents, v2 hash displays as N/A and vice-versa. For hybrid torrents, both hashes are shown.

👍
Assuming we don't have to provide Torrent ID in Add torrent dialog. Or we have to do?

I'm not sure what you mean by "What options should we provide in case of single/multiple torrents of same version". The only change I would make to this PR regarding the copying from the context menu is the one mentioned in my review: #15097 (comment)

What you suggest is the simplest in essence. But is this what the user expects? You didn't give an example of how you would use it.
Let's say torrents of the same version are selected. Then it is not a problem to show only one menu item (either "Copy infohash v1" or "Copy infohash v2"), or both for hybrid torrents.
But if selected torrents of different versions and user click, for e.g., "Copy infohash v1", is it Ok that count of copied hashes will not match the count of selected torrent? Damn, I just can't imagine why such a hash list can be used...
But maybe just make it as you suggest and wait for real feedback?

[glassez]

PR is updated. Please review it again. I think we should start with it and wait feedback from users.
One question I want to hear answer to is about Run external program parameter %I:

Although I would preserve previously existed one for Torrent ID instead of v1 infohash since existing scripts may rely on it for each torrent.

[thalieht]

Copy Infohash v1/v2 isn't hidden when unavailable in WebUI like in the GUI.

/off topic
Hiding things in menus instead of disabling them, hurts prevents muscle memory of mouse movement to an action because actions can move around. Just bringing it to your attention in case you guys deem it worthy of consideration.

[glassez]

Copy Infohash v1/v2 isn't hidden when unavailable in WebUI like in the GUI.

Apparently, my nature does not consider it an integral part of the application (but only some external application), so I never care about it... I would prefer never deal with it.
Okay, I'll try to do something with it.

Hiding things in menus instead of disabling them, hurts prevents muscle memory of mouse movement to an action because actions can move around.

It definitely makes sense 👍

[glassez]

Hiding things in menus instead of disabling them, hurts prevents muscle memory of mouse movement to an action because actions can move around.

It definitely makes sense 👍

Done!

Apparently, my nature does not consider it an integral part of the application (but only some external application), so I never care about it... I would prefer never deal with it.
Okay, I'll try to do something with it.

Honestly, nothing simple came to my mind. I'm not even sure that it behaves in the same way in the Web UI. Well, I'll leave that to those who know more about it.

[glassez]

@Chocobo1
WebUI CI / Check WebUI seems to want me to add an extra indentation inside switch statement. Is this correct? Note that this is different from the style we use in C++.

[Chocobo1]

WebUI CI / Check WebUI seems to want me to add an extra indentation inside switch statement. Is this correct?

Yes.

Note that this is different from the style we use in C++.

IMO better start with something than leaving it unorganized.

[glassez]

Note that this is different from the style we use in C++.

IMO better start with something than leaving it unorganized.

I agree.
I mentioned it just in case it wasn't intentional, so you can fix it later.

Addressed

[FranciscoPombal]

@glassez

Hey, I thought I had posted this piece of feedback before, so here goes. It's nothing that would have affected the merge anyway, so don't worry about that.

+1
We'll just provide 3 params like it's currently implemented in this PR.
Although I would preserve previously existed one for Torrent ID instead of v1 infohash since existing scripts may rely on it for each torrent.

👍 I agree with preserving the existing parameter for the identifier instead of the V1 hash. This is exactly the same logic as the BEP - the info_hash parameter in the tracker announces and etc contains the ID (which, for the case of V1 torrents, happens to coincide with the full SHA-1 hash).

+1
Assuming we don't have to provide Torrent ID in Add torrent dialog. Or we have to do?

I don't think we do. We just need to show exactly the same thing in both the General tab and the add torrent dialog. If people request the ID to be shown in the dialog later we can add it later.

What you suggest is the simplest in essence. But is this what the user expects? You didn't give an example of how you would use it.
Let's say torrents of the same version are selected. Then it is not a problem to show only one menu item (either "Copy infohash v1" or "Copy infohash v2"), or both for hybrid torrents.
But if selected torrents of different versions and user click, for e.g., "Copy infohash v1", is it Ok that count of copied hashes will not match the count of selected torrent? Damn, I just can't imagine why such a hash list can be used...
But maybe just make it as you suggest and wait for real feedback?

Ah, I see. I did not know you could copy such a list from several torrents. Then in the case of multiple torrents, I think it only makes sense to copy the ID (the other options should be greyed out).
I agree that the resulting list of hashes should have length equal to the number of torrents the user selected.
Otherwise, it would be quite unintuitive.