From 25ef63ecd7c0b9b3307166aaf5c9831172230149 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= <mosra@centrum.cz>
Date: Thu, 28 Feb 2019 12:45:12 +0100
Subject: [PATCH] TgaImporter: report error message on empty file being opened.

Also updated the test cases to be consistent with other image plugins.
---
 doc/changelog.dox                             |  2 ++
 .../TgaImporter/Test/TgaImporterTest.cpp      | 36 ++++++++++++++++---
 src/MagnumPlugins/TgaImporter/TgaImporter.cpp | 12 +++++++
 3 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/doc/changelog.dox b/doc/changelog.dox
index b03543051..407ac4777 100644
--- a/doc/changelog.dox
+++ b/doc/changelog.dox
@@ -96,6 +96,8 @@ See also:
     @ref Trade::AnySceneImporter "AnySceneImporter" plugins now correctly
     recognize also uppercase file extensions (see
     [mosra/magnum#312](https://github.com/mosra/magnum/pull/312))
+    @ref Trade::TgaImporter "TgaImporter" now properly reports an error message
+    when opening an empty file
 
 @subsection changelog-latest-bugfixes Bug fixes
 
diff --git a/src/MagnumPlugins/TgaImporter/Test/TgaImporterTest.cpp b/src/MagnumPlugins/TgaImporter/Test/TgaImporterTest.cpp
index 099e09b88..522cece0e 100644
--- a/src/MagnumPlugins/TgaImporter/Test/TgaImporterTest.cpp
+++ b/src/MagnumPlugins/TgaImporter/Test/TgaImporterTest.cpp
@@ -41,7 +41,9 @@ namespace Magnum { namespace Trade { namespace Test { namespace {
 struct TgaImporterTest: TestSuite::Tester {
     explicit TgaImporterTest();
 
+    void openEmpty();
     void openShort();
+
     void paletted();
     void compressed();
 
@@ -52,14 +54,17 @@ struct TgaImporterTest: TestSuite::Tester {
     void grayscaleBits8();
     void grayscaleBits16();
 
-    void useTwice();
+    void openTwice();
+    void importTwice();
 
     /* Explicitly forbid system-wide plugin dependencies */
     PluginManager::Manager<AbstractImporter> _manager{"nonexistent"};
 };
 
 TgaImporterTest::TgaImporterTest() {
-    addTests({&TgaImporterTest::openShort,
+    addTests({&TgaImporterTest::openEmpty,
+              &TgaImporterTest::openShort,
+
               &TgaImporterTest::paletted,
               &TgaImporterTest::compressed,
 
@@ -70,7 +75,8 @@ TgaImporterTest::TgaImporterTest() {
               &TgaImporterTest::grayscaleBits8,
               &TgaImporterTest::grayscaleBits16,
 
-              &TgaImporterTest::useTwice});
+              &TgaImporterTest::openTwice,
+              &TgaImporterTest::importTwice});
 
     /* Load the plugin directly from the build tree. Otherwise it's static and
        already loaded. */
@@ -79,6 +85,17 @@ TgaImporterTest::TgaImporterTest() {
     #endif
 }
 
+void TgaImporterTest::openEmpty() {
+    Containers::Pointer<AbstractImporter> importer = _manager.instantiate("TgaImporter");
+
+    std::ostringstream out;
+    Error redirectError{&out};
+    char a{};
+    /* Explicitly checking non-null but empty view */
+    CORRADE_VERIFY(!importer->openData({&a, 0}));
+    CORRADE_COMPARE(out.str(), "Trade::TgaImporter::openData(): the file is empty\n");
+}
+
 void TgaImporterTest::openShort() {
     Containers::Pointer<AbstractImporter> importer = _manager.instantiate("TgaImporter");
     const char data[] = { 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 };
@@ -201,11 +218,20 @@ void TgaImporterTest::grayscaleBits16() {
     CORRADE_COMPARE(debug.str(), "Trade::TgaImporter::image2D(): unsupported grayscale bits-per-pixel: 16\n");
 }
 
-void TgaImporterTest::useTwice() {
+void TgaImporterTest::openTwice() {
+    Containers::Pointer<AbstractImporter> importer = _manager.instantiate("TgaImporter");
+
+    CORRADE_VERIFY(importer->openFile(Utility::Directory::join(TGAIMPORTER_TEST_DIR, "file.tga")));
+    CORRADE_VERIFY(importer->openFile(Utility::Directory::join(TGAIMPORTER_TEST_DIR, "file.tga")));
+
+    /* Shouldn't crash, leak or anything */
+}
+
+void TgaImporterTest::importTwice() {
     Containers::Pointer<AbstractImporter> importer = _manager.instantiate("TgaImporter");
     CORRADE_VERIFY(importer->openFile(Utility::Directory::join(TGAIMPORTER_TEST_DIR, "file.tga")));
 
-    /* Verify that the file is rewinded for second use */
+    /* Verify that everything is working the same way on second use */
     {
         Containers::Optional<Trade::ImageData2D> image = importer->image2D(0);
         CORRADE_VERIFY(image);
diff --git a/src/MagnumPlugins/TgaImporter/TgaImporter.cpp b/src/MagnumPlugins/TgaImporter/TgaImporter.cpp
index e6c641258..3b6ef9513 100644
--- a/src/MagnumPlugins/TgaImporter/TgaImporter.cpp
+++ b/src/MagnumPlugins/TgaImporter/TgaImporter.cpp
@@ -53,6 +53,18 @@ bool TgaImporter::doIsOpened() const { return _in; }
 void TgaImporter::doClose() { _in = nullptr; }
 
 void TgaImporter::doOpenData(const Containers::ArrayView<const char> data) {
+    /* Because here we're copying the data and using the _in to check if file
+       is opened, having them nullptr would mean openData() would fail without
+       any error message. It's not possible to do this check on the importer
+       side, because empty file is valid in some formats (OBJ or glTF). We also
+       can't do the full import here because then doImage2D() would need to
+       copy the imported data instead anyway. This way it'll also work nicely
+       with a future openMemory(). */
+    if(data.empty()) {
+        Error{} << "Trade::TgaImporter::openData(): the file is empty";
+        return;
+    }
+
     _in = Containers::Array<char>{data.size()};
     std::copy(data.begin(), data.end(), _in.begin());
 }