Fix some comments. Enhance LEGACY_PLUGIN such that it can tolerate, then fix bad legacy footprint libraries containing duplicate footprint names.

This may have been an undocumented bug from more than a year ago, which manifested itself in *.mod files containing duplicate names.
LEGACY_PLUGIN loads those now quietly, but appends "_v2", "_v3", etc. to each succeeding duplicate sharing the same name.

common/base_screen.cpp

@@ -122,7 +122,7 @@ bool BASE_SCREEN::SetZoom( double iu_per_du )
     if( iu_per_du == m_Zoom )
         return false;
 
-    wxLogDebug( "Zoom:%16g  1/Zoom:%16g", iu_per_du, 1/iu_per_du );
+    wxLogDebug( "Zoom:%.16g  1/Zoom:%.16g", iu_per_du, 1/iu_per_du );
 
     if( iu_per_du < GetMinAllowedZoom() )
         return false;

common/common_plotPDF_functions.cpp

@@ -410,17 +410,17 @@ void PDF_PLOTTER::closePdfStream()
 {
     wxASSERT( workFile );
 
-    // Ask for the stream length
     int stream_len = ftell( workFile );
 
-    // Now we do a trick: rewind the file, read in the page stream and FLATE it
+    // Rewind the file, read in the page stream and DEFLATE it
     fseek( workFile, 0, SEEK_SET );
     unsigned char *inbuf = new unsigned char[stream_len];
 
     int rc = fread( inbuf, 1, stream_len, workFile );
     wxASSERT( rc == stream_len );
+    (void) rc;
 
-    // We have done with the temporary file, junk it
+    // We are done with the temporary file, junk it
     fclose( workFile );
     workFile = 0;
     ::wxRemoveFile( workFilename );
@@ -430,7 +430,7 @@ void PDF_PLOTTER::closePdfStream()
 
     {
         /* Somewhat standard parameters to compress in DEFLATE. The PDF spec is
-           misleading, it says it wants a FLATE stream but it really want a ZLIB
+           misleading, it says it wants a DEFLATE stream but it really want a ZLIB
            stream! (a DEFLATE stream would be generated with -15 instead of 15)
         rc = deflateInit2( &zstrm, Z_BEST_COMPRESSION, Z_DEFLATED, 15,
                                8, Z_DEFAULT_STRATEGY );
@@ -440,7 +440,9 @@ void PDF_PLOTTER::closePdfStream()
 
         zos.Write( inbuf, stream_len );
 
-    }   // flush the zip stream using destructor
+        delete[] inbuf;
+
+    }   // flush the zip stream using zos destructor
 
     wxStreamBuffer* sb = memos.GetOutputStreamBuffer();
 
@@ -448,8 +450,6 @@ void PDF_PLOTTER::closePdfStream()
 
     fwrite( sb->GetBufferStart(), 1, out_count, outputFile );
 
-    delete[] inbuf;
-
     fputs( "endstream\n", outputFile );
     closePdfObject();
 

include/class_base_screen.h

@@ -123,12 +123,12 @@ public:
     bool    m_FirstRedraw;
 
     // Undo/redo list of commands
-    UNDO_REDO_CONTAINER m_UndoList;          ///< Objects list for the undo command (old data)
+    UNDO_REDO_CONTAINER m_UndoList;         ///< Objects list for the undo command (old data)
-    UNDO_REDO_CONTAINER m_RedoList;          ///< Objects list for the redo command (old data)
+    UNDO_REDO_CONTAINER m_RedoList;         ///< Objects list for the redo command (old data)
-    unsigned            m_UndoRedoCountMax;  ///< undo/Redo command Max depth
+    unsigned            m_UndoRedoCountMax; ///< undo/Redo command Max depth
 
     // block control
-    BLOCK_SELECTOR      m_BlockLocate;       ///< Block description for block commands
+    BLOCK_SELECTOR      m_BlockLocate;      ///< Block description for block commands
 
     int                 m_ScreenNumber;
     int                 m_NumberOfScreen;
@@ -303,26 +303,30 @@ public:
      */
     double GetMinAllowedZoom() const    { return m_ZoomList.size() ? *m_ZoomList.begin() : 1.0; }
 
-    /**
-     * Function GetScalingFactor
-     * returns the the current scale used to draw items on screen
-     * draw coordinates are user coordinates * GetScalingFactor()
-     */
-    double GetScalingFactor() const;
-
     /**
      * Function SetScalingFactor
-     * sets the scaling factor of "device units per internal unit".
+     * sets the scaling factor of "internal unit per device unit".
      * If the output device is a screen, then "device units" are pixels.  The
      * "logical unit" is wx terminology, and corresponds to KiCad's "Internal Unit (IU)".
-     *
+     * <p>
-     * Another way of thinking of scaling factor, when applied to a screen,
+     * This scaling factor is "internal units per device unit".  This function is
-     * is "pixelsPerIU"
+     * the same thing currently as SetZoom(), but clamps the argument within a
+     * legal range.
 
-     * @param aScale = the the current scale used to draw items onto the device context wxDC.
+     * @param iu_per_du is the current scale used to draw items onto the device
-     *   device coordinates (pixels) = IU coordinates * GetScalingFactor()
+     *   context wxDC.
      */
-    void SetScalingFactor( double aScale );
+    void SetScalingFactor( double iu_per_du );
+
+    /**
+     * Function GetScalingFactor
+     * returns the inverse of the current scale used to draw items on screen.
+     * <p>
+     * This function somehow got designed to be the inverse of SetScalingFactor().
+     * <p>
+     * device coordinates = user coordinates * GetScalingFactor()
+     */
+    double GetScalingFactor() const;
 
 
     //----<grid stuff>----------------------------------------------------------

pcbnew/legacy_plugin.cpp

@@ -942,6 +942,8 @@ MODULE* LEGACY_PLUGIN::LoadMODULE()
 
         else if( TESTLINE( "Li" ) )         // Library name of footprint
         {
+            // There can be whitespace in the footprint name on some old libraries.
+            // Grab everything after "Li" up to end of line:
             module->SetLibRef( FROM_UTF8( StrPurge( line + SZ( "Li" ) ) ) );
         }
 
@@ -3903,16 +3905,54 @@ void FPL_CACHE::LoadModules( LINE_READER* aReader )
             // wxString footprintName = m->GetReference();
             wxString footprintName = m->GetLibRef();
 
-            std::pair<MODULE_ITER, bool> r = m_modules.insert( footprintName, m );
+            /*
 
-            // m's module is gone here, both on success or failure of insertion.
+            There was a bug in old legacy library management code
-            // no memory leak, container deleted m on failure.
+            (pre-LEGACY_PLUGIN) which was introducing duplicate footprint names
+            in legacy libraries without notification. To best recover from such
+            bad libraries, and use them to their fullest, there are a few
+            strategies that could be used. (Note: footprints must have unique
+            names to be accepted into this cache.) The strategy used here is to
+            append a differentiating version counter to the end of the name as:
+            _v2, _v3, etc.
 
-            if( !r.second )
+            */
+
+            MODULE_CITER it = m_modules.find( footprintName );
+
+            if( it == m_modules.end() )  // footprintName is not present in cache yet.
             {
-                THROW_IO_ERROR( wxString::Format(
+                std::pair<MODULE_ITER, bool> r = m_modules.insert( footprintName, m );
-                    _( "library '%s' has a duplicate footprint named '%s'" ),
+
-                    m_lib_name.GetData(), footprintName.GetData() ) );
+                wxASSERT_MSG( r.second, wxT( "error doing cache insert using guaranteed unique name" ) );
+                (void) r;
+            }
+
+            // Bad library has a duplicate of this footprintName, generate a
+            // unique footprint name and load it anyway.
+            else
+            {
+                bool    nameOK = false;
+                int     version = 2;
+
+                while( !nameOK )
+                {
+                    wxString newName = footprintName;
+                    newName << wxT( "_v" ) << version++;
+
+                    it = m_modules.find( newName );
+
+                    if( it == m_modules.end() )
+                    {
+                        nameOK = true;
+
+                        m->SetLibRef( newName );
+                        std::pair<MODULE_ITER, bool> r = m_modules.insert( newName, m );
+
+                        wxASSERT_MSG( r.second, wxT( "error doing cache insert using guaranteed unique name" ) );
+                        (void) r;
+                    }
+                }
             }
         }