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

[Olivier Trichet]

> 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. :-)
> 
> Matthew Woehlke wrote:
>     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...)

No problem. I had to try ;-)


> 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.
> 
> Matthew Woehlke wrote:
>     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?)

Again, we change the style only when we modify the code.
There are many working branches of KDEPIM and changing the style of every files would mess up merge between these branches. Even if, to be honest, KNode is the only that is not concerned by these merges.


> 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 :-)
> 
> Matthew Woehlke wrote:
>     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".

ok, I don't really have a rstrong opinion on this one.

With debug area 5003 check in kdebugdialog, the debug should have been displayed in the log after a restart of knode. Unless you are compiling in release mode.

PS : the debug area (5003) is declared in the CMakeLists.txt file, anyway. So "kDebug()" can be use instead of "kDebug(5003)" now.


> 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)
> 
> Matthew Woehlke wrote:
>     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.

I am already doing this mix of naming scheme when I add attributes. So it would be consistent :-)


- Olivier


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


On 2010-01-11 17:52:39, Matthew Woehlke wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/2526/
> -----------------------------------------------------------
> 
> (Updated 2010-01-11 17:52:39)
> 
> 
> 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 1073089 
>   /trunk/KDE/kdepim/knode/knmainwidget.cpp 1073089 
>   /trunk/KDE/kdepim/knode/knodeui.rc 1073089 
> 
> 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/