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

Proposal: Opening/Closing Mechanism for Zip Files #1413

Draft
wants to merge 35 commits into
base: master
Choose a base branch
from

Conversation

Michael5601
Copy link
Contributor

Proposal: Opening/Closing Mechanism for Zip Files

Background

The Eclipse IDE has no built in functionality to open Zip Files and read or manipulate their content. Because of this, other operations like searching inside of Zip Files or comparing two Zip Files were also not possible.

Goal:

This pull request introduces a mechanism for handling Zip Files within the Eclipse workspace, enhancing the functionality to read and write Zip files. The primary goal is to provide a seamless experience for developers working with zip archives directly within Eclipse.

Description:

Zip files must be opened manually within the workspace by using the new command "Open Zip File" in the menu when right clicking the zip file. It is also possible to open nested zip files.

Zip Files are opened by replacing the file in the workspace with a linked folder that reads and writes the Zip File in the file system. By closing the Zip FIle, the linked folder will be deleted and the file can be seen in the workspace again.

Please note that only ZIP Archives are supported in this current implementation. Other archive types can be added in future improvements. Also linked Zip Files can not be opened with this implementation because the Zip File must be local.

An additional PR for the repository eclipse.platform.ui that grants access to the open/close mechanism for zip files over UI can be found in the following:

eclipse-platform/eclipse.platform.ui#1947

Co-Authored-By: CodeLtDave [email protected]

@laeubi
Copy link
Contributor

laeubi commented Jun 10, 2024

Given there is a Zip Filesystem for local files in the JDK already that support read/write I wonder if one really needs the "Open Zip FIle" step? Also the examples contain such a ZipFile system for eclipse already.

So I wonder if you maybe can explain why this design decision was made with linking / extraction of the Zip?

Copy link
Contributor

github-actions bot commented Jun 10, 2024

Test Results

0 files   -   586  0 suites   - 586   0s ⏱️ - 29m 48s
0 tests  - 3 980  0 ✅  - 3 934  0 💤  - 46  0 ❌ ±0 
0 runs   - 4 179  0 ✅  - 4 132  0 💤  - 47  0 ❌ ±0 

Results for commit ba2cef4. ± Comparison against base commit 0b69e82.

♻️ This comment has been updated with latest results.

Michael5601 pushed a commit to CodeLtDave/eclipse.platform that referenced this pull request Jun 10, 2024
The Eclipse IDE has no built in functionality to open Zip Files and read or manipulate their content. Because of this, other operations like searching inside of Zip Files or comparing two Zip Files were also not possible.

This pull request introduces a mechanism for handling Zip Files within the Eclipse workspace, enhancing the functionality to read and write Zip files. The primary goal is to provide a seamless experience for developers working with zip archives directly within Eclipse.

Zip files must be opened manually within the workspace by using the new command "Open Zip File" in the menu when right clicking the zip file. It is also possible to open nested zip files.

Zip Files are opened by replacing the file in the workspace with a linked folder that reads and writes the Zip File in the file system. By closing the Zip FIle, the linked folder will be deleted and the file can be seen in the workspace again.

Please note that only ZIP Archives are supported in this current implementation. Other archive types can be added in future improvements. Also linked Zip Files can not be opened with this implementation because the Zip File must be local.

An additional PR for the repository **eclipse.platform.ui** that grants access to the open/close mechanism for zip files over UI can be found in the following:

https: //github.com/eclipse-platform/pull/1413
Co-Authored-By: David <[email protected]>
@Michael5601 Michael5601 force-pushed the ZipFileImplementation branch 2 times, most recently from 7d05bd1 to ff816b0 Compare June 10, 2024 13:19
@mickaelistria
Copy link
Contributor

I think before diving in the code, @laeubi 's comment at #1413 (comment) is most important to answer. IMO, if we're to include a Zip filesystem in Platform, it'd better be the one that is already in the example. UI also could be built on top of this filesystem in example. One benefit is that we could make progress with UI without being blocked by Platform.

@laeubi
Copy link
Contributor

laeubi commented Jun 10, 2024

One benefit is that we could make progress with UI without being blocked by Platform.

Honestly I think we don't need any new UI but it seems I have missed the discussion about the requirements for this change, but take as an example the Classpath Container, if there is a jar file (what actually is a zip) it simply has a handle so I can expand it and it shows the content and I can double click files and open editors and so on, I would expect something similar for "something" that support open zip files without any need to open/close/extract/... UI because that's the whole point of a file-system abstraction that you can have "virtual" items pointing to something that is not a regular file.

