> 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 > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
