Code::Blocks

Developer forums (C::B DEVELOPMENT STRICTLY!) => Plugins development => Topic started by: frithjofh on November 20, 2015, 09:46:46 am

Title: ToDo plugin patch doing some refactoring
Post by: frithjofh on November 20, 2015, 09:46:46 am

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

Title: Re: ToDo plugin patch doing some refactoring
Post by: frithjofh on November 22, 2015, 09:33:58 am

made some changes to the original post to include the changes made

Title: Re: ToDo plugin patch doing some refactoring
Post by: oBFusCATed on November 23, 2015, 02:10:16 am

Please minimize the number of topics you post, especially when you post patches for a single plugin...

Title: Re: ToDo plugin patch doing some refactoring
Post by: frithjofh on November 23, 2015, 09:54:27 am

understood ;) - will do

will you be testing these patches?

Title: Re: ToDo plugin patch doing some refactoring
Post by: oBFusCATed on November 23, 2015, 04:19:03 pm

Won't have time in the next two weeks.

Title: Re: ToDo plugin patch doing some refactoring
Post by: frithjofh on November 30, 2015, 12:34:52 pm

looked through the todolistview.h and todolistview.cpp

attached patch:

- removed two functions from header and made them non member functions to clean up the interface
- refactored the line counting function to reflect more transparently what it is doing and how it is used
- refactored some functions to take their arguments as const&
- added const to some member fucnctions
- refactored some functions to use for loops instead of while loops
- for consistency changed post-increment to increment where possible
- refactored the parsebuffer function:
   - added const where ever possible
   - simplified some code using already existent functions
   - replaced << with += on strings where ever possible
   - replaced wxString and wxArrayString functions with their stl counterparts admitted by wxWidgets. makes code clearer and makes better use of highlighting
   - made a string static

regards

Title: Re: ToDo plugin patch doing some refactoring
Post by: ollydbg on December 05, 2015, 03:12:43 pm

Quote from: frithjofh on November 20, 2015, 09:46:46 am

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:

Code: [Select]

-    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:

Code: [Select]

-        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:

Code: [Select]

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:

Code: [Select]

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:

Code: [Select]

    if (users.empty())
        cmb->Append(wxGetUserId());


Title: Re: ToDo plugin patch doing some refactoring
Post by: ollydbg on December 05, 2015, 03:39:16 pm

Quote from: frithjofh on November 30, 2015, 12:34:52 pm

looked through the todolistview.h and todolistview.cpp

attached patch:

- removed two functions from header and made them non member functions to clean up the interface
- refactored the line counting function to reflect more transparently what it is doing and how it is used
- refactored some functions to take their arguments as const&
- added const to some member fucnctions
- refactored some functions to use for loops instead of while loops
- for consistency changed post-increment to increment where possible
- refactored the parsebuffer function:
   - added const where ever possible
   - simplified some code using already existent functions
   - replaced << with += on strings where ever possible
   - replaced wxString and wxArrayString functions with their stl counterparts admitted by wxWidgets. makes code clearer and makes better use of highlighting
   - made a string static

regards

Thanks for the contribution! I read the patch, and I see generally its OK, but one question:
What is the difference between

Code: [Select]

buffer.GetChar(pos)and

Code: [Select]

buffer[pos]If I remember correctly, in our CodeCompletion's code, it use GetChar, does it have performance issue? Since the operator return a reference?

Title: Re: ToDo plugin patch doing some refactoring
Post by: frithjofh on December 05, 2015, 05:51:58 pm

@ollydbg

about the buffer.GetChar() and the buffer[] difference.

before and after the change I instrumented the code with std::chrono::high_resolution_clock to get the actual time spent in the function. this particular change didn't show any slowdown. for me it was more a matter of habit and readability. but I did not look into the generated machine code to see, if there was any actual difference. please feel free to revert to .GetChar().

and many thanks for the feedback

Title: Re: ToDo plugin patch doing some refactoring
Post by: frithjofh on December 05, 2015, 05:59:27 pm

Quote

1, I see you have such change:

Code: [Select]

-    if (cmb->GetCount()>0)
-      cmb->SetSelection(0);
+    cmb->SetSelection(0);
It is the same thing?

I could find none. if the numbers of items is 0, selecting the zero index element has no effect at all. the widget takes care of that by itself, no need to double-check

Quote

2, I see that you have removed the two member variables:

Code: [Select]

-        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:

Code: [Select]

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:

Code: [Select]

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:

Code: [Select]

    if (users.empty())
        cmb->Append(wxGetUserId());


I think you are right. these arguments are not really needed either. as it is right now, the plugin interchanges this state via the configmgr already. slipped my attention...

Title: Re: ToDo plugin patch doing some refactoring
Post by: frithjofh on December 12, 2015, 05:43:22 pm

for the speed argument between subscript and GetChar() on wxString i put this quote from wxWidgets:

Quote

Reference counting and why you shouldn't care about it

All considerations for wxObject-derived reference counted objects are valid also for wxString, even if it does not derive from wxObject.

Probably the unique case when you might want to think about reference counting is when a string character is taken from a string which is not a constant (or a constant reference). In this case, due to C++ rules, the "read-only" operator[] (which is the same as GetChar()) cannot be chosen and the "read/write" operator[] (the same as GetWritableChar()) is used instead. As the call to this operator may modify the string, its data is unshared (COW is done) and so if the string was really shared there is some performance loss (both in terms of speed and memory consumption). In the rare cases when this may be important, you might prefer using GetChar() instead of the array subscript operator for this reasons. Please note that at() method has the same problem as the subscript operator in this situation and so using it is not really better. Also note that if all string arguments to your functions are passed as const wxString& (see the section Some advice) this situation will almost never arise because for constant references the correct operator is called automatically.

