new wxWidget(wxSmith) project

[killerbot]

I just created a new wxWidgets project and choose wxSmith as the gui builder. I choose the dialog based app.
On linux !
Unfortunately it doesn't compile, probably doesn't know where the headers are, seems this is not dealed with by the wizard ??
[Edit] : fixed : I can build now my build options needed to be changed to `wx-config-2.8 --cflags` --> append of -2.8

But I have some important remarks with the generated code, could one of the wizards gurus adopt it please.

In total 5 files are generated (I called my project wxSmith1) :

1) wxSmith1App.h
this header files is including precompiled headers, one should not to do in header files !!
So I would suggest to remove the following construct :

Code

#include <wx/wxprec.h>

#ifdef __BORLANDC__
    #pragma hdrstop
#endif

#ifndef WX_PRECOMP
    #include <wx/wx.h>
#endif

by just :

Code

#include <wx/app.h>

Remark : I never got the choice to say in I want pch or not !!!
Question : is that _BORLANDC_ pragma really needed. I just looks and feels plain ugly ?

2) wxSmith1App.cpp
- again that _BORLANDC_ stuuf is there --> needed (note : maybe here, but for sure not in the header file ?)
- do NOT use NULL, that's not standard C/C++ ==> the correct value is : 0


3) wxSmith1Main.h
This is something where wxSmith also is not doing the best it could do: In the header file it is including a bunch of headers which are not needed at that point, forward declaration are sufficient. Those headers are only needed at the cpp.
Could this please be changed, so there are less dependencies in the headers.

So in my example this :

Code

#include "wxSmith1App.h"

//(*Headers(wxSmith1Dialog)
#include <wx/button.h>
#include <wx/dialog.h>
#include <wx/sizer.h>
#include <wx/statline.h>
#include <wx/stattext.h>
//*)

class wxSmith1Dialog: public wxDialog
{
	public:

		wxSmith1Dialog(wxWindow* parent,wxWindowID id = -1);
		virtual ~wxSmith1Dialog();

	private:

		//(*Handlers(wxSmith1Dialog)
		void OnQuit(wxCommandEvent& event);
		void OnAbout(wxCommandEvent& event);
		//*)

		//(*Identifiers(wxSmith1Dialog)
		static const long ID_STATICTEXT1;
		static const long ID_BUTTON1;
		static const long ID_STATICLINE1;
		static const long ID_BUTTON2;
		//*)

		//(*Declarations(wxSmith1Dialog)
		wxBoxSizer* BoxSizer1;
		wxStaticText* StaticText1;
		wxBoxSizer* BoxSizer2;
		wxButton* Button1;
		wxStaticLine* StaticLine1;
		wxButton* Button2;
		//*)

		DECLARE_EVENT_TABLE()
};

could, well actually should become :

Code

#include "wxSmith1App.h"

//(*Headers(wxSmith1Dialog)
#include <wx/dialog.h>
class wxButton;
class wxBoxSizer;
class wxStaticText;
class wxStaticLine;
class wxCommandEvent;
//*)

class wxSmith1Dialog: public wxDialog
{
	public:

		wxSmith1Dialog(wxWindow* parent,wxWindowID id = -1);
		virtual ~wxSmith1Dialog();

	private:

		//(*Handlers(wxSmith1Dialog)
		void OnQuit(wxCommandEvent& event);
		void OnAbout(wxCommandEvent& event);
		//*)

		//(*Identifiers(wxSmith1Dialog)
		static const long ID_STATICTEXT1;
		static const long ID_BUTTON1;
		static const long ID_STATICLINE1;
		static const long ID_BUTTON2;
		//*)

		//(*Declarations(wxSmith1Dialog)
		wxBoxSizer* BoxSizer1;
		wxStaticText* StaticText1;
		wxBoxSizer* BoxSizer2;
		wxButton* Button1;
		wxStaticLine* StaticLine1;
		wxButton* Button2;
		//*)

