Fixed an issue of invalid placement of external tools on the toolbar (#…

…390)

* Refactoring

* External tools now have uuids, toolbar uses them to identify elements

* Generate unique names for new tools, selecting the same tool after drag&drop

* ExternalToolsStorage enforces that UUIDs are unique

* Fixed the incorrect button height. fixed #378

Source/Base/include/Base/UUID.h

@@ -1,16 +1,4 @@
-/* Copyright (c) 2023 Michael G. Kazakov
- * Permission is hereby granted, free of charge, to any person obtaining a copy of this software
- * and associated documentation files (the "Software"), to deal in the Software without restriction,
- * including without limitation the rights to use, copy, modify, merge, publish, distribute,
- * sublicense, and/or sell copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- * The above copyright notice and this permission notice shall be included in all copies or
- * substantial portions of the Software.
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING
- * BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
- * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
- * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
- * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. */
+// Copyright (C) 2023-2024 Michael Kazakov. Subject to GNU General Public License version 3.
 #pragma once
 #include <array>
 #include <cstdint>
@@ -26,6 +14,7 @@ class UUID
 public:
     UUID() noexcept;
     std::string ToString() const noexcept;
+    size_t Hash() const noexcept;
     static UUID Generate() noexcept;
     static std::optional<UUID> FromString(std::string_view _str) noexcept;
     constexpr bool operator==(const UUID &) const noexcept = default;
@@ -35,3 +24,8 @@ class UUID
 };
 
 } // namespace nc::base
+
+template <>
+struct std::hash<nc::base::UUID> {
+    size_t operator()(const nc::base::UUID &u) const noexcept;
+};

Source/Base/source/UUID.cpp

@@ -1,19 +1,8 @@
-/* Copyright (c) 2023 Michael G. Kazakov
- * Permission is hereby granted, free of charge, to any person obtaining a copy of this software
- * and associated documentation files (the "Software"), to deal in the Software without restriction,
- * including without limitation the rights to use, copy, modify, merge, publish, distribute,
- * sublicense, and/or sell copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- * The above copyright notice and this permission notice shall be included in all copies or
- * substantial portions of the Software.
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING
- * BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
- * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
- * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
- * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. */
+// Copyright (C) 2023-2024 Michael Kazakov. Subject to GNU General Public License version 3.
 #include "UUID.h"
 #include <uuid/uuid.h>
 #include <algorithm>
+#include <UnorderedUtil.h>
 
 namespace nc::base {
 
@@ -58,4 +47,15 @@ std::optional<UUID> UUID::FromString(std::string_view _str) noexcept
     return u;
 }
 
+size_t UUID::Hash() const noexcept
+{
+    const std::string_view v(reinterpret_cast<const char *>(m_Data.data()), m_Data.size());
+    return ankerl::unordered_dense::hash<std::string_view>{}(v);
+}
+
 } // namespace nc::base
+
+size_t std::hash<nc::base::UUID>::operator()(const nc::base::UUID &u) const noexcept
+{
+    return u.Hash();
+}

Source/NimbleCommander/NimbleCommander/Preferences/PreferencesWindowToolsTab.mm

@@ -129,8 +129,7 @@ - (NSView *)tableView:(NSTableView *) [[maybe_unused]] tableView
     auto &tool = m_Tools[row];
 
     NSTextField *tf = [[NSTextField alloc] initWithFrame:NSMakeRect(0, 0, 0, 0)];
-    tf.stringValue = tool->m_Title.empty() ? [NSString stringWithFormat:@"Tool #%ld", row]
-                                           : [NSString stringWithUTF8StdString:tool->m_Title];
+    tf.stringValue = [NSString stringWithUTF8StdString:tool->m_Title];
     tf.bordered = false;
     tf.editable = false;
     tf.drawsBackground = false;
