Review Request: Obey locale KCM settings for data format in the Digital Clock Plasma applet

[Aaron Seigo]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/6272/#review9522
-----------------------------------------------------------


a few issues, mostly simple things about whitespacing, but one significant issue with the launching of the kcm. once that is fixed up, please commit :)


/trunk/KDE/kdebase/workspace/plasma/generic/applets/digital-clock/clock.cpp
<http://svn.reviewboard.kde.org/r/6272/#comment10521>

    whitespace, should be: if (cg.hasKey("showYear")) {



/trunk/KDE/kdebase/workspace/plasma/generic/applets/digital-clock/clock.cpp
<http://svn.reviewboard.kde.org/r/6272/#comment10523>

    whitspace; also {} are missing (we use them around even single-line if statements)



/trunk/KDE/kdebase/workspace/plasma/generic/applets/digital-clock/clock.cpp
<http://svn.reviewboard.kde.org/r/6272/#comment10524>

    probably unecessary; doesn't do any harm to have it sitting there, so no reason to spend cycles on it or leave code related to it there for us to figure out later if we should keep it :)



/trunk/KDE/kdebase/workspace/plasma/generic/applets/digital-clock/clock.cpp
<http://svn.reviewboard.kde.org/r/6272/#comment10526>

    whitespace: no spaces after '(' or before ')'



/trunk/KDE/kdebase/workspace/plasma/generic/applets/digital-clock/clock.cpp
<http://svn.reviewboard.kde.org/r/6272/#comment10525>

    this warning already occurs in the kcm in KCMLocale::save(), though it is currently only triggered on language changes. should probably be also cover dates.
    
    this warning isn't needed here, however, but in the kcm.



/trunk/KDE/kdebase/workspace/plasma/generic/applets/digital-clock/clock.cpp
<http://svn.reviewboard.kde.org/r/6272/#comment10527>

    } else if (m_dateStyle == 2) {



/trunk/KDE/kdebase/workspace/plasma/generic/applets/digital-clock/clock.cpp
<http://svn.reviewboard.kde.org/r/6272/#comment10528>

    exec() is blocking, which means it will hang the rest of plasma-desktop, so that can't happen.
    
    instead, try this:
    
        KService::List offers = KServiceTypeTrader::self()->query("KCModule", "Library == 'kcm_locale'");
        if (!offers.isEmpty()) {
            KService::Ptr service = offers.first();
            KRun::run(*service, KUrl::List(), 0);
        }


- Aaron


On 2011-01-04 19:47:45, Teo Mrnjavac wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/6272/
> -----------------------------------------------------------
> 
> (Updated 2011-01-04 19:47:45)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> Replaces the hardcoded date formats in the digital clock applet config dialog with the following choices:
> * No date
> * Compact date
> * Short date (obeys the KCM)
> * Long date (obeys the KCM)
> * ISO date
> 
> Also adds a button to launch the country/region and language settings KCM.
> 
> 
> This addresses bugs 195837 and 247277.
>     https://bugs.kde.org/show_bug.cgi?id=195837
>     https://bugs.kde.org/show_bug.cgi?id=247277
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/digital-clock/CMakeLists.txt 1211741 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/digital-clock/clock.h 1211741 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/digital-clock/clock.cpp 1211741 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/digital-clock/clockConfig.ui 1211741 
> 
> Diff: http://svn.reviewboard.kde.org/r/6272/diff
> 
> 
> Testing
> -------
> 
> During and after coding.
> 
> 
> Thanks,
> 
> Teo
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/plasma-devel/attachments/20110104/a96b66d7/attachment-0001.htm