Review Request 123980: [Ark] Improve hide/show logic for information panel

[Raphael Kubo da Costa]

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


Thanks, this looks better! Being able to set the default values in the kcfg file and saving some lines of code in `part.cpp` is nice, but I still don't get the benefit of adding that boolean there: whereas before you only had to worry about the splitter sizes and calling `setSizes()` in some places, you now need to look at the boolean and from there decide whether to call `setSizes()` or `hide()`. What upside do you see in this approach?


part/part.cpp (line 109)
<https://git.reviewboard.kde.org/r/123980/#comment55559>

    Please end the comment with a period ('.') when landing.



part/part.cpp (line 114)
<https://git.reviewboard.kde.org/r/123980/#comment55560>

    You don't really need `splitterSizes` anymore, just call `m_splitter->setSizes(ArkSettings::splitterSizes())` below.



part/part.cpp (line 152)
<https://git.reviewboard.kde.org/r/123980/#comment55562>

    Small nitpick: it's better to remove this empty line so that all lines dealing with the splitters are grouped together.



part/part.cpp (line 913)
<https://git.reviewboard.kde.org/r/123980/#comment55561>

    Won't calling `show()` before `setSizes()` cause the info panel to have the wrong size for a short period of time?



part/part.cpp (line 917)
<https://git.reviewboard.kde.org/r/123980/#comment55563>

    "the user", and please remember to end the sentence with a period.


- Raphael Kubo da Costa


On June 3, 2015, 4:43 p.m., Ragnar Thomsen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123980/
> -----------------------------------------------------------
> 
> (Updated June 3, 2015, 4:43 p.m.)
> 
> 
> Review request for KDE Utils, Elvis Angelaccio and Raphael Kubo da Costa.
> 
> 
> Repository: ark
> 
> 
> Description
> -------
> 
> The logic governing hiding/showing of the infopanel as well as storing/retrieving the size of the QSplitter was reworked into a more intuitive and simple approach. A configuration option "showInfoPanel" of type bool was added, which stores whether or not the infopanel should be shown. The option "splitterSizesWithBothWidgets" was removed, since it's no longer needed. Default values of "splitterSizes" are now set in the kcfg file, instead of in the C++ code.
> 
> 
> Diffs
> -----
> 
>   kerfuffle/ark.kcfg 0c22014 
>   part/part.h 6afd90f 
>   part/part.cpp 7261631 
> 
> Diff: https://git.reviewboard.kde.org/r/123980/diff/
> 
> 
> Testing
> -------
> 
> Show/hide the information panel via the Settings menu works as expected. Ark remembers whether or not it was shown after closing/opening the program. The sizes of the splitter are also remembered after hiding/showing and after program restart. Deleting the splitterSizes line in ~/.config/arkrc and opening Ark results in the default sizes of 200/100 being used.
> 
> 
> Thanks,
> 
> Ragnar Thomsen
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-utils-devel/attachments/20150603/8d9f3232/attachment.html>