- renamed instance::name() to displayName() to reduce confusion with portable instances

- better errors when running incorrect shortcuts
- fixed shortcuts being created with moshortcut://Portable:exe instead of an empty instance name for portable instances

10 files changed, 136 insertions, 65 deletions

diff --git a/src/commandline.cpp b/src/commandline.cpp
index 22755dda..f64ff7cb 100644
--- a/src/commandline.cpp
+++ b/src/commandline.cpp
@@ -251,7 +251,7 @@ std::optional<int> CommandLine::runPostApplication(MOApplication& a)
     env::Console c;
 
     if (auto i=InstanceManager::singleton().currentInstance()) {
-      std::cout << i->name().toStdString() << "\n";
+      std::cout << i->displayName().toStdString() << "\n";
     } else {
       std::cout << "no instance configured\n";
     }
@@ -289,9 +289,8 @@ std::optional<int> CommandLine::runPostOrganizer(OrganizerCore& core)
 
         return 0;
       }
-      catch (const std::exception &e) {
-        reportError(
-          QObject::tr("failed to start shortcut: %1").arg(e.what()));
+      catch (std::exception&) {
+        // user was already warned
         return 1;
       }
     }
@@ -451,7 +450,7 @@ std::optional<QString> CommandLine::instance() const
   // note that moshortcut:// overrides -i
 
   if (m_shortcut.isValid() && m_shortcut.hasInstance()) {
-    return m_shortcut.instance();
+    return m_shortcut.instanceName();
   } else if (m_vm.count("instance")) {
     return QString::fromStdString(m_vm["instance"].as<std::string>());
   }
@@ -817,7 +816,7 @@ std::optional<int> RunCommand::runPostOrganizer(OrganizerCore& core)
           reportError(
             QObject::tr("Executable '%1' not found in instance '%2'.")
               .arg(program)
-              .arg(InstanceManager::singleton().currentInstance()->name()));
+              .arg(InstanceManager::singleton().currentInstance()->displayName()));
 
           return 1;
         }
diff --git a/src/envshortcut.cpp b/src/envshortcut.cpp
index b13a7d9f..4c50a20e 100644
--- a/src/envshortcut.cpp
+++ b/src/envshortcut.cpp
@@ -145,11 +145,13 @@ Shortcut::Shortcut()
 Shortcut::Shortcut(const Executable& exe)
   : Shortcut()
 {
+  const auto i = *InstanceManager::singleton().currentInstance();
+
   m_name = exe.title();
   m_target = QFileInfo(qApp->applicationFilePath()).absoluteFilePath();
 
   m_arguments = QString("\"moshortcut://%1:%2\"")
-    .arg(InstanceManager::singleton().currentInstance()->name())
+    .arg(i.isPortable() ? "" : i.displayName())
     .arg(exe.title());
 
   m_description = QString("Run %1 with ModOrganizer").arg(exe.title());
diff --git a/src/instancemanager.cpp b/src/instancemanager.cpp
index 8add4af8..a1b17577 100644
--- a/src/instancemanager.cpp
+++ b/src/instancemanager.cpp
@@ -48,7 +48,7 @@ Instance::Instance(QString dir, bool portable, QString profileName) :
 {
 }
 
-QString Instance::name() const
+QString Instance::displayName() const
 {
   if (isPortable())
     return QObject::tr("Portable");
@@ -105,7 +105,7 @@ bool Instance::isActive() const
     if (m_portable) {
       return i->isPortable();
     } else {
-      return (i->name() == name());
+      return (i->displayName() == displayName());
     }
   }
 
@@ -822,7 +822,7 @@ SetupInstanceResults selectGame(Instance& instance, PluginContainer& pc)
   CreateInstanceDialog dlg(pc, nullptr);
 
   // only show the game page
-  dlg.setSinglePage<cid::GamePage>(instance.name());
+  dlg.setSinglePage<cid::GamePage>(instance.displayName());
 
   dlg.show();
   dlg.activateWindow();
@@ -860,7 +860,7 @@ SetupInstanceResults selectVariant(Instance& instance, PluginContainer& pc)
     instance.gamePlugin(), instance.gameDirectory());
 
   // only show the variant page
-  dlg.setSinglePage<cid::VariantsPage>(instance.name());
+  dlg.setSinglePage<cid::VariantsPage>(instance.displayName());
 
   dlg.show();
   dlg.activateWindow();
