[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-panel-devel
Subject: Re: Review Request: [Picture Frame] Update function for url's and
From: Sebastian_Kügler <sebas () kde ! org>
Date: 2010-03-29 11:08:29
Message-ID: 20100329110829.6481.57497 () localhost
[Download RAW message or body]
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/3334/#review4733
-----------------------------------------------------------
trunk/KDE/kdeplasma-addons/applets/frame/configdialog.h
<http://reviewboard.kde.org/r/3334/#comment4233>
trailing spaces, please get rid of those
trunk/KDE/kdeplasma-addons/applets/frame/configdialog.h
<http://reviewboard.kde.org/r/3334/#comment4235>
Maybe reloadAutomatically() or toggleAutoReload(), the function name isn't really \
clear
trunk/KDE/kdeplasma-addons/applets/frame/configdialog.cpp
<http://reviewboard.kde.org/r/3334/#comment4234>
remove spacs (... and trailing
trunk/KDE/kdeplasma-addons/applets/frame/configdialog.cpp
<http://reviewboard.kde.org/r/3334/#comment4236>
the livecam naming is a bit confusing, maybe put what it does? Something with \
automatic reloading?
no const needed here afaik
trunk/KDE/kdeplasma-addons/applets/frame/frame.cpp
<http://reviewboard.kde.org/r/3334/#comment4238>
"" -> QString() (also elsewhere)
trunk/KDE/kdeplasma-addons/applets/frame/frame.cpp
<http://reviewboard.kde.org/r/3334/#comment4237>
Use the one from KGlobal here as default that makes more sense
trunk/KDE/kdeplasma-addons/applets/frame/frame.cpp
<http://reviewboard.kde.org/r/3334/#comment4239>
use kDebug
trunk/KDE/kdeplasma-addons/applets/frame/frame.cpp
<http://reviewboard.kde.org/r/3334/#comment4240>
kDebug()
trunk/KDE/kdeplasma-addons/applets/frame/frame.cpp
<http://reviewboard.kde.org/r/3334/#comment4241>
kDebug(), English please :)
trunk/KDE/kdeplasma-addons/applets/frame/frame.cpp
<http://reviewboard.kde.org/r/3334/#comment4242>
You're creating a magic value here (""), why not use the KGlobal downloadpath?
Also, this statement should read !m_downloadPath.isEmpty()
trunk/KDE/kdeplasma-addons/applets/frame/frame.cpp
<http://reviewboard.kde.org/r/3334/#comment4243>
space between ) and {
trunk/KDE/kdeplasma-addons/applets/frame/imageSettings.ui
<http://reviewboard.kde.org/r/3334/#comment4244>
Useful (single l), came -> cam
trunk/KDE/kdeplasma-addons/applets/frame/imageSettings.ui
<http://reviewboard.kde.org/r/3334/#comment4245>
hh:mm should be good enough, this way the text would look less confusing
trunk/KDE/kdeplasma-addons/applets/frame/picture.h
<http://reviewboard.kde.org/r/3334/#comment4246>
trailing space
trunk/KDE/kdeplasma-addons/applets/frame/picture.cpp
<http://reviewboard.kde.org/r/3334/#comment4247>
4 spaces indenting
trunk/KDE/kdeplasma-addons/applets/frame/urlSettings.ui
<http://reviewboard.kde.org/r/3334/#comment4248>
get -> gets
trunk/KDE/kdeplasma-addons/applets/frame/urlSettings.ui
<http://reviewboard.kde.org/r/3334/#comment4249>
overwritten
- Sebastian
On 2010-03-27 15:08:02, eti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3334/
> -----------------------------------------------------------
>
> (Updated 2010-03-27 15:08:02)
>
>
> Review request for Plasma.
>
>
> Summary
> -------
>
> I was quite happy that the picture frame could handle url's in 4.4.
> So i added the ability to update the picture with an url periodicaly (i.e. live \
> cams, weather data or <picture that changes over time>). This would be a rather \
> small fix, but i also came accross Bug 222759 and with the updatefunction the \
> download path got quite spamed. Now it's possible to set the download path and the \
> ability to ovewrite the existing picture when downloaded.
> By the way, i also removed the picture class from the slideshow class. The \
> slideshow now manages only the path's and the frame class acceses the picture class \
> directly.
> Let me know wat you think;)
>
>
> Diffs
> -----
>
> trunk/KDE/kdeplasma-addons/applets/frame/CMakeLists.txt 1105894
> trunk/KDE/kdeplasma-addons/applets/frame/configdialog.h 1105894
> trunk/KDE/kdeplasma-addons/applets/frame/configdialog.cpp 1105894
> trunk/KDE/kdeplasma-addons/applets/frame/frame.h 1105894
> trunk/KDE/kdeplasma-addons/applets/frame/frame.cpp 1105894
> trunk/KDE/kdeplasma-addons/applets/frame/imageSettings.ui 1105894
> trunk/KDE/kdeplasma-addons/applets/frame/picture.h 1105894
> trunk/KDE/kdeplasma-addons/applets/frame/picture.cpp 1105894
> trunk/KDE/kdeplasma-addons/applets/frame/slideshow.h 1105894
> trunk/KDE/kdeplasma-addons/applets/frame/slideshow.cpp 1105894
> trunk/KDE/kdeplasma-addons/applets/frame/urlSettings.ui PRE-CREATION
>
> Diff: http://reviewboard.kde.org/r/3334/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> eti
>
>
_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic