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

feat: Add basic msp file reading functionality #171

Merged
merged 2 commits into from
Oct 31, 2021

Conversation

learn-more
Copy link
Contributor

@learn-more learn-more commented Oct 26, 2021

This functionality was something that I needed, and it seems to be working fine (from my limited testing).
There are no tests for it yet, and it is a bit simple (file tab does not work, but all others seem to be working the same as for .msi files)

@activescott is this something that you are interested in?
If so, please list what you would like me to add/improve.

Did not do these (yet):

ps: I have not forgotten about my older PR's, will get to address those eventually.

Testing was done on the .msp files from the German version of Microsoft's Excel Viewer available at:
https://web.archive.org/web/20170104231942if_/http://download.microsoft.com/download/F/8/8/F88CB355-ECAA-4B74-87D6-C05C81D215BF/ExcelViewer.exe

image

@activescott
Copy link
Owner

@learn-more So glad to see you back here! Hey all tests pass and this is a very long standing request! See #1 !

I'm open to merging this. Only things I'd really like to see are:

  • A msp test automated test? Honestly, I don't even know what an msp is and I don't know where to get one. So that's one of the reasons that's been such a long-standing issue. So if you have one you could upload and add a test for that would be great. If you can just attach one here or on send me a link, I'd be willing to help add a quick automated test if that helps.
  • Just ensure you do some manual testing of the GUI with traditional msi's too. Maybe you've already been using it and can confirm it works, and that is good for me too. No huge ask here.

Thanks and let me know how I can help. iIdon't want to be an obstacle to getting this merged!

@learn-more learn-more marked this pull request as ready for review October 27, 2021 12:53
@learn-more
Copy link
Contributor Author

learn-more commented Oct 27, 2021

@activescott I have included some basic tests for 3 msp files that I found on my machine.
Most of the logic that is interesting here (streams, table parsing) is actually implemented in the view itself, so the tests are not really exciting.

Normal MSI files that I tested work fine, but I don't have an extensive testing collection for that.

Additionally, I amended the initial commit with a bugfix: I forgot to add dragdrop support for msp files, but that is handled now as well.

Oh, and I've got a PR lined up for the names of the Streams (that are mojibake right now),
after this PR is done I can add the new PR for that (in case you want to do some regression testing, it might make sense to wait for that second PR).

@activescott activescott merged commit b4db2bb into activescott:master Oct 31, 2021
@activescott
Copy link
Owner

🎉 This PR is included in version 1.9.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@learn-more learn-more deleted the feat/msp branch October 31, 2021 15:49
@activescott
Copy link
Owner

@learn-more Thanks again for your contributions. Just a reminder that you should be able to go to https://tip4commit.com/github/activescott/lessmsi and claim a "tip" in bitcoin for your commits to this project.

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

Successfully merging this pull request may close these issues.

2 participants