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

Added torrent file trees to the download endpoint #7705

Merged
merged 1 commit into from
Nov 27, 2023

Conversation

qstokkink
Copy link
Contributor

@qstokkink qstokkink commented Nov 21, 2023

Fixes #5399 but not beautifully1,2,3 (because this PR is becoming too big).

This PR includes the REST API integrations necessary for paginated views of files in torrents and "infinite" scrolling of the file list. Additionally, a spinner (gui/images/spinner.gif) is displayed while loading the file list. Screenshot:

screenshot

1One of the big remaining beautification steps afterward is partial selection support for folders. Right now, the checkboxes for folders only function as "select all"/"deselect all" triggers. I'll open a separate issue for this afterward.
2Another nice feature would be to use our tree arrow icons/images instead of unicode arrows.
3Resizes of the download details widget cause all visible information in the files list to be refreshed, bombing the core with requests. This is inelegant, see #7705 (comment).

@qstokkink qstokkink force-pushed the add_dlep_pagination branch 7 times, most recently from 9af9c7e to d313894 Compare November 24, 2023 13:03
@qstokkink qstokkink marked this pull request as ready for review November 24, 2023 13:17
@qstokkink qstokkink requested review from a team and drew2a and removed request for a team November 24, 2023 13:17
Comment on lines +485 to +486
if not selected_files:
selected_files = range(total_files)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dislike that our logic is that if "no files are selected to apply the given priority to" then "the given priority is applied to all files". However, I didn't want to change our current functionality in this PR (perhaps in some future PR).

Comment on lines +182 to +192
return [
{
"index": download.get_file_index(path),
"name": str(PurePosixPath(path_str)),
"size": download.get_file_length(path),
"included": download.is_file_selected(path),
"progress": download.get_file_completion(path)
}
for path_str in download.tdef.torrent_file_tree.view(view_start, view_size)
if (path := Path(path_str))
]
Copy link
Contributor Author

@qstokkink qstokkink Nov 24, 2023

Choose a reason for hiding this comment

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

This is the main connection between the core and the GUI (see #7705 (comment) for the GUI handler code).

@@ -341,7 +346,7 @@ def view(self, start_path: tuple[Directory, Path] | Path, number: int) -> list[s

# This is a collapsed directory, it has no elements.
if fetch_directory.collapsed:
return []
return self._view_up_after_files(number, fetch_path)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a bug in the previous implementation: a unit test has been added.

return query != other_query


class PreformattedTorrentFileTreeWidget(QTableWidget):
Copy link
Contributor Author

@qstokkink qstokkink Nov 24, 2023

Choose a reason for hiding this comment

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

This is the main GUI widget to render core responses (see #7705 (comment) for the core response format).

Comment on lines +742 to +744
for page_index in (range(self.view_start_index, self.view_start_index + last_visible_row - first_visible_row)
or [self.view_start_index]):
self.fetch_page(page_index)
Copy link
Contributor Author

@qstokkink qstokkink Nov 24, 2023

Choose a reason for hiding this comment

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

This is inelegant. It would be nicer to add refresh logic for page ranges that preceed the tail of the pages list, just like the incremental growth after the (current) tail of the pages list. On the plus side, this implementation won't cause "flickering".

@qstokkink qstokkink changed the title WIP: Added torrent file trees to the download endpoint READY: Added torrent file trees to the download endpoint Nov 24, 2023
Set a single file or directory to be selected or not.
"""
tree = self.tdef.torrent_file_tree
prio = 4 if selected else 0
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: the name prio is ambiguous. I would suggest renaming it to full priority.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. There are also some other things I would like to change in download.py. However, I'll save them for a next PR as this one has gotten too big already.

Thanks for the review on this relatively big PR!

@qstokkink qstokkink changed the title READY: Added torrent file trees to the download endpoint Added torrent file trees to the download endpoint Nov 27, 2023
@qstokkink qstokkink merged commit 4e25843 into Tribler:main Nov 27, 2023
@qstokkink qstokkink deleted the add_dlep_pagination branch November 27, 2023 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

[7.5.0] large torrent can't download metadata
2 participants