<div dir="ltr">Hi<div>According to <span style="background-color:rgb(255, 255, 255)">Nikhil's</span> suggestions,im attaching a new patch for a review.Please test it :)</div><div><br></div><div>
Regards</div><div>Rohan Garg<br clear="all"><a href="http://www.launchpad.net/~rohangarg" target="_blank">www.launchpad.net/~rohangarg</a><br>
<br><br><div class="gmail_quote">On Mon, Jul 5, 2010 at 2:27 AM, Nikhil Marathe <span dir="ltr"><<a href="mailto:nsm.nikhil@gmail.com" target="_blank">nsm.nikhil@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div>On Mon, Jul 5, 2010 at 12:29 AM, Rohan Garg <<a href="mailto:rohan16garg@gmail.com" target="_blank">rohan16garg@gmail.com</a>> wrote:<br>
> Hi<br>
> After working for about half an hour on this,i came up with the attached<br>
> patch for apturl support in rekonq.It would be nice if this can be<br>
> incorporated in the 0.5 release.<br>
<br>
</div>Hey Rohan,<br>
<br>
I have a couple of comments about the patch.<br>
<br>
1. Could you move the initialisation of the QStringList and the url<br>
handling inside the if conditional for the "apt" url.<br>
Prevents unnecessary init and allocation.<br>
<br>
2. I believe you should check for KProcess exit status and report it.<br>
BTW does just passing the URL to the exec() make it interpret it<br>
correctly.<br>
<br>
3. How do you handle non-debian systems. Like if I execute this on a<br>
archlinux system it should have a well defined behaviour. Perhaps you<br>
could check if the 'apt' program exists?<br>
<font color="#888888"><br>
Nikhil<br>
</font></blockquote></div><br></div></div>