GuVAnj8Gv3RJ/NeosAccountDownloader

MIME

Closed this issue · 16 comments

GuVAnj8Gv3RJ commented

Many users want files to have extension
Windows will then show them better
This is possible see: https://github.com/GuVAnj8Gv3RJ/NeosAccountDownloader/tree/feat/mime

But has following issues:

stiefeljackal commented

lzma files are the lz4bson files; this may have been the older format that was used before 7zbson. Correction: lz4 is the older format for lz4bson and lzma is the newer format for 7zbson. There is something in BaseX that can convert from lzma to json if I remember correctly. I can take a crack at this and the other checklist item this afternoon (currently 5:27 AM EDT, 9:27 UTC).

stiefeljackal commented

Since some users may want to keep compressed files (‘7zbson’, ‘lz4bson’, ‘lzma’) as compressed instead of uncompressed (‘json’), I will include the option.

Although animations assets (‘animx’) are not common, they are there and have a proper conversion to ‘animj’ (the json format of animx). Although the serialization is not present in the dlls, my mod has this serialization and can be added to the downloader. This will go through a separate pull request.

epicEaston197 commented

The local inventory mod I believe does convert it into a proper Jason file not sure if that would be useful I don't know if it even works how I think it works
https://github.com/art0007i/LocalStorage/

ThomFox commented

Those lzma files are raw lzma streams with a custom header.

The header is made of 5 bytes and is encoded like this:

    (options.pb as u8 * 5 + options.lp as u8) * 9 + options.lc as u8,
    options.dict_size as u8,
    (options.dict_size >> 8) as u8,
    (options.dict_size >> 16) as u8,
    (options.dict_size >> 24) as u8

However it might be better to get the mime-type from CloudX instead of guessing with Mime-Detective. Yet it looks like guessing is still needed, when mime-type is "application/x-compressed" or "application/x-lzma-stream", which might be lz4bson and 7zbson. I can provide decoders for those or you could just pick LZMA.dll and LZ4.dll from a Neos installation.

The lz4 format is a .NET specific stream format that packetized blocks with headers, which consist of three 7bit encoded integers which are flags, uncompressed size, compressed size. Where compressed size only exists for compressed blocks. The flags are 0x1 for compressed and 0x2 for high compression, which are added together, so 0x0 is uncompressed and 0x1 and 0x3 are compressed. If a block is compressed, standard lz4 block compression is used.

stiefeljackal commented

The local inventory mod I believe does convert it into a proper Jason file not sure if that would be useful I don't know if it even works how I think it works https://github.com/art0007i/LocalStorage/

Yep. That is in the DataTreeConverter static class where you can convert the formats into json and back.

stiefeljackal commented

I'm making this comment to let you know that I am still working on this and haven't forgotten. Life had gotten busy after I came back from my trip, but I still have this on my mind and working on it. @ThomFox has been assisting whenever possible.

stiefeljackal commented

Finishing up and verifying that it works. I am hoping to make a pull request tonight or tomorrow.

stiefeljackal commented

@GuVAnj8Gv3RJ I have completed the changes to introduce the mime type into the download as requested. Before I make the pull request, I humbly request that the license of your repository to be changed from MIT to GPLv2. Although I do appreciate your work in building this downloader and I believe you have good intentions; however, I have contributed a lot of work into this change which includes both development and unit tests. I would like to guarantee that credit is given and that both the repository and the modifications made remain free and open. You may review my existing fork to see what changes I have made. At a high level, I have the following changes:

  • Retooled the interfaces so that IAccountDataGather remains solely for gathering information while IAccountDataStore remains storing information (separation of concerns).
  • Implemented methods to fetch the Asset stream and the Asset Metadata.
  • Impelmented methods to save the Asset stream as a file with an extension and to save the Asset Metadata. I chose to save the metadata since I noticed that variant information is missing.
  • Added the ability for the tool to rename the existing downloaded files to have their file extension.
  • Updated MimeDetector to look for file extensions and mime types based on either the given filename, stream, or byte array.
  • Added Mime to File Extension and vice-versa conversion to MimeDetector.
  • Added the missing animx file type to MimeDetector.
  • Placed 7zbson first to make it the priority file type for the lzma mime type.
  • Produced unit tests as both http calls and file system testing will make testing slow. A total of 255 tests are produced; please use Visual Studio's Test Explorer to initiate tests. You will need to run Get.ps1 again to get additional Dlls that CloudXInterace requires; however, these dlls are only needed for the Test library and do not need to be included in the application build.

While working on this, I do have to ask: Why did you make a request to have file extensions? I can understand viewing pictures and videos, but there are files in there that are not viewable (7zbson) and that are proprietary (meshx). It also does not make sense how you did not know what lzma is since that is used to compress 7zbson. Any Neos modder would know what that algorithm is.

GuVAnj8Gv3RJ commented

I took some time to review and think about this one.

  1. I can't allow license changes at the moment. I cannot go into it further. I can setup a contributor system that credits authors but this will have to remain MIT/MIT Compatible at this moment.
  2. This PR would be too big, it's desirable to make PRs as small as possible. For example this Issue is for Mime support but we have other functionality here.
  3. I prefer MSTest for tests. While I appreciate the added tests. This PR would force me into deciding a Test Library ahead of schedule.

A good way to view PRs is to count the number of things the reviewer would have to research in order to merge the PR. To review this I need to:

  • Research License types
  • Research Unit Test Providers
  • Research the Mock and HTTP libraries you have added.
  • Review the actual code changes.

This makes it difficult.

This repository will exist and be open source.

