haskal
/
kicad
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

LibEdit: fix library tree refresh issue when library is removed. 

A bug in LIB_MANAGER::LibraryExists() prevented a library removed from
the symbol library table from being removed from the tree view.  Add the
proper check to SYMBOL_TREE_SYNCHRONIZING_ADAPTER::Sync().

This exposed another issue with synchronization between the library
table editor and the tree view which could lead to orphaned library
modifications and/or a segfault when the currently selected library was
removed from the symbol library table.  Give the user a chance to save
or revert any changes before allowing changes to the symbol library
table.

Fixes lp:1821691

https://bugs.launchpad.net/kicad/+bug/1821691
This commit is contained in:
Wayne Stambaugh 2019-06-13 07:44:12 -04:00
parent b9eb627bc3
commit 028973d182
7 changed files with 191 additions and 58 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

48
eeschema/dialogs/panel_sym_lib_table.cpp
View File

@ -713,11 +713,44 @@ void InvokeSchEditSymbolLibTable( KIWAY* aKiway, wxWindow *aParent )
wxString projectPath = aKiway->Prj().GetProjectPath();
wxFileName projectTableFn( projectPath, SYMBOL_LIB_TABLE::GetSymbolLibTableFileName() );
wxString msg;
wxString currentLib;
// This prevents an ugly crash on OSX (https://bugs.launchpad.net/kicad/+bug/1765286)
if( libEditor )
{
currentLib = libEditor->GetCurLib();
// This prevents an ugly crash on OSX (https://bugs.launchpad.net/kicad/+bug/1765286)
libEditor->FreezeSearchTree();
// Check the symbol library editor for modifications to give the user a chance to save
// or revert changes before allowing changes to the library table.
if( libEditor->HasLibModifications() )
{
wxMessageDialog saveDlg( aParent,
_( "Modifications have been made to one or more symbol "
"libraries. Changes must be saved or discared before "
"the symbol library table can be modified." ),
_( "Warning" ),
wxYES_NO | wxCANCEL | wxYES_DEFAULT | wxCENTER );
saveDlg.SetYesNoCancelLabels( wxMessageDialog::ButtonLabel( _( "Save Changes" ) ),
wxMessageDialog::ButtonLabel(_( "Discard Changes" ) ),
wxMessageDialog::ButtonLabel( _( "Cancel" ) ) );
int resp = saveDlg.ShowModal();
if( resp == wxID_CANCEL )
{
libEditor->ThawSearchTree();
return;
}
if( resp == wxID_YES )
libEditor->SaveAll();
else
libEditor->RevertAll();
}
}
DIALOG_EDIT_LIBRARY_TABLES dlg( aParent, _( "Symbol Libraries" ) );
dlg.InstallPanel( new PANEL_SYM_LIB_TABLE( &dlg, globalTable, globalTablePath,
@ -725,7 +758,12 @@ void InvokeSchEditSymbolLibTable( KIWAY* aKiway, wxWindow *aParent )
aKiway->Prj().GetProjectPath() ) );
if( dlg.ShowModal() == wxID_CANCEL )
{
if( libEditor )
libEditor->ThawSearchTree();
return;
}
if( dlg.m_GlobalTableChanged )
{
@ -763,6 +801,14 @@ void InvokeSchEditSymbolLibTable( KIWAY* aKiway, wxWindow *aParent )
if( libEditor )
{
// Check if the currently selected symbol library been removed or disabled.
if( !currentLib.empty()
&& !projectTable->HasLibrary( currentLib, true ) )
{
libEditor->SetCurLib( wxEmptyString );
libEditor->emptyScreen();
}
libEditor->SyncLibraries( true );
libEditor->ThawSearchTree();
}

12
eeschema/libedit/lib_edit_frame.cpp
View File

@ -152,7 +152,7 @@ LIB_EDIT_FRAME::LIB_EDIT_FRAME( KIWAY* aKiway, wxWindow* aParent ) :
SetSize( m_FramePos.x, m_FramePos.y, m_FrameSize.x, m_FrameSize.y );
setupTools();
m_libMgr = new LIB_MANAGER( *this );
SyncLibraries( true );
m_treePane = new SYMBOL_TREE_PANE( this, m_libMgr );
@ -580,7 +580,7 @@ LIB_ID LIB_EDIT_FRAME::GetTreeLIBID( int* aUnit ) const
LIB_PART* LIB_EDIT_FRAME::getTargetPart() const
{
LIB_ID libId = GetTreeLIBID();
if( libId.IsValid() )
{
LIB_ALIAS* alias = m_libMgr->GetAlias( libId.GetLibItemName(), libId.GetLibNickname() );
@ -733,6 +733,7 @@ bool LIB_EDIT_FRAME::isCurrentPart( const LIB_ID& aLibId ) const
void LIB_EDIT_FRAME::emptyScreen()
{
m_treePane->GetLibTree()->Unselect();
SetCurLib( wxEmptyString );
SetCurPart( nullptr );
SetScreen( m_dummyScreen );
@ -875,3 +876,10 @@ void LIB_EDIT_FRAME::SwitchCanvas( EDA_DRAW_PANEL_GAL::GAL_TYPE aCanvasType )
GetGalCanvas()->GetGAL()->SetAxesEnabled( true );
}
bool LIB_EDIT_FRAME::HasLibModifications() const
{
wxCHECK( m_libMgr, false );
return m_libMgr->HasModifications();
}

21
eeschema/libedit/lib_edit_frame.h
View File

@ -131,6 +131,16 @@ public:
*/
void SwitchCanvas( EDA_DRAW_PANEL_GAL::GAL_TYPE aCanvasType ) override;
/**
* Check if any pending libraries have been modified.
*
* This only checks for modified libraries. If a new symbol was created and
* modified and no libraries have been modified, the return value will be false.
*
* @return True if there are any pending library modifications.
*/
bool HasLibModifications() const;
/** The nickname of the current library being edited and empty string if none. */
wxString GetCurLib() const;
@ -232,10 +242,11 @@ public:
/**
* Reverts unsaved changes in a part, restoring to the last saved state.
*/
void Revert();
void Revert( bool aConfirm = true );
void RevertAll();
void DeletePartFromLibrary();
void CopyPartToClipboard();
void LoadPart( const wxString& aLibrary, const wxString& aPart, int Unit );
@ -467,6 +478,9 @@ public:
void KiwayMailIn( KIWAY_EXPRESS& mail ) override;
///> Restores the empty editor screen, without any part or library selected.
void emptyScreen();
private:
///> Helper screen used when no part is loaded
SCH_SCREEN* m_dummyScreen;
@ -507,9 +521,6 @@ private:
///> Returns true if \a aLibId is an alias for the editor screen part.
bool isCurrentPart( const LIB_ID& aLibId ) const;
///> Restores the empty editor screen, without any part or library selected.
void emptyScreen();
///> Renames LIB_PART aliases to avoid conflicts before adding a component to a library
void fixDuplicateAliases( LIB_PART* aPart, const wxString& aLibrary );

60
eeschema/libedit/lib_manager.cpp
View File

@ -2,6 +2,7 @@
* This program source code file is part of KiCad, a free EDA CAD application.
*
* Copyright (C) 2017 CERN
* Copyright (C) 2019 KiCad Developers, see AUTHORS.txt for contributors.
* @author Maciej Suminski <maciej.suminski@cern.ch>
*
* This program is free software; you can redistribute it and/or
@ -37,7 +38,7 @@
LIB_MANAGER::LIB_MANAGER( LIB_EDIT_FRAME& aFrame ) :
m_frame( aFrame ),
m_frame( aFrame ),
m_syncHash( 0 )
{
m_adapter = SYMBOL_TREE_SYNCHRONIZING_ADAPTER::Create( &m_frame, this );
@ -45,7 +46,8 @@ LIB_MANAGER::LIB_MANAGER( LIB_EDIT_FRAME& aFrame ) :
}
void LIB_MANAGER::Sync( bool aForce, std::function<void(int, int, const wxString&)> aProgressCallback )
void LIB_MANAGER::Sync( bool aForce,
std::function<void( int, int, const wxString& )> aProgressCallback )
{
int libTableHash = symTable()->GetModifyHash();
@ -57,6 +59,18 @@ void LIB_MANAGER::Sync( bool aForce, std::function<void(int, int, const wxString
}
bool LIB_MANAGER::HasModifications() const
{
for( auto lib : m_libs )
{
if( lib.second.IsModified() )
return true;
}
return false;
}
int LIB_MANAGER::GetHash() const
{
int hash = symTable()->GetModifyHash();
@ -507,6 +521,32 @@ bool LIB_MANAGER::RevertLibrary( const wxString& aLibrary )
}
bool LIB_MANAGER::RevertAll()
{
bool retv = true;
// Nothing to revert.
if( GetHash() == 0 )
return true;
for( auto lib : m_libs )
{
if( !lib.second.IsModified() )
continue;
for( auto buffer : lib.second.GetBuffers() )
{
if( !buffer->IsModified() )
continue;
RevertPart( lib.first, buffer->GetOriginal()->GetName() );
}
}
return retv;
}
bool LIB_MANAGER::RemovePart( const wxString& aAlias, const wxString& aLibrary )
{
LIB_BUFFER& libBuf = getLibraryBuffer( aLibrary );
@ -561,7 +601,7 @@ bool LIB_MANAGER::PartExists( const wxString& aAlias, const wxString& aLibrary )
try
{
alias = symTable()->LoadSymbol( aLibrary, aAlias );
}
}
catch( IO_ERROR& )
{
// checking if certain symbol exists, so its absence is perfectly fine
@ -582,6 +622,7 @@ bool LIB_MANAGER::LibraryExists( const wxString& aLibrary, bool aCheckEnabled )
return symTable()->HasLibrary( aLibrary, aCheckEnabled );
}
wxString LIB_MANAGER::GetUniqueLibraryName() const
{
wxString name = "New_Library";
@ -688,7 +729,7 @@ std::set<LIB_PART*> LIB_MANAGER::getOriginalParts( const wxString& aLibrary )
}
catch( const IO_ERROR& e )
{
DisplayErrorMessage( &m_frame,
DisplayErrorMessage( &m_frame,
wxString::Format( _( "Cannot enumerate library \"%s\"" ), aLibrary ),
e.What() );
}
@ -716,7 +757,7 @@ LIB_MANAGER::LIB_BUFFER& LIB_MANAGER::getLibraryBuffer( const wxString& aLibrary
LIB_MANAGER::PART_BUFFER::PART_BUFFER( LIB_PART* aPart, std::unique_ptr<SCH_SCREEN> aScreen ) :
m_screen( std::move( aScreen ) ),
m_screen( std::move( aScreen ) ),
m_part( aPart )
{
m_original = new LIB_PART( *aPart );
@ -798,7 +839,8 @@ bool LIB_MANAGER::LIB_BUFFER::CreateBuffer( LIB_PART* aCopy, SCH_SCREEN* aScreen
}
bool LIB_MANAGER::LIB_BUFFER::UpdateBuffer( LIB_MANAGER::PART_BUFFER::PTR aPartBuf, LIB_PART* aCopy )
bool LIB_MANAGER::LIB_BUFFER::UpdateBuffer( LIB_MANAGER::PART_BUFFER::PTR aPartBuf,
LIB_PART* aCopy )
{
bool ret = true;
@ -829,7 +871,8 @@ bool LIB_MANAGER::LIB_BUFFER::SaveBuffer( LIB_MANAGER::PART_BUFFER::PTR aPartBuf
wxCHECK( aPartBuf, false );
LIB_PART* part = aPartBuf->GetPart();
wxCHECK( part, false );
wxCHECK( aLibTable->SaveSymbol( m_libName, new LIB_PART( *part ) ) == SYMBOL_LIB_TABLE::SAVE_OK, false );
wxCHECK( aLibTable->SaveSymbol( m_libName,
new LIB_PART( *part ) ) == SYMBOL_LIB_TABLE::SAVE_OK, false );
aPartBuf->SetOriginal( new LIB_PART( *part ) );
++m_hash;
@ -865,7 +908,8 @@ bool LIB_MANAGER::LIB_BUFFER::addAliases( PART_BUFFER::PTR aPartBuf )
for( unsigned int i = 0; i < part->GetAliasCount(); ++i )
{
bool newAlias;
std::tie( std::ignore, newAlias ) = m_aliases.emplace( part->GetAlias( i )->GetName(), aPartBuf );
std::tie( std::ignore, newAlias ) = m_aliases.emplace( part->GetAlias( i )->GetName(),
aPartBuf );
if( !newAlias ) // Overwrite check
{

19
eeschema/libedit/lib_manager.h
View File

@ -2,6 +2,7 @@
* This program source code file is part of KiCad, a free EDA CAD application.
*
* Copyright (C) 2017 CERN
* Copyright (C) 2019 KiCad Developers, see AUTHORS.txt for contributors.
* @author Maciej Suminski <maciej.suminski@cern.ch>
*
* This program is free software; you can redistribute it and/or
@ -59,8 +60,10 @@ public:
int GetHash() const;
bool HasModifications() const;
/**
* Retruns a library hash value to determine if it has changed.
* Returns a library hash value to determine if it has changed.
*
* For buffered libraries, it returns a number corresponding to the number
* of modifications. For original libraries, hash is computed basing on the
@ -217,6 +220,13 @@ public:
*/
bool RevertLibrary( const wxString& aLibrary );
/**
* Revert all pending changes.
*
* @return True if all changes successfully reverted.
*/
bool RevertAll();
/**
* Returns a library name that is not currently in use.
* Used for generating names for new libraries.
@ -385,7 +395,7 @@ private:
///> to disk as well, depending on the row properties.
bool SaveBuffer( PART_BUFFER::PTR aPartBuf, SYMBOL_LIB_TABLE* aLibTable );
///> Saves stored modificatiosn using a plugin. aBuffer decides whether the changes
///> Saves stored modifications using a plugin. aBuffer decides whether the changes
///> should be cached or stored directly to the disk (for SCH_LEGACY_PLUGIN).
bool SaveBuffer( PART_BUFFER::PTR aPartBuf, SCH_PLUGIN* aPlugin, bool aBuffer );
@ -449,7 +459,10 @@ private:
wxString m_currentPart;
SYMBOL_TREE_SYNCHRONIZING_ADAPTER::PTR m_adapter;
SYMBOL_TREE_SYNCHRONIZING_ADAPTER* getAdapter() { return static_cast<SYMBOL_TREE_SYNCHRONIZING_ADAPTER*>( m_adapter.get() ); }
SYMBOL_TREE_SYNCHRONIZING_ADAPTER* getAdapter()
{
return static_cast<SYMBOL_TREE_SYNCHRONIZING_ADAPTER*>( m_adapter.get() );
}
};
#endif /* LIB_MANAGER_H */

19
eeschema/libedit/libedit.cpp
View File

@ -196,7 +196,7 @@ bool LIB_EDIT_FRAME::LoadComponentFromCurrentLib( const wxString& aAliasName, in
}
/**
* Synchronise screen settings from a current screen into another screen.
* Synchronize screen settings from a current screen into another screen.
*
* This can be used, for example, when loading a new screen into a frame,
* but you want the new screen to inherit some settings (e.g. grids) from the
@ -205,7 +205,7 @@ bool LIB_EDIT_FRAME::LoadComponentFromCurrentLib( const wxString& aAliasName, in
* @param aCurrentScreen the existing frame screen
* @param aIncomingScreen a screen that is intended to replace the current screen
*/
static void synchronizeLibEditScreenSettings( const SCH_SCREEN& aCurrentScreen,
static void synchronizeLibEditScreenSettings( const SCH_SCREEN& aCurrentScreen,
SCH_SCREEN& aIncomingScreen )
{
aIncomingScreen.SetGrid( aCurrentScreen.GetGridSize() );
@ -648,7 +648,7 @@ void LIB_EDIT_FRAME::fixDuplicateAliases( LIB_PART* aPart, const wxString& aLibr
}
void LIB_EDIT_FRAME::Revert()
void LIB_EDIT_FRAME::Revert( bool aConfirm )
{
LIB_ID libId = getTargetLibId();
const wxString& libName = libId.GetLibNickname();
@ -657,7 +657,7 @@ void LIB_EDIT_FRAME::Revert()
wxString msg = wxString::Format( _( "Revert \"%s\" to last version saved?" ),
partName.IsEmpty() ? libName : partName );
if( !ConfirmRevertDialog( this, msg ) )
if( aConfirm && !ConfirmRevertDialog( this, msg ) )
return;
bool reload_currentPart = false;
@ -704,6 +704,15 @@ void LIB_EDIT_FRAME::Revert()
}
void LIB_EDIT_FRAME::RevertAll()
{
wxCHECK_RET( m_libMgr, "Library manager object not created." );
Revert( false );
m_libMgr->RevertAll();
}
void LIB_EDIT_FRAME::LoadPart( const wxString& aAlias, const wxString& aLibrary, int aUnit )
{
wxCHECK( m_libMgr->PartExists( aAlias, aLibrary ), /* void */ );
@ -854,5 +863,3 @@ bool LIB_EDIT_FRAME::saveAllLibraries( bool aRequireConfirmation )
return true;
}

70
eeschema/symbol_tree_synchronizing_adapter.cpp
View File

@ -2,6 +2,7 @@
* This program source code file is part of KiCad, a free EDA CAD application.
*
* Copyright (C) 2017 CERN
* Copyright (C) 2019 KiCad Developers, see AUTHORS.txt for contributors.
* @author Maciej Suminski <maciej.suminski@cern.ch>
*
* This program is free software; you can redistribute it and/or
@ -61,7 +62,8 @@ bool SYMBOL_TREE_SYNCHRONIZING_ADAPTER::IsContainer( const wxDataViewItem& aItem
#define PROGRESS_INTERVAL_MILLIS 120
void SYMBOL_TREE_SYNCHRONIZING_ADAPTER::Sync( bool aForce, std::function<void(int, int, const wxString&)> aProgressCallback )
void SYMBOL_TREE_SYNCHRONIZING_ADAPTER::Sync( bool aForce,
std::function<void( int, int, const wxString& )> aProgressCallback )
{
wxLongLong nextUpdate = wxGetUTCTimeMillis() + (PROGRESS_INTERVAL_MILLIS / 2);
@ -84,7 +86,11 @@ void SYMBOL_TREE_SYNCHRONIZING_ADAPTER::Sync( bool aForce, std::function<void(in
nextUpdate = wxGetUTCTimeMillis() + PROGRESS_INTERVAL_MILLIS;
}
if( !m_libMgr->LibraryExists( name, true ) )
// There is a bug in LIB_MANAGER::LibraryExists() that uses the buffered modified
// libraries before the symbol library table which prevents the library from being
// removed from the tree control.
if( !m_libMgr->LibraryExists( name, true )
|| !m_frame->Prj().SchSymbolLibTable()->HasLibrary( name, true ) )
{
it = deleteLibrary( it );
continue;
@ -146,7 +152,7 @@ void SYMBOL_TREE_SYNCHRONIZING_ADAPTER::updateLibrary( LIB_TREE_NODE_LIB& aLibNo
}
else if( hashIt->second != m_libMgr->GetLibraryHash( aLibNode.Name ) )
{
// update an existing libary
// update an existing library
std::list<LIB_ALIAS*> aliases = m_libMgr->GetAliases( aLibNode.Name );
// remove the common part from the aliases list
@ -209,11 +215,11 @@ void SYMBOL_TREE_SYNCHRONIZING_ADAPTER::GetValue( wxVariant& aVariant, wxDataVie
case 0:
aVariant = node->Name;
// mark modified libs with an asterix
// mark modified libs with an asterisk
if( node->Type == LIB_TREE_NODE::LIB && m_libMgr->IsLibraryModified( node->Name ) )
aVariant = node->Name + " *";
// mark modified parts with an asterix
// mark modified parts with an aster-ix
if( node->Type == LIB_TREE_NODE::LIBID
&& m_libMgr->IsPartModified( node->Name, node->Parent->Name ) )
aVariant = node->Name + " *";
@ -256,46 +262,44 @@ bool SYMBOL_TREE_SYNCHRONIZING_ADAPTER::GetAttr( wxDataViewItem const& aItem, un
switch( node->Type )
{
case LIB_TREE_NODE::LIB:
// mark modified libs with bold font
aAttr.SetBold( m_libMgr->IsLibraryModified( node->Name ) );
case LIB_TREE_NODE::LIB:
// mark modified libs with bold font
aAttr.SetBold( m_libMgr->IsLibraryModified( node->Name ) );
#ifdef __WXGTK__
// The native wxGTK+ impl ignores background colour, so set the text colour instead.
// This works reasonably well in dark themes, and quite poorly in light ones....
if( node->Name == m_libMgr->GetCurrentLib() )
aAttr.SetColour( wxSystemSettings::GetColour( wxSYS_COLOUR_HIGHLIGHT ) );
// The native wxGTK+ impl ignores background colour, so set the text colour instead.
// This works reasonably well in dark themes, and quite poorly in light ones....
if( node->Name == m_libMgr->GetCurrentLib() )
aAttr.SetColour( wxSystemSettings::GetColour( wxSYS_COLOUR_HIGHLIGHT ) );
#else
// mark the current library with background color
if( node->Name == m_libMgr->GetCurrentLib() )
aAttr.SetBackgroundColour( wxSystemSettings::GetColour( wxSYS_COLOUR_HIGHLIGHT ) );
// mark the current library with background color
if( node->Name == m_libMgr->GetCurrentLib() )
aAttr.SetBackgroundColour( wxSystemSettings::GetColour( wxSYS_COLOUR_HIGHLIGHT ) );
#endif
break;
break;
case LIB_TREE_NODE::LIBID:
// mark modified part with bold font
aAttr.SetBold( m_libMgr->IsPartModified( node->Name, node->Parent->Name ) );
case LIB_TREE_NODE::LIBID:
// mark modified part with bold font
aAttr.SetBold( m_libMgr->IsPartModified( node->Name, node->Parent->Name ) );
// mark aliases with italic font
aAttr.SetItalic( !node->IsRoot );
// mark aliases with italic font
aAttr.SetItalic( !node->IsRoot );
#ifdef __WXGTK__
// The native wxGTK+ impl ignores background colour, so set the text colour instead.
// This works reasonably well in dark themes, and quite poorly in light ones....
if( node->LibId == m_libMgr->GetCurrentLibId() )
aAttr.SetColour( wxSystemSettings::GetColour( wxSYS_COLOUR_HIGHLIGHT ) );
// The native wxGTK+ impl ignores background colour, so set the text colour instead.
// This works reasonably well in dark themes, and quite poorly in light ones....
if( node->LibId == m_libMgr->GetCurrentLibId() )
aAttr.SetColour( wxSystemSettings::GetColour( wxSYS_COLOUR_HIGHLIGHT ) );
#else
// mark the current part with background color
if( node->LibId == m_libMgr->GetCurrentLibId() )
aAttr.SetBackgroundColour( wxSystemSettings::GetColour( wxSYS_COLOUR_HIGHLIGHT ) );
// mark the current part with background color
if( node->LibId == m_libMgr->GetCurrentLibId() )
aAttr.SetBackgroundColour( wxSystemSettings::GetColour( wxSYS_COLOUR_HIGHLIGHT ) );
#endif
break;
break;
default:
return false;
default:
return false;
}
return true;
}