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

[Viranch Mehta]

> On March 12, 2012, 10:31 a.m., Sebastian Kügler wrote:
> > plasma/generic/applets/batterymonitor/contents/ui/PopupDialog.qml, line 81
> > <http://git.reviewboard.kde.org/r/104226/diff/1/?file=52425#file52425line81>
> >
> >     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.

I don't see a showRemainingTime config in C++ version (I'm looking at master). Should I add this in the QML version?


> On March 12, 2012, 10:31 a.m., Sebastian Kügler wrote:
> > plasma/generic/dataengines/powermanagement/powermanagementjob.cpp, line 87
> > <http://git.reviewboard.kde.org/r/104226/diff/1/?file=52432#file52432line87>
> >
> >     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?

The control reaches here only if there were no 'return's earlier. It works because there is a return inside each if case. So if the operation name was invalid, it would setResult(false), otherwise setResult(whatever the operation returns).


> On March 12, 2012, 10:31 a.m., Sebastian Kügler wrote:
> > plasma/generic/applets/batterymonitor/contents/ui/PopupDialog.qml, line 66
> > <http://git.reviewboard.kde.org/r/104226/diff/1/?file=52425#file52425line66>
> >
> >     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.

For now, I'm dropping i18n() completely on such computed strings.


> On March 12, 2012, 10:31 a.m., Sebastian Kügler wrote:
> > plasma/generic/applets/batterymonitor/contents/ui/batterymonitor.qml, line 65
> > <http://git.reviewboard.kde.org/r/104226/diff/1/?file=52426#file52426line65>
> >
> >     instead of plasmoid.rootItem.pmSource, try using just pmSource. If necessary, that means moving pmSource somewhere visible.
> >     
> >     Should make porting to QML2 easier.

Just pmSource doesn't work because compactRepresentation is a Component, and hence pmSource cannot be moved to any visible location. This is the only way to do it AFAICT. Use of plasmoid.rootItem.pmSource can be reduced by assigning it to another variable inside the component. (Look at updated diff).


> On March 12, 2012, 10:31 a.m., Sebastian Kügler wrote:
> > plasma/generic/applets/batterymonitor/contents/ui/batterymonitor.qml, line 89
> > <http://git.reviewboard.kde.org/r/104226/diff/1/?file=52426#file52426line89>
> >
> >     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).

Getting this info from dialog requires two clicks (opening and closing the dialog). While this overlay simply displays it. We can still drop it if desired.


> On March 12, 2012, 10:31 a.m., Sebastian Kügler wrote:
> > plasma/generic/applets/batterymonitor/contents/ui/batterymonitor.qml, line 144
> > <http://git.reviewboard.kde.org/r/104226/diff/1/?file=52426#file52426line144>
> >
> >     What's missing here? 
> >     
> >     Either ditch // TODO, or add a note what's missing

I figured out what should be done here, and implemented (see updated diff).


- Viranch


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


On March 13, 2012, 9:51 a.m., Viranch Mehta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104226/
> -----------------------------------------------------------
> 
> (Updated March 13, 2012, 9:51 a.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/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/20120313/0ffca285/attachment-0001.html>