The LineEditNode constructors take both a name parameter and potentially a LineEditProperties struct (Line 39) which also contains a label. Clarify if name is for the node's title and properties.label for the input field itself, or if there's redundancy.
See comment for Line 35 regarding potential redundancy between name and properties.label.
The return; statement here prevents any visual output for array types, despite the new fmt::join logic being added to convert them to a string. If the intention is to display the converted array string (even if basic), this return should be removed.
The warning message "Array to string conversion not fully implemented." seems to contradict the new implementation using fmt::join. If the new implementation is intended to be the "proper array to string conversion" (as per the old TODO), this warning should be removed or clarified to reflect remaining limitations (e.g., "Array element conversion for complex types is still incomplete.").
The TODO regarding NodeData::to_string() robustness is critical for reliable array visualization. Consider creating a separate task/issue to address this dependency comprehensively.
class ContextMenuDelegate; - This forward declaration in the ui namespace seems unused. app::ContextMenu::Delegate is defined within the app namespace. Clarify its purpose or remove it if not used.
class Item; - This is a good forward declaration. However, the relationship between app::ContextMenu::Item and the globally declared ContextMenuItem (L61) is unclear. Are they the same, or is one a base class for the other?
explicit ContextMenu(std::vector<Item*> items); - Changing from std::vector<Item> to std::vector<Item*> implies a significant change in ownership. Clarify who owns the Item* objects and is responsible for their lifetime (e.g., using std::unique_ptr<Item> if ContextMenu should own them, or documenting the caller's responsibility).
std::vector<Item*> items_; - Same ownership concern as for the constructor parameter. This needs careful consideration to avoid memory leaks or use-after-free issues.
class ContextMenuItem {}; - Defining an empty global class ContextMenuItem here is problematic. It should ideally be in a namespace (e.g., app) for consistency with ContextMenu and fully defined or clearly stated what its purpose is. The TODO indicates it will be moved, but shipping an empty global class is poor practice.
Consider moving #include <functional> to the .cc file if app::ContextMenu::Item (which previously used std::function) is now defined elsewhere and std::function is not directly used in this header.
class ContextMenuItem; - This forward declaration in the ui namespace seems unused. The actual ContextMenuItem is later declared globally. Clarify its purpose or remove it if not used.
Consider removing <unordered_set> if it is not used in this file. Unused includes can increase compile times.
If core::path::Path has an efficient move constructor, consider using pathsToMerge.push_back(std::move(it->second)); to avoid potentially expensive copies. However, ensure it->second is not accessed again from m_paths after being moved in this loop iteration. (Note: it->second refers to the original path in m_paths, so moving out would leave a moved-from state in the map. A copy is likely correct here, unless the merge operation could consume the original objects without modifying the map directly).
Ensure generateUniquePathId() has robust error handling or guarantees to always return a unique ID to prevent potential issues if all PathId values are exhausted.
Similar to line 427, if core::path::Path has an efficient move constructor, consider pathToSplit = std::move(it->second); if it->second is not needed from m_paths after this line (which it isn't, as pathId is erased later). This can avoid an unnecessary copy.
Consider providing a more detailed LOG_INFO message, e.g., "Path with ID {} could not be split further, it is already an atomic component." to clearly indicate the reason for not splitting.
Ensure generateUniquePathId() has robust error handling or guarantees to always return a unique ID.
The name setProperty is typically used for a slot or a method, not a signal. Consider renaming signals to reflect an event that has occurred or a request, e.g., propertySetRequested, requestSetProperty, or propertyValueToSet.
Clarify the distinction between this new setProperty signal and the existing propertyChanged(quint32 property, QVariant value) signal, as they have the same signature and conceptually similar meanings. If the purpose is to "set property value from outside" (as per the comment), is propertyChanged emitted for internal changes and this for external instructions, or is there another nuance?