Copy link
Member

@iloveeclipse iloveeclipse left a comment

Choose a reason for hiding this comment

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

Co-Authored-By: CodeLtDave [email protected]

Here and on other PR: ALL contributors must sign ECA.

If [email protected] has not signed ECA, we can't merge this PR.

@laeubi
Copy link
Contributor

laeubi commented Jun 10, 2024

eclipsefdn/eca succeeds so I don't see an immediate issue here:
grafik

@iloveeclipse
Copy link
Member

eclipsefdn/eca succeeds so I don't see an immediate issue here:

OK. Haven't noticed the check was done for the other person as the PR owner.
Looks weird however to see only one author checked. What about @Michael5601 ECA agreement then?

@mickaelistria
Copy link
Contributor

Can someone (a project lead?) gives some opinions about the fact that we already have code for it in the codebase? IMO, this particular PR is not necessarily and we should only discuss whether we want to include the existing zip filesystem implementation in the main feature or not.

@laeubi
Copy link
Contributor

laeubi commented Jun 10, 2024

Looks weird however to see only one author checked. What about @Michael5601 ECA agreement then?

I don't know the details @waynebeaton maybe can tell or we need a servicedesk ticket to clarify what / when is checked...

@CodeLtDave
Copy link
Contributor

CodeLtDave commented Jun 10, 2024

Given there is a Zip Filesystem for local files in the JDK already that support read/write I wonder if one really needs the "Open Zip FIle" step? Also the examples contain such a ZipFile system for eclipse already.

I appreciate everyone's interest in our project. A frequent topic of discussion has been the existing zip example provided in Eclipse. I want to clarify that our pull request builds upon this example, adding more functionality and integrating it into the main feature.

As @mickaelistria mentioned, the key question is whether this functionality should be part of the main feature, and I firmly believe it should be.

Regarding the nio Zip Filesystem available in the JDK, we utilize its capabilities extensively. Our pull request essentially incorporates the NIO Zip Filesystem into the existing example, resulting in significant performance improvements. Most of the logic can be found in the ZipFileStore.java where the nio Zip Filesystem is opened.

@mickaelistria
Copy link
Contributor

I want to clarify that our pull request builds upon this example, adding more functionality and integrating it into the main feature.

OK, great. Thank you for clarifying!
Do you think it would be possible for you to create a dedicated PR for those improvements and additional tests, but keeping the code in the "example" bundle for the moment? Such a PR could be merged in a breeze and be a valuable iteration while we still discuss the best place to host the filesystem in the end.

@laeubi
Copy link
Contributor

laeubi commented Jun 10, 2024

@CodeLtDave can you explain why explicitly Open/Close is required? Can't we reuse the existing functionality (and possibly extend if something is missing here)?

What we already have

To make it more clear I just choose this random project with some java files and it looks like this
grafik

Now I expand the handle and it looks like this:
grafik

So I have a File (.java) and it shows we there is maybe more to explore and after that I see some more items that are "inside" the file...

As a user I would expect same/similar if it was not a java but a Zip-File for browsing the content similar to what I have for example with Plugin-Dependencies:

grafik

What would be new

Now if I open one file in an editor, I would like to be able to save this (and maybe getting asked if I really want to update the zip archive).

Next when I choose the File Search Dialog I would maybe expect some new option to include searching inside archives as well:

grafik

Conclusion

So to summarize I don't see we really need any new Actions for that (load/unload) but should use whats already is offered here. I haven't checked in detail how these functionality is implemented so maybe there are good reasons not to do it like this... but it would better match user expectation I think.

@CodeLtDave
Copy link
Contributor

So I wonder if you maybe can explain why this design decision was made with linking / extraction of the Zip?

Regarding the functionality of the ZipFilesystem, as implemented in the example, we don't extract any files for opening or editing. Opening the Zip is akin to walking down the File Tree and collecting all necessary file information.

When a file is modified, it's done by opening the Zip with NIO and using the respective operations.

Based on our testing, this approach is much more lightweight and performant than always extracting the entire Zip and repackaging it. For small changes, this saves significant time and reduces overhead. We didn't investigate the internal workings of NIO in detail, so it's possible that some extraction and repackaging might be happening in the background. However, since it's much faster than manual extraction and repackaging, we believe this is not the case.

Regarding the approach with a linked resource, this seems to be the best method for linking any virtual FileSystem. Since it was already implemented this way in the example project, we believe it is appropriate and fitting for our use case.

@jukzi
Copy link
Contributor

jukzi commented Jun 10, 2024

