I had some no-quality spare time sitting in a train and looked over one of the classes in the plugin.
only refactoring, functionality should be identical.
changes to addtododlg.h :
- changed the constructor: now takes const& instead of copying arguments
- made the destructor virtual
- removed two not necessary members
- some clean up
changes to addtododlg.cpp :
- changed the constructor: now takes const& instead of copying arguments
- moved empty destructor to header file
- removed two not necessary members
- removed some loops to add entries to wxWidgets gui elements which support adding an array of entries
- removed some loops getting the entries from gui elements, when they support retrieving an array
- some clean up
regards
First, thanks for the contribution.
I just review the patch, and I have two questions.
1, I see you have such change:
- if (cmb->GetCount()>0)
- cmb->SetSelection(0);
+ cmb->SetSelection(0);
It is the same thing?
2, I see that you have removed the two member variables:
- wxArrayString m_Users;
- wxArrayString m_Types;
std::bitset<(int)tdctError+1> m_supportedTdcts;
DECLARE_EVENT_TABLE()
When I read the code, I see that inside the function:
void AddTodoDlg::EndModal(int retVal)
{
if (retVal == wxID_OK)
{
- // "save" users
wxChoice* cmb = XRCCTRL(*this, "chcUser", wxChoice);
- m_Users.Clear();
- if (cmb->FindString(cmb->GetStringSelection(), true) == wxNOT_FOUND)
- m_Users.Add(cmb->GetStringSelection());
- for (unsigned int i = 0; i < cmb->GetCount(); ++i)
- m_Users.Add(cmb->GetString(i));
- Manager::Get()->GetConfigManager(_T("todo_list"))->Write(_T("users"), m_Users);
+ Manager::Get()->GetConfigManager(_T("todo_list"))->Write(_T("users"), cmb->GetStrings());
Manager::Get()->GetConfigManager(_T("todo_list"))->Write(_T("last_used_user"), cmb->GetStringSelection());
- // "save" types
cmb = XRCCTRL(*this, "chcType", wxChoice);
- m_Types.Clear();
- if (cmb->FindString(cmb->GetStringSelection(), true) == wxNOT_FOUND)
- m_Types.Add(cmb->GetStringSelection());
- for (unsigned int i = 0; i < cmb->GetCount(); ++i)
- m_Types.Add(cmb->GetString(i));
- Manager::Get()->GetConfigManager(_T("todo_list"))->Write(_T("types"), m_Types);
+ Manager::Get()->GetConfigManager(_T("todo_list"))->Write(_T("types"), cmb->GetStrings());
Manager::Get()->GetConfigManager(_T("todo_list"))->Write(_T("last_used_type"), cmb->GetStringSelection());
cmb = XRCCTRL(*this, "chcStyle", wxChoice);
Manager::Get()->GetConfigManager(_T("todo_list"))->Write(_T("last_used_style"), cmb->GetStringSelection());
Here, you just save the types and users from the GUI (through cmb->GetStrings()) to the configure files.
But when I looked at the usage of the dialog:
void ToDoList::OnAddItem(cb_unused wxCommandEvent& event)
{
cbEditor* ed = Manager::Get()->GetEditorManager()->GetBuiltinActiveEditor();
if (!ed)
return;
EditorColourSet* colour_set = Manager::Get()->GetEditorManager()->GetColourSet();
if (!colour_set)
return;
HighlightLanguage hlang = colour_set->GetLanguageName(ed->GetLanguage());
bool edIsCCpp = (hlang == _T("C/C++"));
CommentToken token = colour_set->GetCommentToken(ed->GetLanguage());
bool hasStreamComments = not token.streamCommentStart.IsEmpty();
bool hasLineComments = not token.lineComment.IsEmpty();
if (!(edIsCCpp||hasLineComments||hasStreamComments))
return;
std::bitset<(int)tdctError+1> supportedTdcts;
supportedTdcts[tdctLine] = !(token.lineComment.IsEmpty());
supportedTdcts[tdctStream] = !(token.streamCommentStart.IsEmpty());
supportedTdcts[tdctDoxygenLine] = !(token.doxygenLineComment.IsEmpty());
supportedTdcts[tdctDoxygenStream] = !(token.doxygenStreamCommentStart.IsEmpty());
supportedTdcts[tdctWarning] = edIsCCpp;
supportedTdcts[tdctError] = edIsCCpp;
// display todo dialog
AddTodoDlg dlg(Manager::Get()->GetAppWindow(), m_Users, m_Types, supportedTdcts );
PlaceWindow(&dlg);
if (dlg.ShowModal() != wxID_OK)
return;
// Re-load users and types as they might have changed by the user via AddTodoDlg
LoadUsers();
LoadTypes();
Here, in the LoadUsers(); and LoadTypes(); function, it was saved again in the configure file.
My question is: why do we need to pass the "m_Users, m_Types" as an argument to the AddTodoDlg's constructor, it looks like in the ToDoList class, we don't need the two "m_Users, m_Types" member variables either.
Inside the constructor of AddTodoDlg, it also contains some code like:
if (users.empty())
cmb->Append(wxGetUserId());
Is GetChar() really checked? I couldn’t find anything about it (that means, I may just not know, where to look...)
Check wx/string.h:
wxChar GetChar(size_t n) const
{ return at(n); }
...later on:
value_type at(size_type n) const
{ wxASSERT_VALID_INDEX( n ); return m_pchData[n]; }
The assert macro is probably "nothing" in release builds. But it helps debugging in debug builds.
Please use GetChar. We handle many files, make complex parsing, may encounter weird encodings - its the best you can do it such situation.
Trying a C:B compiled with wx-3.0 I get an error, that I have not seen before:
ASSERT INFO:
/usr/include/wx-3.0/wx/string.h(1536): assert "!empty()" failed in Last(): wxString: index out of bounds
clean vanilla trunk svn10781, ubuntu-16.04 x64. Full log including backtrace is attached.
in file todolistview.cpp:569 there is:
It must have been called on an empty wxString user.
In the attached patch I inserted check for being empty.
Unfortunately, I am stuck in starting C::B from within C::B. Somehow I can not get the CODEBLOCKS_DATA_DIR right.
I Tried to enter the proposed path via global settings and from a shell via --prefix and environment variable there.
Usually, I compile C::B always via makefile. Please bear with me, that I am not used to do it.
error while loading shared libraries: libwxpropgrid.so cannot open shared object file: No such file or directory.
I checked. It is in the same directory as the newly created codeblocks binary.
If possible, can somebody give me a hint, please?
Thank you.
I see such code change:
@ -215,24 +184,26 @@ void AddTodoDlg::EndModal(int retVal)
void AddTodoDlg::OnAddUser(wxCommandEvent&)
{
// ask for the new user to be added to the "choice" list
- const wxString &User = cbGetTextFromUser(_T("Enter the user you wish to add"), _T("Add user"), wxEmptyString, this);
- if (!User.empty())
+ wxTextEntryDialog dlg(this, _T("Enter the user you wish to add"), _T("Add user"), _T(""), wxOK|wxCANCEL);
+ if(dlg.ShowModal() == wxID_OK)
+ {
+ const wxString User = dlg.GetValue();
+ if(!User.empty())
XRCCTRL(*this, "chcUser", wxChoice)->Append(User);
-}
+ }
It looks like we use cbGetTextFromUser in many places in our C::B's source code, so why do you use wxTextEntryDialog?
In global.cpp, this is the code:
wxString cbGetTextFromUser(const wxString& message, const wxString& caption, const wxString& defaultValue,
wxWindow *parent, wxCoord x, wxCoord y, bool centre)
{
if (!parent)
parent = Manager::Get()->GetAppWindow();
long style = wxTextEntryDialogStyle;
if (centre)
style |= wxCENTRE;
else
style &= ~wxCENTRE;
wxTextEntryDialog dialog(parent, message, caption, defaultValue, style, wxPoint(x, y));
PlaceWindow(&dialog);
wxString str;
if (dialog.ShowModal() == wxID_OK)
str = dialog.GetValue();
return str;
}
PS1: please add a SF ticket, so this contribution won't lost.
PS2: I think the big patch need to split into some small ones. :)
@frithjofh, I commit the changes you made in rev 10923 and 10924.
The change I don't commit are below:
cc864a0d38bf4b359097c256b51ec772dfe59cf6
src/plugins/todo/addtododlg.cpp | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/plugins/todo/addtododlg.cpp b/src/plugins/todo/addtododlg.cpp
index dbbacbb..e28df93 100644
--- a/src/plugins/todo/addtododlg.cpp
+++ b/src/plugins/todo/addtododlg.cpp
@@ -196,7 +196,7 @@ void AddTodoDlg::OnDelUser(wxCommandEvent&)
if (sel == wxNOT_FOUND)
return;
- wxString msg; msg.Printf(_T("Are you sure you want to delete the user '%s'?"), cmb->GetString(sel).c_str());
+ wxString msg; msg.Printf(_T("Are you sure you want to delete the user '%s'?"), cmb->GetStringSelection());
if (cbMessageBox(msg, _T("Confirmation"), wxICON_QUESTION | wxYES_NO, this) == wxID_NO)
return;
@@ -220,7 +220,7 @@ void AddTodoDlg::OnDelType(wxCommandEvent&)
if (sel == wxNOT_FOUND)
return;
- wxString msg; msg.Printf(_T("Are you sure you want to delete the type '%s'?"), cmb->GetString(sel).c_str());
+ wxString msg; msg.Printf(_T("Are you sure you want to delete the type '%s'?"), cmb->GetStringSelection());
if (cbMessageBox(msg, _T("Confirmation"), wxICON_QUESTION | wxYES_NO, this) == wxID_NO)
return;
This cause build error, I think either c_str() or wx_str() is OK.
src/plugins/todo/addtododlg.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/plugins/todo/addtododlg.h b/src/plugins/todo/addtododlg.h
index 42eb607..a19d28c 100644
--- a/src/plugins/todo/addtododlg.h
+++ b/src/plugins/todo/addtododlg.h
@@ -12,8 +12,8 @@
#include "scrollingdialog.h"
-class wxArrayString;
class wxWindow;
+class wxArrayString;
class wxCommandEvent;
enum ToDoPosition
I don't think this is needed.
src/plugins/todo/addtododlg.cpp | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/src/plugins/todo/addtododlg.cpp b/src/plugins/todo/addtododlg.cpp
index e28df93..1d6b8cf 100644
--- a/src/plugins/todo/addtododlg.cpp
+++ b/src/plugins/todo/addtododlg.cpp
@@ -16,6 +16,7 @@
#include <wx/spinctrl.h>
#include <wx/textctrl.h>
#include <wx/xrc/xmlres.h>
+ #include <wx/textdlg.h> //wxTextEntryDialog
#include "manager.h"
#include "configmanager.h"
@@ -184,9 +185,13 @@ void AddTodoDlg::EndModal(int retVal)
void AddTodoDlg::OnAddUser(wxCommandEvent&)
{
// ask for the new user to be added to the "choice" list
- const wxString &User = cbGetTextFromUser(_T("Enter the user you wish to add"), _T("Add user"), wxEmptyString, this);
- if (!User.empty())
+ wxTextEntryDialog dlg(this, _T("Enter the user you wish to add"), _T("Add user"), _T(""), wxOK|wxCANCEL);
+ if(dlg.ShowModal() == wxID_OK)
+ {
+ const wxString User = dlg.GetValue();
+ if(!User.empty())
XRCCTRL(*this, "chcUser", wxChoice)->Append(User);
+ }
}
void AddTodoDlg::OnDelUser(wxCommandEvent&)
@@ -208,9 +213,13 @@ void AddTodoDlg::OnDelUser(wxCommandEvent&)
void AddTodoDlg::OnAddType(wxCommandEvent&)
{
// ask for the new type to be added to the "choice" list
- const wxString &Type = cbGetTextFromUser(_T("Enter the type you wish to add"), _T("Add type"), wxEmptyString, this);
- if (!Type.empty())
+ wxTextEntryDialog dlg(this, _T("Enter the type you wish to add"), _T("Add type"), _T(""), wxOK|wxCANCEL);
+ if(dlg.ShowModal() == wxID_OK)
+ {
+ const wxString Type = dlg.GetValue();
+ if(!Type.empty())
XRCCTRL(*this, "chcType", wxChoice)->Append(Type);
+ }
}
void AddTodoDlg::OnDelType(wxCommandEvent&)
I think we need the cb buidin's method cbGetTextFromUser.