Review Request: Actually use ExecuteBrowserPlugin in XDebugJob

[Dominik Schmidt]

> On April 15, 2012, 8:47 p.m., Milian Wolff wrote:
> > plugins/executebrowser/iexecutebrowserplugin.h, line 52
> > <http://git.reviewboard.kde.org/r/104595/diff/1/?file=56601#file56601line52>
> >
> >     I don't understand this comment after the e.g. part - could you rephrase this? or give a proper example? what happens by default, i.e. if the url is empty?

Sure.

How about ..

"// if you provide a valid url as second parameter that one is opened instead of the url stored in the launch configuration
 // e.g. this is used in xdebug plugin to add additional GET parameters to the configured url"

?


> On April 15, 2012, 8:47 p.m., Milian Wolff wrote:
> > plugins/executebrowser/browserappjob.cpp, line 79
> > <http://git.reviewboard.kde.org/r/104595/diff/1/?file=56598#file56598line79>
> >
> >     is it ok to emit the result directly? or should one wait for the "started" signal (that should exist afaik)? that would also allow one to properly handle error cases, i.e. if. m_browser points to a non-existing browser-app or if something else goes wrong, or?

I'll check, sounds reasonable.


> On April 15, 2012, 8:47 p.m., Milian Wolff wrote:
> > debuggers/xdebug/debugjob.cpp, line 297
> > <http://git.reviewboard.kde.org/r/104595/diff/1/?file=56596#file56596line297>
> >
> >     this is unstable, it will crash as soon as the executebrowserplugin is disabled, or is that a requirement of the xdebug plugin? hm would make sense - just make sure that this is so in the *.desktop file please

Good hint!

It doesn't do that so far, but it perfectly makes sense:
a) the settings are already stored in an executebrowserplugin instance
b) you'd need to implement fallback code in the xdebug plugin which is already inside the EBP
c) if not for this, what is the EBP good for then? ;)


- Dominik


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104595/#review12488
-----------------------------------------------------------


On April 13, 2012, 11:29 p.m., Dominik Schmidt wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104595/
> -----------------------------------------------------------
> 
> (Updated April 13, 2012, 11:29 p.m.)
> 
> 
> Review request for KDevelop and Quanta.
> 
> 
> Description
> -------
> 
> Currently the XDebugJob always uses QDesktopServices::openUrl() to open the debug page in the browser, this patch makes use of the ExecuteBrowserPlugin instance that is already present in XDebug job so the configured browser is launched.
> 
> Also it makes BrowserAppJob launch the external browser KProc with .startDetached() instead of .execute() to prevent freezing of the KDevelop GUI.
> 
> 
> Arguments are currently ignored still, a patch for that is following
> 
> Feel free to nitpick, I haven't done any KDE coding in a while and would like to hear any suggestion for improvements :-)
> 
> 
> Diffs
> -----
> 
>   debuggers/xdebug/debugjob.h 9925733 
>   debuggers/xdebug/debugjob.cpp 0f04914 
>   plugins/executebrowser/browserappjob.h 37ff700 
>   plugins/executebrowser/browserappjob.cpp a211205 
>   plugins/executebrowser/executebrowserplugin.h 7c78733 
>   plugins/executebrowser/executebrowserplugin.cpp 921142f 
>   plugins/executebrowser/iexecutebrowserplugin.h f786622 
> 
> Diff: http://git.reviewboard.kde.org/r/104595/diff/
> 
> 
> Testing
> -------
> 
> It works ... ;-)
> 
> 
> Thanks,
> 
> Dominik Schmidt
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20120417/e3c3255e/attachment.html>