Review Request 122753: [Krita] add the option to show/hide the title bars of the dockers to the settings menu

[Friedrich W. H. Kossebau]

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


Seems fine for what I tested :)

One possible UI thing is that collapsed dockers are completely invisible (besides a small empty stripe) if the docker titlebars are hidden. Which means that without titles shown dockers cannot be uncollapsed in any way. But that surely is a cornercase and people would simply need to reactivate headers if they want to change collapsing.

A few things in the patch should be improved though, so no Ship it yet from my side.


krita/ui/KisMainWindow.h
<https://git.reviewboard.kde.org/r/122753/#comment52884>

    Please remove the whitespace from empty lines that you added (but only from those, not from existing lines). See all the red marks in the patch as shown on Reviewboard.
    Unneeded trailing spaces are avoided if possible



libs/main/KoMainWindow.h
<https://git.reviewboard.kde.org/r/122753/#comment52881>

    Please remove the `const` here, make it only `bool show`). Calligra API conventions do not use it on parameters passed by type.
    Same also for `KisMainWindow::showDockerTitleBars` etc.



libs/main/KoMainWindow.cpp
<https://git.reviewboard.kde.org/r/122753/#comment52885>

    Please also here add a comment "@action:inmenu"



libs/main/KoMainWindow.cpp
<https://git.reviewboard.kde.org/r/122753/#comment52882>

    Small optimization: move this before the `foreach (QDockWidget *wdg, d->dockWidgets)` and cache the fetched config value, instead of doing all the fetching for each dockwidget
    ```
    KConfigGroup group = KGlobal::config()->group("Interface");
    const bool showDockerTitleBars = group.readEntry("ShowDockerTitleBars", true);
    foreach (QDockWidget *wdg, d->dockWidgets) {
        if ((wdg->features() & QDockWidget::DockWidgetClosable) == 0) {
            QWidget *titleBarWidget = wdg->titleBarWidget();
            if (titleBarWidget) {
                titleBarWidget->setVisible(showDockerTitleBars);
    ```



libs/main/KoMainWindow.cpp
<https://git.reviewboard.kde.org/r/122753/#comment52883>

    Not only for consistency please also check here if there is a titleBarWidget() or not, like done in `KoMainWindow::setActivePart(...)`.
    
    Same also for `KisMainWindow::showDockerTitleBars(...)`.


- Friedrich W. H. Kossebau


On März 1, 2015, 10:51 nachm., Moritz Molch wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122753/
> -----------------------------------------------------------
> 
> (Updated März 1, 2015, 10:51 nachm.)
> 
> 
> Review request for Calligra.
> 
> 
> Repository: calligra
> 
> 
> Description
> -------
> 
> Inspired by kdenlive, this small patch adds the option to show/hide the titlebars of all dockers to the settings menu.
> 
> 
> Diffs
> -----
> 
>   krita/krita.action b683185 
>   krita/krita.rc 0ec0931 
>   krita/ui/KisMainWindow.h 9cdcce5 
>   krita/ui/KisMainWindow.cpp e8c6009 
>   krita/ui/kis_config.h b6852de 
>   krita/ui/kis_config.cc a09c7b8 
>   libs/main/KoMainWindow.h ac6cc82 
>   libs/main/KoMainWindow.cpp b66fb49 
>   libs/main/calligra_shell.rc 3f09d7e 
>   libs/widgets/KoDockWidgetTitleBar.cpp cf0c722 
> 
> Diff: https://git.reviewboard.kde.org/r/122753/diff/
> 
> 
> Testing
> -------
> 
> Tested on Ubuntu 14.04
> 
> 
> Thanks,
> 
> Moritz Molch
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20150302/f8257f92/attachment.htm>