Issue 983 (#986)

* WIP: first attempt to fix issue-983

* WIP: bugfixes

* Fixed a problem with displaying empty cell dimensions, one warning

* Another fix: don't allow proxy cells to be selected in the instance properties dialog. This creates a confusing behaviour

* Fixed a few flaws in the cell selection scheme on the instance properties dialog.

* Early warning when trying to build a recursive hierarchy.

* Another fix: avoid too much undo in case of errors thrown during 'apply' followed by 'cancel'

* Fixed issue-983 solution

* Fixed the modification status of PCell parameters for 'apply to all'

src/db/db/dbManager.cc

@@ -144,6 +144,28 @@ Manager::last_transaction_id () const
   return m_transactions.empty () ? 0 : reinterpret_cast<transaction_id_t> (& m_transactions.back ());
 }
 
+Manager::transaction_id_t
+Manager::transaction_id_for_undo () const
+{
+  transactions_t::iterator c = m_current;
+  if (c == m_transactions.begin ()) {
+    return 0;
+  } else {
+    --c;
+    return reinterpret_cast<transaction_id_t> (c.operator-> ());
+  }
+}
+
+Manager::transaction_id_t
+Manager::transaction_id_for_redo () const
+{
+  if (m_current == m_transactions.end ()) {
+    return 0;
+  } else {
+    return reinterpret_cast<transaction_id_t> (m_current.operator-> ());
+  }
+}
+
 void
 Manager::cancel ()
 {
@@ -289,7 +311,7 @@ Manager::last_queued (db::Object *object)
   tl_assert (m_opened);
   tl_assert (! m_replay);
 
-  if (m_current->first.empty () || m_current->first.back ().first != object->id ()) {
+  if (m_current == m_transactions.end () || m_current->first.empty () || (object && m_current->first.back ().first != object->id ())) {
     return 0;
   } else {
     return m_current->first.back ().second;

src/db/db/dbManager.h

@@ -143,6 +143,16 @@ public:
    */
   transaction_id_t last_transaction_id () const;
 
+  /**
+   *  @brief Gets the id of the next transaction to undo
+   */
+  transaction_id_t transaction_id_for_undo () const;
+
+  /**
+   *  @brief Gets the id of the next transaction to redo
+   */
+  transaction_id_t transaction_id_for_redo () const;
+
   /**
    *  @brief Close a transaction successfully.
    */
@@ -334,6 +344,11 @@ public:
     }
   }
 
+  bool is_empty () const
+  {
+    return ! mp_manager || mp_manager->last_queued (0) == 0;
+  }
+
   db::Manager::transaction_id_t id () const
   {
     return m_transaction_id;

src/edt/edt/edtInstPropertiesPage.cc

@@ -23,6 +23,8 @@
 
 #include "dbLibrary.h"
 #include "dbPCellHeader.h"
+#include "dbLibraryProxy.h"
+#include "dbPCellVariant.h"
 #include "edtInstPropertiesPage.h"
 #include "edtPropertiesPageUtils.h"
 #include "edtPCellParametersPage.h"
@@ -135,6 +137,21 @@ BEGIN_PROTECTED
 END_PROTECTED
 }
 
+static void
+get_cell_or_pcell_ids_by_name (const db::Layout *layout, const std::string &name, std::pair<bool, db::cell_index_type> &ci, std::pair<bool, db::pcell_id_type> &pci)
+{
+  ci = layout->cell_by_name (name.c_str ());
+  pci = layout->pcell_by_name (name.c_str ());
+
+  if (pci.first) {
+    //  prefer PCell names
+    ci.first = false;
+  } else if (ci.first && layout->cell (ci.second).is_proxy ()) {
+    //  don't let us select proxy names (they are eventually virtual cells)
+    ci.first = false;
+  }
+}
+
 void
 InstPropertiesPage::browse_cell ()
 {
@@ -153,19 +170,18 @@ BEGIN_PROTECTED
     layout = &cv->layout ();
   }
 
-  lay::LibraryCellSelectionForm form (this, layout, "browse_lib_cell");
+  lay::LibraryCellSelectionForm form (this, layout, "browse_lib_cell", false, lib != 0 /*for libs show top cells only*/);
   if (lib) {
     form.setWindowTitle (tl::to_qstring (tl::to_string (QObject::tr ("Select Cell - Library: ")) + lib->get_description ()));
   }
 
-  std::pair<bool, db::pcell_id_type> pc = layout->pcell_by_name (tl::to_string (cell_name_le->text ()).c_str ());
+  std::pair<bool, db::pcell_id_type> pc;
+  std::pair<bool, db::cell_index_type> c;
+  get_cell_or_pcell_ids_by_name (layout, tl::to_string (cell_name_le->text ()), c, pc);
   if (pc.first) {
     form.set_selected_pcell_id (pc.second);
-  } else {
-    std::pair<bool, db::cell_index_type> c = layout->cell_by_name (tl::to_string (cell_name_le->text ()).c_str ());
-    if (c.first) {
-      form.set_selected_cell_index (c.second);
-    }
+  } else if (c.first) {
+    form.set_selected_cell_index (c.second);
   }
 
   if (form.exec ()) {
@@ -305,8 +321,13 @@ InstPropertiesPage::update ()
   bool du = dbu_cb->isChecked ();
 
   db::Box cell_bbox = def_cell.bbox ();
-  cw_le->setText (tl::to_qstring (coord_to_string (cell_bbox.width (), dbu, du)));
-  ch_le->setText (tl::to_qstring (coord_to_string (cell_bbox.height (), dbu, du)));
+  if (cell_bbox.empty ()) {
+    cw_le->setText (QString ());
+    ch_le->setText (QString ());
+  } else {
+    cw_le->setText (tl::to_qstring (coord_to_string (cell_bbox.width (), dbu, du)));
+    ch_le->setText (tl::to_qstring (coord_to_string (cell_bbox.height (), dbu, du)));
+  }
 
   db::Trans t (pos->back ().inst_ptr.front ());
 
@@ -409,12 +430,22 @@ InstPropertiesPage::create_applicator (db::Cell & /*cell*/, const db::Instance &
 
   try {
 
-    std::pair<bool, db::cell_index_type> ci = layout->cell_by_name (tl::to_string (cell_name_le->text ()).c_str ());
-    std::pair<bool, db::pcell_id_type> pci = layout->pcell_by_name (tl::to_string (cell_name_le->text ()).c_str ());
+    std::pair<bool, db::cell_index_type> ci;
+    std::pair<bool, db::pcell_id_type> pci;
+    get_cell_or_pcell_ids_by_name (layout, tl::to_string (cell_name_le->text ()), ci, pci);
     if (! ci.first && ! pci.first) {
       throw tl::Exception (tl::to_string (QObject::tr ("Not a valid cell or PCell name: %s")).c_str (), tl::to_string (cell_name_le->text ()).c_str ());
     }
 
+    //  detect recursions in the hierarchy
+    if (lib == 0 && ci.first) {
+      std::set<db::cell_index_type> called;
+      layout->cell (ci.second).collect_called_cells (called);
+      if (ci.second == cv.cell_index () || called.find (cv.cell_index ()) != called.end ()) {
+        throw tl::Exception (tl::to_string (QObject::tr ("Trying to build a recursive hierarchy")).c_str ());
+      }
+    }
+
     lay::indicate_error (cell_name_le, (tl::Exception *) 0);
 
   } catch (tl::Exception &ex) {
@@ -424,29 +455,81 @@ InstPropertiesPage::create_applicator (db::Cell & /*cell*/, const db::Instance &
 
   try {
 
-    std::pair<bool, db::cell_index_type> ci = layout->cell_by_name (tl::to_string (cell_name_le->text ()).c_str ());
-    std::pair<bool, db::pcell_id_type> pci = layout->pcell_by_name (tl::to_string (cell_name_le->text ()).c_str ());
-    tl_assert (ci.first || pci.first);
+    std::pair<bool, db::cell_index_type> ci;
+    std::pair<bool, db::pcell_id_type> pci;
+    get_cell_or_pcell_ids_by_name (layout, tl::to_string (cell_name_le->text ()), ci, pci);
 
-    db::cell_index_type inst_cell_index = 0;
+    db::Layout *current_layout = &cv->layout ();
+    db::cell_index_type current_ci = pos->back ().inst_ptr.cell_index ();
 
-    //  instantiate the PCell
-    if (pci.first) {
-      tl_assert (mp_pcell_parameters != 0);
-      tl_assert (layout->pcell_declaration (pci.second) == mp_pcell_parameters->pcell_decl ());
-      inst_cell_index = layout->get_pcell_variant (pci.second, mp_pcell_parameters->get_parameters ());
-    } else {
-      inst_cell_index = ci.second;
+    std::pair<bool, db::pcell_id_type> current_pci = current_layout->is_pcell_instance (current_ci);
+    std::pair<db::Library *, db::cell_index_type> l = current_layout->defining_library (current_ci);
+
+    db::Library *current_lib = l.first;
+    if (current_lib) {
+      current_layout = &current_lib->layout ();
+      current_ci = l.second;
     }
 
-    //  reference the library
-    if (lib) {
-      layout = & cv->layout ();
-      inst_cell_index = layout->get_lib_proxy (lib, inst_cell_index);
-    }
+    if (! ci.first && ! pci.first) {
+
+      //  invalid cell name ...
+
+    } else if (pci.first != current_pci.first || (! pci.first && std::string (layout->cell_name (ci.second)) != current_layout->cell_name (current_ci))) {
+
+      //  a cell has been changed into pcell or vice versa, or the cell name has changed -> we can generate a new proxy and exchange cell indexes
+
+      db::cell_index_type inst_cell_index = 0;
+
+      //  instantiates the PCell
+      if (pci.first) {
+        tl_assert (mp_pcell_parameters != 0);
+        tl_assert (layout->pcell_declaration (pci.second) == mp_pcell_parameters->pcell_decl ());
+        inst_cell_index = layout->get_pcell_variant (pci.second, mp_pcell_parameters->get_parameters ());
+      } else {
+        inst_cell_index = ci.second;
+      }
+
+      //  references the library
+      if (lib) {
+        inst_cell_index = cv->layout ().get_lib_proxy (lib, inst_cell_index);
+      }
 
-    if (inst_cell_index != pos->back ().inst_ptr.cell_index ()) {
       appl->add (new ChangeTargetCellApplicator (inst_cell_index));
+
+    } else if (pci.first) {
+
+      //  pcell name has changed -> apply parameter deltas to other selected cells or pcells
+      //  otherwise keep pcell or cell name, change library if possible and required and apply parameter deltas to other selected cells or pcells
+
+      bool adjust_pcell_id = layout->pcell_declaration (pci.second)->name () != current_layout->pcell_declaration (current_pci.second)->name ();
+
+      std::map<std::string, tl::Variant> modified_param_by_name;
+
+      tl_assert (mp_pcell_parameters);
+
+      std::vector<tl::Variant> param = mp_pcell_parameters->get_parameters (0);
+      const std::vector<tl::Variant> &initial_param = mp_pcell_parameters->initial_parameters ();
+
+      const std::vector<db::PCellParameterDeclaration> &pcp = mp_pcell_parameters->pcell_decl ()->parameter_declarations ();
+      for (std::vector<db::PCellParameterDeclaration>::const_iterator pd = pcp.begin (); pd != pcp.end (); ++pd) {
+        unsigned int index = pd - pcp.begin ();
+        if (index < param.size () && index < initial_param.size () && param [index] != initial_param [index]) {
+          modified_param_by_name.insert (std::make_pair (pd->get_name (), param [index]));
+        }
+      }
+
+      if (adjust_pcell_id || lib != current_lib || ! modified_param_by_name.empty ()) {
+        appl->add (new ChangeTargetPCellApplicator (pci.second, adjust_pcell_id, lib, lib != current_lib, modified_param_by_name));
+      }
+
+    } else if (lib != current_lib) {
+
+      //  only library name has changed -> try to apply library to all selected instances keeping the cell name
+
+      //  NOTE: changing the library only is a special case of the ChangeTargetPCellApplicator
+      appl->add (new ChangeTargetPCellApplicator (0, false, lib, true, std::map<std::string, tl::Variant> ()));
+
     }
 
   } catch (tl::Exception &) {
@@ -776,8 +859,9 @@ InstPropertiesPage::update_pcell_parameters ()
 
   }
 
-  std::pair<bool, db::pcell_id_type> pc = layout->pcell_by_name (tl::to_string (cell_name_le->text ()).c_str ());
-  std::pair<bool, db::cell_index_type> cc = layout->cell_by_name (tl::to_string (cell_name_le->text ()).c_str ());
+  std::pair<bool, db::pcell_id_type> pc;
+  std::pair<bool, db::cell_index_type> cc;
+  get_cell_or_pcell_ids_by_name (layout, tl::to_string (cell_name_le->text ()), cc, pc);
 
   //  indicate an invalid cell name
   if (! pc.first && ! cc.first) {

src/edt/edt/edtPCellParametersPage.cc

@@ -697,6 +697,7 @@ PCellParametersPage::get_parameters (bool *ok)
 void
 PCellParametersPage::set_parameters (const std::vector<tl::Variant> &parameters)
 {
+  m_parameters = parameters;
   set_parameters_internal (parameters, false);
 }
 

src/edt/edt/edtPCellParametersPage.h

@@ -100,6 +100,16 @@ public:
    */
   std::vector<tl::Variant> get_parameters (bool *ok = 0);
 
+  /**
+   *  @brief Gets the initial parameters
+   *
+   *  The initial parameters are the ones present on "setup".
+   */
+  const std::vector<tl::Variant> &initial_parameters () const
+  {
+    return m_parameters;
+  }
+
   /**
    *  @brief Get the PCell declaration pointer
    */

src/edt/edt/edtPropertiesPageUtils.cc

@@ -25,6 +25,8 @@
 
 #include "dbShapes.h"
 #include "dbLayout.h"
+#include "dbLibrary.h"
+#include "dbPCellDeclaration.h"
 
 #include <QLineEdit>
 
@@ -579,6 +581,86 @@ db::Instance ChangeTargetCellApplicator::do_apply_inst (db::Cell &cell, const db
   return cell.replace (instance, arr);
 }
 
+// -------------------------------------------------------------------------
+//  ChangeTargetPCellApplicator implementation
+
+ChangeTargetPCellApplicator::ChangeTargetPCellApplicator (db::pcell_id_type pcell_id, bool apply_new_id, db::Library *new_lib, bool apply_new_lib, const std::map<std::string, tl::Variant> &modified_parameters)
+  : m_pcell_id (pcell_id), m_apply_new_id (apply_new_id), mp_new_lib (new_lib), m_apply_new_lib (apply_new_lib), m_modified_parameters (modified_parameters)
+{
+  //  .. nothing yet ..
+}
+
+db::Instance
+ChangeTargetPCellApplicator::do_apply_inst (db::Cell &cell, const db::Instance &instance, double /*dbu*/, bool /*relative*/) const
+{
+  tl_assert (cell.layout ());
+
+  db::Layout *layout = cell.layout ();
+
+  std::pair<bool, db::pcell_id_type> pci = layout->is_pcell_instance (instance.cell_index ());
+  std::pair<bool, db::cell_index_type> ci (false, 0);
+
+  db::Library *lib = layout->defining_library (instance.cell_index ()).first;
+
+  std::map<std::string, tl::Variant> named_parameters;
+  if (pci.first) {
+    named_parameters = layout->get_named_pcell_parameters (instance.cell_index ());
+  }
+  for (std::map<std::string, tl::Variant>::const_iterator p = m_modified_parameters.begin (); p != m_modified_parameters.end (); ++p) {
+    named_parameters [p->first] = p->second;
+  }
+
+  if ((m_apply_new_lib && lib != mp_new_lib) || (m_apply_new_id && (lib != mp_new_lib || ! pci.first || pci.second != m_pcell_id))) {
+
+    if (m_apply_new_id) {
+
+      lib = mp_new_lib;
+      pci.first = true;
+      pci.second = m_pcell_id;
+
+    } else if (m_apply_new_lib) {
+
+      if (! pci.first) {
+        std::string cell_name = (lib ? &lib->layout () : layout)->cell_name (instance.cell_index ());
+        ci = (mp_new_lib ? &mp_new_lib->layout () : layout)->cell_by_name (cell_name.c_str ());
+      } else {
+        std::string pcell_name = (lib ? &lib->layout () : layout)->pcell_declaration (pci.second)->name ();
+        pci = (mp_new_lib ? &mp_new_lib->layout () : layout)->pcell_by_name (pcell_name.c_str ());
+      }
+
+      lib = mp_new_lib;
+
+    }
+
+  }
+
+  db::CellInstArray arr = instance.cell_inst ();
+  db::cell_index_type inst_cell_index = arr.object ().cell_index ();
+
+  if (ci.first || pci.first) {
+
+    //  instantiates the PCell
+    if (pci.first) {
+      inst_cell_index = (lib ? &lib->layout () : layout)->get_pcell_variant_dict (pci.second, named_parameters);
+    } else {
+      inst_cell_index = ci.second;
+    }
+
+    //  references the library
+    if (lib) {
+      inst_cell_index = layout->get_lib_proxy (lib, inst_cell_index);
+    }
+
+  }
+
+  if (arr.object ().cell_index () != inst_cell_index) {
+    arr.object ().cell_index (inst_cell_index);
+    return cell.replace (instance, arr);
+  } else {
+    return instance;
+  }
+}
+
 // -------------------------------------------------------------------------
 //  ChangeInstanceTransApplicator implementation
 

src/edt/edt/edtPropertiesPageUtils.h

@@ -66,6 +66,10 @@ public:
   { 
     return db::Instance ();
   }
+
+private:
+  ChangeApplicator (const ChangeApplicator &);
+  ChangeApplicator &operator= (const ChangeApplicator &);
 };
 
 /**
@@ -334,6 +338,26 @@ private:
   db::cell_index_type m_cell_index;
 };
 
+/**
+ *  @brief An applicator changing the target pcell of an instance
+ */
+class ChangeTargetPCellApplicator
+  : public ChangeApplicator
+{
+public:
+  ChangeTargetPCellApplicator (db::pcell_id_type pcell_id, bool apply_new_id, db::Library *new_lib, bool apply_new_lib, const std::map<std::string, tl::Variant> &modified_parameters);
+
+  bool supports_relative_mode () const { return false; }
+  db::Instance do_apply_inst (db::Cell &cell, const db::Instance &instance, double dbu, bool relative) const;
+
+private:
+  db::pcell_id_type m_pcell_id;
+  bool m_apply_new_id;
+  db::Library *mp_new_lib;
+  bool m_apply_new_lib;
+  std::map<std::string, tl::Variant> m_modified_parameters;
+};
+
 /**
  *  @brief An applicator changing the transformation properties of an instance
  */

src/edt/edt/edtServiceImpl.cc

@@ -1410,7 +1410,6 @@ InstService::make_cell (const lay::CellView &cv)
   //  head transaction, hence releasing (thus: deleting) cells. To prevert interference, create
   //  the transaction at the beginning.
   db::Transaction tr (manager (), tl::to_string (QObject::tr ("Create reference cell")), m_reference_transaction_id);
-  m_reference_transaction_id = tr.id ();
 
   lay::LayerState layer_state = view ()->layer_snapshot ();
 
@@ -1478,6 +1477,10 @@ InstService::make_cell (const lay::CellView &cv)
   m_has_valid_cell = true;
   m_current_cell = inst_cell_index;
 
+  if (! tr.is_empty ()) {
+    m_reference_transaction_id = tr.id ();
+  }
+
   return std::pair<bool, db::cell_index_type> (true, inst_cell_index);
 }
 
@@ -1611,7 +1614,7 @@ void
 InstService::do_cancel_edit ()
 {
   //  Undo "create reference" transactions which basically unfinished "create instance" transactions
-  if (m_reference_transaction_id > 0 && manager ()->last_transaction_id () == m_reference_transaction_id) {
+  if (m_reference_transaction_id > 0 && manager ()->transaction_id_for_undo () == m_reference_transaction_id) {
     manager ()->undo ();
   }
 

src/laybasic/laybasic/layCellSelectionForm.cc

@@ -537,7 +537,7 @@ CellSelectionForm::hide_cell ()
 
 // ------------------------------------------------------------
 
-LibraryCellSelectionForm::LibraryCellSelectionForm (QWidget *parent, db::Layout *layout, const char *name, bool all_cells)
+LibraryCellSelectionForm::LibraryCellSelectionForm (QWidget *parent, db::Layout *layout, const char *name, bool all_cells, bool top_cells_only)
   : QDialog (parent), Ui::LibraryCellSelectionForm (),
     mp_lib (0), mp_layout (layout),
     m_name_cb_enabled (true),
@@ -545,7 +545,8 @@ LibraryCellSelectionForm::LibraryCellSelectionForm (QWidget *parent, db::Layout
     m_cell_index (-1),
     m_pcell_id (-1),
     m_is_pcell (false),
-    m_all_cells (all_cells)
+    m_all_cells (all_cells),
+    m_top_cells_only (top_cells_only)
 {
   setObjectName (QString::fromUtf8 (name));
 
@@ -571,7 +572,7 @@ LibraryCellSelectionForm::LibraryCellSelectionForm (QWidget *parent, db::Layout
   update_cell_list ();  
 }
 
-LibraryCellSelectionForm::LibraryCellSelectionForm (QWidget *parent, const char *name, bool all_cells)
+LibraryCellSelectionForm::LibraryCellSelectionForm (QWidget *parent, const char *name, bool all_cells, bool top_cells_only)
   : QDialog (parent), Ui::LibraryCellSelectionForm (),
     mp_lib (0), mp_layout (0),
     m_name_cb_enabled (true),
@@ -579,7 +580,8 @@ LibraryCellSelectionForm::LibraryCellSelectionForm (QWidget *parent, const char
     m_cell_index (-1),
     m_pcell_id (-1),
     m_is_pcell (false),
-    m_all_cells (all_cells)
+    m_all_cells (all_cells),
+    m_top_cells_only (top_cells_only)
 {
   mp_lib = db::LibraryManager::instance ().lib_ptr_by_name ("Basic");
   mp_layout = &mp_lib->layout ();
@@ -680,8 +682,16 @@ LibraryCellSelectionForm::update_cell_list ()
 
   if (mp_layout) {
 
+    unsigned int flags = lay::CellTreeModel::Flat;
+    if (! m_all_cells) {
+      flags |= lay::CellTreeModel::BasicCells;
+      if (m_top_cells_only) {
+        flags |= lay::CellTreeModel::TopCells;
+      }
+    }
+
     //  TODO: get rid of that const_cast
-    lay::CellTreeModel *model = new lay::CellTreeModel (lv_cells, const_cast<db::Layout *> (mp_layout), lay::CellTreeModel::Flat | (m_all_cells ? 0 : (lay::CellTreeModel::TopCells | lay::CellTreeModel::BasicCells)));
+    lay::CellTreeModel *model = new lay::CellTreeModel (lv_cells, const_cast<db::Layout *> (mp_layout), flags);
     
     lv_cells->setModel (model);
     //  connect can only happen after setModel()

src/laybasic/laybasic/layCellSelectionForm.h

@@ -113,16 +113,18 @@ public:
    *
    *  This version does not provide library selection. \get_current_library will
    *  always return 0.
-   *  If all_cells is true, all cells (not only top cells and basic cells) are shown.
+   *  If all_cells is true, all cells (not just top cells and basic cells) are shown.
+   *  If top_cells_only is false, child cells are shown as well.
    */
-  LibraryCellSelectionForm (QWidget *parent, db::Layout *layout, const char *name, bool all_cells = false);
+  LibraryCellSelectionForm (QWidget *parent, db::Layout *layout, const char *name, bool all_cells = false, bool top_cells_only = true);
 
   /**
    *  @brief Create a selection form for cells plus the library
    *
    *  If all_cells is true, all cells (not only top cells and basic cells) are shown.
+   *  If top_cells_only is false, child cells are shown as well.
    */
-  LibraryCellSelectionForm (QWidget *parent, const char *name, bool all_cells = false);
+  LibraryCellSelectionForm (QWidget *parent, const char *name, bool all_cells = false, bool top_cells_only = true);
 
   /**
    *  @brief Set the selected library
@@ -187,6 +189,7 @@ private:
   db::pcell_id_type m_pcell_id;
   bool m_is_pcell;
   bool m_all_cells;
+  bool m_top_cells_only;
 
   void select_entry (db::cell_index_type n);
   void select_pcell_entry (db::pcell_id_type n);

src/laybasic/laybasic/layCellTreeModel.cc

@@ -594,8 +594,10 @@ CellTreeModel::build_top_level ()
     while (top != mp_layout->end_top_down ()) {
 
       if (m_flat) {
-        CellTreeItem *item = new CellTreeItem (mp_layout, false, *top, true, m_sorting);
-        m_toplevel.push_back (item);
+        if ((m_flags & BasicCells) == 0 || ! mp_layout->cell (*top).is_proxy ()) {
+          CellTreeItem *item = new CellTreeItem (mp_layout, false, *top, true, m_sorting);
+          m_toplevel.push_back (item);
+        }
       } else if (mp_layout->cell (*top).is_top ()) {
         if ((m_flags & BasicCells) == 0 || ! mp_layout->cell (*top).is_proxy ()) {
           CellTreeItem *item = new CellTreeItem (mp_layout, false, *top, (m_flags & TopCells) != 0, m_sorting);

src/laybasic/laybasic/layPropertiesDialog.cc

@@ -139,7 +139,9 @@ BEGIN_PROTECTED
   if (! mp_properties_pages [m_index]->readonly ()) {
     db::Transaction t (mp_manager, tl::to_string (QObject::tr ("Apply changes")), m_transaction_id);
     mp_properties_pages [m_index]->apply ();
-    m_transaction_id = t.id ();
+    if (! t.is_empty ()) {
+      m_transaction_id = t.id ();
+    }
   }
 
   //  advance the current entry
@@ -181,7 +183,9 @@ BEGIN_PROTECTED
   if (! mp_properties_pages [m_index]->readonly ()) {
     db::Transaction t (mp_manager, tl::to_string (QObject::tr ("Apply changes")), m_transaction_id);
     mp_properties_pages [m_index]->apply ();
-    m_transaction_id = t.id ();
+    if (! t.is_empty ()) {
+      m_transaction_id = t.id ();
+    }
   }
 
   if (mp_properties_pages [m_index]->at_begin ()) {
@@ -280,7 +284,10 @@ BEGIN_PROTECTED
     //  we assume the page somehow indicates the error and does not apply the values
   }
 
-  m_transaction_id = t.id ();
+  //  remember transaction ID for undo on "Cancel" unless nothing happened
+  if (! t.is_empty ()) {
+    m_transaction_id = t.id ();
+  }
 
 END_PROTECTED
 }
@@ -294,7 +301,9 @@ PropertiesDialog::cancel_pressed ()
     //  because undo does not maintain a valid selection we clear it
     mp_editables->clear_selection ();
 
-    mp_manager->undo ();
+    if (mp_manager->transaction_id_for_undo () == m_transaction_id) {
+      mp_manager->undo ();
+    }
     m_transaction_id = 0;
 
   }
@@ -317,7 +326,9 @@ BEGIN_PROTECTED
     mp_properties_pages [m_index]->apply ();
     mp_properties_pages [m_index]->update ();
 
-    m_transaction_id = t.id ();
+    if (! t.is_empty ()) {
+      m_transaction_id = t.id ();
+    }
 
   }