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

Offline frithjofh

  • Regular
  • ***
  • Posts: 376
ToDo plugin patch doing some refactoring
« 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
« Last Edit: November 22, 2015, 09:33:32 am by frithjofh »
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 #1 on: November 22, 2015, 09:33:58 am »
made some changes to the original post to include the changes made
architect with some spare time  -  c::b compiled from last svn  -   openSuSE leap x86_64  -  AMD FX-4100

Offline oBFusCATed

  • Developer
  • Lives here!
  • *****
  • Posts: 13413
    • Travis build status
Re: ToDo plugin patch doing some refactoring
« Reply #2 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...
(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!]

Offline frithjofh

  • Regular
  • ***
  • Posts: 376
Re: ToDo plugin patch doing some refactoring
« Reply #3 on: November 23, 2015, 09:54:27 am »
understood ;) - will do

will you be testing these patches?
architect with some spare time  -  c::b compiled from last svn  -   openSuSE leap x86_64  -  AMD FX-4100

Offline oBFusCATed

  • Developer
  • Lives here!
  • *****
  • Posts: 13413
    • Travis build status
Re: ToDo plugin patch doing some refactoring
« Reply #4 on: November 23, 2015, 04:19:03 pm »
Won't have time in the next two weeks.
(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!]

Offline frithjofh

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

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5905
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re: ToDo plugin patch doing some refactoring
« Reply #6 on: December 05, 2015, 03:12:43 pm »
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
-    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
-        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
 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
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
    if (users.empty())
        cmb->Append(wxGetUserId());
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.

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5905
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re: ToDo plugin patch doing some refactoring
« Reply #7 on: December 05, 2015, 03:39:16 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
buffer.GetChar(pos)
and
Code
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?

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.

Offline frithjofh

  • Regular
  • ***
  • Posts: 376
Re: ToDo plugin patch doing some refactoring
« Reply #8 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
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 #9 on: December 05, 2015, 05:59:27 pm »
Quote
1, I see you have such change:
Code
-    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
-        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
 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
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
    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...
« Last Edit: December 05, 2015, 06:01:12 pm by frithjofh »
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 #10 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...
architect with some spare time  -  c::b compiled from last svn  -   openSuSE leap x86_64  -  AMD FX-4100

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5905
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re: ToDo plugin patch doing some refactoring
« Reply #11 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.  :)
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.

Offline MortenMacFly

  • Administrator
  • Lives here!
  • *****
  • Posts: 9694
Re: ToDo plugin patch doing some refactoring
« Reply #12 on: December 13, 2015, 06:58:12 am »
What is the difference between
Code
buffer.GetChar(pos)
and
Code
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.
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 #13 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...)
« Last Edit: December 13, 2015, 06:53:21 pm by frithjofh »
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 #14 on: December 13, 2015, 07:48:06 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: cpp
    wxChar  GetChar(size_t n) const
      { return at(n); }
...later on:

Code: cpp
  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.
« Last Edit: December 13, 2015, 07:55:58 pm by MortenMacFly »
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 #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!]

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5905
  • 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.

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5905
  • 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.