@@ -897,7 +897,7 @@ SetupInstanceResults setupInstance(Instance& instance, PluginContainer& pc)
 
       reportError(
         QObject::tr("Cannot open instance '%1', failed to read INI file %2.")
-        .arg(instance.name()).arg(instance.iniPath()));
+        .arg(instance.displayName()).arg(instance.iniPath()));
 
       return SetupInstanceResults::SelectAnother;
     }
@@ -914,7 +914,7 @@ SetupInstanceResults setupInstance(Instance& instance, PluginContainer& pc)
         QObject::tr(
           "Cannot open instance '%1', the managed game was not found in the INI "
           "file %2. Select the game managed by this instance.")
-        .arg(instance.name()).arg(instance.iniPath()));
+        .arg(instance.displayName()).arg(instance.iniPath()));
 
       return selectGame(instance, pc);
     }
@@ -928,7 +928,7 @@ SetupInstanceResults setupInstance(Instance& instance, PluginContainer& pc)
         QObject::tr(
           "Cannot open instance '%1', the game plugin '%2' doesn't exist. It "
           "may have been deleted by an antivirus. Select another instance.")
-        .arg(instance.name()).arg(instance.gameName()));
+        .arg(instance.displayName()).arg(instance.gameName()));
 
       return SetupInstanceResults::SelectAnother;
     }
@@ -943,7 +943,7 @@ SetupInstanceResults setupInstance(Instance& instance, PluginContainer& pc)
           "Cannot open instance '%1', the game directory '%2' doesn't exist or "
           "the game plugin '%3' doesn't recognize it. Select the game managed "
           "by this instance.")
-        .arg(instance.name())
+        .arg(instance.displayName())
         .arg(instance.gameDirectory())
         .arg(instance.gameName()));
 
diff --git a/src/instancemanager.h b/src/instancemanager.h
index 893b88e4..f1781a70 100644
--- a/src/instancemanager.h
+++ b/src/instancemanager.h
@@ -132,9 +132,14 @@ public:
   // returns the instance name; this is the directory name or "Portable" for
   // portable instances
   //
+  // be careful when using this function to check whether two instances are the
+  // same, some parts of MO use an empty string to represent portable instances,
+  // but this function will return "Portable" for them; it's safer to check
+  // for isPortable() first
+  //
   // can be called without setup()
   //
-  QString name() const;
+  QString displayName() const;
 
   // returns either:
   //   1) the game name from the INI, if readFromIni() was called;
