Terminal configuration improvements (patches)

[BlueDarknezz]

Hi folks,

after playing around with Code::Blocks I've tried CodeLite and was impressed from the functionality of this young project. I like the work-flow, which is parallel to the one of VS (I must use this at work). But one "feature" which was annoying me, was/is the implemented terminal configuration and the hardcoded xterm-dependency for debugging with CodeLite (I feel good without xterm). So I've checked the src of CodeLite and the used functions.

You can find the results of my changes/improvements in the attachment (base was revision 5439). Sorry for the binary, but the board limits to 3 files only.

It would be good, when the changes will be merged into trunk .

[eranif]

Did you check the options under: Settings -> Global Editor Preferences -> Terminal?

You can re-configure codelite to use another terminal. I chose xterm because it is the lowest common denominator‏‏‏‏ for most Linux / FreeBSD distros

Eran

[BlueDarknezz]

Did you check the options under: Settings -> Global Editor Preferences -> Terminal?

Ok, this question I didn't expect, but...sure I did .

You can re-configure codelite to use another terminal.

Correct, but only to run the console program, which is in my opinion only a little part of an IDE. The configuration isn't considered, if you want to debug your own program (IMHO the feature of an IDE) or try to open a shell in the "explorer"-window or from the tab-bar.
Where my changes really reviewed? Because they aren't only for the debugging-purpose. Was there ever a try to open a terminal different from "gnome-terminal", "konsole" and "xterm" out of the explorer or tab-bar?

Code: Select all

bool ProcUtils::Shell()
{
[...]
#else //non-windows
	//try to locate the default terminal
	wxString terminal;
	wxString where;
	if (Locate(wxT("gnome-terminal"), where)) {
		terminal = where;
	} else if (Locate(wxT("konsole"), where)) {
		wxString path = wxGetCwd();
		terminal << where << wxT(" --workdir \"") << path << wxT("\"");
	} else if (Locate(wxT("xterm"), where)) {
		terminal = where;
	}
	cmd = terminal;
#endif
	return wxExecute(cmd, wxEXEC_ASYNC) != 0;
}

I expect "terminal" from XFCE wouldn't work. Am I right?

Code: Select all

void Manager::DbgStart ( long attachPid )
{
[...]
#if defined(__WXGTK__)
	wxString where;
	if ( !ExeLocator::Locate ( wxT ( "xterm" ), where ) ) { // COMMENT INSERTED FROM ME: Does a "which" on the provided string
		wxMessageBox ( _( "Failed to locate 'xterm' application required by CodeLite, please install it and try again!" ), _("CodeLite" ), wxOK|wxCENTER|wxICON_WARNING, clMainFrame::Get() );
		return;
	}
#endif
[...]
}