as buffer is a const wxString& there should be no difference. But I will do a measurement after Christmas or when ever I will have time...

Title: Re: ToDo plugin patch doing some refactoring
Post by: ollydbg on December 13, 2015, 06:22:49 am

Thanks for the clarification.
I think I will handle those patches after our 15.12 release. You can just ping on this thread if I forget about it.  :)

Title: Re: ToDo plugin patch doing some refactoring
Post by: MortenMacFly on December 13, 2015, 06:58:12 am

Quote from: ollydbg on December 05, 2015, 03:39:16 pm

What is the difference between

Code: [Select]

buffer.GetChar(pos)and

Code: [Select]

buffer[pos]If I remember correctly, in our CodeCompletion's code, it use GetChar, does it have performance issue? Since the operator return a reference?

IMHO the difference is that the first one is checked, the other one not and therefore a potential crash candidate.

Title: Re: ToDo plugin patch doing some refactoring
Post by: frithjofh on December 13, 2015, 06:36:52 pm

if GetChar() is checked, then it must be slower than subscript...

What would be the right decision? Crash candidate or speed? Using subscript the function surely could be written so, that pos must always be a valid index into buffer, with only one check for validity some place...

Is GetChar() really checked? I couldn’t find anything about it (that means, I may just not know, where to look...)

Title: Re: ToDo plugin patch doing some refactoring
Post by: MortenMacFly on December 13, 2015, 07:48:06 pm

Quote from: frithjofh on December 13, 2015, 06:36:52 pm

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:

Code: C++

1.     wxChar  GetChar(size_t n) const
2.       { return at(n); }
3.  

...later on:

Code: C++

1.   value_type at(size_type n) const
2.     { wxASSERT_VALID_INDEX( n ); return m_pchData[n]; }
3.  

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.

Title: Re: ToDo plugin patch doing some refactoring
Post by: frithjofh on December 13, 2015, 09:03:04 pm

ok, thx,

I will take another look at the changes I made. not sure if I will have time before Xmas though.

Title: Re: ToDo plugin patch doing some refactoring
Post by: frithjofh on December 14, 2015, 11:25:43 am

@morten

I looked through my implementation of wxwidgets and there the subscript is implemented just like that too...

It has a the same body as GetChar(), that is, just a call to at()... essentially the same it seems.

I only looked in wxGTK, the current 2.8 stable, so maybe there is some difference with the implementation you checked.

I am a bit confused now...

Title: Re: ToDo plugin patch doing some refactoring
Post by: frithjofh on January 17, 2016, 03:29:27 pm

just to revive this thread

current situation is the question whether GetChar() and subscript are essentially the same, as both are implemented using at()

Title: Re: ToDo plugin patch doing some refactoring
Post by: MortenMacFly on January 21, 2016, 11:39:59 am

Quote from: frithjofh on January 17, 2016, 03:29:27 pm

current situation is the question whether GetChar() and subscript are essentially the same, as both are implemented using at()

In that case it seems the same. But you cannot always rely on it. IMHO (at least in earlier versions) in STL its not.

As a rule of thumb: To make it bullet-proof don't use [] unless you're 100% sure you are doing the right thing and/or care for speed.

Title: Re: ToDo plugin patch doing some refactoring
Post by: frithjofh on January 21, 2016, 11:57:27 am

so, should I edit the patch accordingly or leave it as is?

Title: Re: ToDo plugin patch doing some refactoring
Post by: MortenMacFly on January 21, 2016, 12:07:34 pm

Quote from: frithjofh on January 21, 2016, 11:57:27 am

so, should I edit the patch accordingly or leave it as is?

I would prefer to edit it for the mentioned reasons.

Title: Re: ToDo plugin patch doing some refactoring
Post by: frithjofh on January 22, 2016, 12:37:42 pm

attached the modified patch

Title: Re: ToDo plugin patch doing some refactoring
Post by: blauzahn on February 21, 2016, 05:48:08 pm

Trying a C:B compiled with wx-3.0 I get an error, that I have not seen before:

Code: [Select]

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:

Code: [Select]

item.user.Last()
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.

Code: [Select]

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.

Title: Re: ToDo plugin patch doing some refactoring
Post by: oBFusCATed on February 21, 2016, 06:10:04 pm

Quote from: blauzahn on February 21, 2016, 05:48:08 pm

In the attached patch I inserted check for being empty.

The attached patch is not 100% correct, next time please at least try to compile the code. :)
Anyway, I'll commit the proper patch, soon...

Quote from: blauzahn on February 21, 2016, 05:48:08 pm

Unfortunately, I am stuck in starting C::B from within C::B. Somehow I can not get the CODEBLOCKS_DATA_DIR right.....

You have to run the update or update30 script in the src folder.  8)

Title: Re: ToDo plugin patch doing some refactoring
Post by: ollydbg on September 15, 2016, 05:42:34 pm

I see such code change:

Code: [Select]

@ -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:

Code: [Select]

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.  :)

Title: Re: ToDo plugin patch doing some refactoring
Post by: ollydbg on November 26, 2016, 09:05:23 am

@frithjofh, I commit the changes you made in rev 10923 and 10924.

The change I don't commit are below:

Code: [Select]

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.

Code: [Select]

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 ToDoPositionI don't think this is needed.

Code: [Select]

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.