@@ -226,11 +225,15 @@ - (IBAction)onPlusMinusButton:(id) [[maybe_unused]] sender
 {
     const auto segment = self.toolsAddRemove.selectedSegment;
     if( segment == 0 ) {
-        m_ToolsStorage().InsertTool(ExternalTool());
+        ExternalTool new_tool;
+        new_tool.m_UUID = nc::base::UUID::Generate();
+        new_tool.m_Title = m_ToolsStorage().NewTitle();
+        m_ToolsStorage().InsertTool(new_tool);
         dispatch_to_main_queue_after(10ms, [=] {
-            if( self.toolsTable.numberOfRows > 0 ) {
-                [self.toolsTable selectRowIndexes:[NSIndexSet indexSetWithIndex:self.toolsTable.numberOfRows - 1]
-                             byExtendingSelection:false];
+            if( const long rows = self.toolsTable.numberOfRows; rows > 0 ) {
+                const long new_row = rows - 1;
+                [self.toolsTable selectRowIndexes:[NSIndexSet indexSetWithIndex:new_row] byExtendingSelection:false];
+                [self.toolsTable scrollRowToVisible:new_row];
                 [self.view.window makeFirstResponder:self.toolTitle];
             }
         });
@@ -373,11 +376,17 @@ - (BOOL)tableView:(NSTableView *) [[maybe_unused]] aTableView
         drag_to == drag_from + 1 ) // same index, below
         return false;
 
+    const bool selected = [self.toolsTable isRowSelected:drag_from];
+
     if( drag_from < drag_to )
         drag_to--;
 
     m_ToolsStorage().MoveTool(drag_from, drag_to);
 
+    if( selected ) {
+        [self.toolsTable selectRowIndexes:[NSIndexSet indexSetWithIndex:drag_to] byExtendingSelection:false];
+    }
+
     return true;
 }
 

Source/NimbleCommander/NimbleCommander/States/FilePanels/MainWindowFilePanelState.h

@@ -72,7 +72,6 @@ struct MainWindowFilePanelState_OverlappedTerminalSupport;
 
 @property(nonatomic, readonly) NCMainWindowController *mainWindowController;
 @property(nonatomic, readonly) FilePanelMainSplitView *splitView;
-@property(nonatomic, readonly) nc::panel::ExternalToolsStorage &externalToolsStorage;
 @property(nonatomic, readonly) nc::ops::Pool &operationsPool;
 @property(nonatomic, readonly) bool isPanelActive;
 @property(nonatomic, readonly) bool goToForcesPanelActivation;

Source/NimbleCommander/NimbleCommander/States/FilePanels/MainWindowFilePanelState.mm

@@ -275,7 +275,9 @@ - (void)CreateControls
     m_SeparatorLine.borderColor = nc::CurrentTheme().FilePanelsGeneralTopSeparatorColor();
     [self addSubview:m_SeparatorLine];
 
-    m_ToolbarDelegate = [[MainWindowFilePanelsStateToolbarDelegate alloc] initWithFilePanelsState:self];
+    m_ToolbarDelegate =
+        [[MainWindowFilePanelsStateToolbarDelegate alloc] initWithToolsStorage:NCAppDelegate.me.externalTools
+                                                             andOperationsPool:self.operationsPool];
 
     auto views = NSDictionaryOfVariableBindings(m_SeparatorLine, m_SplitView);
     auto contraints = {@"V:|-(==0@250)-[m_SeparatorLine(==1)]-(==0)-[m_SplitView(>=150@500)]",
@@ -933,11 +935,6 @@ - (void)addNewControllerOnRightPane:(PanelController *)_pc
     [m_SplitView.rightTabbedHolder addPanel:_pc.view];
 }
 
-- (ExternalToolsStorage &)externalToolsStorage
-{
-    return NCAppDelegate.me.externalTools;
-}
-
 - (void)revealPanel:(PanelController *)panel
 {
     if( [self isRightController:panel] ) {

Source/NimbleCommander/NimbleCommander/States/FilePanels/MainWindowFilePanelsStateToolbarDelegate.h

@@ -1,12 +1,19 @@
-// Copyright (C) 2016-2021 Michael Kazakov. Subject to GNU General Public License version 3.
+// Copyright (C) 2016-2024 Michael Kazakov. Subject to GNU General Public License version 3.
 #pragma once
 
-@class MainWindowFilePanelState;
 @class NCOpsPoolViewController;
 
+namespace nc::panel {
+class ExternalToolsStorage;
+}
+namespace nc::ops {
+class Pool;
+}
+
 @interface MainWindowFilePanelsStateToolbarDelegate : NSObject <NSToolbarDelegate>
 
-- (instancetype)initWithFilePanelsState:(MainWindowFilePanelState *)_state;
+- (instancetype)initWithToolsStorage:(nc::panel::ExternalToolsStorage &)_storage
+                   andOperationsPool:(nc::ops::Pool &)_pool;
 
 @property(nonatomic, readonly) NSToolbar *toolbar;
 @property(nonatomic, readonly) NSButton *leftPanelGoToButton;

Source/NimbleCommander/NimbleCommander/States/FilePanels/MainWindowFilePanelsStateToolbarDelegate.mm

@@ -3,7 +3,6 @@
 #include "StateActionsDispatcher.h"
 #include "../../Core/ActionsShortcutsManager.h"
 #include "MainWindowFilePanelsStateToolbarDelegate.h"
-#include "StateActionsDispatcher.h"
 #include <Operations/PoolViewController.h>
 #include "Actions/ExecuteExternalTool.h"
 #include <NimbleCommander/Core/AnyHolder.h>
@@ -13,19 +12,16 @@
 #include <Utility/StringExtras.h>
 #include <Utility/ObjCpp.h>
 #include <deque>
+#include <algorithm>
+#include <fmt/format.h>
+
+using nc::panel::ExternalTool;
 
 // do not change these strings, they are used for persistency in NSUserDefaults
 static auto g_ToolbarIdentifier = @"FilePanelsToolbar";
-static auto g_ExternalToolsIdentifiersPrefix = @"external_tool_";
-
-@interface MainWindowFilePanelsStateToolbarDelegate ()
-
-@property(nonatomic, readonly) MainWindowFilePanelState *state;
-
-@end
+static std::string_view g_ExternalToolsIdentifiersPrefix = "external_tool_";
 
 @implementation MainWindowFilePanelsStateToolbarDelegate {
-    __weak MainWindowFilePanelState *m_State;
     NSToolbar *m_Toolbar;
     NSButton *m_LeftPanelGoToButton;
     NSButton *m_RightPanelGoToButton;
@@ -34,6 +30,7 @@ @implementation MainWindowFilePanelsStateToolbarDelegate {
     NSToolbarItem *m_PoolViewToolbarItem;
 
     NSArray *m_AllowedToolbarItemsIdentifiers;
+    nc::panel::ExternalToolsStorage *m_Storage;
     nc::panel::ExternalToolsStorage::ObservationTicket m_ToolsChangesTicket;
 
     id m_RepresentedObject;
@@ -44,34 +41,29 @@ @implementation MainWindowFilePanelsStateToolbarDelegate {
 @synthesize rightPanelGoToButton = m_RightPanelGoToButton;
 @synthesize operationsPoolViewController = m_PoolViewController;
 
-- (instancetype)initWithFilePanelsState:(MainWindowFilePanelState *)_state
+- (instancetype)initWithToolsStorage:(nc::panel::ExternalToolsStorage &)_storage
+                   andOperationsPool:(nc::ops::Pool &)_pool
 {
-    assert(_state != nil);
     self = [super init];
     if( self ) {
-        m_State = _state;
+        m_Storage = &_storage;
 
         [self buildBasicControls];
         [self buildToolbar];
         [self buildAllowedIdentifiers];
 
         __weak MainWindowFilePanelsStateToolbarDelegate *weak_self = self;
-        m_ToolsChangesTicket = _state.externalToolsStorage.ObserveChanges([=] {
+        m_ToolsChangesTicket = m_Storage->ObserveChanges([=] {
             dispatch_to_main_queue(
                 [=] { [static_cast<MainWindowFilePanelsStateToolbarDelegate *>(weak_self) externalToolsChanged]; });
         });
 
-        m_PoolViewController = [[NCOpsPoolViewController alloc] initWithPool:self.state.operationsPool];
+        m_PoolViewController = [[NCOpsPoolViewController alloc] initWithPool:_pool];
         [m_PoolViewController loadView];
     }
     return self;
 }
 
-- (MainWindowFilePanelState *)state
-{
-    return static_cast<MainWindowFilePanelState *>(m_State);
-}
-
 - (void)buildBasicControls
 {
     m_LeftPanelGoToButton = [[NSButton alloc] initWithFrame:NSMakeRect(0, 0, 42, 27)];
@@ -136,15 +128,32 @@ - (void)buildToolbar
     return img;
 }
 
-- (void)setupExternalToolItem:(NSToolbarItem *)_item forTool:(const nc::panel::ExternalTool &)_et no:(int)_no
+static NSString *EncodeToolIdentifier(const ExternalTool &_et)
+{
+    const std::string identifier = fmt::format("{}{}", g_ExternalToolsIdentifiersPrefix, _et.m_UUID.ToString());
+    return [NSString stringWithUTF8StdString:identifier];
+}
+
+- (std::shared_ptr<const ExternalTool>)findToolWithIdentifier:(NSString *)_identifier
+{
+    std::string_view identifier = _identifier.UTF8String;
+    if( identifier.starts_with(g_ExternalToolsIdentifiersPrefix) ) {
+        identifier.remove_prefix(g_ExternalToolsIdentifiersPrefix.length());
+        if( const auto uuid = nc::base::UUID::FromString(identifier) ) {
+            return m_Storage->GetTool(uuid.value());
+        }
+    }
+    return nullptr;
+}
+
+- (void)setupExternalToolItem:(NSToolbarItem *)_item forTool:(const nc::panel::ExternalTool &)_et
 {
     const auto title = [NSString stringWithUTF8StdString:_et.m_Title];
     _item.image = ImageForTool(_et);
     _item.label = title;
     _item.paletteLabel = title;
     _item.target = self;
     _item.action = @selector(onExternalToolAction:);
-    _item.tag = _no;
     _item.toolTip = [&] {
         const auto hotkey = _et.m_Shorcut.PrettyString();
         if( hotkey.length == 0 )
@@ -180,13 +189,10 @@ - (NSToolbarItem *)toolbar:(NSToolbar *) [[maybe_unused]] _toolbar
         m_PoolViewToolbarItem = item;
         return item;
     }
-    if( [itemIdentifier hasPrefix:g_ExternalToolsIdentifiersPrefix] ) {
-        const int n = atoi(itemIdentifier.UTF8String + g_ExternalToolsIdentifiersPrefix.length);
-        if( const auto tool = self.state.externalToolsStorage.GetTool(n) ) {
-            NSToolbarItem *item = [[NSToolbarItem alloc] initWithItemIdentifier:itemIdentifier];
-            [self setupExternalToolItem:item forTool:*tool no:n];
-            return item;
-        }
+    if( const auto tool = [self findToolWithIdentifier:itemIdentifier] ) {
+        NSToolbarItem *item = [[NSToolbarItem alloc] initWithItemIdentifier:itemIdentifier];
+        [self setupExternalToolItem:item forTool:*tool];
+        return item;
     }
 
     return nil;
@@ -200,7 +206,7 @@ - (id)representedObject
 - (IBAction)onExternalToolAction:(id)sender
 {
     if( auto i = nc::objc_cast<NSToolbarItem>(sender) )
-        if( auto tool = self.state.externalToolsStorage.GetTool(i.tag) ) {
+        if( auto tool = m_Storage->GetTool(i.tag) ) {
             m_RepresentedObject = [[AnyHolder alloc] initWithAny:std::any{tool}];
             [NSApp sendAction:@selector(onExecuteExternalTool:) to:nil from:self];
             m_RepresentedObject = nil;
@@ -234,9 +240,10 @@ - (void)buildAllowedIdentifiers
     [a addObject:@"filepanels_right_goto_button"];
     [a addObject:@"operations_pool"];
 
-    auto tools = m_State.externalToolsStorage.GetAllTools();
-    for( int i = 0, e = static_cast<int>(tools.size()); i != e; ++i )
-        [a addObject:[NSString stringWithFormat:@"%@%d", g_ExternalToolsIdentifiersPrefix, i]];
+    const std::vector<std::shared_ptr<const ExternalTool>> tools = m_Storage->GetAllTools();
+    for( const auto &tool : tools ) {
+        [a addObject:EncodeToolIdentifier(*tool)];
+    }
 
     [a addObject:NSToolbarFlexibleSpaceItemIdentifier];
     [a addObject:NSToolbarSpaceItemIdentifier];
@@ -247,20 +254,22 @@ - (void)buildAllowedIdentifiers
 - (void)externalToolsChanged
 {
     dispatch_assert_main_queue();
-    std::deque<int> to_remove;
+    std::vector<int> to_remove;
     for( NSToolbarItem *i in m_Toolbar.items ) {
-        if( [i.itemIdentifier hasPrefix:g_ExternalToolsIdentifiersPrefix] ) {
-            const int n = atoi(i.itemIdentifier.UTF8String + g_ExternalToolsIdentifiersPrefix.length);
-            if( const auto tool = self.state.externalToolsStorage.GetTool(n) ) {
-                [self setupExternalToolItem:i forTool:*tool no:n];
+        //        if( [i.itemIdentifier hasPrefix:g_ExternalToolsIdentifiersPrefix] ) {
+        if( std::string_view(i.itemIdentifier.UTF8String).starts_with(g_ExternalToolsIdentifiersPrefix) ) {
+            //            const int n = atoi(i.itemIdentifier.UTF8String + g_ExternalToolsIdentifiersPrefix.length);
+            if( const auto tool = [self findToolWithIdentifier:i.itemIdentifier] ) {
+                [self setupExternalToolItem:i forTool:*tool];
             }
             else
-                to_remove.push_front(static_cast<int>([m_Toolbar.items indexOfObject:i]));
+                to_remove.push_back(static_cast<int>([m_Toolbar.items indexOfObject:i]));
         }
     }
 
     // this will immediately trigger removing of same elements from other windows' toolbars.
     // this is intended and should work fine.
+    std::ranges::reverse(to_remove);
     for( auto i : to_remove )
         [m_Toolbar removeItemAtIndex:i];