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

[Matthew Woehlke]

> On 2010-01-09 16:02:34, Olivier Trichet wrote:
> > 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. :-)

That's far more complicated than I'm interested in taking on, sorry. (And if KNode ends up subsumed into KMail anyway, possibly a waste of time...)


> On 2010-01-09 16:02:34, Olivier Trichet wrote:
> > /trunk/KDE/kdepim/knode/knmainwidget.h, line 275
> > <http://reviewboard.kde.org/r/2526/diff/1/?file=16717#file16717line275>
> >
> >     Name it "mActArtLayout" to follow the KDEPIM coding style (http://pim.kde.org/development/coding-korganizer.php)

I named it consistent with the existing actions. I don't think naming it differently is desirable, but I can look into fixing everything as a separate commit.


> On 2010-01-09 16:02:34, Olivier Trichet wrote:
> > /trunk/KDE/kdepim/knode/knmainwidget.cpp, lines 716-719
> > <http://reviewboard.kde.org/r/2526/diff/1/?file=16718#file16718line716>
> >
> >     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.

Again, I prefer to keep consistent style or fix everything. Is there an invocation astyle that can be used? (Is it the same as for kdelibs, perhaps?)


> On 2010-01-09 16:02:34, Olivier Trichet wrote:
> > /trunk/KDE/kdepim/knode/knmainwidget.cpp, line 1678
> > <http://reviewboard.kde.org/r/2526/diff/1/?file=16718#file16718line1678>
> >
> >     Remove the needless debug; there is far to much already :-)

I would have found it useful in testing, except that I don't seem to get any of the 5003 debug messages (I /think/ I have it turned on in kdebugdialog; checked is on?). I actually had a 'fprintf(stderr, ...)' in here during testing. IMHO this one should stay, or all the slot debugs should go (or maybe better, be moved to a different debug area). It's not going to make a lot of noise compared to some of the others that are at least as "needless".


- Matthew


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


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/