Prevent reentrancy in footprint loading.

Also removes parallel implementation in favour of using the
normal one (with a new wxGauge-backed PROGRESS_REPORTER).

Fixes: lp:1764196
* https://bugs.launchpad.net/kicad/+bug/1764196

common/footprint_info.cpp

@@ -181,29 +181,6 @@ bool FOOTPRINT_ASYNC_LOADER::Join()
 }
 
 
-int FOOTPRINT_ASYNC_LOADER::GetProgress() const
-{
-    if( !m_started )
-        return 0;
-    else if( m_total_libs == 0 || !m_list )
-        return 100;
-    else
-    {
-        int loaded = m_list->CountFinished();
-        int prog = ( 100 * loaded ) / m_total_libs;
-
-        if( loaded == m_total_libs )
-            return 100;
-        else if( loaded < m_total_libs && prog >= 100 )
-            return 99;
-        else if( prog <= 0 )
-            return 1;
-        else
-            return prog;
-    }
-}
-
-
 void FOOTPRINT_ASYNC_LOADER::SetCompletionCallback( std::function<void()> aCallback )
 {
     m_completion_cb = std::move(aCallback);

common/widgets/footprint_select_widget.cpp

@@ -34,7 +34,7 @@
 #include <wx/timer.h>
 #include <wx/utils.h>
 #include <wx/wupdlock.h>
-
+#include <widgets/progress_reporter.h>
 
 /**
  * Fixed positions for standard items in the list
@@ -73,9 +73,8 @@ FOOTPRINT_SELECT_WIDGET::FOOTPRINT_SELECT_WIDGET( wxWindow* aParent,
 {
     m_zero_filter = true;
     m_sizer = new wxBoxSizer( wxVERTICAL );
-    m_progress_timer = std::make_unique<wxTimer>( this );
     m_book = new wxSimplebook( this, wxID_ANY );
-    m_progress_ctrl = new wxGauge( m_book, wxID_ANY, 100 );
+    m_progress_ctrl = new GAUGE_PROGRESS_REPORTER( m_book, 2 );
     m_fp_sel_ctrl = new FOOTPRINT_CHOICE( m_book, wxID_ANY );
 
     m_book->SetEffect( wxSHOW_EFFECT_BLEND );
@@ -87,7 +86,6 @@ FOOTPRINT_SELECT_WIDGET::FOOTPRINT_SELECT_WIDGET( wxWindow* aParent,
     Layout();
     m_sizer->Fit( this );
 
-    Bind( wxEVT_TIMER, &FOOTPRINT_SELECT_WIDGET::OnProgressTimer, this, m_progress_timer->GetId() );
     m_fp_sel_ctrl->Bind( wxEVT_COMBOBOX, &FOOTPRINT_SELECT_WIDGET::OnComboBox, this );
     m_fp_sel_ctrl->Bind(
             EVT_INTERACTIVE_CHOICE, &FOOTPRINT_SELECT_WIDGET::OnComboInteractive, this );
@@ -103,14 +101,7 @@ void FOOTPRINT_SELECT_WIDGET::Load( KIWAY& aKiway, PROJECT& aProject )
         auto fp_lib_table = aProject.PcbFootprintLibs( aKiway );
         m_fp_list = FOOTPRINT_LIST::GetInstance( aKiway );
 
-        if( m_fp_list->RequiresLoading( fp_lib_table ) )
+        m_fp_list->ReadFootprintFiles( fp_lib_table, nullptr, m_progress_ctrl );
-        {
-            m_fp_loader.SetList( &*m_fp_list );
-            m_fp_loader.Start( fp_lib_table );
-
-            m_progress_timer->Start( 200 );
-        }
-        else
         FootprintsLoaded();
     }
     catch( ... )
@@ -120,27 +111,8 @@ void FOOTPRINT_SELECT_WIDGET::Load( KIWAY& aKiway, PROJECT& aProject )
 }
 
 
-void FOOTPRINT_SELECT_WIDGET::OnProgressTimer( wxTimerEvent& aEvent )
-{
-    int prog = m_fp_loader.GetProgress();
-    m_progress_ctrl->SetValue( prog );
-
-    if( prog == 100 )
-    {
-        wxBusyCursor busy;
-
-        m_fp_loader.Join();
-        m_progress_timer->Stop();
-
-        FootprintsLoaded();
-    }
-}
-
-
 void FOOTPRINT_SELECT_WIDGET::FootprintsLoaded()
 {
-    m_progress_ctrl->SetValue( 100 );
-
     m_fp_filter.SetList( *m_fp_list );
 
     m_book->SetSelection( PAGE_SELECT );

common/widgets/progress_reporter.cpp

@@ -23,7 +23,7 @@
  */
 
 #include <widgets/progress_reporter.h>
-
+#include <wx/evtloop.h>
 #include <thread>
 
 PROGRESS_REPORTER::PROGRESS_REPORTER( int aNumPhases ) :
@@ -138,3 +138,26 @@ bool WX_PROGRESS_REPORTER::updateUI()
 }
 
 
