----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109773/#review30684 -----------------------------------------------------------
runners/translator/translator.cpp <http://git.reviewboard.kde.org/r/109773/#comment22823> "<language code>" needs to be translated runners/translator/translator.cpp <http://git.reviewboard.kde.org/r/109773/#comment22824> as above, this needs to be i18n'd .. and languagce -> language (small typo) runners/translator/translator.cpp <http://git.reviewboard.kde.org/r/109773/#comment22844> see comment on line 109 below runners/translator/translator.cpp <http://git.reviewboard.kde.org/r/109773/#comment22825> you should use "!language.second.isEmpty()" instead of comparing with an empty string, but in any case this is not necessary as supportedLanguages should never contain "", right? :) runners/translator/translator.cpp <http://git.reviewboard.kde.org/r/109773/#comment22830> might be nice to pop a small comment here why this is required runners/translator/translator.cpp <http://git.reviewboard.kde.org/r/109773/#comment22829> should be commented out prior to committing runners/translator/translator.cpp <http://git.reviewboard.kde.org/r/109773/#comment22828> indentation looks like it went a bit off from here? runners/translator/translator.cpp <http://git.reviewboard.kde.org/r/109773/#comment22831> l0 is not a great variable name. please name it something descriptive to its purpose. runners/translator/translator.cpp <http://git.reviewboard.kde.org/r/109773/#comment22832> same as with l0 above runners/translator/translator.cpp <http://git.reviewboard.kde.org/r/109773/#comment22833> same as with l0 above runners/translator/translator.cpp <http://git.reviewboard.kde.org/r/109773/#comment22836> a comment here about why there is a new and old "foundWord" might be useful for people touching this code in future runners/translator/translator.cpp <http://git.reviewboard.kde.org/r/109773/#comment22835> last2.at(1) implies at least 2 entries in list2.. so this should probably be: bool foundWordNew = list2.size() > 1 && !list2.at(1).toList().isEmpty(); runners/translator/translator.cpp <http://git.reviewboard.kde.org/r/109773/#comment22834> indexOf(l1) could be cached above to avoid having to re-search the list? runners/translator/translator.cpp <http://git.reviewboard.kde.org/r/109773/#comment22838> this should of type Informational rather than ExactMatch since there is no further action to be taken (and selecting it should copy it to the clipboard) runners/translator/translator.cpp <http://git.reviewboard.kde.org/r/109773/#comment22837> elsewhere you do "} else {"; would be good to be consistent runners/translator/translator.cpp <http://git.reviewboard.kde.org/r/109773/#comment22843> 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 :) runners/translator/translator.cpp <http://git.reviewboard.kde.org/r/109773/#comment22840> pref to use tmp.isEmpty() runners/translator/translator.cpp <http://git.reviewboard.kde.org/r/109773/#comment22839> type Informational runners/translator/translator.cpp <http://git.reviewboard.kde.org/r/109773/#comment22827> as this is used as a look up table, to make it faster use a QSet<QString> instead. here the difference will be tiny as string comparisons aren't THAT slow and there are only a few dozen 2 char strings to match against. still .. :) runners/translator/translatorjob.cpp <http://git.reviewboard.kde.org/r/109773/#comment22847> this is leaking. from the QNetworkAccessManager docs: "Note: After the request has finished, it is the responsibility of the user to delete the QNetworkReply object at an appropriate time. Do not directly delete it inside the slot connected to finished(). You can use the deleteLater() function." so in jobCompleted, a reply->deleteLater() is required. - Aaron J. Seigo On April 5, 2013, 10:39 p.m., David Baum wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/109773/ > ----------------------------------------------------------- > > (Updated April 5, 2013, 10:39 p.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/translator/CMakeLists.txt PRE-CREATION > runners/CMakeLists.txt bb4b491b10e6fef8183a66f55f5d5832dd7bc41a > 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
