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

[Sebastian Kügler]

Hi Viranch,

On Tuesday, March 13, 2012 09:52:27 Viranch Mehta wrote:
> > 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#file52425lin
> > > e81>> >
> > >     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?

kde-workspace/plasma/generic/applets/battery and grep for showRemainingTime, 
it's in there (master).

> > 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#file52432lin
> > > e87>> >
> > >     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).

Hm, the context even included in the diff does not have returns for every 
case, so it does skip to the last one.

> > 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#file52425lin
> > > e66>> >
> > >     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.

Then we can't merge it. Proper translation is really a hard requirement.

> > 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#file52426li
> > > ne65>> >
> > >     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).

OK, cool. Thanks for investigating.

Cheers,
-- 
sebas

http://www.kde.org | http://vizZzion.org | GPG Key ID: 9119 0EF9