+GAUGE_PROGRESS_REPORTER::GAUGE_PROGRESS_REPORTER( wxWindow* aParent, int aNumPhases ) :
+        PROGRESS_REPORTER( aNumPhases ),
+        wxGauge( aParent, wxID_ANY, 1000, wxDefaultPosition, wxDefaultSize,
+                 wxGA_HORIZONTAL, wxDefaultValidator, wxGaugeNameStr )
+{
+}
+
+
+bool GAUGE_PROGRESS_REPORTER::updateUI()
+{
+    int cur = currentProgress();
+
+    if( cur < 0 || cur > 1000 )
+        cur = 0;
+
+    wxGauge::SetValue( cur );
+    wxEventLoopBase::GetActive()->YieldFor(wxEVT_CATEGORY_UI);
+
+    return true;  // No cancel button on a wxGauge
+}
+
+
+

eeschema/dialogs/dialog_choose_component.cpp

@@ -48,6 +48,9 @@
 wxSize DIALOG_CHOOSE_COMPONENT::m_last_dlg_size( -1, -1 );
 int DIALOG_CHOOSE_COMPONENT::m_tree_canvas_sash_position = 0;
 
+std::mutex DIALOG_CHOOSE_COMPONENT::g_Mutex;
+
+
 DIALOG_CHOOSE_COMPONENT::DIALOG_CHOOSE_COMPONENT( SCH_BASE_FRAME* aParent, const wxString& aTitle,
         CMP_TREE_MODEL_ADAPTER::PTR& aAdapter, int aDeMorganConvert, bool aAllowFieldEdits,
         bool aShowFootprints )
