Author Topic: ToDo plugin patch doing some refactoring  (Read 21824 times)

Offline frithjofh

  • Regular
  • ***
  • Posts: 376
Re: ToDo plugin patch doing some refactoring
« Reply #15 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.
architect with some spare time  -  c::b compiled from last svn  -   openSuSE leap x86_64  -  AMD FX-4100

Offline frithjofh

  • Regular
  • ***
  • Posts: 376
Re: ToDo plugin patch doing some refactoring
« Reply #16 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...
architect with some spare time  -  c::b compiled from last svn  -   openSuSE leap x86_64  -  AMD FX-4100

Offline frithjofh

  • Regular
  • ***
  • Posts: 376
Re: ToDo plugin patch doing some refactoring
« Reply #17 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()
architect with some spare time  -  c::b compiled from last svn  -   openSuSE leap x86_64  -  AMD FX-4100

Offline MortenMacFly

  • Administrator
  • Lives here!
  • *****
  • Posts: 9694
Re: ToDo plugin patch doing some refactoring
« Reply #18 on: January 21, 2016, 11:39:59 am »
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.
Compiler logging: Settings->Compiler & Debugger->tab "Other"->Compiler logging="Full command line"
C::B Manual: https://www.codeblocks.org/docs/main_codeblocks_en.html
C::B FAQ: https://wiki.codeblocks.org/index.php?title=FAQ

Offline frithjofh

  • Regular
  • ***
  • Posts: 376
Re: ToDo plugin patch doing some refactoring
« Reply #19 on: January 21, 2016, 11:57:27 am »
so, should I edit the patch accordingly or leave it as is?
architect with some spare time  -  c::b compiled from last svn  -   openSuSE leap x86_64  -  AMD FX-4100

Offline MortenMacFly

  • Administrator
  • Lives here!
  • *****
  • Posts: 9694
Re: ToDo plugin patch doing some refactoring
« Reply #20 on: January 21, 2016, 12:07:34 pm »
so, should I edit the patch accordingly or leave it as is?
I would prefer to edit it for the mentioned reasons.
Compiler logging: Settings->Compiler & Debugger->tab "Other"->Compiler logging="Full command line"
C::B Manual: https://www.codeblocks.org/docs/main_codeblocks_en.html
C::B FAQ: https://wiki.codeblocks.org/index.php?title=FAQ

Offline frithjofh

  • Regular
  • ***
  • Posts: 376
Re: ToDo plugin patch doing some refactoring
« Reply #21 on: January 22, 2016, 12:37:42 pm »
attached the modified patch
architect with some spare time  -  c::b compiled from last svn  -   openSuSE leap x86_64  -  AMD FX-4100

Offline blauzahn

  • Almost regular
  • **
  • Posts: 153
Re: ToDo plugin patch doing some refactoring
« Reply #22 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
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.


Offline oBFusCATed

  • Developer
  • Lives here!
  • *****
  • Posts: 13413
    • Travis build status
Re: ToDo plugin patch doing some refactoring
« Reply #23 on: February 21, 2016, 06:10:04 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...

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)
(most of the time I ignore long posts)
[strangers don't send me private messages, I'll ignore them; post a topic in the forum, but first read the rules!]

Online ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5906
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re: ToDo plugin patch doing some refactoring
« Reply #24 on: September 15, 2016, 05:42:34 pm »
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.  :)
« Last Edit: September 16, 2016, 04:49:24 pm by ollydbg »
If some piece of memory should be reused, turn them to variables (or const variables).
If some piece of operations should be reused, turn them to functions.
If they happened together, then turn them to classes.

Online ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5906
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re: ToDo plugin patch doing some refactoring
« Reply #25 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
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.
If some piece of memory should be reused, turn them to variables (or const variables).
If some piece of operations should be reused, turn them to functions.
If they happened together, then turn them to classes.