diff --git a/src/instancemanagerdialog.cpp b/src/instancemanagerdialog.cpp
index ecc08c1f..1fd43128 100644
--- a/src/instancemanagerdialog.cpp
+++ b/src/instancemanagerdialog.cpp
@@ -221,7 +221,7 @@ void InstanceManagerDialog::updateInstances()
 
   // sort first, prepend portable after so it's always on top
   std::sort(m_instances.begin(), m_instances.end(), [](auto&& a, auto&& b) {
-    return (MOBase::naturalCompare(a->name(), b->name()) < 0);
+    return (MOBase::naturalCompare(a->displayName(), b->displayName()) < 0);
   });
 
   if (m.portableInstanceExists()) {
@@ -249,7 +249,7 @@ void InstanceManagerDialog::updateList()
   for (std::size_t i=0; i<m_instances.size(); ++i) {
     const auto& ii = *m_instances[i];
 
-    auto* item = new QStandardItem(ii.name());
+    auto* item = new QStandardItem(ii.displayName());
     item->setIcon(instanceIcon(m_pc, ii));
 
     m_model->appendRow(item);
@@ -295,7 +295,7 @@ void InstanceManagerDialog::select(std::size_t i)
 void InstanceManagerDialog::select(const QString& name)
 {
   for (std::size_t i=0; i<m_instances.size(); ++i) {
-    if (m_instances[i]->name() == name) {
+    if (m_instances[i]->displayName() == name) {
       select(i);
       return;
     }
@@ -310,7 +310,7 @@ void InstanceManagerDialog::selectActiveInstance()
 
   if (active) {
     for (std::size_t i=0; i<m_instances.size(); ++i) {
-      if (m_instances[i]->name() == active->name()) {
+      if (m_instances[i]->displayName() == active->displayName()) {
         select(i);
 
         ui->list->scrollTo(
@@ -340,7 +340,7 @@ void InstanceManagerDialog::openSelectedInstance()
   if (to.isPortable()) {
     InstanceManager::singleton().setCurrentInstance("");
   } else {
-    InstanceManager::singleton().setCurrentInstance(to.name());
+    InstanceManager::singleton().setCurrentInstance(to.displayName());
   }
 
   if (m_restartOnSelect) {
@@ -372,7 +372,7 @@ bool InstanceManagerDialog::confirmSwitch(const Instance& to)
   const auto r = dlg
     .title(tr("Switching instances"))
     .main(tr("Mod Organizer must restart to manage the instance '%1'.")
-      .arg(to.name()))
+      .arg(to.displayName()))
     .content(tr("This confirmation can be disabled in the settings."))
     .icon(QMessageBox::Question)
     .button({tr("Restart Mod Organizer"), QMessageBox::Ok})
@@ -401,7 +401,7 @@ void InstanceManagerDialog::rename()
 
   // getting new name
   const auto newName = getInstanceName(
-    this, tr("Rename instance"), "", tr("Instance name"), i->name());
+    this, tr("Rename instance"), "", tr("Instance name"), i->displayName());
 
   if (newName.isEmpty()) {
     return;
@@ -661,7 +661,7 @@ const Instance* InstanceManagerDialog::singleSelection() const
 
 void InstanceManagerDialog::fillData(const Instance& ii)
 {
-  ui->name->setText(ii.name());
+  ui->name->setText(ii.displayName());
   ui->location->setText(ii.directory());
   ui->baseDirectory->setText(ii.baseDirectory());
   ui->gameName->setText(ii.gameName());
diff --git a/src/moapplication.cpp b/src/moapplication.cpp
index 6d21745e..11053e66 100644
--- a/src/moapplication.cpp
+++ b/src/moapplication.cpp
@@ -392,10 +392,14 @@ void MOApplication::externalMessage(const QString& message)
 

   if (moshortcut.isValid()) {

     if(moshortcut.hasExecutable()) {

-      m_core->processRunner()

-        .setFromShortcut(moshortcut)

-        .setWaitForCompletion(ProcessRunner::TriggerRefresh)

-        .run();

+      try {

+        m_core->processRunner()

+          .setFromShortcut(moshortcut)

+          .setWaitForCompletion(ProcessRunner::TriggerRefresh)

+          .run();

+      } catch(std::exception&) {

+        // user was already warned

+      }

     }

   } else if (isNxmLink(message)) {

     MessageDialog::showMessage(tr("Download started"), qApp->activeWindow(), false);

@@ -414,11 +418,11 @@ void MOApplication::externalMessage(const QString& message)
     if (auto i=cl.instance()) {

       const auto ci = InstanceManager::singleton().currentInstance();

 

-      if (*i != ci->name()) {

+      if (*i != ci->displayName()) {

         reportError(tr(

           "This shortcut or command line is for instance '%1', but the current "

           "instance is '%2'.")

-            .arg(*i).arg(ci->name()));

+            .arg(*i).arg(ci->displayName()));

 

         return;

       }

diff --git a/src/moshortcut.cpp b/src/moshortcut.cpp
index 4efedbdb..4bbd61a4 100644
--- a/src/moshortcut.cpp
+++ b/src/moshortcut.cpp
@@ -1,25 +1,5 @@
-/*
-Copyright (C) 2016 Sebastian Herbord. All rights reserved.
-
-This file is part of Mod Organizer.
-
-Mod Organizer is free software: you can redistribute it and/or modify
-it under the terms of the GNU General Public License as published by
-the Free Software Foundation, either version 3 of the License, or
-(at your option) any later version.
-
-Mod Organizer is distributed in the hope that it will be useful,
-but WITHOUT ANY WARRANTY; without even the implied warranty of
-MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-GNU General Public License for more details.
-
-You should have received a copy of the GNU General Public License
-along with Mod Organizer.  If not, see <http://www.gnu.org/licenses/>.
-*/
-
-
 #include "moshortcut.h"
-
+#include "instancemanager.h"
 
 MOShortcut::MOShortcut(const QString& link)
   : m_valid(link.startsWith("moshortcut://"))
@@ -41,6 +21,51 @@ MOShortcut::MOShortcut(const QString& link)
   }
 }
 
+bool MOShortcut::isValid() const
+{
+  return m_valid;
+}
+
+bool MOShortcut::hasInstance() const
+{
+  return m_hasInstance;
+}
+
+bool MOShortcut::hasExecutable() const
+{
+  return m_hasExecutable;
+}
+
+QString MOShortcut::instanceDisplayName() const
+{
+  return (m_instance == "" ? QObject::tr("Portable") : m_instance);
+}
+
+const QString& MOShortcut::instanceName() const
+{
+  return m_instance;
+}
+
+const QString& MOShortcut::executableName() const
+{
+  return m_executable;
+}
+
+bool MOShortcut::isForInstance(const Instance& i) const
+{
+  if (!m_hasInstance) {
+    // no instance name was specified, so the current one is fine
+    return true;
+  }
+
+  if (m_instance == "") {
+    // empty instance name means portable
+    return i.isPortable();
+  } else {
+    return (i.displayName() == m_instance);
+  }
+}
+
 QString MOShortcut::toString() const
 {
   if (m_hasInstance) {
diff --git a/src/moshortcut.h b/src/moshortcut.h
index 33346bb9..a693b6df 100644
--- a/src/moshortcut.h
+++ b/src/moshortcut.h
@@ -3,21 +3,42 @@
 
 #include <QString>
 
+class Instance;
+
 class MOShortcut
 {
 public:
   MOShortcut(const QString& link={});
 
-  /// true iff intialized using a valid moshortcut link
-  bool isValid() const { return m_valid; }
+  // true if initialized using a valid moshortcut link
+  //
+  bool isValid() const;
+
+  // whether an instance name was given
+  //
+  bool hasInstance() const;
+
+  // whether an executable name was given
+  //
+  bool hasExecutable() const;
 
-  bool hasInstance() const { return m_hasInstance; }
+  // name of the instance given, "Portable" for portable; undefined if
+  // hasInstance() returns false
+  //
+  QString instanceDisplayName() const;
 
-  bool hasExecutable() const { return m_hasExecutable; }
+  // name of the instance given, empty for portable; undefined if hasInstance()
+  // returns false
+  //
+  const QString& instanceName() const;
 
-  const QString& instance() const { return m_instance; }
+  // name of the executable given
+  //
+  const QString& executableName() const;
 
-  const QString& executable() const { return m_executable; }
+  // whether this shortcut is for the given instance
+  //
+  bool isForInstance(const Instance& i) const;
 
   QString toString() const;
 
diff --git a/src/processrunner.cpp b/src/processrunner.cpp
index c932ce44..4d83d0c8 100644
--- a/src/processrunner.cpp
+++ b/src/processrunner.cpp
@@ -4,6 +4,7 @@
 #include "iuserinterface.h"
 #include "envmodule.h"
 #include "env.h"
+#include <report.h>
 #include <iplugingame.h>
 #include <log.h>
 
@@ -607,16 +608,30 @@ ProcessRunner& ProcessRunner::setFromShortcut(const MOShortcut& shortcut)
 
   if (currentInstance)
   {
-    if (shortcut.hasInstance() && shortcut.instance() != currentInstance->name()) {
-      throw std::runtime_error(
-        QString("Refusing to run executable from different instance %1:%2")
-        .arg(shortcut.instance(),shortcut.executable())
-        .toLocal8Bit().constData());
+    if (shortcut.hasInstance() && !shortcut.isForInstance(*currentInstance)) {
+      MOBase::reportError(QObject::tr(
+        "This shortcut is for instance '%1' but Mod Organizer is currently "
+        "running for '%2'. Exit Mod Organizer before running the shortcut or "
+        "change the active instance.")
+        .arg(shortcut.instanceDisplayName())
+        .arg(currentInstance->displayName()));
+
+      throw std::exception();
     }
   }
 
-  const Executable& exe = m_core.executablesList()->get(shortcut.executable());
-  setFromExecutable(exe);
+  const auto* exes = m_core.executablesList();
+  const auto exe = exes->find(shortcut.executableName());
+
+  if (exe != exes->end()) {
+    setFromExecutable(*exe);
+  } else {
+    MOBase::reportError(QObject::tr(
+      "Executable '%1' does not exist in instance '%2'.")
+      .arg(shortcut.executableName()).arg(currentInstance->displayName()));
+
+    throw std::exception();
+  }
 
   return *this;
 }
diff --git a/src/statusbar.cpp b/src/statusbar.cpp
index 8fa43d5b..39acc32e 100644
--- a/src/statusbar.cpp
+++ b/src/statusbar.cpp
@@ -155,7 +155,7 @@ void StatusBar::updateNormalMessage(OrganizerCore& core)
 
   QString instance = "?";
   if (auto i=InstanceManager::singleton().currentInstance())
-    instance = i->name();
+    instance = i->displayName();
 
   QString profile = core.profileName();