		DECLARE_EVENT_TABLE()
};

4) wxSmith1Main.cpp
Let's first show the code and then discuss what should be changed :

Code

#ifdef WX_PRECOMP
#include "wx_pch.h"
#endif

#ifdef __BORLANDC__
#pragma hdrstop
#endif //__BORLANDC__

#include "wxSmith1Main.h"

//helper functions
enum wxbuildinfoformat {
    short_f, long_f };

wxString wxbuildinfo(wxbuildinfoformat format)
{
    wxString wxbuild(wxVERSION_STRING);

    if (format == long_f )
    {
#if defined(__WXMSW__)
        wxbuild << _T("-Windows");
#elif defined(__UNIX__)
        wxbuild << _T("-Linux");
#endif

#if wxUSE_UNICODE
        wxbuild << _T("-Unicode build");
#else
        wxbuild << _T("-ANSI build");
#endif // wxUSE_UNICODE
    }

    return wxbuild;
}

//(*InternalHeaders(wxSmith1Dialog)
#include <wx/bitmap.h>
#include <wx/font.h>
#include <wx/fontenum.h>
#include <wx/fontmap.h>
#include <wx/image.h>
#include <wx/intl.h>
#include <wx/settings.h>
//*)

//(*IdInit(wxSmith1Dialog)
const long wxSmith1Dialog::ID_STATICTEXT1 = wxNewId();
const long wxSmith1Dialog::ID_BUTTON1 = wxNewId();
const long wxSmith1Dialog::ID_STATICLINE1 = wxNewId();
const long wxSmith1Dialog::ID_BUTTON2 = wxNewId();
//*)

BEGIN_EVENT_TABLE(wxSmith1Dialog,wxDialog)
	//(*EventTable(wxSmith1Dialog)
	//*)
END_EVENT_TABLE()

wxSmith1Dialog::wxSmith1Dialog(wxWindow* parent,wxWindowID id)
{
	//(*Initialize(wxSmith1Dialog)
	Create(parent,id,_("wxWidgets app"),wxDefaultPosition,wxDefaultSize,wxDEFAULT_DIALOG_STYLE,_T("wxDialog"));
	BoxSizer1 = new wxBoxSizer(wxHORIZONTAL);
	StaticText1 = new wxStaticText(this,ID_STATICTEXT1,_("Welcome to\nwxWidgets"),wxDefaultPosition,wxDefaultSize,0,_T("ID_STATICTEXT1"));
	wxFont StaticText1Font = wxSystemSettings::GetFont(wxSYS_DEFAULT_GUI_FONT);
	if ( !StaticText1Font.Ok() ) StaticText1Font = wxSystemSettings::GetFont(wxSYS_DEFAULT_GUI_FONT);
	StaticText1Font.SetPointSize(20);
	StaticText1->SetFont(StaticText1Font);
	BoxSizer1->Add(StaticText1,1,wxALL|wxALIGN_CENTER_HORIZONTAL|wxALIGN_CENTER_VERTICAL,10);
	BoxSizer2 = new wxBoxSizer(wxVERTICAL);
	Button1 = new wxButton(this,ID_BUTTON1,_("About"),wxDefaultPosition,wxDefaultSize,0,wxDefaultValidator,_T("ID_BUTTON1"));
	BoxSizer2->Add(Button1,1,wxALL|wxALIGN_CENTER_HORIZONTAL|wxALIGN_CENTER_VERTICAL,4);
	StaticLine1 = new wxStaticLine(this,ID_STATICLINE1,wxDefaultPosition,wxSize(10,-1),wxLI_HORIZONTAL,_T("ID_STATICLINE1"));
	BoxSizer2->Add(StaticLine1,0,wxALL|wxEXPAND|wxALIGN_CENTER_HORIZONTAL|wxALIGN_CENTER_VERTICAL,4);
	Button2 = new wxButton(this,ID_BUTTON2,_("Quit"),wxDefaultPosition,wxDefaultSize,0,wxDefaultValidator,_T("ID_BUTTON2"));
	BoxSizer2->Add(Button2,1,wxALL|wxALIGN_CENTER_HORIZONTAL|wxALIGN_CENTER_VERTICAL,4);
	BoxSizer1->Add(BoxSizer2,0,wxALIGN_CENTER_HORIZONTAL|wxALIGN_CENTER_VERTICAL,4);
	SetSizer(BoxSizer1);
	BoxSizer1->Fit(this);
	BoxSizer1->SetSizeHints(this);
	Connect(ID_BUTTON1,wxEVT_COMMAND_BUTTON_CLICKED,(wxObjectEventFunction)&wxSmith1Dialog::OnAbout);
	Connect(ID_BUTTON2,wxEVT_COMMAND_BUTTON_CLICKED,(wxObjectEventFunction)&wxSmith1Dialog::OnQuit);
	//*)
}

