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

Browse zip archives & other things #5885

Merged
merged 15 commits into from
Aug 22, 2021
Merged

Browse zip archives & other things #5885

merged 15 commits into from
Aug 22, 2021

Conversation

gave92
Copy link
Member

@gave92 gave92 commented Aug 21, 2021

Resolved / Related Issues
Items resolved / related issues by this PR.

Details of Changes
Add details of changes here.
Replaced most of references to StorageFile and StorageFolder with new base classes that implements IStorageItem (BaseStorageFile[Folder]). The standard StorageFile[Folder], the FtpStorageFile[Folder] and the new ZipStorageFile[Folder] all derive from these base classes. This enables the following features "for free".

Features enabled by this PR

  • Browse zip archives as folders
  • Add files to zip by drag & drop
  • Delete from zip
  • Open FTP files with double click
  • Search and navigation bar suggestions in FTP folders
  • Copy folders to/from FTP
  • Preview files in zip and ftp folders

Comments, testing and contributions are welcome xD

Validation
How did you test these changes?

  • Built and ran the app

@gave92 gave92 force-pushed the zip_browse_pub branch 2 times, most recently from c5aa87c to 2f6c35e Compare August 21, 2021 14:24
- Add files to zip by drag & drop
- Delete from zip
- Open FTP files with double click
- Search and navigation bar suggestions in FTP folders
- Copy folders to/from FTP
- Preview files in zip and ftp folders
- Simplified FilesystemOperations
@gave92 gave92 marked this pull request as ready for review August 21, 2021 18:26
Enable pin to favotites in FTP
Enable create directory in FTP
@winston-de
Copy link
Contributor

@gave92 do you mind if I implement making files a handler for zip files to the pr (so that it can be set as the default app for archives)?

@gave92
Copy link
Member Author

gave92 commented Aug 22, 2021

@gave92 do you mind if I implement making files a handler for zip files to the pr (so that it can be set as the default app for archives)?

That's a great idea, please do 👍

@winston-de
Copy link
Contributor

Love it, just a couple of minor things I noticed:

  • dragging a file from a zip to another directory moves it, should it copy instead?
  • changes to zip files don't seem to be detected, so operations like delete don't show until refreshing

@gave92
Copy link
Member Author

gave92 commented Aug 22, 2021

add zip file type assosciation

Thank you!

  • dragging a file from a zip to another directory moves it, should it copy instead?

True, the default action should be copy. There's also another issue for which dragging a file into a zip archive defaults to move (which fails), default should be copy here as well.

  • changes to zip files don't seem to be detected, so operations like delete don't show until refreshing

True, it's the same for FTP, I don't think I'm adding either here

@d2dyno1
Copy link
Member

d2dyno1 commented Aug 22, 2021

Wow! Excellent job @gave92!

@winston-de
Copy link
Contributor

winston-de commented Aug 22, 2021

I implemented making copy the default operation when dragging a file from a zip over a tab by using the same function for dropping into the layout blank space (also reduces code duplication), is that good with you?

also, aside from the merge conflicts, lgtm

@gave92
Copy link
Member Author

gave92 commented Aug 22, 2021

I implemented making copy the default operation

Thanks! Edit: should be ready.

@winston-de
Copy link
Contributor

Amazing

@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed needs - code review labels Aug 22, 2021
@yaira2 yaira2 merged commit db2aa56 into main Aug 22, 2021
@yaira2 yaira2 deleted the zip_browse_pub branch August 22, 2021 23:52
using var ftpClient = new FtpClient();
ftpClient.Host = FtpHelpers.GetFtpHost(Path);
ftpClient.Port = FtpHelpers.GetFtpPort(Path);
ftpClient.Credentials = FtpManager.Credentials.Get(ftpClient.Host, FtpManager.Anonymous);
Copy link
Member

Choose a reason for hiding this comment

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

I think this should also be distinguished by port.

@gave92 gave92 mentioned this pull request Aug 23, 2021
5 tasks
item.ItemFile = await StorageFile.GetFileFromPathAsync(item.ItemPath);
var text = await FileIO.ReadTextAsync(item.ItemFile);
item.ItemFile = await StorageFileExtensions.DangerousGetFileFromPathAsync(item.ItemPath);
var text = await ReadFileAsText(item.ItemFile); // await FileIO.ReadTextAsync(item.ItemFile);
Copy link
Contributor

Choose a reason for hiding this comment

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

What advantage does this have over FileIO.ReadTextAsync? It seems to break TryLoadAsText as it doesn't throw when a file is not a text file and the resulting string always contains "\0\0\0\0".

Copy link
Member

Choose a reason for hiding this comment

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

Also while at it, StorageFileExtensions.DangerousGetFileFromPathAsync could be replaced with safer StorageItemHelpers.ToStorageItem<StorageFile>(item.Path);

Copy link
Member Author

Choose a reason for hiding this comment

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

@winston-de FileIO.ReadTextAsync did not work for some reason on FTP but if it causes issues with normal previews and can't be easily fixed we can live without previews on FTP paths.
@d2dyno1 good point. (Note: it's StorageItemHelpers.ToStorageItem<BaseStorageFile>(...) now)

Copy link
Member Author

@gave92 gave92 Sep 7, 2021

Choose a reason for hiding this comment

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

Ah well just seen why ReadFileAsText always contains "\0\0\0\0", not sure if that's enough to fix all issues but going to fix it => fixed here f77064c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Pull requests that are approved and ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants