Fix object context menu and other small things (#503)

- Fix object context menu after gtkmm 4 changes
- Fix crash when object is moved to folder where a same named object already exists
- Fix layout of fixture types window
- Make sure random selection doesn't select the same output twice
- Make sure timer effect doesn't initiate fade-out on first activation

Author: aroffringa

gui/components/objectlist.cpp

@@ -2,11 +2,13 @@
 
 #include <sigc++/signal.h>
 
+#include <gtkmm/applicationwindow.h>
 #include <gtkmm/gestureclick.h>
 #include <gtkmm/icontheme.h>
 
 #include "gui/eventtransmitter.h"
 #include "gui/instance.h"
+#include "gui/menufunctions.h"
 
 #include "theatre/chase.h"
 #include "theatre/effect.h"
@@ -233,22 +235,19 @@ void ObjectList::constructContextMenu() {
 
   FolderObject *obj = SelectedObject().Get();
   if (obj != &Instance::Management().RootFolder()) {
-    std::shared_ptr<Gio::SimpleActionGroup> actions =
-        Gio::SimpleActionGroup::create();
+    Gio::ActionMap &actions = Instance::MainWindow();
     // Move up & down
-    menu->append("Move up", "move_object_up");
-    actions->add_action("move_object_up",
-                        sigc::mem_fun(*this, &ObjectList::onMoveUpSelected));
+    AddMenuItem(actions, menu, "Move up", "move_object_up",
+                sigc::mem_fun(*this, &ObjectList::onMoveUpSelected));
 
-    menu->append("Move down", "move_object_down");
-    actions->add_action("move_object_down",
-                        sigc::mem_fun(*this, &ObjectList::onMoveDownSelected));
+    AddMenuItem(actions, menu, "Move down", "move_object_down",
+                sigc::mem_fun(*this, &ObjectList::onMoveDownSelected));
 
     // Move-to submenu
     std::shared_ptr<Gio::Menu> move_to_menu = Gio::Menu::create();
 
     int folder_counter = 0;
-    constructFolderMenu(*move_to_menu, *actions,
+    constructFolderMenu(move_to_menu, actions,
                         Instance::Management().RootFolder(), folder_counter);
     menu->append_submenu("Move to", move_to_menu);
   }
@@ -257,33 +256,34 @@ void ObjectList::constructContextMenu() {
   _contextMenu.set_parent(_listView);
 }
 
-void ObjectList::constructFolderMenu(Gio::Menu &menu,
-                                     Gio::SimpleActionGroup &actions,
-                                     Folder &folder, int &counter) {
-  if (folder.Children().empty()) {
+void ObjectList::constructFolderMenu(const std::shared_ptr<Gio::Menu> &menu,
+                                     Gio::ActionMap &actions, Folder &folder,
+                                     int &counter) {
+  std::shared_ptr<Gio::Menu> sub_menu;
+  for (const ObservingPtr<FolderObject> &object : folder.Children()) {
+    Folder *subFolder = dynamic_cast<Folder *>(object.Get());
+    if (subFolder) {
+      if (!sub_menu) {
+        sub_menu = Gio::Menu::create();
+        const std::string name = "moveto_" + std::to_string(counter);
+        ++counter;
+        AddMenuItem(actions, sub_menu, ".", name,
+                    [&]() { onMoveSelected(&folder); });
+      }
+      constructFolderMenu(sub_menu, actions, *subFolder, counter);
+    }
+  }
+
+  if (sub_menu) {
+    menu->append_submenu(folder.Name(), sub_menu);
+  } else {
+    // If the folder is empty, we create a single menu item
     const std::string name = "moveto_" + std::to_string(counter);
     ++counter;
-    menu.append(folder.Name(), name);
-    std::shared_ptr<Gio::SimpleAction> parent =
-        actions.add_action(name, [&]() { onMoveSelected(&folder); });
+    std::shared_ptr<Gio::SimpleAction> item = AddMenuItem(
+        actions, menu, folder.Name(), name, [&]() { onMoveSelected(&folder); });
 
-    if (&folder == SelectedObject()) parent->set_enabled(false);
-  } else {
-    std::shared_ptr<Gio::Menu> sub_menu;
-    for (const ObservingPtr<FolderObject> &object : folder.Children()) {
-      Folder *subFolder = dynamic_cast<Folder *>(object.Get());
-      if (subFolder) {
-        if (!sub_menu) {
-          sub_menu = Gio::Menu::create();
-          const std::string name = "moveto_" + std::to_string(counter);
-          ++counter;
-          menu.append(".", name);
-          actions.add_action(name, [&]() { onMoveSelected(&folder); });
-        }
-        constructFolderMenu(*sub_menu, actions, *subFolder, counter);
-      }
-    }
-    menu.append_submenu(folder.Name(), sub_menu);
+    if (&folder == SelectedObject()) item->set_enabled(false);
   }
 }
 
@@ -304,8 +304,17 @@ void ObjectList::TreeViewWithMenu::OnButtonPress(double x, double y) {
 }
 
 void ObjectList::onMoveSelected(Folder *destination) {
-  Folder::Move(SelectedObject(), *destination);
-  Instance::Events().EmitUpdate();
+  try {
+    Folder::Move(SelectedObject(), *destination);
+    Instance::Events().EmitUpdate();
+  } catch (std::exception &e) {
+    dialog_ = std::make_unique<Gtk::MessageDialog>(
+        Instance::MainWindow(),
+        "Could not move item to " + destination->Name() + ": " + e.what(),
+        false, Gtk::MessageType::ERROR, Gtk::ButtonsType::OK, true);
+    dialog_->signal_response().connect([this](int) { dialog_.reset(); });
+    dialog_->show();
+  }
 }
 
 void ObjectList::onMoveUpSelected() {

gui/components/objectlist.h

@@ -9,6 +9,7 @@
 #include <gtkmm/iconpaintable.h>
 #include <gtkmm/image.h>
 #include <gtkmm/liststore.h>
+#include <gtkmm/messagedialog.h>
 #include <gtkmm/popovermenu.h>
 #include <gtkmm/scrolledwindow.h>
 #include <gtkmm/treeview.h>
@@ -114,8 +115,9 @@ class ObjectList : public Gtk::ScrolledWindow {
   bool selectObject(const theatre::FolderObject &object,
                     const Gtk::TreeModel::Children &children);
   void constructContextMenu();
-  void constructFolderMenu(Gio::Menu &menu, Gio::SimpleActionGroup &actions,
-                           theatre::Folder &folder, int &counter);
+  void constructFolderMenu(const std::shared_ptr<Gio::Menu> &menu,
+                           Gio::ActionMap &actions, theatre::Folder &folder,
+                           int &counter);
   void onMoveSelected(theatre::Folder *destination);
   void onMoveUpSelected();
   void onMoveDownSelected();
@@ -126,6 +128,7 @@ class ObjectList : public Gtk::ScrolledWindow {
 
   RecursionLock _avoidRecursion;
   Gtk::PopoverMenu _contextMenu;
+  std::unique_ptr<Gtk::MessageDialog> dialog_;
 };
 
 }  // namespace glight::gui

gui/instance.h

@@ -3,6 +3,10 @@
 
 #include <memory>
 
+namespace Gtk {
+class ApplicationWindow;
+}
+
 namespace glight {
 namespace theatre {
 class Management;
@@ -51,13 +55,19 @@ class Instance {
 
   static system::Settings& Settings() { return *Get().settings_; }
 
+  static Gtk::ApplicationWindow& MainWindow() { return *Get().main_window_; }
+  void SetMainWindow(Gtk::ApplicationWindow& main_window) {
+    main_window_ = &main_window;
+  }
+
  private:
   // Because this class is used in many places, all members are pointers
   // so that extra includes can be avoided.
   theatre::Management* management_;
   EventTransmitter* events_;
   FixtureSelection* selection_;
   GUIState* state_;
+  Gtk::ApplicationWindow* main_window_;
   std::unique_ptr<system::Settings> settings_;
 };
 

gui/mainwindow/mainwindow.cpp

@@ -50,6 +50,7 @@ MainWindow::MainWindow() : main_menu_(*this) {
   Instance::Get().SetState(_state);
   Instance::Get().SetSelection(_fixtureSelection);
   Instance::Get().SetEvents(*this);
+  Instance::Get().SetMainWindow(*this);
   Instance::Settings() = system::LoadSettings();
   _management = std::make_unique<theatre::Management>(Instance::Settings());
   Instance::Get().SetManagement(*_management);

gui/menufunctions.h

@@ -8,15 +8,15 @@
 namespace glight::gui {
 
 inline std::shared_ptr<Gio::SimpleAction> AddMenuItem(
-    Gio::ActionMap& actions, std::shared_ptr<Gio::Menu>& menu,
+    Gio::ActionMap& actions, const std::shared_ptr<Gio::Menu>& menu,
     const Glib::ustring& label, const Glib::ustring& action_name,
     const sigc::slot<void()>& slot) {
   menu->append(label, "win." + action_name);
   return actions.add_action(action_name, slot);
 };
 
 inline std::shared_ptr<Gio::SimpleAction> AddToggleMenuItem(
-    Gio::ActionMap& actions, std::shared_ptr<Gio::Menu>& menu,
+    Gio::ActionMap& actions, const std::shared_ptr<Gio::Menu>& menu,
     const Glib::ustring& label, const Glib::ustring& action_name,
     bool initial_value, const sigc::slot<void(bool)>& slot) {
   auto action_toggle =

gui/windows/fixturetypeswindow.cpp

@@ -40,6 +40,7 @@ FixtureTypesWindow::FixtureTypesWindow() : functions_frame_(*this) {
   tree_view_.append_column("Functions", list_columns_.functions_);
   tree_view_.get_selection()->signal_changed().connect(
       [&]() { onSelectionChanged(); });
+  tree_view_.set_expand(true);
   fillList();
   type_scrollbars_.set_child(tree_view_);
 
@@ -48,8 +49,7 @@ FixtureTypesWindow::FixtureTypesWindow() : functions_frame_(*this) {
   left_box_.append(type_scrollbars_);
 
   paned_.set_start_child(left_box_);
-  left_box_.set_hexpand(true);
-  left_box_.set_vexpand(true);
+  left_box_.set_expand(true);
 
   // Right part
   right_grid_.attach(name_label_, 0, 0);

theatre/effects/randomselecteffect.h

@@ -10,16 +10,9 @@ namespace glight::theatre {
 
 class RandomSelectEffect final : public Effect {
  public:
-  RandomSelectEffect()
-      : Effect(1),
-        _active{false, false},
-        _startTime{0.0, 0.0},
-        _delay(10000.0),
-        _count(1){};
+  RandomSelectEffect() : Effect(1){};
 
-  virtual EffectType GetType() const override {
-    return EffectType::RandomSelect;
-  }
+  virtual EffectType GetType() const final { return EffectType::RandomSelect; }
 
   double Delay() const { return _delay; }
   void SetDelay(double delay) { _delay = delay; }
@@ -29,26 +22,23 @@ class RandomSelectEffect final : public Effect {
 
  private:
   virtual void MixImplementation(const ControlValue *values,
-                                 const Timing &timing,
-                                 bool primary) final override {
-    size_t count = std::min(_count, Connections().size());
-    if (values[0] && count != 0) {
+                                 const Timing &timing, bool primary) final {
+    size_t n_active = std::min(_count, Connections().size());
+    if (values[0] && n_active != 0) {
       std::vector<size_t> &activeConnections = _activeConnections[primary];
       if (Connections().size() != activeConnections.size()) {
         activeConnections.resize(Connections().size());
         for (size_t i = 0; i != activeConnections.size(); ++i)
           activeConnections[i] = i;
-        std::shuffle(activeConnections.begin(), activeConnections.end(),
-                     timing.RNG());
+        Shuffle(activeConnections, timing, false);
       }
       if (!_active[primary] ||
           timing.TimeInMS() - _startTime[primary] > _delay) {
         _active[primary] = true;
         _startTime[primary] = timing.TimeInMS();
-        std::shuffle(activeConnections.begin(), activeConnections.end(),
-                     timing.RNG());
+        Shuffle(activeConnections, timing, true);
       }
-      for (size_t i = 0; i != count; ++i) {
+      for (size_t i = 0; i != n_active; ++i) {
         if (activeConnections[i] < Connections().size()) {
           const std::pair<Controllable *, size_t> &connection =
               Connections()[activeConnections[i]];
@@ -60,12 +50,25 @@ class RandomSelectEffect final : public Effect {
     }
   }
 
-  bool _active[2];
+  void Shuffle(std::vector<size_t> &list, const Timing &timing,
+               bool is_restart) {
+    if (!list.empty()) {
+      // If only one output is active, require the output to change
+      // when reshuffling if it is a restart.
+      bool must_change = _count == 1 && list.size() > 1 && is_restart;
+      size_t first_active = list.front();
+      do {
+        std::shuffle(list.begin(), list.end(), timing.RNG());
+      } while (must_change && first_active == list.front());
+    }
+  }
+
+  bool _active[2] = {false, false};
   std::array<std::vector<size_t>, 2> _activeConnections;
-  double _startTime[2];
+  double _startTime[2] = {0.0, 0.0};
 
-  double _delay;
-  size_t _count;
+  double _delay = 10000.0;
+  size_t _count = 1;
 };
 
 }  // namespace glight::theatre

theatre/effects/timereffect.h

@@ -4,6 +4,7 @@
 #include "../effect.h"
 #include "../transition.h"
 
+#include "system/optionalnumber.h"
 #include "system/timepattern.h"
 
 namespace glight::theatre {
@@ -35,40 +36,59 @@ class TimerEffect final : public Effect {
                                  const Timing& timing, bool primary) final {
     if (values[0]) {
       if (NowInRange(start_, end_)) {
-        if (!is_on_) {
-          is_on_ = true;
-          transition_start_ = timing.TimeInMS();
-        }
-        if (timing.TimeInMS() - transition_start_ >
-            transition_in_.LengthInMs()) {
-          setAllOutputs(values[0]);
-        } else {
-          ControlValue multiplier = transition_in_.InValue(
-              timing.TimeInMS() - transition_start_, timing);
-          setAllOutputs(values[0] * multiplier);
-        }
+        MixOn(values[0], timing, primary);
       } else {
-        if (is_on_) {
-          is_on_ = false;
-          transition_start_ = timing.TimeInMS();
-        }
-        if (timing.TimeInMS() - transition_start_ <=
-            transition_out_.LengthInMs()) {
-          ControlValue multiplier = transition_out_.OutValue(
-              timing.TimeInMS() - transition_start_, timing);
-          setAllOutputs(values[0] * multiplier);
-        }
+        MixOff(values[0], timing, primary);
       }
     }
   }
 
  private:
+  void MixOn(const ControlValue& input, const Timing& timing, bool primary) {
+    if (!is_on_[primary]) {
+      is_on_[primary] = true;
+      transition_start_[primary] = timing.TimeInMS();
+    }
+    if (transition_start_[primary]) {
+      const int start = *transition_start_[primary];
+      if (timing.TimeInMS() - start < transition_in_.LengthInMs()) {
+        const ControlValue multiplier =
+            transition_in_.InValue(timing.TimeInMS() - start, timing);
+        setAllOutputs(input * multiplier);
+      } else {
+        transition_start_[primary].Reset();
+      }
+    }
+    if (!transition_start_[primary]) {
+      setAllOutputs(input);
+    }
+  }
+
+  void MixOff(const ControlValue& input, const Timing& timing, bool primary) {
+    if (is_on_[primary]) {
+      is_on_[primary] = false;
+      transition_start_[primary] = timing.TimeInMS();
+    }
+    if (transition_start_[primary]) {
+      const int start = *transition_start_[primary];
+      if (timing.TimeInMS() - start < transition_out_.LengthInMs()) {
+        const ControlValue multiplier =
+            transition_out_.OutValue(timing.TimeInMS() - start, timing);
+        setAllOutputs(input * multiplier);
+      } else {
+        transition_start_[primary].Reset();
+      }
+    }
+  }
+
   Transition transition_in_ = Transition(300, TransitionType::Fade);
   Transition transition_out_ = Transition(300, TransitionType::Fade);
   system::TimePattern start_;
   system::TimePattern end_;
-  int transition_start_ = 0;
-  bool is_on_ = false;
+  /// If unset, no transition is ongoing. If set, it is the transition start
+  /// time.
+  system::OptionalNumber<int> transition_start_[2];
+  bool is_on_[2] = {false, false};
 };
 
 }  // namespace glight::theatre