wxSmith1Dialog::~wxSmith1Dialog()
{
	//(*Destroy(wxSmith1Dialog)
	//*)
}

void wxSmith1Dialog::OnQuit(wxCommandEvent& event)
{
    Close();
}

void wxSmith1Dialog::OnAbout(wxCommandEvent& event)
{
    wxString msg = wxbuildinfo(long_f);
    wxMessageBox(msg, _("Welcome to..."));
}

- first of all all headers should be included at the top, not first some code and then some headers. In this case this can lead to compile errors, since when there's no pch -> wx_pch.h not included. And we already encounter some code for using undefined types at that moment : wxString, _T(), and only after these first implementation lines of code we see a next bunch of includes. Finally bringing in wx/intl.h (_T()), but still no wxString. So in this example not all needed headers are included !!!
Short word : this code is wrong.
- so the second bunch of headers should move up, and the headers I removed in 3) should join them in the cpp file, and the construct should be something like this :

#ifdef WX_PRECOMP
#include "wx_pch.h"
#else
 // include here all headers you need and that in the pch case would be brought in by the wx_pch.h (and hope this never changes in the future (extension is ok, but reduction not)
#endif
// include here all other wx headers needed but that are not delivered by wx_pch.h [so they get included always: pch and non-pch]

- I didn't check the rest of the code, but 1 thing I already noticed was no header included for wxMessageBox, so the include of <wx/utils.h> should be added otherwise wcode

5) wxsmith1dialog.wxs
As said, the name of my project was wxSmith1, all previous 4 generated files are CamelCase, only this one is NOT.
For consistency, it would be best it also follow the CamelCase convention ==> wxSmith1Dialog.wxs


Once again I would like to stress : please use headers and there includes correctly. You get less dependencies and better portable code [most of the time for example if the linux build is broken in CB it is due to incorrect header inclusions]. So this is extremely important.

Aside of that : wxSmith rules !!!


EDIT : I would suggest to get rid of that _BORLANDC_ thing, since CB code is purely targeted for GCC. And I really wonder if Borland still needs such an ugly beast.

[byo]

Ok, I'll take all those things into consideration Very good report indeed.

Remark : I never got the choice to say in I want pch or not !!!
Question : is that _BORLANDC_ pragma really needed. I just looks and feels plain ugly ?

I don't like it either, but don't know what will be the result on borland compiler. In the worst case it will just slow down the compilation. This code is just taken from wx examples so it's the suggested way. But I also would like to get rid of it.

- do NOT use NULL, that's not standard C/C++ ==> the correct value is : 0

Right, it should be easy to find and replace.

This is something where wxSmith also is not doing the best it could do: I nthe header file it is including a bunch of headers which are not needed at that point, forward declaration are sufficient. Those headers are only needed at the cpp.

I'll have to consider what code changes it will require. It may introduce some problems when someone would like to write inline function accessing controls. So I'll probably leave this problem for later.

- first of all all headers should be included at the top, not first some code and then some headers.

