Review Request: Merge the final and fixed QML battery monitor to master.

[Sebastian Kügler]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104226/#review11311
-----------------------------------------------------------


The changes as they are add a few translation problems. There's also a bit of nitpicking here and there.

I haven't tested it yet, but the problems I'm pointing out will definitely need fixing before we can merge this into master.

Pretty good progress, however! :)


plasma/generic/applets/batterymonitor/contents/ui/PopupDialog.qml
<http://git.reviewboard.kde.org/r/104226/#comment9064>

    The word puzzle here is not translatable. You'll need to enclose a full string into i18n(), with the current code, translators can't figure out what the message is.
    
    Also, appending strings to each other doesn't work, as the word order might be different. So you have to identify the cases, and then return a completely translated string.



plasma/generic/applets/batterymonitor/contents/ui/PopupDialog.qml
<http://git.reviewboard.kde.org/r/104226/#comment9065>

    This should not be in there, basically only if it has been enabled by config showRemainingTime (look at the C++ version when it's shown). We explicitely excluded this feature by default since the remaining time cannot be accurately computed.



plasma/generic/applets/batterymonitor/contents/ui/PopupDialog.qml
<http://git.reviewboard.kde.org/r/104226/#comment9066>

    Word puzzles do not work for translators.
    
    This has to be done through KLocale, I don't see an option how to do it nicely and translatable.



plasma/generic/applets/batterymonitor/contents/ui/batterymonitor.qml
<http://git.reviewboard.kde.org/r/104226/#comment9067>

    instead of plasmoid.rootItem.pmSource, try using just pmSource. If necessary, that means moving pmSource somewhere visible.
    
    Should make porting to QML2 easier.



plasma/generic/applets/batterymonitor/contents/ui/batterymonitor.qml
<http://git.reviewboard.kde.org/r/104226/#comment9068>

    Same for plasmoid.pmSource location



plasma/generic/applets/batterymonitor/contents/ui/batterymonitor.qml
<http://git.reviewboard.kde.org/r/104226/#comment9069>

    This guy is unnecessary, as the exact same info is already shown in the dialog. I'd prefer getting rid of this overlay altogether (including the config option).



plasma/generic/applets/batterymonitor/contents/ui/batterymonitor.qml
<http://git.reviewboard.kde.org/r/104226/#comment9070>

    What's missing here? 
    
    Either ditch // TODO, or add a note what's missing



plasma/generic/dataengines/powermanagement/powermanagementjob.cpp
<http://git.reviewboard.kde.org/r/104226/#comment9071>

    This line always sets result to false, no matter what happened earlier.
    
    I think the code you added here is correct, could you check why it worked earlier?



plasma/generic/dataengines/powermanagement/powermanagementjob.cpp
<http://git.reviewboard.kde.org/r/104226/#comment9072>

    if it always returns true, it's useless to return anything. Make it void.


- Sebastian Kügler


On March 11, 2012, 1:59 p.m., Viranch Mehta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104226/
> -----------------------------------------------------------
> 
> (Updated March 11, 2012, 1:59 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Description
> -------
> 
> I fixed the QML battery monitor to be fairly usable and this diff merges it to master.
> 
> 
> Diffs
> -----
> 
>   plasma/generic/applets/CMakeLists.txt 2dedcb2 
>   plasma/generic/applets/battery/CMakeLists.txt 7794f88 
>   plasma/generic/applets/battery/Messages.sh 8b06e2d 
>   plasma/generic/applets/battery/README.txt 5b352e8 
>   plasma/generic/applets/battery/battery-oxygen-inkscape.svgz b68ba66 
>   plasma/generic/applets/battery/battery-oxygen.svgz a037e60 
>   plasma/generic/applets/battery/battery.h ebc1a3d 
>   plasma/generic/applets/battery/battery.cpp 3a5cda3 
>   plasma/generic/applets/battery/batteryConfig.ui 5595ca2 
>   plasma/generic/applets/battery/plasma-battery-default.desktop e254028 
>   plasma/generic/applets/batterymonitor/CMakeLists.txt PRE-CREATION 
>   plasma/generic/applets/batterymonitor/Messages.sh PRE-CREATION 
>   plasma/generic/applets/batterymonitor/README.txt PRE-CREATION 
>   plasma/generic/applets/batterymonitor/battery-oxygen-inkscape.svgz PRE-CREATION 
>   plasma/generic/applets/batterymonitor/battery-oxygen.svgz PRE-CREATION 
>   plasma/generic/applets/batterymonitor/contents/config/main.xml PRE-CREATION 
>   plasma/generic/applets/batterymonitor/contents/ui/IconButton.qml PRE-CREATION 
>   plasma/generic/applets/batterymonitor/contents/ui/PopupDialog.qml PRE-CREATION 
>   plasma/generic/applets/batterymonitor/contents/ui/batterymonitor.qml PRE-CREATION 
>   plasma/generic/applets/batterymonitor/contents/ui/config.ui PRE-CREATION 
>   plasma/generic/applets/batterymonitor/metadata.desktop PRE-CREATION 
>   plasma/generic/dataengines/powermanagement/powermanagementengine.h 20642c2 
>   plasma/generic/dataengines/powermanagement/powermanagementengine.cpp 5572fcb 
>   plasma/generic/dataengines/powermanagement/powermanagementjob.h 2c32015 
>   plasma/generic/dataengines/powermanagement/powermanagementjob.cpp e205bb0 
>   plasma/generic/dataengines/powermanagement/powermanagementservice.operations ad1301f 
> 
> Diff: http://git.reviewboard.kde.org/r/104226/diff/
> 
> 
> Testing
> -------
> 
> Applet and dataengine both tested and work fine.
> 
> 
> Thanks,
> 
> Viranch Mehta
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20120312/44035b95/attachment-0001.html>