> On April 8, 2013, 5:39 p.m., Aaron J. Seigo wrote: > > runners/translator/translator.cpp, lines 116-117 > > <http://git.reviewboard.kde.org/r/109773/diff/5/?file=131524#file131524line116> > > > > might be nice to pop a small comment here why this is required
Google responds with a JSON array like this: ["foo",,"bar"]. Unfortunately this is nod valid JSON, QJSON expects ["foo","","bar"]. I added a comment in the code. > On April 8, 2013, 5:39 p.m., Aaron J. Seigo wrote: > > runners/translator/translator.cpp, line 131 > > <http://git.reviewboard.kde.org/r/109773/diff/5/?file=131524#file131524line131> > > > > l0 is not a great variable name. please name it something descriptive > > to its purpose. You're right. Hope it's better now. > On April 8, 2013, 5:39 p.m., Aaron J. Seigo wrote: > > runners/translator/translator.cpp, lines 184-192 > > <http://git.reviewboard.kde.org/r/109773/diff/5/?file=131524#file131524line184> > > > > i would suggest taking advantage of the fact that QMap is sorted. in > > the while loop, the code already does a check for i.key() == key, so one > > may as well do that from the start and do something like: > > > > QMapIterator<int, QPair<QString, double> > it(sentences); > > int currentKey = -1; > > double currentRel = 1; > > QString currentString; > > > > while (it.hasNext()) { > > pair = it.next(); > > > > // we're on to another key, process previous results, if any > > if (currentKey != it.key()) { > > > > if (!combined.isEmpty()) { > > Plasma::QueryMatch match(this); > > .. setup match .. > > combined.clear(); > > } > > > > currentKey = it.key(); > > currentRel = 1; > > currentString.clear(); > > } > > > > currentString.append(' ').append(pair.first); > > currentRel *= pair.second; > > } > > > > this woudl avoid the more expensive calls of uniqueKeys and find and > > make the code a lot easier to read. (assuming it does what i think it does > > from reading it :) Nice! And yes, it does what you think it does. ;-) - David ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109773/#review30684 ----------------------------------------------------------- On April 15, 2013, 1:17 a.m., David Baum wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/109773/ > ----------------------------------------------------------- > > (Updated April 15, 2013, 1:17 a.m.) > > > Review request for Plasma. > > > Description > ------- > > The Runner uses the Google Translate Homepage and supports all languages > provided by Google. It doesn't use the API, because it's not free of charge. > > > Diffs > ----- > > runners/CMakeLists.txt bb4b491b10e6fef8183a66f55f5d5832dd7bc41a > runners/translator/CMakeLists.txt PRE-CREATION > runners/translator/plasma-runner-translator.desktop PRE-CREATION > runners/translator/translator.h PRE-CREATION > runners/translator/translator.cpp PRE-CREATION > runners/translator/translatorjob.h PRE-CREATION > runners/translator/translatorjob.cpp PRE-CREATION > > Diff: http://git.reviewboard.kde.org/r/109773/diff/ > > > Testing > ------- > > > Thanks, > > David Baum > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
