<div dir="ltr"><span style="font-size:12.8px">Hi,</span><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">My review:</div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">. there are several code style inconsistencies, like "QDialog* parent" and "Ui::AppChooserDialog * m_dialog". In some places you use app_id while in other places you use appId.<br></div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">. you use 0 in same places that you should use nullptr.</div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">. there is no doxygen documentation at all.</div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">. you can optimize</div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">QString("%1:%2").arg(app_id).<wbr>arg(id)<br></div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">by doing this:</div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">QString("%1:%2").arg(app_id, id)<br></div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">since both app_id and id are QStrings.</div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">. in AppChooser::<wbr>chooseApplication() you do</div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">QString heading;<br></div><div style="font-size:12.8px">...</div><div style="font-size:12.8px">heading = options.value(QLatin1String("<wbr>heading")).toBool();<br></div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">shouldn't heading be declared as bool?</div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">. in the same method you do:</div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px"><div>if (appDialog->exec()) {</div><div>        results.insert(QLatin1String("<wbr>choice"), appDialog-><wbr>selectedApplication());</div><div>        appDialog->deleteLater();</div><div>        return 0;</div><div>}</div></div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">That means if the user rejects the dialog you never deletes it. That's a memory leak. You do the same thing in FileChooser::openFile(), FileChooser::saveFile() and Print::preparePrint().</div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">. in DesktopPortal::<wbr>handleMessage() you use message.arguments().at(4) without checking if there are at least four arguments in message (a QDBusMessage object). That is risky, if someone does not provide all the required arguments your code can crash.</div></div><div class="gmail_extra"><br clear="all"><div><div class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div><div dir="ltr">
<p style="margin:0px">Lamarque V. Souza</p>
<p style="margin:0px"><a href="http://planetkde.org/pt-br" target="_blank">http://planetkde.org/pt-br</a></p></div></div></div></div></div>
<br><div class="gmail_quote">On Fri, May 12, 2017 at 6:23 AM, Jan Grulich <span dir="ltr"><<a href="mailto:jgrulich@redhat.com" target="_blank">jgrulich@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi,<br>
<br>
it's been now three weeks and nobody either looked at the code or found any<br>
problem and raised objections. Can we proceed next and move this to place<br>
where is the rest of Plasma repositories? Or is there a longer period for<br>
which I have to wait until this can move on?<br>
<br>
Regards,<br>
Jan<br>
<div class="HOEnZb"><div class="h5"><br>
On pátek 21. dubna 2017 8:10:36 CEST Jan Grulich wrote:<br>
> Hi,<br>
><br>
> I would like to request review of xdg-desktop-portal-kde [1]. We would like<br>
> to make it part of Plasma releases, see [2].<br>
><br>
> What is xdg-desktop-portal-kde:<br>
> It's a KDE implementation of Flatpak portals backend [3], currently with<br>
> support of AppChooser, FileChooser, Notification and Print portals.<br>
><br>
> One mentioned issue on plasma-devel mailing list was usage of Qt's private<br>
> print API. This will most likely go away if I find out it's useless,<br>
> otherwise I'll have to keep it as it's used to set CUPS properties which<br>
> are not available to the outside world.<br>
><br>
> [1] - <a href="https://cgit.kde.org/xdg-desktop-portal-kde.git/" rel="noreferrer" target="_blank">https://cgit.kde.org/xdg-<wbr>desktop-portal-kde.git/</a><br>
> [2] - <a href="https://mail.kde.org/pipermail/plasma-devel/2017-April/069506.html" rel="noreferrer" target="_blank">https://mail.kde.org/<wbr>pipermail/plasma-devel/2017-<wbr>April/069506.html</a><br>
> [3] -<br>
> <a href="http://flatpak.org/xdg-desktop-portal/portal-docs.html#idm140258860052032" rel="noreferrer" target="_blank">http://flatpak.org/xdg-<wbr>desktop-portal/portal-docs.<wbr>html#idm140258860052032</a><br>
><br>
> Regards,<br>
> Jan<br>
<br>
<br>
</div></div></blockquote></div><br></div>