@@ -59,10 +62,9 @@ DIALOG_CHOOSE_COMPONENT::DIALOG_CHOOSE_COMPONENT( SCH_BASE_FRAME* aParent, const
           m_deMorganConvert( aDeMorganConvert >= 0 ? aDeMorganConvert : 0 ),
           m_allow_field_edits( aAllowFieldEdits ),
           m_show_footprints( aShowFootprints ),
+          m_load_footprints( aShowFootprints ),
           m_external_browser_requested( false )
 {
-    wxBusyCursor busy_while_loading;
-
     m_fp_list = FOOTPRINT_LIST::GetInstance( Kiway() );
 
     auto sizer = new wxBoxSizer( wxVERTICAL );
@@ -98,6 +100,7 @@ DIALOG_CHOOSE_COMPONENT::DIALOG_CHOOSE_COMPONENT( SCH_BASE_FRAME* aParent, const
     SetSizer( sizer );
 
     Bind( wxEVT_INIT_DIALOG, &DIALOG_CHOOSE_COMPONENT::OnInitDialog, this );
+    Bind( wxEVT_IDLE, &DIALOG_CHOOSE_COMPONENT::OnIdle, this );
     Bind( wxEVT_TIMER, &DIALOG_CHOOSE_COMPONENT::OnCloseTimer, this, m_dbl_click_timer->GetId() );
     Bind( COMPONENT_PRESELECTED, &DIALOG_CHOOSE_COMPONENT::OnComponentPreselected, this );
     Bind( COMPONENT_SELECTED, &DIALOG_CHOOSE_COMPONENT::OnComponentSelected, this );
@@ -106,8 +109,7 @@ DIALOG_CHOOSE_COMPONENT::DIALOG_CHOOSE_COMPONENT( SCH_BASE_FRAME* aParent, const
     m_sch_view_ctrl->Bind( wxEVT_PAINT, &DIALOG_CHOOSE_COMPONENT::OnSchViewPaint, this );
 
     if( m_fp_sel_ctrl )
-        m_fp_sel_ctrl->Bind(
+        m_fp_sel_ctrl->Bind( EVT_FOOTPRINT_SELECTED, &DIALOG_CHOOSE_COMPONENT::OnFootprintSelected, this );
-                EVT_FOOTPRINT_SELECTED, &DIALOG_CHOOSE_COMPONENT::OnFootprintSelected, this );
 
     Layout();
 
@@ -192,9 +194,17 @@ void DIALOG_CHOOSE_COMPONENT::OnInitDialog( wxInitDialogEvent& aEvent )
         // This hides the GAL panel and shows the status label
         m_fp_view_ctrl->SetStatusText( wxEmptyString );
     }
+}
 
-    if( m_fp_sel_ctrl )
+
+// Let the dialog display before starting the footprint load
+void DIALOG_CHOOSE_COMPONENT::OnIdle( wxIdleEvent& aEvent )
+{
+    if( m_load_footprints && m_fp_sel_ctrl )
+    {
+        m_load_footprints = false;
         m_fp_sel_ctrl->Load( Kiway(), Prj() );
+    }
 }
 
 

eeschema/dialogs/dialog_choose_component.h

@@ -140,6 +140,8 @@ public:
         return m_external_browser_requested;
     }
 
+    static std::mutex g_Mutex;
+
 protected:
     static constexpr int DblClickDelay = 100; // milliseconds
 
@@ -147,7 +149,7 @@ protected:
 
     void OnInitDialog( wxInitDialogEvent& aEvent );
     void OnCloseTimer( wxTimerEvent& aEvent );
-    void OnProgressTimer( wxTimerEvent& aEvent );
+    void OnIdle( wxIdleEvent& aEvent );
 
     void OnSchViewDClick( wxMouseEvent& aEvent );
     void OnSchViewPaint( wxPaintEvent& aEvent );
@@ -205,6 +207,7 @@ protected:
     int                         m_deMorganConvert;
     bool                        m_allow_field_edits;
     bool                        m_show_footprints;
+    bool                        m_load_footprints;
     bool                        m_external_browser_requested;
     wxString                    m_fp_override;
 

eeschema/getpart.cpp

@@ -109,9 +109,14 @@ SCH_BASE_FRAME::COMPONENT_SELECTION SCH_BASE_FRAME::SelectComponentFromLibrary(
         const LIB_ID*                       aHighlight,
         bool                                aAllowFields )
 {
+    std::unique_lock<std::mutex> dialogLock( DIALOG_CHOOSE_COMPONENT::g_Mutex, std::defer_lock );
     wxString                     dialogTitle;
     SYMBOL_LIB_TABLE*            libs = Prj().SchSymbolLibTable();
 
+    // One CHOOSE_COMPONENT dialog at a time.  User probaby can't handle more anyway.
+    if( !dialogLock.try_lock() )
+        return COMPONENT_SELECTION();
+
     auto adapter( CMP_TREE_MODEL_ADAPTER::Create( libs ) );
     bool loaded = false;
 

eeschema/viewlibs.cpp

@@ -43,9 +43,14 @@
 
 void LIB_VIEW_FRAME::OnSelectSymbol( wxCommandEvent& aEvent )
 {
+    std::unique_lock<std::mutex> dialogLock( DIALOG_CHOOSE_COMPONENT::g_Mutex, std::defer_lock );
     wxString                     dialogTitle;
     SYMBOL_LIB_TABLE*            libs = Prj().SchSymbolLibTable();
 
+    // One CHOOSE_COMPONENT dialog at a time.  User probaby can't handle more anyway.
+    if( !dialogLock.try_lock() )
+        return;
+
     // Container doing search-as-you-type.
     auto adapter( CMP_TREE_MODEL_ADAPTER::Create( libs ) );
 

include/footprint_info.h

@@ -50,7 +50,7 @@ class FP_LIB_TABLE;
 class FOOTPRINT_LIST;
 class FOOTPRINT_LIST_IMPL;
 class FOOTPRINT_ASYNC_LOADER;
-class WX_PROGRESS_REPORTER;
+class PROGRESS_REPORTER;
 class wxTopLevelWindow;
 class KIWAY;
 
@@ -242,15 +242,6 @@ public:
         return error;
     }
 
-    /**
-     * Indicates whether or not the table requires loading for the given \a aNickname.
-     */
-    virtual bool RequiresLoading( FP_LIB_TABLE* aTable, const wxString* aNickname = nullptr )
-    {
-        return true;    // Implementations which support caching should implement this.
-    }
-
-
     /**
      * Read all the footprints provided by the combination of aTable and aNickname.
      *
@@ -264,7 +255,7 @@ public:
      *  GetErrorCount() for that, should be zero to indicate success.
      */
     virtual bool ReadFootprintFiles( FP_LIB_TABLE* aTable, const wxString* aNickname = nullptr,
-                                     WX_PROGRESS_REPORTER* aProgressReporter = nullptr ) = 0;
+                                     PROGRESS_REPORTER* aProgressReporter = nullptr ) = 0;
 
     void DisplayErrors( wxTopLevelWindow* aCaller = NULL );
 
@@ -293,12 +284,6 @@ protected:
      * Join worker threads. Part of the FOOTPRINT_ASYNC_LOADER implementation.
      */
     virtual bool JoinWorkers() = 0;
-
-
-    /**
-     * Return the number of libraries finished (successfully or otherwise).
-     */
-    virtual size_t CountFinished() = 0;
 };
 
 
