Review Request 109773: New runner that translates words and sentences via Google Translate.
Aaron J. Seigo
aseigo at kde.org
Mon Apr 8 15:39:43 UTC 2013
-----------------------------------------------------------
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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20130408/7a2c5b3f/attachment-0001.html>
More information about the Plasma-devel
mailing list