True, true...

Finally bringing in wx/intl.h (_T()), but still no wxString. So in this example not all needed headers are included !!!

Actually <wx/intl.h> internally includes <wx/string.h>. And it's certain that at least any other include will also use <wx/string.h> (what can not be said about <wx/intl.h>). But you may be right that this "list" doesn't look complete and including <wx/string.h> shouldn't be time-consuming. So I'll probably add it.

#ifdef WX_PRECOMP
#include "wx_pch.h"
#else
 // include here all headers you need and that in the pch case would be brought in by the wx_pch.h (and hope this never changes in the future (extension is ok, but reduction not)
#endif
// include here all other wx headers needed but that are not delivered by wx_pch.h [so they get included always: pch and non-pch]

Huh, that may be challenging since wxSmith doesn't split headers into pch-included and the rest (I just wonder why there are such headers that are not in pch). Since it may require changes of bigger wxSmith code parts, I'll leave it as post-1.0 task. Currently I'll just threat all headers as non-pch. They sohuldn't slow things down very much since header guard macros simply elliminate everything at preprocessor level.

- I didn't check the rest of the code, but 1 thing I already noticed was no header included for wxMessageBox, so the include of <wx/utils.h> should be added otherwise wcode

Actually, here it would be <wx/msgdlg.h>

As said, the name of my project was wxSmith1, all previous 4 generated files are CamelCase, only this one is NOT.

Not a problem. It followed previous scheme used for class wizard (all file names lower case).

Once again I would like to stress : please use headers and there includes correctly. You get less dependencies and better portable code [most of the tie for example if the linux build is broken in CB it is due to incorrect header inclusions]. So this is extremely important.

Hmm, I agree only partially. Of course I agree that headers should (in most cases) reside at the beginning of file. But forward declaration may cause more problems than few extre includes and for sure it wont do any harm when including external library (forward declarations are required only when there's case of cyclic includes), just a slower compilation.

Aside of that : wxSmith rules !!!

  Glad to hear that

Regards
  BYO

[killerbot]

Quote
- I didn't check the rest of the code, but 1 thing I already noticed was no header included for wxMessageBox, so the include of <wx/utils.h> should be added otherwise wcode

Actually, here it would be <wx/msgdlg.h> Wink

Correct, my mistake.


EDIT : normally forward declarations won't bring any problems, since for the compiler it's more then enough info, and as such if the full header include would have been there, you create a dependency on those files, if they change everyone using your header also recompiles, where only your implementation should recompile. The user of your hader doesn't care that internally there's a wxButton, but for your implementation this is very important ;-) .

[Biplab]

I just created a new wxWidgets project and choose wxSmith as the gui builder. I choose the dialog based app.
On linux !
Unfortunately it doesn't compile, probably doesn't know where the headers are, seems this is not dealed with by the wizard ??
[Edit] : fixed : I can build now my build options needed to be changed to `wx-config-2.8 --cflags` --> append of -2.8

wxWidgets wizard was tested several times on Linux before putting it in to repo. Why didn't you report the issue that time?

FYI, it works perfectly provided you select the appropriate options. Unless you specify exactly what happened with you (your OS, wx installation, errors, etc) I would consider that the wizard works perfectly.

It's a bad gesture from a developer to comment on other people's work by trying it once.

Remark : I never got the choice to say in I want pch or not !!!
Question : is that _BORLANDC_ pragma really needed. I just looks and feels plain ugly ?

Ok, noted. This will be added. _BORLANDC_ pragma is necessary if Borland C++ 5.5 compiler is used.

EDIT : I would suggest to get rid of that _BORLANDC_ thing, since CB code is purely targeted for GCC. And I really wonder if Borland still needs such an ugly beast.

Is it written anywhere? Or am I missing anything?

If you are talking about compiling C::B's code with Borland C++ 5.5 compiler, then you may remove that pragma. BCC 5.5 can't compile Squirrel.

