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

[Thomas Coopman]

---------- Forwarded message ----------
From: Thomas Coopman <thomas.coopman at gmail.com>
Date: Feb 8, 2008 9:12 PM
Subject: Re: [Kde-graphics-devel] [PATCH] ksnapshot: add feedback
about when snapshot will be taken.
To: "Aaron J. Seigo" <aseigo at kde.org>


On Feb 8, 2008 5:50 PM, Aaron J. Seigo <aseigo at kde.org> wrote:
> 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 =)

done
>
> * 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 );

done
>
> 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)

done
>
> 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()
4 space tabs: done,
about the braces: regiongrabber.cpp has it braces always on a new line, or I
>
> 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..
>
I also fixed regiongrabber to use QToolTip::pallete() and
QToolTip::font(), and commited that change.

> --
> 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
>
> _______________________________________________
> Kde-graphics-devel mailing list
> Kde-graphics-devel at kde.org
> https://mail.kde.org/mailman/listinfo/kde-graphics-devel
>
>



--

Thomas Coopman
Thomas.coopman at gmail.com



-- 
Thomas Coopman
Thomas.coopman at gmail.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ksnapshot_feedback3.diff
Type: text/x-patch
Size: 6099 bytes
Desc: not available
Url : http://mail.kde.org/pipermail/kde-graphics-devel/attachments/20080208/88d3bcb2/attachment-0001.bin