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

[Peek] Support for archives #26839

Merged
merged 5 commits into from
Jun 28, 2023
Merged

Conversation

davidegiacometti
Copy link
Collaborator

@davidegiacometti davidegiacometti commented Jun 14, 2023

Summary of the Pull Request

Support for archives preview in peek: .zip, .rar, .7z, .tar, .nupkg, .jar, .gz, .tar, .tar.gz, .tgz

A month ago I was working on an explorer preview pane for archives (branch).
Chatted with @crutkas about the new preview pane and the WinUI3 startup was a concern.
I am shifting the job into peek based on #26726.
If preview panes will stay in PT we may reconsider adding also the preview pane (it would be nice to share logic with peek if possible).

Still a WIP but early feedbacks will be appreciated ☺️

PR Checklist

Detailed Description of the Pull Request / Additional comments

1 2

Validation Steps Performed

@davidegiacometti davidegiacometti marked this pull request as draft June 14, 2023 10:17
@github-actions

This comment has been minimized.

@htcfreek
Copy link
Collaborator

htcfreek commented Jun 14, 2023

If possible, supporting iso, cab would be amazing.

And we should have scroll bars in both directions on small window size. - Not sure if this is by default.

@htcfreek
Copy link
Collaborator

@davidegiacometti
Can you please check the open issues to reference an existing one or create a new one.

@davidegiacometti
Copy link
Collaborator Author

@htcfreek
I have created #26855
I have added horizontal scrolling.
Unfortunately the library doesn't support .cab and .iso. This could be a future enhancement if we find a way to enumerate the content of these formats (a library would be nice).

@crutkas
Copy link
Member

crutkas commented Jun 19, 2023

🔥🔥🔥

@htcfreek
Copy link
Collaborator

htcfreek commented Jun 19, 2023

@davidegiacometti
There is a long time outstanding issue for .iso format on the SharpCompress repository. (adamhathcock/sharpcompress/issues/545, adamhathcock/sharpcompress/issues/647)

Iread somewhere that there is a DiskUtils library that supports .iso. But I don't know if we can use it in this case and I am not sure if it is actively maintained.

For .cab I created an issue in the SharpCompress repository. (adamhathcock/sharpcompress/issues/747)

@davidegiacometti davidegiacometti force-pushed the users/davidegiacometti/peek-archives branch from 90f2fd3 to 5e8670f Compare June 19, 2023 09:10
@davidegiacometti
Copy link
Collaborator Author

I am having hard times to investigate an issue with the TreeView that is leaking memory.
The issue may be related to WinUI but I can't fully understand the lifecycle of the IPreviewer: some of them implement IDisposable but are always reassigned and never disposed.

Previewer = previewerFactory.Create(Item);

It would be nice if the someone of the core team could help.

@SamChaps
Copy link
Contributor

This looks awesome! Could be consider supporting previewing files from within that ArchiveControl?

@davidegiacometti
Copy link
Collaborator Author

davidegiacometti commented Jun 22, 2023

This looks awesome! Could be consider supporting previewing files from within that ArchiveControl?

Thank you!

Do you mean to move the code that open the archive, create the treeview, sizes and counters inside the user control?
Could make sense but based on the other implemented IPreviewer this job should be done in the LoadPreviewAsync method.
What do you think?

@davidegiacometti davidegiacometti force-pushed the users/davidegiacometti/peek-archives branch from 5e8670f to 9a88f4d Compare June 23, 2023 16:33
@davidegiacometti davidegiacometti marked this pull request as ready for review June 23, 2023 16:37
@SamChaps
Copy link
Contributor

@davidegiacometti no, what I mean is to be able to Ctrl+Space within that control and allow preview of each single archived file.

@davidegiacometti
Copy link
Collaborator Author

Oh! My bad 😅I totally misunderstood your feedback 😄
I think this will require to extract the file from the archive and somehow switch between previewers. I will create an issue to track this.

Copy link
Collaborator

@stefansjfw stefansjfw left a comment

Choose a reason for hiding this comment

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

The code looks good to me. It works flawlessly as well. Very nice! I see the mentioned memory leak, as far as I understood this is probably TreeView issue. Something to track for sure. It would be good if @SeraphimaZykova and @SamChaps could take another look before merging

@SeraphimaZykova
Copy link
Collaborator

I've noticed that there is no NativeMethods.DeleteObject after getting icons. Take a look at ImagePreviewer (Clear method), I've fixed the mem leak that way there. Perhaps it could help.

@SeraphimaZykova
Copy link
Collaborator

Overall, nice work! :)

@stefansjfw
Copy link
Collaborator

I've noticed that there is no NativeMethods.DeleteObject after getting icons. Take a look at ImagePreviewer (Clear method), I've fixed the mem leak that way there. Perhaps it could help.

There is DestroyIcon call in GetBitmapFromHIconAsync

@crutkas
Copy link
Member

crutkas commented Jun 27, 2023

This ready for .71?

@stefansjfw
Copy link
Collaborator

This ready for .71?

yes, almost there. Just a bit of some more reviewing left

@davidegiacometti
Copy link
Collaborator Author

Yeah, the DestroyIcon is called inside the helper to align the behavior with the existing method.

I have pushed a minor cleanup that removes a redundant helper for file size since Peek already had one b112b71 😇

@davidegiacometti
Copy link
Collaborator Author

Created #27084

@davidegiacometti davidegiacometti deleted the users/davidegiacometti/peek-archives branch June 28, 2023 12:34
@htcfreek
Copy link
Collaborator

Created #27084

Canyou please dedup all the existing issues. I think it makes sense to do that.

@davidegiacometti
Copy link
Collaborator Author

@htcfreek what do you mean? Which issues?

@htcfreek
Copy link
Collaborator

@htcfreek what do you mean? Which issues?

I have in mind that there are multiple bug reports that peek can't preview files in archives.

@davidegiacometti
Copy link
Collaborator Author

@htcfreek maybe I am still misunderstanding but I think these are two things 🤯

  • These bugs refer to Peek not able to preview file from an archived opened in Explorer.exe
  • [Peek] Preview Files in Archives #27084 and @SamChaps feedback refer to be able to preview a file contained in the archive that is being previewed in Peek

@htcfreek
Copy link
Collaborator

@davidegiacometti
Yep. These are two things.🤯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants