[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-pim
Subject:    Re: [Kde-pim] Review Request: knode: side-by-side view
From:       "Matthew Woehlke" <mw_triad () users ! sourceforge ! net>
Date:       2010-01-11 17:49:16
Message-ID: 20100111174916.24631.19619 () localhost
[Download RAW message or body]



> 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@kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/


[prev in list] [next in list] [prev in thread] [next in thread]