Oh and I want file extensions so people have more trust/enthusiasm that this is backing up stuff. Non-technical users are unable to work with files that don't have extensions.

stiefeljackal commented

Thank you for taking the time to review my response and my changes. I took some time to think over your responses.

  1. I will not press further on the license change for the repository; however, I do request that the mime detection changes that I made to be under the GPLv2 license. Doing this will keep the repository under MIT, but also keep the mime detection changes under GPLv2. My reasoning for this will be explained in (4).
  2. I understand that PRs should be small as possible as it makes reviewing changes easier on the repository owner's end. However, you have requested not only support for mime types, but also to have files with the appropriate extension. I can simply split this request into two (one for mime type detection and the other for saving the stream as a blob on the file system with the file extension) if that will make your review easier. I also did not want to ruin your Dependency Injection design either, so that is why you may see slightly more changes than normal for this particular request. I also wanted to make sure that your two interfaces, one that is the gatherer and the other the storer, follow the separation of concerns principle as much as possible. I saw this being broken with the gatherer interface as it was storing the stream from CloudX onto the file system where that particular action should be from the storer role. Again, I can make two separate pull requests for this, the first request being for the mime detection.
  3. I can always switch the unit test framework from xunit to MSTest, and I will do it for this case as I believe unit tests are very important since they help speed development, help design the application, help document test cases for smaller components of your application, help prevent a large amount of unforeseeable issues, and help boost confidence of the written code. I am a firm believer of Test Driven Development (TDD) for medium to large applications, and I felt this was necessary to include in order to help you. Having these tests are also crucial for testing against something involving so many different file types and for speeding up both testing and development when working with the file system. Again, I can always change the unit test framework to MSTest as I want to keep your unit test framework preference.
  4. I do not buy this answer. People already have trust in the downloader as it has been promoted and talked about throughout the Neos Discord and beyond by both regular members, Mentors, Modders, and even the Neos Team. Sure there have been a few that wish to see the contents of the files as well as others that questioned the format of the files, but overall the majority (if not all) of the Neos community believes in this tool. Not only that, but if this request was truly just for file extensions, then why are you specifically looking for something that can retrieve the mime type by reading a stream? Why not read the 7zbson file from the Record's assetUri to retrieve the SHA256 hashes with their file extension? Why did you specifically ask for something to fetch the mime type? That is the question I asked myself that lead me to believe that the mime detection code might be used for something else in addition to tagging a file extension, which lead to me wanting to at least keep my changes for mime detection under GPLv2. Not only that, but the design of the application from the use of interfaces to a few of the "unique" property names and comments that I saw has also lead me to believe that there is another purpose for the mime detection whether that purpose is for using the mime type detection for something else in the same repository and/or using the mime detection code within a closed source setting due to the weak and pushover nature of MIT. Despite all of this, I do firmly believe you have good intentions for this tool. I also believe that you have been very considerate crediting people and taking the time to read these issues and PRs from different contributors including myself. I want to see this tool be successful, which is why I spent the extra time creating these unit tests for you. That, and I wanted to make sure that the mime detection is fine-tuned. If I am entirely wrong about my speculations, then shame on me. However, if my speculations are indeed correct, then this would be a dick move on your end as you are disguising the request for a different purpose.

I am willing to compromise with you as long as you are willing to do the same. Perhaps I might drop the GPLv2 requirement depending on our discussion in this issue thread, or perhaps I may not. You may just go to someone else for this job for all I know after reading this, but I do have a feeling you do not want to do that considering the work that went into this update. I want to see this tool be successful as much as you do, but I want to make sure it is done in a fair and just matter.

GuVAnj8Gv3RJ commented

Thanks for your update. I just want users to be able to see textures, sfx etc in their file browser at the moment. That's why we need the file extension.

Everything else should ideally be in a different issue/pr.

The cultural, political and social issues described here while read and understood are unfortunately causing confusion in this area.

I don't know how to get around the complications here. My temptation is to continue to work on this issue myself, but that will lead to lost work upon yourself. As such I won't be doing that. I'm going to go work on: #21 while I think some more.

stiefeljackal commented

As mentioned, I can decoupled the PR if that will work for you. I can also convert the unit tests into MSTest if that is your preferred framework.

As far as getting around the complications, can you elaborate? Are you referring to the bulleted list of things that you need to review? Or is this referring to my previous statements?

Either way, I appreciate the fact that you do not want to see the work I have done to go to waste. I will be thinking on this as well.

stiefeljackal commented

I thought about it long and hard; I think I will keep my changes to myself. Additionally, I am going to maintain my own version of the Neos Account Downloader where I am going to keep my version of the repository kosher.

Again, thank you for reading my comments and expressing the appreciation of my hard work. I still believe you have good intentions; however, there are things in this repository that I am uncomfortable with. This is where I depart with my changes.

stiefeljackal commented

I'm just curious how far you have gotten into this. If you have any questions for me, feel free to ask; I don't mind answering.

GuVAnj8Gv3RJ commented

Ok we now have file extensions.

Its likely we'll need updated mime types but I put stuff in place to basically "guess" and then still save as normal if there was no results.

Subsequent versions will go through and re-guess the file extension so as we add more we'll be sorted.

Please open GH issues for any missing extensions.

Huge thanks to @ThomFox and @stiefeljackal

@all-contributors please add @ThomFox for research.

allcontributors commented

@GuVAnj8Gv3RJ

I couldn't determine any contributions to add, did you specify any contributions?
Please make sure to use valid contribution names.

I've put up a pull request to add @ThomFox! 🎉