Review Request: Fix for bug 300248

[Frank Reininghaus]

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


Thanks for the new patch, looks good! And thanks for the hint about the arrow icon, Todd, I agree that being consistent with other apps here is important.

One thing that I noticed: maybe the variable 'visible' could get a better name to make it clear at first sight what exactly is visible. Maybe 'advancedOptionsShown' or something like that?

Another thing that I noticed: I think it might be better to move the line 'SearchSettings::setShowFacetsWidget(visible)' from DolphinSearchBox::slotFacetsButtonToggled() to DolphinSearchBox::saveSettings(). This would make this more consistent with the other settings and prevent that the settings are saved before the 'showFacetsWidget' setting is changed in the SearchSettings (note that we can't make any assumptions about the order in which the slots invoked by clicking the button are called).

Do you agree that these changes make sense? If you have a git account, you can go ahead and push this to master. Otherwise, just upload the final version of the patch here and I'll do it for you.

Thanks to everyone who helped to find the best solution for this issue!

- Frank Reininghaus


On Sept. 5, 2012, 10:09 a.m., Panos Kanavos wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106325/
> -----------------------------------------------------------
> 
> (Updated Sept. 5, 2012, 10:09 a.m.)
> 
> 
> Review request for Dolphin.
> 
> 
> Description
> -------
> 
> Provides a possible improvement to Dolphin's additional search criteria button, according to the request in https://bugs.kde.org/show_bug.cgi?id=300248.
> 
> 
> Diffs
> -----
> 
>   dolphin/src/search/dolphinsearchbox.h ee9987a 
>   dolphin/src/search/dolphinsearchbox.cpp 28f1f1a 
> 
> Diff: http://git.reviewboard.kde.org/r/106325/diff/
> 
> 
> Testing
> -------
> 
> The modified button behaves at it should.
> 
> 
> Thanks,
> 
> Panos Kanavos
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20120905/002535be/attachment.htm>