@@ -357,19 +342,6 @@ public:
      */
     bool Join();
 
-    /**
-     * Get the current completion percentage. 0 and 100 are reserved values:
-     * 0 will only be returned if Start() has not yet been called, and 100
-     * will only be returned if totally complete (i.e. rounding errors will
-     * never cause a 100% progress despite not being complete).
-     *
-     * If there are no libraries at all, returns 100 (as loading zero libraries
-     * is always complete).
-     *
-     * Threadsafe.
-     */
-    int GetProgress() const;
-
     /**
      * Set a callback to receive notice when loading is complete.
      *

include/widgets/footprint_select_widget.h

@@ -29,7 +29,7 @@
 class KIWAY;
 class PROJECT;
 class FOOTPRINT_CHOICE;
-class wxGauge;
+class GAUGE_PROGRESS_REPORTER;
 class wxMenu;
 class wxTimer;
 class wxTimerEvent;
@@ -130,13 +130,11 @@ public:
 
 private:
     KIWAY*                   m_kiway;
-    wxGauge*          m_progress_ctrl;
+    GAUGE_PROGRESS_REPORTER* m_progress_ctrl;
     FOOTPRINT_CHOICE*        m_fp_sel_ctrl;
     wxSizer*                 m_sizer;
     wxSimplebook*            m_book;
 
-    std::unique_ptr<wxTimer> m_progress_timer;
-
     bool                     m_update;
     bool                     m_finished_loading;
     int                      m_max_items;
@@ -144,13 +142,11 @@ private:
     wxString                 m_other_footprint;
     int                      m_last_item;
 
-    FOOTPRINT_ASYNC_LOADER   m_fp_loader;
     FOOTPRINT_LIST*          m_fp_list;
     FOOTPRINT_FILTER         m_fp_filter;
     bool                     m_zero_filter;
 
     void FootprintsLoaded();
-    void OnProgressTimer( wxTimerEvent& aEvent );
     void OnComboBox( wxCommandEvent& aEvent );
     void OnComboInteractive( wxCommandEvent& aEvent );
 

include/widgets/progress_reporter.h

@@ -29,6 +29,7 @@
 #include <atomic>
 
 #include <wx/progdlg.h>
+#include <wx/gauge.h>
 
 /**
  * A progress reporter for use in multi-threaded environments.  The various advancement
@@ -113,4 +114,22 @@ private:
     virtual bool updateUI() override;
 };
 
+
+class GAUGE_PROGRESS_REPORTER : public PROGRESS_REPORTER, public wxGauge
+{
+public:
+    /**
+     * @param aParent is the parent of the wxGauge control
+     * @param aNumPhases is the number of "virtual sections" of the progress bar
+     * aNumPhases = 1 is the usual progress bar
+     * aNumPhases = n creates n virtual progress bar zones: a 0 to 100 percent width
+     * of a virtual zone fills 0 to 1/n progress bar full size of the nth virtual zone index
+     */
+    GAUGE_PROGRESS_REPORTER( wxWindow* aParent, int aNumPhases );
+
+private:
+
+    bool updateUI() override;
+};
+
 #endif

