Merge lp:~xzcvczx/kicad/gestfich-cleanup into lp:kicad/product

[xzcvczx]

Cleaned up an old work-around from OpenPDF
Made ExecuteFile quote the command name on osx due to it not working if the text editor had a space in the name

[Wayne Stambaugh (stambaughw)]

1) wxMimeTypeManager::GetFileTypeFromExtenstion() can return NULL. Your change ignores the return value and calls GetOpenCommand() without testing for NULL. It may not segfault depending on the behavior of GetOpenCommand() but you should still test for NULL and warn the use that the PDF mime type is not assigned.

2) I'm not sure removing all of the default platform specific PDF viewer applications is a good idea. If users do not have the default mime type set for PDF files (it can happen), my guess it that they would expect a reasonable default action to occur. We should get some user input on this before we remove it.

3) There are some coding policy violations. The "if (" statements should be "if( ".

review: Needs Fixing

[xzcvczx (xzcvczx)]

> 1) wxMimeTypeManager::GetFileTypeFromExtenstion() can return NULL. Your
> change ignores the return value and calls GetOpenCommand() without testing for
> NULL. It may not segfault depending on the behavior of GetOpenCommand() but
> you should still test for NULL and warn the use that the PDF mime type is not
> assigned.
>
> 2) I'm not sure removing all of the default platform specific PDF viewer
> applications is a good idea. If users do not have the default mime type set
> for PDF files (it can happen), my guess it that they would expect a reasonable
> default action to occur. We should get some user input on this before we
> remove it.
>
> 3) There are some coding policy violations. The "if (" statements should be
> "if( ".

i had meant to do the if NULL but got distracted and apparently forgot

With #2 would it not be better to just provide a dialog box saying that no handler was found for pdf files and provide the option to set one?

woops hadn't noticed #3 will fix that as well

[lp:~xzcvczx/kicad/gestfich-cleanup]

6412. By xzcvczx on 2016-01-01

Ensure filetype is not null before using open command, Fixed if statements to follow coding guidelines

6413. By xzcvczx on 2016-01-02

Reorder includes to a more logical order

[Wayne Stambaugh (stambaughw)]

On 1/1/2016 2:00 AM, Simon Wells wrote:
>> 1) wxMimeTypeManager::GetFileTypeFromExtenstion() can return NULL. Your
>> change ignores the return value and calls GetOpenCommand() without testing for
>> NULL. It may not segfault depending on the behavior of GetOpenCommand() but
>> you should still test for NULL and warn the use that the PDF mime type is not
>> assigned.
>>
>> 2) I'm not sure removing all of the default platform specific PDF viewer
>> applications is a good idea. If users do not have the default mime type set
>> for PDF files (it can happen), my guess it that they would expect a reasonable
>> default action to occur. We should get some user input on this before we
>> remove it.
>>
>> 3) There are some coding policy violations. The "if (" statements should be
>> "if( ".
>
> i had meant to do the if NULL but got distracted and apparently forgot
>
> With #2 would it not be better to just provide a dialog box saying that no handler was found for pdf files and provide the option to set one?

This is fine as long as no one else objects. The only issue may be
users who don't have their PDF mime type configured. They may be
unhappy when their PDF viewer no long works.

>
> woops hadn't noticed #3 will fix that as well
>

Let me know when you've made these changes.

Thanks,

Wayne

[Chris Pavlina (pavlina-chris)]

If users have no PDF viewer configured, why should it work? It won't work anywhere else either. Their whole system will tell them there's no configured viewer and yell at them, not just KiCad.

[Wayne Stambaugh (stambaughw)]

Yes, but this change removes the list of "reasonable" default platform
specific PDF applications which KiCad checks for when the mime type is
not defined so some users may be affected. I agree that only the mime
type check should be used but we will be breaking the current behavior
which almost always results in user issues.

On 1/4/2016 11:10 AM, Chris Pavlina wrote:
> If users have no PDF viewer configured, why should it work? It won't work anywhere else either. Their whole system will tell them there's no configured viewer and yell at them, not just KiCad.
>

[Chris Pavlina (pavlina-chris)]

Every change results in user issues. Seriously, I've been yelled at by _somebody_ for everything I've done and many things other people have. We really shouldn't be performing the function of the operating system by handling file associations.

[Wayne Stambaugh (stambaughw)]

The merge request looks fine with the latest changes.

review: Approve

[Nick Østergaard (nickoe)]

I think it makes sense to remove those "magic" defaults. I just tested this on archlinux and it opens evince, which is my system default.

I wonder why a space was added to the error message for the if( ProcessExecute( command ) ).

