[Kde-graphics-devel] [PATCH] ksnapshot: add feedback about when snapshot will be taken.

[Aaron J. Seigo]

On Friday 08 February 2008, Thomas Coopman wrote:
> I separated the patch in two because I also fixed a bug in
> regiongrabber (double clicking to grab a region didn't work correctly)
...
> and I changed the use of QBasicTimer to QTimer

comments:

* need to add license headers to your new files. they can't go into svn 
without them =)

* in SnapshotTimer::paintEvent() instead of using palette(), use the tooltip 
palette, e.g.:

      QPalette pal(QToolTip::palette());
      QColor handleColor = pal.color( QPalette::Active, QPalette::Highlight );
      handleColor.setAlpha( 160 );
      QColor overlayColor( 0, 0, 0, 160 );
      QColor textColor = pal.color( QPalette::Active, QPalette::Text );
      QColor textBackgroundColor = pal.color( QPalette::Active, 
QPalette::Base );

this way your tooltip will look like the other tooltips in KDE (e.g. follows 
the user's colour scheme).

* "Prepare for snapshot in %1" should maybe include the unit, in this case 
seconds? e.g. "Snapshot will be taken in %1 second(s)" ... so:

i18np("Snapshot will be taken in %1 second", "Snapshot will be taken in %1 
seconds", (length - time) );

feel free to ignore my changing of your text all around, but the 
word "seconds" should be in there and this does need to use the plural form 
of i18n, i18np, to be properly translatable in any case (even without the 
word "seconds" there)

minor stuff:

-        grabTimer.setSingleShot( true );

there's a grabTimer.stop() in the slot it's connected to, which i suppose 
works just fine. it might make just as much, if not more, sense to set both 
the timers in use there to single shots in the constructor and then get rid 
of all the stop() calls as they are then unecessary.

one other comment i have is on coding style. the rest of the ksnapshot 
codebase seems to use 4 space tabs (ok, except for that one ugly area where 
it uses tabs *puke* ;) .. it also seems to *mostly* use the "braces on the 
same line" style.. so as to not continue the 
devolution-into-random-indentation of ksnapshot, perhaps you could indent 
your feedback class with 4 space tabs and make this:

+    if ( !r.isNull() && r.isValid() )
+    {

into this:

+    if ( !r.isNull() && r.isValid() )  {

in RegionGrabber::grabRect()

otherwise .. i like! =) i'd say commit once some of these issues are taken 
care of (do you have an svn account?)... thanks for the patches, and good 
catch on the double click fubar..

-- 
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 194 bytes
Desc: This is a digitally signed message part.
Url : http://mail.kde.org/pipermail/kde-graphics-devel/attachments/20080208/a365fe5b/attachment.pgp