pcbnew/footprint_info_impl.cpp

@@ -115,14 +115,8 @@ void FOOTPRINT_LIST_IMPL::loader_job()
 }
 
 
-bool FOOTPRINT_LIST_IMPL::RequiresLoading( FP_LIB_TABLE* aTable, const wxString* aNickname )
-{
-    return m_list_timestamp != aTable->GenerateTimestamp( aNickname );
-}
-
-
 bool FOOTPRINT_LIST_IMPL::ReadFootprintFiles( FP_LIB_TABLE* aTable, const wxString* aNickname,
-                                              WX_PROGRESS_REPORTER* aProgressReporter )
+                                              PROGRESS_REPORTER* aProgressReporter )
 {
     if( m_list_timestamp == aTable->GenerateTimestamp( aNickname ) )
         return true;
@@ -141,7 +135,7 @@ bool FOOTPRINT_LIST_IMPL::ReadFootprintFiles( FP_LIB_TABLE* aTable, const wxStri
         m_progress_reporter->Report( _( "Fetching Footprint Libraries" ) );
     }
 
-    while( !m_cancelled && loader.GetProgress() < 100 )
+    while( !m_cancelled && m_count_finished.load() < m_loader->m_total_libs )
     {
         if( m_progress_reporter )
             m_cancelled = !m_progress_reporter->KeepRefreshing();
@@ -307,12 +301,6 @@ bool FOOTPRINT_LIST_IMPL::JoinWorkers()
 }
 
 
-size_t FOOTPRINT_LIST_IMPL::CountFinished()
-{
-    return m_count_finished.load();
-}
-
-
 FOOTPRINT_LIST_IMPL::FOOTPRINT_LIST_IMPL() :
     m_loader( nullptr ),
     m_count_finished( 0 ),

pcbnew/footprint_info_impl.h

@@ -28,15 +28,14 @@
 
 #include <footprint_info.h>
 #include <sync_queue.h>
-#include <widgets/progress_reporter.h>
 
 class LOCALE_IO;
 
 class FOOTPRINT_INFO_IMPL : public FOOTPRINT_INFO
 {
 public:
-    FOOTPRINT_INFO_IMPL(
+    FOOTPRINT_INFO_IMPL( FOOTPRINT_LIST* aOwner, const wxString& aNickname,
-            FOOTPRINT_LIST* aOwner, const wxString& aNickname, const wxString& aFootprintName )
+                         const wxString& aFootprintName )
     {
         m_owner = aOwner;
         m_loaded = false;
@@ -64,7 +63,7 @@ class FOOTPRINT_LIST_IMPL : public FOOTPRINT_LIST
     SYNC_QUEUE<wxString>     m_queue_out;
     std::atomic_size_t       m_count_finished;
     long long                m_list_timestamp;
-    WX_PROGRESS_REPORTER*    m_progress_reporter;
+    PROGRESS_REPORTER*       m_progress_reporter;
     std::atomic_bool         m_cancelled;
 
     /**
@@ -75,10 +74,9 @@ class FOOTPRINT_LIST_IMPL : public FOOTPRINT_LIST
     bool CatchErrors( const std::function<void()>& aFunc );
 
 protected:
-    virtual void StartWorkers( FP_LIB_TABLE* aTable, wxString const* aNickname,
+    void StartWorkers( FP_LIB_TABLE* aTable, wxString const* aNickname,
                        FOOTPRINT_ASYNC_LOADER* aLoader, unsigned aNThreads ) override;
-    virtual bool   JoinWorkers() override;
+    bool JoinWorkers() override;
-    virtual size_t CountFinished() override;
 
     /**
      * Function loader_job
@@ -90,10 +88,8 @@ public:
     FOOTPRINT_LIST_IMPL();
     virtual ~FOOTPRINT_LIST_IMPL();
 
-    bool RequiresLoading( FP_LIB_TABLE* aTable, const wxString* aNickname = nullptr ) override;
-
     bool ReadFootprintFiles( FP_LIB_TABLE* aTable, const wxString* aNickname = nullptr,
-                             WX_PROGRESS_REPORTER* aProgressReporter = nullptr ) override;
+                             PROGRESS_REPORTER* aProgressReporter = nullptr ) override;
 };
 
 extern FOOTPRINT_LIST_IMPL GFootprintList;        // KIFACE scope.