fix "referencing nonexistant file" bug

improve internal error message

src/data/action/value.cpp

@@ -7,17 +7,10 @@
 // ----------------------------------------------------------------------------- : Includes
 
 #include <util/prec.hpp>
+#include <util/tagged_string.hpp>
 #include <data/action/value.hpp>
 #include <data/field.hpp>
-#include <data/field/text.hpp>
-#include <data/field/choice.hpp>
-#include <data/field/multiple_choice.hpp>
-#include <data/field/color.hpp>
-#include <data/field/image.hpp>
-#include <data/field/symbol.hpp>
-#include <data/field/package_choice.hpp>
 #include <data/card.hpp>
-#include <util/tagged_string.hpp>
 #include <data/set.hpp> // for ValueActionPerformer
 #include <wx/imaglist.h>
 
@@ -44,48 +37,6 @@ void ValueAction::setCard(CardP const& card) {
 
 // ----------------------------------------------------------------------------- : Simple
 
-/// Swap the value in a Value object with a new one
-inline void swap_value(ChoiceValue&         a, ChoiceValue        ::ValueType& b) { swap(a.value,    b); }
-inline void swap_value(ColorValue&          a, ColorValue         ::ValueType& b) { swap(a.value,    b); }
-inline void swap_value(ImageValue&          a, ImageValue         ::ValueType& b) { swap(a.filename, b); a.last_update.update(); }
-inline void swap_value(SymbolValue&         a, SymbolValue        ::ValueType& b) { swap(a.filename, b); a.last_update.update(); }
-inline void swap_value(TextValue&           a, TextValue          ::ValueType& b) { swap(a.value,    b); a.last_update.update(); }
-inline void swap_value(PackageChoiceValue&  a, PackageChoiceValue ::ValueType& b) { swap(a.package_name, b); }
-inline void swap_value(MultipleChoiceValue& a, MultipleChoiceValue::ValueType& b) {
-  swap(a.value,       b.value);
-  swap(a.last_change, b.last_change);
-}
-
-/// A ValueAction that swaps between old and new values
-template <typename T, bool ALLOW_MERGE>
-class SimpleValueAction : public ValueAction {
-public:
-  inline SimpleValueAction(const intrusive_ptr<T>& value, const typename T::ValueType& new_value)
-    : ValueAction(value), new_value(new_value)
-  {}
-  
-  void perform(bool to_undo) override {
-    ValueAction::perform(to_undo);
-    swap_value(static_cast<T&>(*valueP), new_value);
-    valueP->onAction(*this, to_undo); // notify value
-  }
-  
-  bool merge(const Action& action) override {
-    if (!ALLOW_MERGE) return false;
-    TYPE_CASE(action, SimpleValueAction) {
-      if (action.valueP == valueP) {
-        // adjacent actions on the same value, discard the other one,
-        // because it only keeps an intermediate value
-        return true;
-      }
-    }
-    return false;
-  }
-  
-private:
-  typename T::ValueType new_value;
-};
-
 unique_ptr<ValueAction> value_action(const ChoiceValueP& value, const Defaultable<String>& new_value) {
   return make_unique<SimpleValueAction<ChoiceValue, true>>(value, new_value);
 }

src/data/action/value.hpp

@@ -16,6 +16,13 @@
 #include <util/prec.hpp>
 #include <util/action_stack.hpp>
 #include <util/defaultable.hpp>
+#include <data/field/text.hpp>
+#include <data/field/choice.hpp>
+#include <data/field/multiple_choice.hpp>
+#include <data/field/color.hpp>
+#include <data/field/image.hpp>
+#include <data/field/symbol.hpp>
+#include <data/field/package_choice.hpp>
 
 class StyleSheet;
 class LocalFileName;
@@ -53,6 +60,47 @@ private:
 
 // ----------------------------------------------------------------------------- : Simple
 
+/// Swap the value in a Value object with a new one
+inline void swap_value(ChoiceValue&         a, ChoiceValue        ::ValueType& b) { swap(a.value,    b); }
+inline void swap_value(ColorValue&          a, ColorValue         ::ValueType& b) { swap(a.value,    b); }
+inline void swap_value(ImageValue&          a, ImageValue         ::ValueType& b) { swap(a.filename, b); a.last_update.update(); }
+inline void swap_value(SymbolValue&         a, SymbolValue        ::ValueType& b) { swap(a.filename, b); a.last_update.update(); }
+inline void swap_value(TextValue&           a, TextValue          ::ValueType& b) { swap(a.value,    b); a.last_update.update(); }
+inline void swap_value(PackageChoiceValue&  a, PackageChoiceValue ::ValueType& b) { swap(a.package_name, b); }
+inline void swap_value(MultipleChoiceValue& a, MultipleChoiceValue::ValueType& b) {
+  swap(a.value,       b.value);
+  swap(a.last_change, b.last_change);
+}
+
+/// A ValueAction that swaps between old and new values
+template <typename T, bool ALLOW_MERGE>
+class SimpleValueAction : public ValueAction {
+public:
+  inline SimpleValueAction(const intrusive_ptr<T>& value, const typename T::ValueType& new_value)
+    : ValueAction(value), new_value(new_value)
+  {}
+  
+  void perform(bool to_undo) override {
+    ValueAction::perform(to_undo);
+    swap_value(static_cast<T&>(*valueP), new_value);
+    valueP->onAction(*this, to_undo); // notify value
+  }
+  
+  bool merge(const Action& action) override {
+    if (!ALLOW_MERGE) return false;
+    TYPE_CASE(action, SimpleValueAction) {
+      if (action.valueP == valueP) {
+        // adjacent actions on the same value, discard the other one,
+        // because it only keeps an intermediate value
+        return true;
+      }
+    }
+    return false;
+  }
+  
+  typename T::ValueType new_value;
+};
+
 /// Action that updates a Value to a new value
 unique_ptr<ValueAction> value_action(const ChoiceValueP&         value, const Defaultable<String>& new_value);
 unique_ptr<ValueAction> value_action(const MultipleChoiceValueP& value, const Defaultable<String>& new_value, const String& last_change);

src/data/set.cpp

@@ -16,6 +16,9 @@
 #include <data/field.hpp>
 #include <data/field/text.hpp>    // for 0.2.7 fix
 #include <data/field/information.hpp>
+#include <data/field/image.hpp>
+#include <data/field/symbol.hpp>
+#include <data/action/value.hpp>
 #include <util/tagged_string.hpp> // for 0.2.7 fix
 #include <util/order_cache.hpp>
 #include <util/delayed_index_maps.hpp>
@@ -104,6 +107,28 @@ IndexMap<FieldP, ValueP>& Set::stylingDataFor(const CardP& card) {
   else                           return stylingDataFor(stylesheetFor(card));
 }
 
+void Set::referenceActionStackFiles() {
+  referenceActionStackFiles(true);
+  referenceActionStackFiles(false);
+}
+void Set::referenceActionStackFiles(bool undo) {
+  for (auto&& action : undo ? actions.undo_actions : actions.redo_actions) {
+    try {
+      SimpleValueAction<ImageValue, false>& v = dynamic_cast<SimpleValueAction<ImageValue, false>&>(*action);
+      if (ImageValue* v2 = dynamic_cast<ImageValue*>(v.valueP.get())) {
+        referenceFile(v.new_value.toStringForWriting());
+        referenceFile(v2->filename.toStringForWriting());
+      }
+    } catch (...) { try {
+      SimpleValueAction<SymbolValue, false>& v = dynamic_cast<SimpleValueAction<SymbolValue, false>&>(*action);
+      if (SymbolValue* v2 = dynamic_cast<SymbolValue*>(v.valueP.get())) {
+        referenceFile(v.new_value.toStringForWriting());
+        referenceFile(v2->filename.toStringForWriting());
+      }
+    } catch (...) {} }
+  }
+}
+
 String Set::identification() const {
   // an identifying field
   FOR_EACH_CONST(v, data) {

src/data/set.hpp

@@ -89,8 +89,13 @@ public:
   /// Styling information for a particular stylesheet
   IndexMap<FieldP, ValueP>& stylingDataFor(const StyleSheet&);
   /// Styling information for a particular card
-  IndexMap<FieldP, ValueP>& stylingDataFor(const CardP& card);
+  IndexMap<FieldP, ValueP>& stylingDataFor(const CardP& card);
-  
+
+  /// Make sure the image and symbol files from
+  /// the ActionStack are saved so we can undo
+  void referenceActionStackFiles();
+  void referenceActionStackFiles(bool undo);
+
   /// Get the identification of this set, an identification is something like a name, title, etc.
   /** May return "" */
   String identification() const;

src/util/action_stack.hpp

@@ -103,11 +103,12 @@ public:
   /// Tell all listeners about an action
   void tellListeners(const Action&, bool undone);
   
-private:
   /// Actions to be undone.
   vector<unique_ptr<Action>> undo_actions;
   /// Actions to be redone
-  vector<unique_ptr<Action>> redo_actions;
+  vector<unique_ptr<Action>> redo_actions;
+
+private:
   /// Point at which the file was saved, corresponds to the top of the undo stack at that point
   const Action* save_point;
   /// Was the last thing the user did addAction? (as opposed to undo/redo)

src/util/error.cpp

@@ -110,10 +110,12 @@ String Error::what() const {
 
 InternalError::InternalError(const String& str)
   : Error(
-    _("An internal error occured:\n\n") +
+    _("An internal error occurred:\n\n") +
-    str + _("\n")
+    str + _("\n\n")
-    _("Please save your work (use 'save as' to so you don't overwrite things)\n")
+    _("Please save your work (use 'save as' so you don't overwrite things)\n")
-    _("and restart Magic Set Editor.\n\n")
+    _("and restart Magic Set Editor.\n")
+    _("You can also find a backup of your set in the same folder as your set file\n")
+    _("called 'SETNAME.mse-set.bak'. Rename it to 'SETNAME-backup.mse-set' to open it.\n")
     _("You should leave a bug report on https://github.com/twanvl/MagicSetEditor2/issues/\n")
     _("Press Ctrl+C to copy this message to the clipboard.")
   )

src/util/io/package.cpp

@@ -12,6 +12,7 @@
 #include <util/error.hpp>
 #include <script/to_value.hpp> // for reflection
 #include <script/profiler.hpp> // for PROFILER
+#include <data/set.hpp>
 #include <wx/wfstream.h>
 #include <wx/zipstrm.h>
 #include <wx/dir.h>
@@ -19,7 +20,7 @@
 // ----------------------------------------------------------------------------- : Package : outside
 
 IMPLEMENT_DYNAMIC_ARG(Package*, writing_package,   nullptr);
-IMPLEMENT_DYNAMIC_ARG(Package*, clipboard_package, nullptr);
+IMPLEMENT_DYNAMIC_ARG(Package*, clipboard_package, nullptr);
 
 Package::Package()
   : zipStream (nullptr)
@@ -97,7 +98,8 @@ void Package::save(bool remove_unused) {
   saveAs(filename, remove_unused);
 }
 
-void Package::saveAs(const String& name, bool remove_unused, bool as_directory) {
+void Package::saveAs(const String& name, bool remove_unused, bool as_directory) {
+  if (Set* s = dynamic_cast<Set*>(this)) s->referenceActionStackFiles();
   // type of package
   if (wxDirExists(name) || as_directory) {
     saveToDirectory(name, remove_unused, false);
@@ -109,7 +111,8 @@ void Package::saveAs(const String& name, bool remove_unused, bool as_directory)
   reopen();
 }
 
-void Package::saveCopy(const String& name) {
+void Package::saveCopy(const String& name) {
+  if (Set* s = dynamic_cast<Set*>(this)) s->referenceActionStackFiles();
   saveToZipfile(name, true, true);
   clearKeepFlag();
 }
@@ -298,7 +301,7 @@ LocalFileName Package::newFileName(const String& prefix, const String& suffix) {
 void Package::referenceFile(const String& file) {
   if (file.empty()) return;
   FileInfos::iterator it = files.find(file);
-  if (it == files.end()) throw InternalError(_("referencing a nonexistant file"));
+  if (it == files.end()) throw InternalError(_("Referencing an inexistant file!"));
   it->second.keep = true;
 }