kicad-developers team mailing list archive
-
kicad-developers team
-
Mailing list archive
-
Message #30078
[PATCH] - Improved error messages
A lot of the error messages presented to the users are full of
developer-only data, that often serves to confuse rather than help end
users. However the developer data is great for bug reports and dev feedback.
I have updated the DisplayErrorMessage and DisplayInfoMessage functions to
have an extra information parameter, which is initially hidden in a "Show
details" drop-down box.
I have not fixed all the error messages. However I have updated a few to
start the ball rolling. In particular I have focused on messages that I
have seen and thought to be unhelpful.
I think that following this pattern can provide better UX when errors
occur, and improve the professional look of KiCad.
Patch set attached.
Regards,
Oliver
From 7c655a97c90c211cc805715d225ef687fa793d00 Mon Sep 17 00:00:00 2001
From: Oliver Walters <oliver.henry.walters@xxxxxxxxx>
Date: Thu, 20 Jul 2017 23:03:33 +1000
Subject: [PATCH 1/2] Added extra information to error and info messages
Optional extra information string which is displayed in a drop-down "details" box
---
AUTHORS.txt | 2 ++
common/confirm.cpp | 51 ++++++++++++++++++++++++++++++++++-----------------
eeschema/annotate.cpp | 2 +-
include/confirm.h | 17 +++++++++++++++--
4 files changed, 52 insertions(+), 20 deletions(-)
diff --git a/AUTHORS.txt b/AUTHORS.txt
index 656723f..b32fd48 100644
--- a/AUTHORS.txt
+++ b/AUTHORS.txt
@@ -41,6 +41,8 @@ Mateusz Skowroński <skowri[at]gmail-dot-com>
Cheng Sheng <chengsheng[at]google-dot-com>
Google Inc.
Kristoffer Ödmark <kristoffer.odmark90[at]gmail-dot-com>
+Oliver Walters <oliver.henry.walters[at]gmail-dot-com>
+
See also CHANGELOG.txt for contributors.
diff --git a/common/confirm.cpp b/common/confirm.cpp
index af25736..d8711b4 100644
--- a/common/confirm.cpp
+++ b/common/confirm.cpp
@@ -2,7 +2,7 @@
* This program source code file is part of KiCad, a free EDA CAD application.
*
* Copyright (C) 2007 Jean-Pierre Charras, jp.charras at wanadoo.fr
- * Copyright (C) 1992-2013 KiCad Developers, see AUTHORS.txt for contributors.
+ * Copyright (C) 1992-2017 KiCad Developers, see AUTHORS.txt for contributors.
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License
@@ -28,6 +28,7 @@
*/
#include <wx/stockitem.h>
+#include <wx/richmsgdlg.h>
#include <bitmaps.h>
#include <html_messagebox.h>
@@ -70,31 +71,47 @@ void DisplayError( wxWindow* parent, const wxString& text, int displaytime )
{
wxMessageDialog* dialog;
- if( displaytime > 0 )
- dialog = new wxMessageDialog( parent, text, _( "Warning" ),
- wxOK | wxCENTRE | wxICON_INFORMATION
- | wxRESIZE_BORDER
- );
- else
- dialog = new wxMessageDialog( parent, text, _( "Error" ),
- wxOK | wxCENTRE | wxICON_ERROR
- | wxRESIZE_BORDER
- );
+ int icon = displaytime > 0 ? wxICON_INFORMATION : wxICON_ERROR;
+
+ dialog = new wxMessageDialog( parent, text, _( "Warning" ),
+ wxOK | wxCENTRE | wxRESIZE_BORDER | icon );
dialog->ShowModal();
dialog->Destroy();
}
-void DisplayInfoMessage( wxWindow* parent, const wxString& text, int displaytime )
+void DisplayErrorMessage( wxWindow* aParent, const wxString& aText, const wxString aExtraInfo )
{
- wxMessageDialog* dialog;
+ wxRichMessageDialog* dlg;
- dialog = new wxMessageDialog( parent, text, _( "Info" ),
- wxOK | wxCENTRE | wxICON_INFORMATION );
+ dlg = new wxRichMessageDialog( aParent, aText, _( "Error" ),
+ wxOK | wxCENTRE | wxRESIZE_BORDER | wxICON_ERROR );
- dialog->ShowModal();
- dialog->Destroy();
+ if( !aExtraInfo.IsEmpty() )
+ {
+ dlg->ShowDetailedText( aExtraInfo );
+ }
+
+ dlg->ShowModal();
+ dlg->Destroy();
+}
+
+
+void DisplayInfoMessage( wxWindow* aParent, const wxString& aMessage, const wxString aExtraInfo )
+{
+ wxRichMessageDialog* dlg;
+
+ dlg = new wxRichMessageDialog( aParent, aMessage, _( "Info" ),
+ wxOK | wxCENTRE | wxRESIZE_BORDER | wxICON_INFORMATION );
+
+ if( !aExtraInfo.IsEmpty() )
+ {
+ dlg->ShowDetailedText( aExtraInfo );
+ }
+
+ dlg->ShowModal();
+ dlg->Destroy();
}
diff --git a/eeschema/annotate.cpp b/eeschema/annotate.cpp
index 5f626b5..e1a50e1 100644
--- a/eeschema/annotate.cpp
+++ b/eeschema/annotate.cpp
@@ -85,7 +85,7 @@ void SCH_EDIT_FRAME::AnnotateComponents( bool aAnnotateSchematic,
{
wxString msg;
msg.Printf( _( "%d duplicate time stamps were found and replaced." ), count );
- DisplayInfoMessage( NULL, msg, 2 );
+ DisplayInfoMessage( NULL, msg );
}
}
diff --git a/include/confirm.h b/include/confirm.h
index ff2c64e..9d8822d 100644
--- a/include/confirm.h
+++ b/include/confirm.h
@@ -58,12 +58,25 @@ int DisplayExitDialog( wxWindow* aParent, const wxString& aMessage );
void DisplayError( wxWindow* parent, const wxString& aMessage, int displaytime = 0 );
/**
+ * Function DisplayErrorMessage
+ * displays an error message with \a aMessage
+ *
+ * @param aParent is the parent window
+ * @param aMessage is the message text to display
+ * @param aExtraInfo is extra data that can be optionally displayed in a collapsible pane
+ */
+void DisplayErrorMessage( wxWindow* aParent, const wxString& aMessage, const wxString aExtraInfo = wxEmptyString );
+
+
+/**
* Function DisplayInfoMessage
* displays an informational message box with \a aMessage.
*
- * @warning Setting \a displaytime does not work. Do not use it.
+ * @param aParent is the parent window
+ * @param aMessage is the message text to display
+ * @param aExtraInfo is the extra data that can be optionally displayed in a collapsible pane
*/
-void DisplayInfoMessage( wxWindow* parent, const wxString& aMessage, int displaytime = 0 );
+void DisplayInfoMessage( wxWindow* parent, const wxString& aMessage, const wxString aExtraInfo = wxEmptyString );
/**
* Function IsOK
--
2.7.4
From 219cffd64b05fc67aa6582a0e8a118816f4f035c Mon Sep 17 00:00:00 2001
From: Oliver Walters <oliver.henry.walters@xxxxxxxxx>
Date: Fri, 21 Jul 2017 00:06:41 +1000
Subject: [PATCH 2/2] Improved various error messages
- Moved developer "jargon" to details pane
- Changed error messages to "WHAT" rather than "WHY" or "WHERE"
---
AUTHORS.txt | 2 +-
common/confirm.cpp | 1 +
common/gal/opengl/utils.cpp | 5 ++++-
common/project.cpp | 4 ++--
cvpcb/cvpcb.cpp | 10 ++++------
eeschema/eeschema.cpp | 11 +++++------
include/confirm.h | 2 +-
pcbnew/append_board_to_current.cpp | 8 +++-----
pcbnew/basepcbframe.cpp | 4 +++-
pcbnew/files.cpp | 8 +++-----
pcbnew/onleftclick.cpp | 6 +++++-
pcbnew/pcbnew.cpp | 12 ++++++------
pcbnew/pcbnew_config.cpp | 6 +++++-
pcbnew/specctra_export.cpp | 6 +++---
pcbnew/specctra_import.cpp | 19 ++++++++++++-------
pcbnew/tool_onrightclick.cpp | 4 +++-
pcbnew/tool_pcb.cpp | 5 +++--
pcbnew/zones_by_polygon.cpp | 14 +++++++-------
18 files changed, 71 insertions(+), 56 deletions(-)
diff --git a/AUTHORS.txt b/AUTHORS.txt
index b32fd48..2bd8fb3 100644
--- a/AUTHORS.txt
+++ b/AUTHORS.txt
@@ -1,5 +1,5 @@
* Copyright (C) 1992-2015 Jean-Pierre Charras
-* Copyright (C) 1992-2015 Kicad Developers Team
+* Copyright (C) 1992-2017 Kicad Developers Team
* under GNU General Public License (see copyright.txt)
== Main Authors
diff --git a/common/confirm.cpp b/common/confirm.cpp
index d8711b4..9c3ab3d 100644
--- a/common/confirm.cpp
+++ b/common/confirm.cpp
@@ -67,6 +67,7 @@ int DisplayExitDialog( wxWindow* parent, const wxString& aMessage )
}
+// DisplayError should be deprecated, use DisplayErrorMessage instead
void DisplayError( wxWindow* parent, const wxString& text, int displaytime )
{
wxMessageDialog* dialog;
diff --git a/common/gal/opengl/utils.cpp b/common/gal/opengl/utils.cpp
index 55366c7..ad32d1d 100644
--- a/common/gal/opengl/utils.cpp
+++ b/common/gal/opengl/utils.cpp
@@ -76,7 +76,10 @@ int checkGlError( const std::string& aInfo, bool aThrow )
if( aThrow )
throw std::runtime_error( (const char*) errorMsg.char_str() );
else
- DisplayError( nullptr, errorMsg );
+ DisplayErrorMessage(
+ nullptr,
+ _( "OpenGL error occurred" ),
+ errorMsg );
}
return result;
diff --git a/common/project.cpp b/common/project.cpp
index 12e3647..4b481cc 100644
--- a/common/project.cpp
+++ b/common/project.cpp
@@ -248,7 +248,7 @@ static bool copy_pro_file_template( const SEARCH_STACK& aSearchS, const wxString
"Unable to find '%s' template config file." ),
GetChars( templateFile ) );
- DisplayError( NULL, msg );
+ DisplayErrorMessage( nullptr, _( "Error copying project file template" ), msg );
return false;
}
@@ -412,7 +412,7 @@ FP_LIB_TABLE* PROJECT::PcbFootprintLibs( KIWAY& aKiway )
}
catch( const IO_ERROR& ioe )
{
- DisplayError( NULL, ioe.What() );
+ DisplayErrorMessage( NULL, _( "Error loading project footprint library table" ), ioe.What() );
}
}
diff --git a/cvpcb/cvpcb.cpp b/cvpcb/cvpcb.cpp
index c7a98aa..5d65456 100644
--- a/cvpcb/cvpcb.cpp
+++ b/cvpcb/cvpcb.cpp
@@ -177,12 +177,10 @@ bool IFACE::OnKifaceStart( PGM_BASE* aProgram, int aCtlBits )
}
catch( const IO_ERROR& ioe )
{
- wxString msg = wxString::Format( _(
- "An error occurred attempting to load the global footprint library "
- "table:\n\n%s" ),
- GetChars( ioe.What() )
- );
- DisplayError( NULL, msg );
+ DisplayErrorMessage(
+ nullptr,
+ _( "An error occurred attempting to load the global footprint library table" ),
+ ioe.What() );
return false;
}
diff --git a/eeschema/eeschema.cpp b/eeschema/eeschema.cpp
index bb42096..768deb2 100644
--- a/eeschema/eeschema.cpp
+++ b/eeschema/eeschema.cpp
@@ -267,13 +267,12 @@ bool IFACE::OnKifaceStart( PGM_BASE* aProgram, int aCtlBits )
// if we are here, a incorrect global symbol library table was found.
// Incorrect global symbol library table is not a fatal error:
// the user just has to edit the (partially) loaded table.
- wxString msg = wxString::Format( _(
- "An error occurred attempting to load the global symbol library table:"
- "\n\n%s\n\n"
- "Please edit this global symbol library table in Preferences menu" ),
- GetChars( ioe.What() )
+ wxString msg = _(
+ "An error occurred attempting to load the global symbol library table.\n"
+ "Please edit this global symbol library table in Preferences menu"
);
- DisplayError( NULL, msg );
+
+ DisplayErrorMessage( NULL, msg, ioe.What() );
}
return true;
diff --git a/include/confirm.h b/include/confirm.h
index 9d8822d..6dab366 100644
--- a/include/confirm.h
+++ b/include/confirm.h
@@ -2,7 +2,7 @@
* This program source code file is part of KiCad, a free EDA CAD application.
*
* Copyright (C) 2007 Jean-Pierre Charras, jp.charras at wanadoo.fr
- * Copyright (C) 1992-2013 KiCad Developers, see AUTHORS.txt for contributors.
+ * Copyright (C) 1992-2017 KiCad Developers, see AUTHORS.txt for contributors.
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License
diff --git a/pcbnew/append_board_to_current.cpp b/pcbnew/append_board_to_current.cpp
index 8c52f67..60b4aee 100644
--- a/pcbnew/append_board_to_current.cpp
+++ b/pcbnew/append_board_to_current.cpp
@@ -86,13 +86,11 @@ bool PCB_EDIT_FRAME::AppendBoardFile( const wxString& aFullFileName, int aCtl )
catch( const IO_ERROR& ioe )
{
for( TRACK* track = GetBoard()->m_Track; track; track = track->Next() )
+ {
track->ClearFlags( FLAG0 );
+ }
- wxString msg = wxString::Format( _(
- "Error loading board.\n%s" ),
- GetChars( ioe.What() )
- );
- DisplayError( this, msg );
+ DisplayErrorMessage( this, _( "Error loading board in AppendBoardFile" ), ioe.What() );
return false;
}
diff --git a/pcbnew/basepcbframe.cpp b/pcbnew/basepcbframe.cpp
index 2e7de03..ba16929 100644
--- a/pcbnew/basepcbframe.cpp
+++ b/pcbnew/basepcbframe.cpp
@@ -160,7 +160,9 @@ FP_LIB_TABLE* PROJECT::PcbFootprintLibs()
}
catch( const IO_ERROR& ioe )
{
- DisplayError( NULL, ioe.What() );
+ DisplayErrorMessage( nullptr,
+ _( "Error loading project footprint libraries" ),
+ ioe.What() );
}
}
diff --git a/pcbnew/files.cpp b/pcbnew/files.cpp
index 229e3ad..a88dc56 100644
--- a/pcbnew/files.cpp
+++ b/pcbnew/files.cpp
@@ -520,11 +520,9 @@ bool PCB_EDIT_FRAME::OpenProjectFiles( const std::vector<wxString>& aFileSet, in
}
catch( const IO_ERROR& ioe )
{
- wxString msg = wxString::Format( _(
- "Error loading board.\n%s" ),
- GetChars( ioe.What() )
- );
- DisplayError( this, msg );
+ DisplayErrorMessage( this,
+ wxString::Format( _( "Error loading board file:\n%s" ), fullFileName ),
+ ioe.What() );
return false;
}
diff --git a/pcbnew/onleftclick.cpp b/pcbnew/onleftclick.cpp
index 0da6287..0144cad 100644
--- a/pcbnew/onleftclick.cpp
+++ b/pcbnew/onleftclick.cpp
@@ -544,7 +544,11 @@ void PCB_EDIT_FRAME::OnLeftDClick( wxDC* aDC, const wxPoint& aPosition )
if( curr_item->Type() != PCB_LINE_T )
{
- DisplayError( this, wxT( "curr_item Type error" ) );
+ DisplayErrorMessage(
+ this,
+ _( "Item type is incorrect" ),
+ wxString::Format( _( "Selected item type is %d\n"
+ "Expected: %d" ), curr_item->Type(), PCB_LINE_T ) );
m_canvas->SetAutoPanRequest( false );
break;
}
diff --git a/pcbnew/pcbnew.cpp b/pcbnew/pcbnew.cpp
index 84b6edd..255aa37 100644
--- a/pcbnew/pcbnew.cpp
+++ b/pcbnew/pcbnew.cpp
@@ -357,13 +357,13 @@ bool IFACE::OnKifaceStart( PGM_BASE* aProgram, int aCtlBits )
// if we are here, a incorrect global footprint library table was found.
// Incorrect global footprint library table is not a fatal error:
// the user just has to edit the (partially) loaded table.
- wxString msg = wxString::Format( _(
- "An error occurred attempting to load the global footprint library "
- "table:\n\n%s\n\n"
- "Please edit this global footprint library table in Preferences menu" ),
- GetChars( ioe.What() )
+
+ wxString msg = _(
+ "An error occurred attempting to load the global footprint library table:\n"
+ "Please edit this global footprint library table in Preferences menu"
);
- DisplayError( NULL, msg );
+
+ DisplayErrorMessage( NULL, msg, ioe.What() );
}
#if defined(KICAD_SCRIPTING)
diff --git a/pcbnew/pcbnew_config.cpp b/pcbnew/pcbnew_config.cpp
index ee9dda9..dd4a245 100644
--- a/pcbnew/pcbnew_config.cpp
+++ b/pcbnew/pcbnew_config.cpp
@@ -256,7 +256,11 @@ void PCB_EDIT_FRAME::Process_Config( wxCommandEvent& event )
break;
default:
- DisplayError( this, wxT( "PCB_EDIT_FRAME::Process_Config error" ) );
+ DisplayErrorMessage(
+ this,
+ _( "Unkown ID in Process Config" ),
+ wxString::Format( _( "PCB_EDIT_FRAME::Process_Config received ID %d" ), id ) );
+ break;
}
}
diff --git a/pcbnew/specctra_export.cpp b/pcbnew/specctra_export.cpp
index 0618598..8eead1c 100644
--- a/pcbnew/specctra_export.cpp
+++ b/pcbnew/specctra_export.cpp
@@ -151,9 +151,9 @@ bool PCB_EDIT_FRAME::ExportSpecctraFile( const wxString& aFullFilename )
}
else
{
- errorText += '\n';
- errorText += _( "Unable to export, please fix and try again." );
- DisplayError( this, errorText );
+ DisplayErrorMessage( this,
+ _( "Unable to export, please fix and try again" ),
+ errorText );
}
return ok;
diff --git a/pcbnew/specctra_import.cpp b/pcbnew/specctra_import.cpp
index fd00eff..135035d 100644
--- a/pcbnew/specctra_import.cpp
+++ b/pcbnew/specctra_import.cpp
@@ -85,9 +85,11 @@ void PCB_EDIT_FRAME::ImportSpecctraSession( wxCommandEvent& event )
false );
if( fullFileName == wxEmptyString )
+ {
return;
+ }
- SetCurItem( NULL );
+ SetCurItem( NULL );
// To avoid issues with undo/redo lists (dangling pointers)
// clear the lists
@@ -104,13 +106,14 @@ void PCB_EDIT_FRAME::ImportSpecctraSession( wxCommandEvent& event )
}
catch( const IO_ERROR& ioe )
{
- wxString msg = ioe.What();
- msg += '\n';
- msg += _("BOARD may be corrupted, do not save it.");
- msg += '\n';
- msg += _("Fix problem and try again.");
+ wxString msg = _(
+ "Board may be corrupted, do not save it.\n"
+ "Fix problem and try again"
+ );
+
+ wxString extra = ioe.What();
- DisplayError( this, msg );
+ DisplayErrorMessage( this, msg, extra);
return;
}
@@ -129,7 +132,9 @@ void PCB_EDIT_FRAME::ImportSpecctraSession( wxCommandEvent& event )
// add imported tracks (previous tracks are removed, therfore all are new)
for( TRACK* track = GetBoard()->m_Track; track; track = track->Next() )
+ {
view->Add( track );
+ }
}
SetStatusText( wxString( _( "Session file imported and merged OK." ) ) );
diff --git a/pcbnew/tool_onrightclick.cpp b/pcbnew/tool_onrightclick.cpp
index c397575..81095ff 100644
--- a/pcbnew/tool_onrightclick.cpp
+++ b/pcbnew/tool_onrightclick.cpp
@@ -101,7 +101,9 @@ void FOOTPRINT_EDIT_FRAME::ToolOnRightClick( wxCommandEvent& event )
break;
default:
- DisplayError( this, wxT( "ToolOnRightClick() error" ) );
+ DisplayErrorMessage( this,
+ _( "Invalid tool ID "),
+ wxString::Format( _( "ToolOnRightClick called with ID %d" ), id ) );
break;
}
}
diff --git a/pcbnew/tool_pcb.cpp b/pcbnew/tool_pcb.cpp
index 6f7fa76..6aa12e8 100644
--- a/pcbnew/tool_pcb.cpp
+++ b/pcbnew/tool_pcb.cpp
@@ -813,8 +813,9 @@ void PCB_EDIT_FRAME::OnSelectOptionToolbar( wxCommandEvent& event )
break;
default:
- DisplayError( this,
- wxT( "PCB_EDIT_FRAME::OnSelectOptionToolbar error \n (event not handled!)" ) );
+ DisplayErrorMessage( this,
+ _( "Invalid toolbar option" ),
+ _( "PCB_EDIT_FRAME::OnSelectOptionToolbar error \n (event not handled!)" ) );
break;
}
}
diff --git a/pcbnew/zones_by_polygon.cpp b/pcbnew/zones_by_polygon.cpp
index 39c3bb9..a268296 100644
--- a/pcbnew/zones_by_polygon.cpp
+++ b/pcbnew/zones_by_polygon.cpp
@@ -129,7 +129,7 @@ void PCB_EDIT_FRAME::duplicateZone( wxDC* aDC, ZONE_CONTAINER* aZone )
// do nothing
if( success && ( aZone->GetLayer() == zoneSettings.m_CurrentZone_Layer ) )
{
- DisplayError( this,
+ DisplayErrorMessage( this,
_( "The duplicated zone cannot be on the same layer as the original zone." ) );
success = false;
}
@@ -360,7 +360,7 @@ void PCB_EDIT_FRAME::End_Move_Zone_Corner_Or_Outlines( wxDC* DC, ZONE_CONTAINER*
if( error_count )
{
- DisplayError( this, _( "Area: DRC outline error" ) );
+ DisplayErrorMessage( this, _( "Area: DRC outline error" ) );
}
}
@@ -418,7 +418,7 @@ void PCB_EDIT_FRAME::Remove_Zone_Corner( wxDC* DC, ZONE_CONTAINER* aZone )
if( error_count )
{
- DisplayError( this, _( "Area: DRC outline error" ) );
+ DisplayErrorMessage( this, _( "Area: DRC outline error" ) );
}
}
@@ -532,7 +532,7 @@ int PCB_EDIT_FRAME::Begin_Zone( wxDC* DC )
{
if( GetToolId() == ID_PCB_KEEPOUT_AREA_BUTT && !IsCopperLayer( GetActiveLayer() ) )
{
- DisplayError( this,
+ DisplayErrorMessage( this,
_( "Error: a keepout area is allowed only on copper layers" ) );
return 0;
}
@@ -682,7 +682,7 @@ int PCB_EDIT_FRAME::Begin_Zone( wxDC* DC )
// SCREEN::SetCurItem(), so the DRC error remains on screen.
// PCB_EDIT_FRAME::SetCurItem() calls DisplayInfo().
GetScreen()->SetCurItem( NULL );
- DisplayError( this,
+ DisplayErrorMessage( this,
_( "DRC error: this start point is inside or too close an other area" ) );
return 0;
}
@@ -747,7 +747,7 @@ bool PCB_EDIT_FRAME::End_Zone( wxDC* DC )
if( g_Drc_On && m_drc->Drc( zone, icorner ) == BAD_DRC ) // we can't validate the closing edge
{
- DisplayError( this,
+ DisplayErrorMessage( this,
_( "DRC error: closing this area creates a DRC error with an other area" ) );
m_canvas->MoveCursorToCrossHair();
return false;
@@ -809,7 +809,7 @@ bool PCB_EDIT_FRAME::End_Zone( wxDC* DC )
if( error_count )
{
- DisplayError( this, _( "Area: DRC outline error" ) );
+ DisplayErrorMessage( this, _( "Area: DRC outline error" ) );
}
UpdateCopyOfZonesList( s_PickedList, s_AuxiliaryList, GetBoard() );
--
2.7.4
Follow ups
