Review Request 129884: Fix for bug 360539

Damir Porobic damir_porobic at live.com
Thu Jan 26 10:40:12 UTC 2017



> On Jan. 26, 2017, 9:58 a.m., Tomaz  Canabrava wrote:
> > I know that this was not accepted because of the other review that exists, but I think it's worth the review, please take a look at my comments for your *next* trial.
> > also, use phabricator (with the arc diff command) instead of reviewboard.

Thanks for providing the feedback, that helps better understand the code. Is it worth reworking this as you have obviously provided a solution for this already and I don't wont to bind resources just for teaching me. 
I was using reviewboard because the KDE handbook said so. Should I skip it and go directly to phabricator (-> create diff)?


> On Jan. 26, 2017, 9:58 a.m., Tomaz  Canabrava wrote:
> > sublime/idealdockwidget.cpp, line 86
> > <https://git.reviewboard.kde.org/r/129884/diff/1/?file=490167#file490167line86>
> >
> >     m_view->document()->title() seems to be the title of the document, not the toolbar, so if you opened two or three tolbars, this would work?
> >     
> >     also, the string "Toolbar for %1" seems strange in a Configuration file (usually there are no spaces)

Usually I would not use spaces but I was not sure how you guys usually do it so I picket this format from another config snippet, the one that was saving new shortcuts and changed the name. 

Regarding the title, is it expected for a for a Toolview to have more then one toolbar? If yes, then yes, it's eventually not going to work as expected.


> On Jan. 26, 2017, 9:58 a.m., Tomaz  Canabrava wrote:
> > sublime/idealdockwidget.cpp, line 84
> > <https://git.reviewboard.kde.org/r/129884/diff/1/?file=490167#file490167line84>
> >
> >     This a Expensive call, it will go thru *all* children of the widget() and test for QToolBars, findChild should be the last option.

I saw from your patch the the toolbar is created in idealcontroller.cpp (totally missed that), would be smarter to create this functionality there, that would avoid using findChild.


> On Jan. 26, 2017, 9:58 a.m., Tomaz  Canabrava wrote:
> > sublime/idealdockwidget.cpp, line 87
> > <https://git.reviewboard.kde.org/r/129884/diff/1/?file=490167#file490167line87>
> >
> >     Qt4 style connects shouldn't be used, because they could have errors that are not checked at build time.

Was not aware of that, I should be using something like this?
connect(toolBar,visibilityChanged(bool),&QAction::toggled,this,toolbarVisibilityChange(bool));


> On Jan. 26, 2017, 9:58 a.m., Tomaz  Canabrava wrote:
> > sublime/idealdockwidget.cpp, line 89
> > <https://git.reviewboard.kde.org/r/129884/diff/1/?file=490167#file490167line89>
> >
> >     Lots of spaces not removed on the code after you saved your files.

Was not aware that I need to avoid them. Will keep an eye on it in future.


- Damir


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129884/#review102247
-----------------------------------------------------------


On Jan. 25, 2017, 7:28 p.m., Damir Porobic wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129884/
> -----------------------------------------------------------
> 
> (Updated Jan. 25, 2017, 7:28 p.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Bugs: 360539
>     http://bugs.kde.org/show_bug.cgi?id=360539
> 
> 
> Repository: kdevplatform
> 
> 
> Description
> -------
> 
> Fix for bug 360539. Saving visibility status of view toolbars
> 
> 
> Diffs
> -----
> 
>   sublime/idealdockwidget.h c966481 
>   sublime/idealdockwidget.cpp c79b1ba 
> 
> Diff: https://git.reviewboard.kde.org/r/129884/diff/
> 
> 
> Testing
> -------
> 
> Tested if toolbars are correctly hidden and shown again after closing and opening up kdevelop. 
> Checked config kdeveloprc to confirm that variables are written corretly
> 
> 
> Thanks,
> 
> Damir Porobic
> 
>

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


More information about the KDevelop-devel mailing list