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

added error popup when an asset is imported but aseprite path is improperly configured #29

Merged
merged 2 commits into from
Feb 14, 2019

Conversation

edwardrowe
Copy link
Contributor

@edwardrowe edwardrowe commented Oct 2, 2018

This addresses the issue I submitted #28.

Some info about my change:

  • The Popup assumes that if the Importer.IsConfigured() test fails, it's because Aseprite wasn't linked. This might be a problem in the future if other import plugins require alternative tests.
  • The only elegant way I could see to test for the file was to add the IsConfigured function to the IAnimationImporterPlugin. Please feel free to rework this or suggest another change.
  • There could easily be some other things I didn't consider with .pyxel files. I did not test this with PyxelEdit
  • You may not like the warning HelpBox when Aseprite path is not setup, as some users just won't use Aseprite. I made it a Warning (not error) for this reason, but it still may be intrusive.

Here's a gif of the new behavior.

animation-importer-badpath-fix

…operly configured

This just assumes PyxelEdit files are ok, as they don't seem to require the
Application location be specified.
@edwardrowe
Copy link
Contributor Author

This in no way blocks us, I just felt like contributing back to the repository for #hacktoberfest =) (https://hacktoberfest.digitalocean.com/)

@edwardrowe
Copy link
Contributor Author

Thinking this should actually throw an exception and be caught on the AnimationImporter Window. This is because we use our own importer that uses an AnimationImporter (and circumvents the window), and we'd want to be able to popup a window there, too, if you drop a file before configuring the Aseprite settings.

I'll try to rework it soon.

@talecrafter talecrafter merged commit 1963cba into talecrafter:master Feb 14, 2019
@talecrafter
Copy link
Owner

The exception might be the way to go, but for now at least the warning is a huge improvement over the current situation. Thank you.

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.

None yet

2 participants