Developer forums (C::B DEVELOPMENT STRICTLY!) > Development
User interface for project glob feature
oBFusCATed:
@bluehazzard: Looks good.
@killerbot: I'm still using wx28 on my works centos 6. I'm in a process of switching to wx3.x there.
BlueHazzard:
Ok, i posted a ticket with a patch
https://sourceforge.net/p/codeblocks/tickets/729/
i can not test it on linux at the moment, but it should work.
(Can we work on reducing the amount of project files? this is so a pain in the a** to keep in sync all this project files)
Missing are the makefiles....
oBFusCATed:
Here are some comments on the patch (starting from the top of the file to the bottom):
1. Mismatch between the comment and the name of AddGlob (glob vs globs).
2. The plugin version should be increased because of the addition of AddGlob
3. Get/SetGlobs should be added to the script bindings. Can we have tests for these 3?
4. dlgEditProjectGlob should be EditProjectGlobDlg for consistency
4.1. StaticText1, StaticText2 shouldn't be members
4.2. The click handle methods should have better names, using the names created by wxsmith is rarely a good idea
4.3. filenames should be lower case and if you need more clarity you can use underscores. This prevents nasty case sensitivity problems in different oses.
4.4. The case for variables should be consistent.
4.5. The custom var logic needs better documentation.
4.6. We don't use underscores for variables, use camelCase.
4.7. Prefer empty instead of IsEmpty and ==wxEmptyString
4.8. the cbMessageBox condition at the end of OnbtnBrowseClick is more complicated than it could be - the two blocks are almost the same
4.9. We align to the open bracket when long lines are truncated.
5. dlgManageGlobs should be ManageGlobsDlg
5.1. same for filenames all lowercase please
5.2. Duplicating Manager::Get()->GetProjectManager()->GetActiveProject() wastes resources and makes code harder to read
5.3. It is better to use wxString::Format when there is a string which will be translated instead of using operator+, different languages have different word order.
5.4. Start all columns with capital letter?
5.5. Again do not use underscores in variable names
5.6. Prefer using std::vector instead of wxArray*
5.7. Are you sure OnbtnDeleteClick works correctly when deleting multiple items? I'm pretty sure you need to iterate items backwards.
5.8. Repeated calls to prj->GetGlobs()[i\] aren't good idea - store them in variables
6. Do not pass nullptr when creating dialogs. It leads to really bad problems on linux.
BlueHazzard:
ok, i will work this list down...
--- Quote ---1. Mismatch between the comment and the name of AddGlob (glob vs globs).
--- End quote ---
Can you point out the line in the patch?
I think AddGlob is the singular (add one glob entry), and SetGlobs is plural (edit multiple globs)?
--- Quote ---3. Get/SetGlobs should be added to the script bindings. Can we have tests for these 3?
--- End quote ---
Not really... there is no independent read back possibility from script bindings, to check the success... If i remember correctly i have not implemented any test for all project functions, because it was not possible to create a project from the script binding and get a independent result from the function (compare a pre made project file for example) ... Testing project binding should not be topic for this patch... If there is need we should create a new topic/ticket
--- Quote ---4.2. The click handle methods should have better names, using the names created by wxsmith is rarely a good idea
--- End quote ---
suggestions? I think OnbtnBrowseClick nicely verbose :
--- Code: ---"On" --- On (obviously english adverb)
"btn" --- Button
"Browse" --- Button name
"Click" --- on what? --> on click
"btnBrowse" --- also Variable name of button...
--- End code ---
--- Quote ---4.3. filenames should be lower case and if you need more clarity you can use underscores. This prevents nasty case sensitivity problems in different oses.
--- End quote ---
I have not found any camel case / upper case filename in the patch?
--- Quote ---5.3. It is better to use wxString::Format when there is a string which will be translated instead of using operator+, different languages have different word order.
--- End quote ---
Doesn't this add unicode problems? Or how do i printf a wxString with unicode support?
--- Quote ---5.8. Repeated calls to prj->GetGlobs()[i\] aren't good idea - store them in variables
--- End quote ---
isn't this optimized out? Isn't a copy slower?
--- Quote ---6. Do not pass nullptr when creating dialogs. It leads to really bad problems on linux.
--- End quote ---
what instead?
oBFusCATed:
--- Quote from: BlueHazzard on August 29, 2018, 12:04:23 am ---Can you point out the line in the patch?
--- End quote ---
This one: "@param glob the globs to add to the project."
--- Quote from: BlueHazzard on August 29, 2018, 12:04:23 am ---Not really... there is no independent read back possibility from script bindings, to check the success... If i remember correctly i have not implemented any test for all project functions, because it was not possible to create a project from the script binding and get a independent result from the function (compare a pre made project file for example) ... Testing project binding should not be topic for this patch... If there is need we should create a new topic/ticket
--- End quote ---
Ok, not testing, but what about setglobs and getglobs support in scripting?
--- Quote from: BlueHazzard on August 29, 2018, 12:04:23 am ---suggestions? I think OnbtnBrowseClick nicely verbose :
--- End quote ---
OnBrowseClick is obvious and clear, IMO. If you want to stick btn or button you should make it CamelCase. Nobody cares if the variable for the button has the same name as the method name.
--- Quote from: BlueHazzard on August 29, 2018, 12:04:23 am ---I have not found any camel case / upper case filename in the patch?
--- End quote ---
There you go:
+#include "dlgEditProjectGlob.h"
+#include "dlgManageGlobs.h"
+#include "dlgEditProjectGlob.h"
It seems only the includes are broken.
--- Quote from: BlueHazzard on August 29, 2018, 12:04:23 am ---Doesn't this add unicode problems? Or how do i printf a wxString with unicode support?
--- End quote ---
No. Use .wx_str() when calling wxString::Format, you can also use the short F function.
--- Quote from: BlueHazzard on August 29, 2018, 12:04:23 am ---
--- Quote ---5.8. Repeated calls to prj->GetGlobs()[i\] aren't good idea - store them in variables
--- End quote ---
isn't this optimized out? Isn't a copy slower?
--- End quote ---
How calling an unknown method from a dll could be optimized away? But I don't really care too much for optimizations, copy pasting shows the laziness of the writer of the code and it is hard to read because it is more text and you should verify that the function call doesn't return different value every time it is called.
--- Quote from: BlueHazzard on August 29, 2018, 12:04:23 am ---
--- Quote ---6. Do not pass nullptr when creating dialogs. It leads to really bad problems on linux.
--- End quote ---
what instead?
--- End quote ---
Either a parent dialog or the main frame or the app window (these two can be accessed from the manager class).
Navigation
[0] Message Index
[#] Next page
[*] Previous page
Go to full version