Fixed the issue of incorrect requests to update a panel (#433)

* Fixed the issue of incorrect requests to update a panel - Added more checks to discard stopped and stale refresh requests. - Limited the length of the reload queue to two jobs. - Added more logging points. - Fixed #357 * Removed a duplicate field * clang-tidy
mikekazakov · Oct 17, 2024 · bfc0510 · bfc0510
1 parent 33c661d
commit bfc0510
Show file tree

Hide file tree

Showing 6 changed files with 131 additions and 40 deletions.
diff --git a/Source/NimbleCommander/NimbleCommander/States/FilePanels/PanelController.h b/Source/NimbleCommander/NimbleCommander/States/FilePanels/PanelController.h
@@ -7,6 +7,8 @@
 #include <Panel/PanelViewKeystrokeSink.h>
 #include <Panel/QuickSearch.h>
 #include <VFS/VFS.h>
+#include <fmt/format.h>
+#include <fmt/ranges.h>
 
 @class PanelController;
 @class PanelView;
@@ -240,3 +242,25 @@ using ContextMenuProvider = std::function<NSMenu *(std::vector<VFSListingItem> _
 @end
 
 #include "PanelController+DataAccess.h"
+
+template <>
+struct fmt::formatter<nc::panel::DirectoryChangeRequest> : fmt::formatter<std::string> {
+    constexpr auto parse(fmt::format_parse_context &ctx) { return ctx.begin(); }
+
+    template <typename FormatContext>
+    auto format(const nc::panel::DirectoryChangeRequest &_req, FormatContext &_ctx)
+    {
+
+        return fmt::format_to(
+            _ctx.out(),
+            "(RequestedDirectory='{}', VFS='{}', RequestFocusedEntry='{}', RequestSelectedEntries='{}', "
+            "PerformAsynchronous={}, LoadPreviousViewState={}, InitiatedByUser={})",
+            _req.RequestedDirectory,
+            _req.VFS->Tag(),
+            _req.RequestFocusedEntry,
+            _req.RequestSelectedEntries,
+            _req.PerformAsynchronous,
+            _req.LoadPreviousViewState,
+            _req.InitiatedByUser);
+    }
+};
diff --git a/Source/NimbleCommander/NimbleCommander/States/FilePanels/PanelController.mm b/Source/NimbleCommander/NimbleCommander/States/FilePanels/PanelController.mm
@@ -399,13 +399,23 @@ - (void)reloadRefreshedListing:(const VFSListingPtr &)_ptr
 
 - (void)refreshPanelDiscardingCaches:(bool)_force
 {
+    Log::Debug("refreshPanelDiscardingCaches:{} was called", _force);
+
     if( m_View == nil )
         return; // guard agains calls from init process
     if( &m_Data.Listing() == VFSListing::EmptyListing().get() )
         return; // guard agains calls from init process
 
-    if( !m_DirectoryLoadingQ.Empty() )
+    if( !m_DirectoryLoadingQ.Empty() ) {
+        Log::Debug("Discarding the refresh request as there is a load request in place");
+        return; // reducing overhead
+    }
+
+    if( m_DirectoryReLoadingQ.Length() >= 2 ) {
+        Log::Debug("Discarding the refresh request as the current length of reload queue is {}",
+                   m_DirectoryReLoadingQ.Length());
         return; // reducing overhead
+    }
 
     // later: maybe check PanelType somehow
 
@@ -415,15 +425,30 @@ - (void)refreshPanelDiscardingCaches:(bool)_force
         const auto vfs = self.vfs;
 
         m_DirectoryReLoadingQ.Run([=] {
-            if( m_DirectoryReLoadingQ.IsStopped() )
+            if( m_DirectoryReLoadingQ.IsStopped() ) {
+                Log::Trace("[PanelController refreshPanelDiscardingCaches] cancelled the refresh");
                 return;
+            }
             VFSListingPtr listing;
             const int ret = vfs->FetchDirectoryListing(
-                dirpath.c_str(), listing, fetch_flags, [&] { return m_DirectoryReLoadingQ.IsStopped(); });
-            if( ret >= 0 )
-                dispatch_to_main_queue([=] { [self reloadRefreshedListing:listing]; });
-            else
-                dispatch_to_main_queue([=] { [self recoverFromInvalidDirectory]; });
+                dirpath, listing, fetch_flags, [&] { return m_DirectoryReLoadingQ.IsStopped(); });
+            if( m_DirectoryReLoadingQ.IsStopped() ) {
+                Log::Trace("[PanelController refreshPanelDiscardingCaches] cancelled the refresh");
+                return;
+            }
+            dispatch_to_main_queue([=] {
+                if( self.currentDirectoryPath != dirpath ) {
+                    Log::Debug(
+                        "[PanelController refreshPanelDiscardingCaches]: discarding a stale request to refresh '{}'",
+                        dirpath);
+                    return;
+                }
+
+                if( ret >= 0 )
+                    [self reloadRefreshedListing:listing];
+                else
+                    [self recoverFromInvalidDirectory];
+            });
         });
     }
     else {
@@ -440,11 +465,13 @@ - (void)refreshPanelDiscardingCaches:(bool)_force
 
 - (void)refreshPanel
 {
+    Log::Trace("[Panel refreshPanel] was called");
     [self refreshPanelDiscardingCaches:false];
 }
 
 - (void)forceRefreshPanel
 {
+    Log::Trace("[Panel forceRefreshPanel] was called");
     [self refreshPanelDiscardingCaches:true];
 }
 
@@ -515,9 +542,8 @@ - (void)doCalculateSizesOfItems:(const std::vector<VFSListingItem> &)_items
             if( !i.IsDir() )
                 continue;
 
-            const auto result =
-                i.Host()->CalculateDirectorySize(!i.IsDotDot() ? i.Path().c_str() : i.Directory().c_str(),
-                                                 [=] { return m_DirectorySizeCountingQ.IsStopped(); });
+            const auto result = i.Host()->CalculateDirectorySize(!i.IsDotDot() ? i.Path() : i.Directory(),
+                                                                 [=] { return m_DirectorySizeCountingQ.IsStopped(); });
 
             if( result < 0 )
                 continue; // silently skip items that caused erros while calculating size
@@ -597,7 +623,7 @@ - (void)updateSpinningIndicator
 - (void)selectEntriesWithFilenames:(const std::vector<std::string> &)_filenames
 {
     for( auto &i : _filenames )
-        m_Data.CustomFlagsSelectSorted(m_Data.SortedIndexForName(i.c_str()), true);
+        m_Data.CustomFlagsSelectSorted(m_Data.SortedIndexForName(i), true);
     [m_View volatileDataChanged];
 }
 
@@ -617,15 +643,27 @@ - (void)setSelectionForItemAtIndex:(int)_index selected:(bool)_selected
 
 - (void)onPathChanged
 {
+    Log::Trace("[PanelController onPathChanged] was called");
     // update directory changes notification ticket
     __weak PanelController *weakself = self;
     m_UpdatesObservationTicket.reset();
     if( self.isUniform ) {
+        const std::string current_directory_path = self.currentDirectoryPath;
         auto dir_change_callback = [=] {
-            dispatch_to_main_queue([=] { [static_cast<PanelController *>(weakself) refreshPanel]; });
+            dispatch_to_main_queue([=] {
+                Log::Debug("Got a notification about a directory change: '{}'", current_directory_path);
+                if( PanelController *const pc = weakself ) {
+                    if( pc.currentDirectoryPath == current_directory_path ) {
+                        [pc refreshPanel];
+                    }
+                    else {
+                        Log::Debug("Discarded a stale directory change notification");
+                    }
+                }
+            });
         };
         m_UpdatesObservationTicket =
-            self.vfs->ObserveDirectoryChanges(self.currentDirectoryPath.c_str(), std::move(dir_change_callback));
+            self.vfs->ObserveDirectoryChanges(current_directory_path, std::move(dir_change_callback));
     }
 
     [self clearFocusingRequest];
@@ -728,7 +766,7 @@ - (void)requestQuickRenamingOfItem:(VFSListingItem)_item to:(const std::string &
     const auto &target_fn = _filename;
 
     // checking for invalid symbols
-    if( !_item.Host()->ValidateFilename(target_fn.c_str()) ) {
+    if( !_item.Host()->ValidateFilename(target_fn) ) {
         ShowAlertAboutInvalidFilename(target_fn);
         return;
     }
@@ -864,6 +902,10 @@ - (bool)probeDirectoryAccessForRequest:(DirectoryChangeRequest &)_request
 
 - (void)doGoToDirWithContext:(std::shared_ptr<DirectoryChangeRequest>)_request
 {
+    assert(_request != nullptr);
+    assert(_request->VFS != nullptr);
+    Log::Debug("[PanelController doGoToDirWithContext] was called with {}", *_request);
+
     try {
         if( [self probeDirectoryAccessForRequest:*_request] == false ) {
             _request->LoadingResultCode = VFSError::FromErrno(EPERM);
@@ -874,7 +916,7 @@ - (void)doGoToDirWithContext:(std::shared_ptr<DirectoryChangeRequest>)_request
         auto &vfs = *_request->VFS;
         const auto canceller = VFSCancelChecker([&] { return m_DirectoryLoadingQ.IsStopped(); });
         VFSListingPtr listing;
-        const auto fetch_result = vfs.FetchDirectoryListing(directory.c_str(), listing, m_VFSFetchingFlags, canceller);
+        const auto fetch_result = vfs.FetchDirectoryListing(directory, listing, m_VFSFetchingFlags, canceller);
         _request->LoadingResultCode = fetch_result;
         if( _request->LoadingResultCallback )
             _request->LoadingResultCallback(fetch_result);
@@ -889,7 +931,7 @@ - (void)doGoToDirWithContext:(std::shared_ptr<DirectoryChangeRequest>)_request
             [m_View savePathState];
             m_Data.Load(listing, data::Model::PanelType::Directory);
             for( auto &i : _request->RequestSelectedEntries )
-                m_Data.CustomFlagsSelectSorted(m_Data.SortedIndexForName(i.c_str()), true);
+                m_Data.CustomFlagsSelectSorted(m_Data.SortedIndexForName(i), true);
             m_DataGeneration++;
             [m_View dataUpdated];
             [m_View panelChangedWithFocusedFilename:_request->RequestFocusedEntry
@@ -912,6 +954,10 @@ - (int)GoToDirWithContext:(std::shared_ptr<DirectoryChangeRequest>)_request
         _request->VFS == nullptr )
         return VFSError::InvalidCall;
 
+    assert(_request != nullptr);
+    assert(_request->VFS != nullptr);
+    Log::Debug("[PanelController GoToDirWithContext] was called with {}", *_request);
+
     if( _request->PerformAsynchronous == false ) {
         assert(dispatch_is_main_queue());
         m_DirectoryLoadingQ.Stop();
@@ -955,7 +1001,7 @@ - (void)recoverFromInvalidDirectory
         const auto &vfs = initial_vfs;
 
         while( true ) {
-            if( vfs->IterateDirectoryListing(path.c_str(), [](const VFSDirEnt &) { return false; }) >= 0 ) {
+            if( vfs->IterateDirectoryListing(path.native(), [](const VFSDirEnt &) { return false; }) >= 0 ) {
                 dispatch_to_main_queue([=] {
                     auto request = std::make_shared<DirectoryChangeRequest>();
                     request->RequestedDirectory = path.native();
@@ -1113,6 +1159,7 @@ - (bool)isDoingBackgroundLoading
 
 - (void)hintAboutFilesystemChange
 {
+    Log::Trace("[PanelController hintAboutFilesystemChange] was called");
     dispatch_assert_main_queue(); // to preserve against fancy threading stuff
     if( self.receivesUpdateNotifications ) {
         // check in some future that the notification actually came

diff --git a/Source/Panel/include/Panel/CursorBackup.h b/Source/Panel/include/Panel/CursorBackup.h
@@ -1,4 +1,4 @@
-// Copyright (C) 2018-2021 Michael Kazakov. Subject to GNU General Public License version 3.
+// Copyright (C) 2018-2024 Michael Kazakov. Subject to GNU General Public License version 3.
 #pragma once
 
 #include <Panel/PanelDataExternalEntryKey.h>
@@ -20,8 +20,7 @@ class CursorBackup
     int FindRestoredCursorPosition() const noexcept;
 
     const data::Model &m_Data;
-    std::string m_OldCursorName;
-    data::ExternalEntryKey m_OldEntrySortKeys;
+    data::ExternalEntryKey m_Keys;
 };
 
 } // namespace nc::panel
diff --git a/Source/Panel/include/Panel/PanelDataExternalEntryKey.h b/Source/Panel/include/Panel/PanelDataExternalEntryKey.h
@@ -1,27 +1,52 @@
-// Copyright (C) 2017-2021 Michael Kazakov. Subject to GNU General Public License version 3.
+// Copyright (C) 2017-2024 Michael Kazakov. Subject to GNU General Public License version 3.
 #pragma once
 
 #include <VFS/VFS.h>
 #include <Base/CFPtr.h>
+#include <fmt/format.h>
 
 namespace nc::panel::data {
 
 struct ItemVolatileData;
 
 struct ExternalEntryKey {
-    ExternalEntryKey();
+    ExternalEntryKey() noexcept;
     ExternalEntryKey(const VFSListingItem &_item, const ItemVolatileData &_item_vd);
 
     std::string name;
     std::string extension;
     nc::base::CFPtr<CFStringRef> display_name;
-    uint64_t size;
-    time_t mtime;
-    time_t btime;
-    time_t atime;
+    uint64_t size = 0;
+    time_t mtime = 0;
+    time_t btime = 0;
+    time_t atime = 0;
     time_t add_time; // -1 means absent
-    bool is_dir;
+    bool is_dir = false;
     bool is_valid() const noexcept;
 };
 
 } // namespace nc::panel::data
+
+template <>
+struct fmt::formatter<nc::panel::data::ExternalEntryKey> : fmt::formatter<std::string> {
+    constexpr auto parse(fmt::format_parse_context &ctx) { return ctx.begin(); }
+
+    template <typename FormatContext>
+    auto format(const nc::panel::data::ExternalEntryKey &_key, FormatContext &_ctx)
+    {
+
+        return fmt::format_to(_ctx.out(),
+                              "(name='{}', extension='{}', display='{}', size={}, directory={}, mtime={}, btime={}, "
+                              "atime={}, addtime={})",
+                              _key.name,
+                              _key.extension,
+                              _key.display_name ? nc::base::CFStringGetUTF8StdString(_key.display_name.get())
+                                                : std::string{},
+                              _key.size,
+                              _key.is_dir,
+                              _key.mtime,
+                              _key.btime,
+                              _key.atime,
+                              _key.add_time);
+    }
+};
diff --git a/Source/Panel/source/CursorBackup.mm b/Source/Panel/source/CursorBackup.mm
@@ -1,4 +1,4 @@
-// Copyright (C) 2018-2021 Michael Kazakov. Subject to GNU General Public License version 3.
+// Copyright (C) 2018-2024 Michael Kazakov. Subject to GNU General Public License version 3.
 #include "CursorBackup.h"
 #include <Panel/PanelData.h>
 #include <Panel/Log.h>
@@ -10,27 +10,26 @@
     Log::Trace("Saving cursor position: {}", _current_cursor_pos);
     if( _current_cursor_pos >= 0 ) {
         assert(_current_cursor_pos < _data.SortedEntriesCount());
-        auto item = _data.EntryAtSortPosition(_current_cursor_pos);
-        assert(item);
-        m_OldCursorName = item.Filename();
-        m_OldEntrySortKeys = _data.EntrySortKeysAtSortPosition(_current_cursor_pos);
+        m_Keys = _data.EntrySortKeysAtSortPosition(_current_cursor_pos);
+        Log::Trace("Saved sort keys: {}", m_Keys);
     }
 }
 
 int CursorBackup::RestoredCursorPosition() const noexcept
 {
+    Log::Trace("Restoring cursor position from the keys {}", m_Keys);
     const int restored_pos = FindRestoredCursorPosition();
     Log::Trace("Restored cursor position: {}", restored_pos);
     return restored_pos;
 }
 
 int CursorBackup::FindRestoredCursorPosition() const noexcept
 {
-    if( m_OldCursorName.empty() ) {
+    if( m_Keys.name.empty() ) {
         return m_Data.SortedEntriesCount() > 0 ? 0 : -1;
     }
 
-    const auto new_cursor_raw_pos = m_Data.RawIndexForName(m_OldCursorName.c_str());
+    const auto new_cursor_raw_pos = m_Data.RawIndexForName(m_Keys.name);
     if( new_cursor_raw_pos >= 0 ) {
         const auto new_cursor_sort_pos = m_Data.SortedIndexForRawIndex(new_cursor_raw_pos);
         if( new_cursor_sort_pos >= 0 )
@@ -39,7 +38,7 @@
             return m_Data.SortedDirectoryEntries().empty() ? -1 : 0;
     }
     else {
-        const auto lower_bound_ind = m_Data.SortLowerBoundForEntrySortKeys(m_OldEntrySortKeys);
+        const auto lower_bound_ind = m_Data.SortLowerBoundForEntrySortKeys(m_Keys);
         if( lower_bound_ind >= 0 ) {
             return lower_bound_ind;
         }

diff --git a/Source/Panel/source/PanelDataExternalEntryKey.cpp b/Source/Panel/source/PanelDataExternalEntryKey.cpp
@@ -1,13 +1,10 @@
-// Copyright (C) 2017-2021 Michael Kazakov. Subject to GNU General Public License version 3.
+// Copyright (C) 2017-2024 Michael Kazakov. Subject to GNU General Public License version 3.
 #include "PanelDataExternalEntryKey.h"
 #include "PanelDataItemVolatileData.h"
 
 namespace nc::panel::data {
 
-ExternalEntryKey::ExternalEntryKey()
-    : name{""}, extension{""}, display_name{}, size{0}, mtime{0}, btime{0}, atime{0}, add_time{-1}, is_dir{false}
-{
-}
+ExternalEntryKey::ExternalEntryKey() noexcept = default;
 
 ExternalEntryKey::ExternalEntryKey(const VFSListingItem &_item, const ItemVolatileData &_item_vd) : ExternalEntryKey()
 {