[lp:~xzcvczx/kicad/gestfich-cleanup]

6414. By xzcvczx on 2016-01-04

Removed the space in the error message after the new line

[Nick Østergaard (nickoe)]

review: Approve

[Wayne Stambaugh (stambaughw)]

Patch committed in r6419. Thanks.

=== modified file 'common/gestfich.cpp'
--- common/gestfich.cpp	2015-09-25 19:38:09 +0000
+++ common/gestfich.cpp	2016-01-04 17:13:05 +0000
@@ -28,33 +28,26 @@
  * @brief Functions for file management
  */
 
+#include <wx/mimetype.h>
+#include <wx/filename.h>
+#include <wx/dir.h>
+
 // For compilers that support precompilation, includes "wx.h".
 #include <fctsys.h>
 #include <pgm_base.h>
 #include <confirm.h>
-
 #include <common.h>
 #include <macros.h>
+
 #include <gestfich.h>
 
-#include <wx/mimetype.h>
-#include <wx/filename.h>
-#include <wx/dir.h>
-
-
 void AddDelimiterString( wxString& string )
 {
-    wxString text;
-
     if( !string.StartsWith( wxT( "\"" ) ) )
-        text = wxT( "\"" );
-
-    text += string;
-
-    if( (text.Last() != '"' ) || (text.length() <= 1) )
-        text += wxT( "\"" );
-
-    string = text;
+    {
+        string.Prepend ( wxT( "\"" ) );
+        string.Append ( wxT( "\"" ) );
+    }
 }
 
 
@@ -227,6 +220,8 @@
 #ifdef __WXMAC__
     else
     {
+        AddDelimiterString( fullFileName );
+
         if( !param.IsEmpty() )
             fullFileName += wxT( " " ) + param;
 
@@ -351,8 +346,6 @@
 {
     wxString command;
     wxString filename = file;
-    wxString type;
-    bool     success = false;
 
     Pgm().ReadPdfBrowserInfos();
 
@@ -363,71 +356,21 @@
     }
     else
     {
-        wxFileType* filetype = NULL;
-        wxFileType::MessageParameters params( filename, type );
-        filetype = wxTheMimeTypesManager->GetFileTypeFromExtension( wxT( "pdf" ) );
+        wxFileType* filetype = wxTheMimeTypesManager->GetFileTypeFromExtension( wxT( "pdf" ) );
 
         if( filetype )
-            success = filetype->GetOpenCommand( &command, params );
+            command = filetype->GetOpenCommand( filename );
 
         delete filetype;
-
-#ifndef __WINDOWS__
-
-        // Bug ? under linux wxWidgets returns acroread as PDF viewer, even if
-        // it does not exist.
-        if( command.StartsWith( wxT( "acroread" ) ) ) // Workaround
-            success = false;
-#endif
-
-        if( success && !command.IsEmpty() )
-        {
-            success = ProcessExecute( command );
-
-            if( success )
-                return success;
-        }
-
-        success = false;
-        command.clear();
-
-        if( !success )
-        {
-#if !defined(__WINDOWS__)
-            AddDelimiterString( filename );
-
-            // here is a list of PDF viewers candidates
-            static const wxChar* tries[] =
-            {
-                wxT( "/usr/bin/evince" ),
-                wxT( "/usr/bin/okular" ),
-                wxT( "/usr/bin/gpdf" ),
-                wxT( "/usr/bin/konqueror" ),
-                wxT( "/usr/bin/kpdf" ),
-                wxT( "/usr/bin/xpdf" ),
-                wxT( "/usr/bin/open" ),     // BSD and OSX file & dir opener
-                wxT( "/usr/bin/xdg-open" ), // Freedesktop file & dir opener
-            };
-
-            for( unsigned ii = 0;  ii<DIM(tries);  ii++ )
-            {
-                if( wxFileExists( tries[ii] ) )
-                {
-                    command = tries[ii];
-                    command += wxT( ' ' );
-                    command += filename;
-                    break;
-                }
-            }
-#endif
-        }
     }
 
     if( !command.IsEmpty() )
     {
-        success = ProcessExecute( command );
-
-        if( !success )
+        if( ProcessExecute( command ) )
+        {
+            return true;
+        }
+        else
         {
             wxString msg;
             msg.Printf( _( "Problem while running the PDF viewer\nCommand is '%s'" ),
@@ -440,10 +383,9 @@
         wxString msg;
         msg.Printf( _( "Unable to find a PDF viewer for <%s>" ), GetChars( filename ) );
         DisplayError( NULL, msg );
-        success = false;
     }
 
-    return success;
+    return false;
 }