[rekonq] Re: Review Request: GCI Task: Copy Context Menu for Network Analyzer

[Panagiotis Papadopoulos]

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


Comments from the rekonq mailing list:

t would be nice to sort the includes by alphabetical order (like for 
WebKit). 


NetworkAnalyzer::contextMenuEvent() mixes spaces and tabs for the 
indentation.


In NetworkAnalyzer::contextMenuEvent(), the parent of the KMenu is 
messing things up. QObjects created on the stack should not have parents.


+       KAction *copy;
+       copy = new KAction(KIcon("edit-copy"),i18n("Copy URL"), this);
Could be:
KAction *const copy = new KAction(KIcon("edit-copy"),i18n("Copy URL"), 
this);
(Just a suggestion, some knows how much I like "const" :)).


In NetworkAnalyzer::contextMenuEvent(), the connect() misses spaces 
after the comas to follow the coding style.


I am not sure how this connection works:
connect( _requestList, SIGNAL(customContextMenuRequested(QPoint)), this, 
SLOT(contextMenuEvent(QContextMenuEvent*))) ;
The arguments seem incompatible to me.


by Benjamin Poulain

——————


> I am not sure how this connection works:
> connect( _requestList, SIGNAL(customContextMenuRequested(QPoint)), this,
> SLOT(contextMenuEvent(QContextMenuEvent*))) ;
> The arguments seem incompatible to me.

Yes, code is wrong there and need fixes.

IMHO, the copyURL() method should be renamed as "copyUrl()".

Apart then those, it seems working well.


by Andrea Diamantini



- Panagiotis


On 2010-12-09 14:26:40, Furkan Üzümcü wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100198/
> -----------------------------------------------------------
> 
> (Updated 2010-12-09 14:26:40)
> 
> 
> Review request for rekonq.
> 
> 
> Summary
> -------
> 
> * Copy context menu for Network Analyzer.
> 
> 
> Diffs
> -----
> 
>   src/analyzer/networkanalyzer.h 9e38663 
>   src/analyzer/networkanalyzer.cpp c5b0883 
> 
> Diff: http://git.reviewboard.kde.org/r/100198/diff
> 
> 
> Testing
> -------
> 
> Tested and works cool!
> 
> 
> Thanks,
> 
> Furkan
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/rekonq/attachments/20101210/868e3dab/attachment.htm