ToDo plugin patch doing some refactoring

[frithjofh]

ok, thx,

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

[frithjofh]

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

[frithjofh]

just to revive this thread

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

[MortenMacFly]

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.

[frithjofh]

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

[MortenMacFly]

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

I would prefer to edit it for the mentioned reasons.

[frithjofh]

attached the modified patch

[blauzahn]

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

Code

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

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

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.

[oBFusCATed]

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

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. 

[ollydbg]

I see such code change:

Code

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

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. 

[ollydbg]

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

The change I don't commit are below:

Code

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

 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.

Code

 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.