Code::Blocks Forums

Developer forums (C::B DEVELOPMENT STRICTLY!) => Development => Topic started by: Bat on October 13, 2014, 11:37:02 pm

Title: "Goto Function" Window
Post by: Bat on October 13, 2014, 11:37:02 pm
I've provided 2 separate patchs for :

1)Try to enhance this window. Presentation is a mater of taste, comment ...
https://sourceforge.net/p/codeblocks/tickets/66/ (https://sourceforge.net/p/codeblocks/tickets/66/)
Before
(http://batcerise.no-ip.com/ro/Image2.png)
After
(http://batcerise.no-ip.com/ro/Image3.png)

2)As I must always resize this window, another patch to save/restore windows position
https://sourceforge.net/p/codeblocks/tickets/65/
One (major) drawback, is that all theses positions will end up in .conf file
Second is that some race case (monitor layout change) have to be handled ...
Title: Re: "Goto Function" Window
Post by: oBFusCATed on October 13, 2014, 11:58:59 pm
For 66:
1. You've left some commented code
2. The indentations look odd
3. There are no comments what you're doing
4. You're setting a font explicitly by name
5. You're using c style struct definitions, which look odd in c++
6. The sort function look slow
8. Why don't you use a wxlistctrl instead of wxlistbox?
9. Naming variables with two letters is rather bad
10. Prefer std::vector to wxarrays
11. The { is placed at the wrong place according to our minimal style guide

If you fix 8 probably most of the others won't be valid any more.
Also I'm not sure if your version of the UI is better so it is probably better idea to introduce an option.

For 65:
This patch looks rather dangerous to me. Who is responsible to create or destroy WindowClosed objects?
What is the value of the this pointer when the on close event is triggered?
What happens if the window is last created on the second monitor which is non-existent anymore!?
Title: Re: "Goto Function" Window
Post by: Bat on October 14, 2014, 08:47:03 am
Thanks for feedback

Ticket 66:
wxlistctrl which option seem the best ?
-Make a specific dialog on src/gotofunctiondlg.cpp (and add here option for enabling/disabling this behaviour)
-Add this 'list ctrl' "option" to IncrementalSelectListDlg (sdk/incrementalselectlistdlg.cpp)
-other ?

[Edited : wrong ticket number]
Title: Re: "Goto Function" Window
Post by: oBFusCATed on October 14, 2014, 09:35:21 am
Quote from: Bat on October 14, 2014, 08:47:03 am
-Make a specific dialog on src/gotofunctiondlg.cpp (and add here option for enablinvg/disabling this behaviour)
This would be the easier option.

Quote from: Bat on October 14, 2014, 08:47:03 am
-Add this 'list ctrl' "option" to IncrementalSelectListDlg (sdk/incrementalselectlistdlg.cpp)
This would probably require making the api hard to use or you'll have to add serious hacks to keep it compatible.
Title: Re: "Goto Function" Window
Post by: Bat on October 27, 2014, 12:05:40 am
Here is a fully reworked version of this patch :
https://sourceforge.net/p/codeblocks/tickets/66/

It's now a custom dialog gotofunctiondlg.cpp that handle standard goto function mode and "ColumnMode"
I've tried to correct all other formatting things.
Title: Re: "Goto Function" Window
Post by: oBFusCATed on October 28, 2014, 01:32:31 am
Could you try to extract the common code in either a base class or in several functions?
I'm not sure I like the code duplication there. (gotofunctiondlg vs incrementalsearchdlg)
Title: Re: "Goto Function" Window
Post by: Bat on October 28, 2014, 10:51:38 pm
I've tried to do something :
https://sourceforge.net/p/codeblocks/tickets/66/
Title: Re: "Goto Function" Window
Post by: oBFusCATed on October 29, 2014, 01:06:46 am
Answered in the ticket.

@devs: Do you see any reason such a change to be rejected. I don't use this feature so I cannot comment on its usefulness. I'm commenting only on the code...
Title: Re: "Goto Function" Window
Post by: Bat on October 29, 2014, 11:04:29 pm
Quote from: oBFusCATed on October 13, 2014, 11:58:59 pm
For 65:
This patch looks rather dangerous to me. Who is responsible to create or destroy WindowClosed objects?
What is the value of the this pointer when the on close event is triggered?
What happens if the window is last created on the second monitor which is non-existent anymore!?

I've updated it :
https://sourceforge.net/p/codeblocks/tickets/65/

For WindowClosed object, good question for creation/deletion. I've tried to remove Class definition here, but no success. From my undesrtanding
Code
    &WindowClosed::OnWindowClosed
is a member function pointer, no instance creation/destruction, static binding

Code
this
pointer in
Code
OnWindowClosed
is also invalid and must not be used. It acts as a pure static function. May be I missed something ?

For multi monitor, I've tested it. I've added basic check that put window back to default position if it's not enought visible on at least one screen of system