[Kde-pim] Review Request: knode: side-by-side view

[Olivier Trichet]

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


Thanks for your interest in KNode!

It is mostly ok. Below I put some comments

If you have time and interest, this would be great if you could look at bug 172608 which request for an extension of your patch. :-)


/trunk/KDE/kdepim/knode/knmainwidget.h
<http://reviewboard.kde.org/r/2526/#comment2935>

    Name it "mActArtLayout" to follow the KDEPIM coding style (http://pim.kde.org/development/coding-korganizer.php)



/trunk/KDE/kdepim/knode/knmainwidget.h
<http://reviewboard.kde.org/r/2526/#comment2936>

    Add space inside brace to follow the coding style.
    
    Better name it changeLayout( int i ) to better reflect its purpose.
    
    Please add API doc. It is mandatory for new code inside KNode.



/trunk/KDE/kdepim/knode/knmainwidget.cpp
<http://reviewboard.kde.org/r/2526/#comment2937>

    Please add KUIT markup (http://techbase.kde.org/Development/Tutorials/Localization/i18n_Semantics). This is also mandatory for new translable string inside KNode.
    
    Otherwise, the same coding style should apply.



/trunk/KDE/kdepim/knode/knmainwidget.cpp
<http://reviewboard.kde.org/r/2526/#comment2938>

    Is it really needed? This sets the sorting inside h_drView that is used at line 924 to set the current item.



/trunk/KDE/kdepim/knode/knmainwidget.cpp
<http://reviewboard.kde.org/r/2526/#comment2939>

    I would prefer if this were added in knode.kcfg and that KNGlobal::setting() was used to retrieve/save the value. Since the code is autogenerated, it's straightforward



/trunk/KDE/kdepim/knode/knmainwidget.cpp
<http://reviewboard.kde.org/r/2526/#comment2940>

    Remove the needless debug; there is far to much already :-)


- Olivier


On 2010-01-07 22:34:09, Matthew Woehlke wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/2526/
> -----------------------------------------------------------
> 
> (Updated 2010-01-07 22:34:09)
> 
> 
> Review request for KDE PIM.
> 
> 
> Summary
> -------
> 
> One of the "must have" features preventing me from switching from Thunderbird is a side by side layout; that is, folder list, message list, and message view in three columns (message view under message list is a waste of space (on my/large screens anyway); both will have almost half the right side always empty space).
> 
> Fortunately, it is quite simple to change the splitter between message list and message view from vertical to horizontal, and not hard to then make that configurable. So that is what this patch does; adds a new menu View->Layout->{Classic,Side by Side} (plus code to save/load the setting).
> 
> There is a small bug fix as well (line 725/734) that fixes slotArtSortHeaders not being bound correctly (and thus that menu being non-functional) since I copied that code for the new menu and had to track down the bug to get the new menu to work.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdepim/knode/knmainwidget.h 1068594 
>   /trunk/KDE/kdepim/knode/knmainwidget.cpp 1068594 
>   /trunk/KDE/kdepim/knode/knodeui.rc 1068594 
> 
> Diff: http://reviewboard.kde.org/r/2526/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Matthew
> 
>

_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/