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

List:       kde-panel-devel
Subject:    Re: [Panel-devel] [PATCH] Beginnings of a panel implementation for
From:       "Aaron J. Seigo" <aseigo () kde ! org>
Date:       2007-08-31 20:18:55
Message-ID: 200708311418.56088.aseigo () kde ! org
[Download RAW message or body]

[Attachment #2 (multipart/signed)]


On Friday 31 August 2007, Robert Knight wrote:
> Updated version of the patch attached in response to initial review by
> richmoore.

looks mostly good =)

i suppose  BoxLayout::expandingDirections() should really just return 
d->expandingDirections()... or even better, get rid of 
Private::expandingDirections and just always use 
BoxLayout::expandingDirections()?

coding conventions aren't followed. e.g.:

+        if ( isExpanding )    
+            sizes[i] = perItemSize;      
+        else    
+            sizes[i] = d->size(item->sizeHint());

should be:


+        if (isExpanding) {
+            sizes[i] = perItemSize;      
+        } else {
+            sizes[i] = d->size(item->sizeHint());
+        }

i can't say i'm overly pleased about putting multiple classes in a single file 
either, even if they are tiny.

it also references layoutAnimator which doesn't seem to be in the patch. i 
guess you could commit it with that commented out before we move on to the 
animation support you've done?


> - Basic testing with nested layouts.

as in "you've done basic testing" or "you have a basic test you want to commit 
to svn"?

-- 
Aaron J. Seigo
humru othro a kohnu se
GPG Fingerprint: 8B8B 2209 0C6F 7C47 B1EA  EE75 D6B7 2EB1 A7F1 DB43

KDE core developer sponsored by Trolltech

[Attachment #5 (application/pgp-signature)]

_______________________________________________
Panel-devel mailing list
Panel-devel@kde.org
https://mail.kde.org/mailman/listinfo/panel-devel


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