Terminal configuration improvements (patches)

Discussion about CodeLite development process and patches
BlueDarknezz
CodeLite Curious
Posts: 5
Joined: Sat Apr 07, 2012 7:41 pm
Genuine User: Yes
IDE Question: C++
Contact:

Terminal configuration improvements (patches)

Post by BlueDarknezz » Sat Apr 07, 2012 8:05 pm

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 ;).
You do not have the required permissions to view the files attached to this post.

User avatar
eranif
CodeLite Plugin
Posts: 6110
Joined: Wed Feb 06, 2008 9:29 pm
Genuine User: Yes
IDE Question: C++
Contact:

Re: Terminal configuration improvements (patches)

Post by eranif » Tue Apr 10, 2012 3:25 pm

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
Make sure you have read the HOW TO POST thread

BlueDarknezz
CodeLite Curious
Posts: 5
Joined: Sat Apr 07, 2012 7:41 pm
Genuine User: Yes
IDE Question: C++
Contact:

Re: Terminal configuration improvements (patches)

Post by BlueDarknezz » Tue Apr 10, 2012 11:06 pm

eranif wrote:Did you check the options under: Settings -> Global Editor Preferences -> Terminal?
Ok, this question I didn't expect, but...sure I did ;).
eranif wrote: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.
eranif wrote: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?":
eranif wrote: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.

User avatar
eranif
CodeLite Plugin
Posts: 6110
Joined: Wed Feb 06, 2008 9:29 pm
Genuine User: Yes
IDE Question: C++
Contact:

Re: Terminal configuration improvements (patches)

Post by eranif » Tue Apr 10, 2012 11:29 pm

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
BlueDarknezz wrote: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
BlueDarknezz wrote:I chose xterm because it is the lowest common denominator‏‏‏‏ for most Linux / FreeBSD distros
Eran
Emphasis on "lowest"
BlueDarknezz wrote: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
Make sure you have read the HOW TO POST thread

BlueDarknezz
CodeLite Curious
Posts: 5
Joined: Sat Apr 07, 2012 7:41 pm
Genuine User: Yes
IDE Question: C++
Contact:

Re: Terminal configuration improvements (patches)

Post by BlueDarknezz » Wed Apr 11, 2012 12:04 am

eranif wrote: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 ;).
eranif wrote: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.
eranif wrote:Which will probably will happen.
Nice to read. Then I don't need to merge my self every time again :D.
eranif wrote: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.
You do not have the required permissions to view the files attached to this post.

User avatar
eranif
CodeLite Plugin
Posts: 6110
Joined: Wed Feb 06, 2008 9:29 pm
Genuine User: Yes
IDE Question: C++
Contact:

Re: Terminal configuration improvements (patches)

Post by eranif » Wed Apr 11, 2012 1:26 am

BlueDarknezz wrote:. 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
You do not have the required permissions to view the files attached to this post.
Make sure you have read the HOW TO POST thread

BlueDarknezz
CodeLite Curious
Posts: 5
Joined: Sat Apr 07, 2012 7:41 pm
Genuine User: Yes
IDE Question: C++
Contact:

Re: Terminal configuration improvements (patches)

Post by BlueDarknezz » Thu Apr 12, 2012 10:01 pm

eranif wrote: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.
eranif wrote: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.
eranif wrote: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.

User avatar
eranif
CodeLite Plugin
Posts: 6110
Joined: Wed Feb 06, 2008 9:29 pm
Genuine User: Yes
IDE Question: C++
Contact:

Re: Terminal configuration improvements (patches)

Post by eranif » Fri Apr 13, 2012 12:02 am

BlueDarknezz wrote: 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
Make sure you have read the HOW TO POST thread

BlueDarknezz
CodeLite Curious
Posts: 5
Joined: Sat Apr 07, 2012 7:41 pm
Genuine User: Yes
IDE Question: C++
Contact:

Re: Terminal configuration improvements (patches)

Post by BlueDarknezz » Fri Apr 13, 2012 12:54 am

eranif wrote: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.
eranif wrote: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.

Post Reply