[Biplab]

For all users information, wxWidgets wizard is Now Broken.

It'll work with wxSmith. But NOT when you'll be using it without wxSmith.

Edit 1: @Byo, Thanks for the update.    But the problem is wxSmith adds the necessary headers whereas without wxSmith the wizard generated codes become blind.

I'll update the wizard generated codes.

Regards,

Biplab

[mandrav]

EDIT : I would suggest to get rid of that _BORLANDC_ thing, since CB code is purely targeted for GCC. And I really wonder if Borland still needs such an ugly beast.

Is it written anywhere? Or am I missing anything?

If you are talking about compiling C::B's code with Borland C++ 5.5 compiler, then you may remove that pragma. BCC 5.5 can't compile Squirrel.

Yes, he's talking about C::B's source code. It's only compilable with gcc.

[killerbot]

@Biplab : don't get me wrong, I am not putting anything or anyone down.

I had tried the wizard on windows, where I get a lot of choices (like #wx). So in linux I did similar. Now I didn't get that choice although I had set up that global variable correctly. So nothing compiled. By looking into the project options I saw the wx-config and then with some help of other people we saw that in the wx284 package we installed on OpenSuse it's called wx-config-2.8. Did you notice the [EDIT] part in the post explaining the issue was fixed !!
SO I looked for the solution and put it in the post, I am sorry there's nothing more that I can do. I wrote down my experience.
And then I started my rattle on the generated code, which can be improved as I suggested.

- don't include pch headers in a header file, which was what happened !!!
- other includes were missing (never depend on a.h including b.h, be clear, if you use wxString --> include wxString.h, I don't think a user should start checking if wx/intl.h includes wxstring.h).
[Meyers and Sutter and Alexandrescu] have some interesting topics in their books on headers.


You know our wizards work, wxSmith does splendid things, and if then the code that comes out of it is clean and good C++, then we have something very beautiful.

Cheers.

[byo]

It'll work with wxSmith. But NOT when you'll be using it without wxSmith.

Edit 1: @Byo, Thanks for the update.    But the problem is wxSmith adds the necessary headers whereas without wxSmith the wizard generated codes become blind.

Sorry for that  :oops:. I don't know what I've done wrong but I also tested all three options for GUI designer and it compiled and run fine, I must have committed wrong script. Again sorry for the inconvenience.

Regards
   BYO

[Biplab]

@Biplab : don't get me wrong, I am not putting anything or anyone down.

First of all, Sorry for my Hot comments. :roll:

I had tried the wizard on windows, where I get a lot of choices (like #wx). So in linux I did similar. Now I didn't get that choice although I had set up that global variable correctly. So nothing compiled.

On Linux this method is not followed which is why we have two project files for Code::Blocks.

By looking into the project options I saw the wx-config and then with some help of other people we saw that in the wx284 package we installed on OpenSuse it's called wx-config-2.8. Did you notice the

It's strange that it didn't work for you. I have OpenSUSE 10.2 installed and that works for me. I'm not using any pre-compiled rpm. But I believe wx-config is the actual one to be used.

- don't include pch headers in a header file, which was what happened !!!

My vote for this one and thanks to Byo for the change.

- other includes were missing (never depend on a.h including b.h, be clear, if you use wxString --> include wxString.h, I don't think a user should start checking if wx/intl.h includes wxstring.h).

I would differ with you on this. The most commonly used headers are included through <wx/wx.h> file. wxString header is also included in that header. So if it is included we don't need to include <wx/string.h> again.

If you notice the dialog sample, you'll find two extra headers are included which are not included through <wx/wx.h> file.

You know our wizards work, wxSmith does splendid things, and if then the code that comes out of it is clean and good C++, then we have something very beautiful.

That's what we all want.

Best Regards,

Biplab

[Biplab]

Sorry for that  :oops:. I don't know what I've done wrong but I also tested all three options for GUI designer and it compiled and run fine, I must have committed wrong script. Again sorry for the inconvenience.

You have made nice changes in the script and it's fine.

The problem is you've removed the following lines

#ifndef WX_PRECOMP
    #include <wx/wx.h>
#endif

Now wx.h includes a number of commonly used headers. So if it's not there, we've to include <wx/frame.h>, _{<wx/button.h>}, etc for all the classes we want to use. There lies the problem.

Regards,

Biplab

[killerbot]

as said, I would suggest to be explicit and not just wx.h.
Things are clearer to the user also, he knows which headers are in play, and no need to search wx.h for which header it could be. Less surprises if certainly a header get's kicked out of wx.h. Hopefully this won't happen but you never know with these wx guys.

And secondly if you don't use pch, wx.h is bringing way to many things into play and will slow down compilation.

[Biplab]

as said, I would suggest to be explicit and not just wx.h.
Things are clearer to the user also, he knows which headers are in play, and no need to search wx.h for which header it could be. Less surprises if certainly a header get's kicked out of wx.h. Hopefully this won't happen but you never know with these wx guys.

And secondly if you don't use pch, wx.h is bringing way to many things into play and will slow down compilation.

Got your point. I'll make the necessary changes.


@All, wxwidgets wizard is now working well with/without GUI designers.

[byo]

The problem is you've removed the following lines

#ifndef WX_PRECOMP
    #include <wx/wx.h>
#endif

Now wx.h includes a number of commonly used headers. So if it's not there, we've to include <wx/frame.h>, _{<wx/button.h>}, etc for all the classes we want to use. There lies the problem.

Now I get it , haven't checked that with pch disabled. Lesson for me: alays consult wxWizard changes with you before applying.

Regards
   BYO

[Biplab]

Byo,

Another point. It seems that you've removed _BORLANDC_ preprocessor directive and the associated pragma.

Please note that this will affect the compilation of Pch enabled App with BCC.

BCC 5.5.1 can't generate Pre-Compiled Header from a Header file.   What it does is that it creates the PCH on the fly while compiling a cpp file. It includes all the headers found in a cpp file to the generated pch file. This pragma is necessary to tell BCC that - "Please don't include any header files inside the PCH file from now on." This prevents regeneration of PCH file while compiling a large app.

If you remove that pragma, then BCC will include all the headers found in that cpp file and will include them in PCH file (if they were not included earlier). This will slow down the compilation process.

I hope I could make my point clear to you.

IMHO, if you remove them, then you may have to tell users that wxSmith doesn't support pch creation with BCC 5.5.

Best Regards,

Biplab

[byo]

Another point. It seems that you've removed _BORLANDC_ preprocessor directive and the associated pragma.

Hmm, I was just wondering - there's this _BORLANDC_ stuff inside wx_pch.h file:

Code

#ifndef WX_PCH_H_INCLUDED
#define WX_PCH_H_INCLUDED

// basic wxWidgets headers
#include <wx/wxprec.h>

#ifdef __BORLANDC__
	#pragma hdrstop
#endif

#ifndef WX_PRECOMP
	#include <wx/wx.h>
#endif

#ifdef WX_PRECOMP
	// put here all your rarely-changing header files
#endif // WX_PRECOMP

#endif // WX_PCH_H_INCLUDED

And when the project is created and uses wxSmith, it includes this wx_pch just at the beginning of each source. In fact it should work as you've said - it will only cache <wx/wxprec.h> header. So it should work correctly. And it really cleans the source. Maybe with one exception: the "// put here all your rarely-changing header files" content should be placed before _BORLANDC_ stuff to cache rarely-changing project headers too. The only problem with wxSmith is that this "wx_pch.h" is not included for newly created resources but I'll probably try to detect such file and add it automatically.

Same technique could be used for other GUI designers too so everything related to pch would be put inside wx_pch.h and there won't be any need of using those #pragma hdrstop in sources anymore. What do you think ?

Regards
   BYO