I wish ZIP is support from core. Currently i have to go windows file explorer to open zip files. awkward. Searching in zip files would be even better, but that can be a follow up step. At best JAR files are handled as ZIP files as well, because technically they are. The file ending is rather irrelevant.

@laeubi
Copy link
Contributor

laeubi commented Jun 10, 2024

I think then this description is confusing:

Zip Files are opened by replacing the file in the workspace with a linked folder that reads and writes the Zip File in the file system. By closing the Zip FIle, the linked folder will be deleted and the file can be seen in the workspace again.

Why is it a folder then? (see example with java file that is not a folder at all)? As far as I understand the example it is done that way so you can link in any external zip file but here we want to show additional content for a file inside the workbench so this indirection should not be required (see java files and classpath jar files).

@CodeLtDave
Copy link
Contributor

I wish ZIP is support from core. Currently i have to go windows file explorer to open zip files. awkward. Searching in zip files would be even better, but that can be a follow up step. At best JAR files are handled as ZIP files as well, because technically they are. The file ending is rather irrelevant.

@jukzi

The use case for searching in ZIP files is honestly the motivation for this project. Many colleagues, including myself, have often expressed frustration over this limitation. As you mentioned, JAR files can only be differentiated from ZIP files by their structure, specifically the presence of a META-INF/MANIFEST.MF file and possibly a Main-Class attribute.

Given that this is the platform repository, we have manually disabled support for JAR files. We plan to add JAR support, along with all the necessary functionality and tests, in the JDT (Java Development Tools) as suggested by @HeikoKlare.

@CodeLtDave
Copy link
Contributor

I think then this description is confusing:

Zip Files are opened by replacing the file in the workspace with a linked folder that reads and writes the Zip File in the file system. By closing the Zip FIle, the linked folder will be deleted and the file can be seen in the workspace again.

I think you are right, that sounds confusing. Essentially, the zip file is hidden as long as there is a linked folder (or opened Zip) accessing the zip file. When it's closed, the link is deleted and the zip file is visible again.

Why is it a folder then? (see example with java file that is not a folder at all)? As far as I understand the example it is done that way so you can link in any external zip file but here we want to show additional content for a file inside the workbench so this indirection should not be required (see java files and classpath jar files).

I think I misunderstand your question. Every Zip/JAR is a compressed folder and is therefore displayed as such. We didn't change anything about that part from the example.

@CodeLtDave
Copy link
Contributor

I want to clarify that our pull request builds upon this example, adding more functionality and integrating it into the main feature.

OK, great. Thank you for clarifying! Do you think it would be possible for you to create a dedicated PR for those improvements and additional tests, but keeping the code in the "example" bundle for the moment? Such a PR could be merged in a breeze and be a valuable iteration while we still discuss the best place to host the filesystem in the end.

Thank you for the input. I agree that splitting the addition of functionality and moving the example into two separate PRs could be beneficial. I will discuss this approach with my team. Considering the size of this PR and the necessary changes based on your comments, we will need some time to implement this.

@laeubi
Copy link
Contributor

laeubi commented Jun 10, 2024

JAR files can only be differentiated from ZIP files by their structure, specifically the presence of a META-INF/MANIFEST.MF file and possibly a Main-Class attribute

Jar files are Zip files, a manifest is not mandatory nor any special headers:
https://docs.oracle.com/en/java/javase/20/docs/specs/jar/jar.html#introduction

A JAR file is essentially a zip file that contains an optional META-INF directory

So I don't see why one should / would disable anything for them?

We didn't change anything about that part from the example.

The example is for linking a filesystem as a new item into the workbench, here we want to show additional childs for an existing file in the workbench. That's an important part I think and would avoid having to "shadow" an existing file, maybe such special resource must implement both IFolder and IFile as otherwise other parts of the system can no longer "see" the zip/jar as a file what could have some implications.

CodeLtDave and others added 29 commits September 25, 2024 15:24
temporary workaround for CloseTest#testCloseZipFileWithZipFileUnderneath
This change fixes various problems regarding ClosedZipFileSystemException, NoZipFileSystemFoundException and ZipFileSystemAlreadyExistsException
The default value of create is false
All ZIP-based zip files like zip, jar and war can be put on the classpath of the project. To avoid UI bugs, only zip files can be opened that are not added to the project's classpath. This commit introduces a check for the classpath.
to make sure that clients like JDT do not have a behaviour change or even errors the code for opening zip files is encapsulated in a IWorkspaceRunnable.
This allows opening zip files on the class path.
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.

9 participants