It doesn't make really sense there to check if xterm is installed, when it's not used at all (haven't seen any call of it after that). Also xterm isn't listed as dependency (on the main page) for CodeLite either.

Why is the function...

Code: Select all

wxString ConsoleFinder::GetConsoleName()
{
[...]
}

...implemented but never used? We (the community) or you can put the complete complexity of linux-terminal-choose-freedom into that function and use it in any other function, where the usage could be useful. Right now there are many redundant parts of it, because *Locate is used that often in different modules.

I chose xterm because it is the lowest common denominator‏‏‏‏ for most Linux / FreeBSD distros

Eran

This could be true (at least I don't need it and haven't installed it) and I really don't want to discuss, which terminal emulator is the best, but this statement was a little bit confusing me in relation to your comment from Sat Feb 06, 2010 1:59 am "Terminal setting not used for debugger?":

Unfortunately yes - I am not happy with it either

You can submit a feature request for this.

Eran

So I was bypassing the feature request, did the change my self, post it in the main-forum and hope for a merge of it. Especially your comment was the reason, why I've done the change by my self. I want to change the situation, without to involve your time for it.
If it's not possible to merge it (why ever), then I want to stimulate a discussion about that current implementation and need of that dependency.

Now it's on you, if that change or maybe a little part of it, will be merged into trunk or at least discussed.

[eranif]

Ok, this question I didn't expect

I am don't know you enough yet to know which questions to ask.. there are newbies out there (just browse the forum) and see why I am asking this obvious questions

I expect "terminal" from XFCE wouldn't work. Am I right?

Maybe, I am not XFCE expert - so I will trust you on this one

I chose xterm because it is the lowest common denominator‏‏‏‏ for most Linux / FreeBSD distros
Eran

Emphasis on "lowest"

So I was bypassing the feature request, did the change my self, post it in the main-forum and hope for a merge of it

Which will probably will happen.

EDIT:

Can you please submit your patch as a single file? relative to the root directory of codelite?

assuming your codelite source are under: /home/me/src/codelite/

Code: Select all

cd /home/me/src/codelite/ 
svn diff > mypatch.diff

Thanks!
Eran

[BlueDarknezz]

I am don't know you enough yet to know which questions to ask.. there are newbies out there (just browse the forum) and see why I am asking this obvious questions

Absolutely no problem, everybody is at the beginning a newbie, so I know both sides .

Maybe, I am not XFCE expert - so I will trust you on this one

Thanks. At least CodeLite should support the big DEs (Gnome, KDE, XFCE and LXDE). But if we read out the input of the user from the configuration (we can expect mostly correct input from linux-users) and provide a fall-back-method, then we are at the safe side.

Which will probably will happen.

Nice to read. Then I don't need to merge my self every time again .

EDIT:
Can you please submit your patch as a single file? relative to the root directory of codelite?

Sure I can. Please check the attached diff and also do a cross-check of my changes, because I've merged it right now with meld back into your newest trunk.

[eranif]

. Please check the attached diff and also do a cross-check of my changes, because I've merged it right now with meld back into your newest trunk.

Your patch cannot be applied as it is. the file "editor_config.h" which is included from CodeLite/procutils.cpp is part of the plugin_sdk project.
Files in the 'CodeLite' project can only includes files from the wxlibrary, standard headers or headers from the CodeLite project. Other projects may include files from the CodeLite project but not the other way around.

Attached is a simple diagram that shows the project dependency tree. Each "module" can include files from the module underneath it but not from the ones that are on top of it

1.png

I fixed it by changing the method: :

Code: Select all

 ProcUtils::Shell()

to accept the program console command as string argument.

so it is now:

Code: Select all

ProcUtils::Shell(const wxString &programConsoleCommand)

This way I removed the need for including editor_config.h in proc_utils.cpp

Eran

[BlueDarknezz]

Your patch cannot be applied as it is. the file "editor_config.h" which is included from CodeLite/procutils.cpp is part of the plugin_sdk project.
Files in the 'CodeLite' project can only includes files from the wxlibrary, standard headers or headers from the CodeLite project. Other projects may include files from the CodeLite project but not the other way around.

Ah, ok. I didn't know that. Seems, that I haven't read the documentation ...sorry for that problem.

Attached is a simple diagram that shows the project dependency tree. Each "module" can include files from the module underneath it but not from the ones that are on top of it

I will consider that for future implementations.

I fixed it by changing the method: :

Code: Select all

 ProcUtils::Shell()

to accept the program console command as string argument.

so it is now:

Code: Select all

ProcUtils::Shell(const wxString &programConsoleCommand)

This way I removed the need for including editor_config.h in proc_utils.cpp

Yes, this was a good solution.

I would prefer to implement the "find"/search functionality inside of...

Code: Select all

ConsoleFinder::GetConsoleName()

...and use this in the levels above:

LiteEditor:
* app
* fileexplorertree
* frame
* manager

Would you approve this approach? Or is there nearly the same conflict, because ConsoleFinder is a plugin and uses the same layer?

Then there would be only 1 place which has to be maintained. This will solve the redundant code segments.

[eranif]

because ConsoleFinder is a plugin and uses the same layer?

Implementing it in ConsoleFinder will make it non-accessible from CodeLite.so layer

FYI: There were few bugs that needed a fix with the latest patch:

• "gnome-terminal" does not accept "-T" argument but only "-t" or "--title"
• There was a wrong parsing of the console name incase it was set to 'konsole'

Anyways, both of them are now fixed in trunk.

Eran

[BlueDarknezz]

Implementing it in ConsoleFinder will make it non-accessible from CodeLite.so layer

Correct, but...

Code: Select all

ProcUtils::Shell()

...for example could also work with the complete pre-build command-string (address, as introduced from you) as input. The token-logic would then be unnecessary.

FYI: There were few bugs that needed a fix with the latest patch:

• "gnome-terminal" does not accept "-T" argument but only "-t" or "--title"
• There was a wrong parsing of the console name incase it was set to 'konsole'

Anyways, both of them are now fixed in trunk.

You're right, terminal wasn't cleared and gnome-terminal uses "-t" for the title and will fail at the debug-session start of CodeLite. Sorry for the inconvenience and shame on me.