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

Make archive detection more robust and add it to the CLI #1394

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

Janfel
Copy link
Contributor

@Janfel Janfel commented Mar 12, 2022

This PR

  • consolidates the list of supported file extensions into three constants (NdsRomExtensions, GbaRomExtensions and ArchiveExtensions),
  • enables archive detection for the command line parameters, fixing Allow passing archives on the command line #1393,
  • filters archive members by file extension, just like the default file picker,
  • makes the handling of command line paths more robust by adding safety checks to MainWindow::preloadROMs,
  • thereby fixing some crashes and hangs when calling melonDS with invalid parameters, and
  • adds .tar.zst, .tar.lz4 and short forms (.tgz, .txz, .tzst, ...) to the list of supported archive file extensions, fixing Add *.tar.zst (zstd) to Archived ROMs support #1063.

If I have missed something or if MainWindow::preloadROMs isn’t the right place to do what I’m trying to do, please let me know.

Janfel added 5 commits March 12, 2022 11:39
This allows me to simplify the functions checking for file extensions.
This function is a safer alternative to the ‘filename.split('|')’ idiom,
that also checks for the existence of the files being referenced.
Now it should be harder to get melonDS to hang/crash/segfault by feeding
it invalid paths.
The only thing not renamed in accordance with the style guide is
MainWindow::splitArchivePath, because it would be jarring to have a
single member function looking different from the rest.
@Anuskuss
Copy link

I've been (kinda) maintaining my own fork with the following changes:

  1. Enable fullscreen when loading from CLI
  2. Enable archive support from CLI
  3. Custom save directory for saves and savestates (#946) + Use save dir for archives

The third feature has been implemented in #1333, this PR implements the second. The only thing left to do is making the emulator fullscreen when started from CLI.
I don't mean to feature creep this PR and I'm thankful that somebody finally stepped up to write a proper solution but the first bullet point is literally the only thing left before I can finally switch back to upstream and delete my (stupid) fork. You can check out how I did it but my code is terrible. You really don't have to overengineer this - just go into fullscreen whenever a ROM is loaded over CLI (I don't see a reason to make this a flag because CLI usually means the emulator has been started from a frontend). Hopefully you can fit this into this PR because multiple people have been messaging me about keeping my fork up-to-date and I can finally see the horizon.

@nadiaholmquist
Copy link
Collaborator

nadiaholmquist commented Mar 13, 2022

When launched through a file association on Linux, the ROM path is passed to the executable as an argument, and in a case like that having the emulator suddenly open in full screen would be rather surprising and undesirable.

Open as fullscreen (among several other things) should be a command line option.

@Anuskuss
Copy link

Open as fullscreen (among several other things) should be a command line option.

Yeah, I know - obviously that would be the preferred solution. PRs are welcome. But if you aren't going to write that it's better to reduce this request to a minimum otherwise there's a chance that progress will stall and then nobody wins. If Janfel doesn't want to implement that, I'll just merge this PR into my own fork and apply my changes on top. Users wanting this functionality will hopefully find my fork and figure out how to download/compile it.

@Janfel
Copy link
Contributor Author

Janfel commented Mar 13, 2022

I might look into improving the CLI, but that is a separate issue and out of scope for this PR.

@Anuskuss
Copy link

No worries, I'll just snatch your changes for the time being. I don't need an overcomplicated solution so I'm fine with using Anuskuss/melonDS.

Janfel added 3 commits March 15, 2022 21:57
Now the input dialog for choosing an archive member only shows members
that have a recognized file extension, just like the default file
picker. The input dialog also isn't shown for archives with multiple
members where only a single member has a recognized file extension.
@Anuskuss
Copy link

Anuskuss commented Nov 3, 2022

melonDS 0.9.5 has some basic functionality for this now. It's not perfect because it doesn't detect if it's a ROM or an archive and it doesn't autoload single ROM archives (you have to use it in conjunction with -a/--archive-file) but it's certainly good enough. Maybe this could be rebased though and finally merged to fill in the gaps?

@patataofcourse
Copy link
Contributor

It does autoload archives iirc, in the same way that File>Open does

Janfel pushed a commit to Janfel/melonDS that referenced this pull request Nov 10, 2022
This commit recreates the changes proposed in melonDS-emu#1394 on top of the
current master (b069a2a).
This also adds support for determining filetypes using the MIME database
provided by `QMimeDatabase`.
@Janfel
Copy link
Contributor Author

Janfel commented Nov 10, 2022

I have rebased the changes on the current master (b069a2a).
Because didn’t know how to correctly rebase a pull request, I have opened a new one as #1560.

RSDuck pushed a commit that referenced this pull request Jan 17, 2023
* Rebase/recreate my changes and add MIME support

This commit recreates the changes proposed in #1394 on top of the
current master (b069a2a).
This also adds support for determining filetypes using the MIME database
provided by `QMimeDatabase`.

* Move member syntax warning to a more appropriate place

* Deduplicate member syntax warning

* Change warning from "vertical bars" to "|"

* Conform brace placement to coding style

* Fix QFileDialog filter when ArchiveExtensions is empty

* Final cleanup and fixes

- Changes the NDS and GBA ROM MIME-Type constants to QStrings.
- Removes a leftover warning message.
- Uses Type() syntax instead of Type{} syntax for temporaries.

* Explain the origin of the supported archive list

Co-authored-by: Jan Felix Langenbach <[email protected]>
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.

4 participants