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.walt...@gmail.com> 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.walt...@gmail.com> 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
_______________________________________________ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp