kdereview - xdg-desktop-portal-kde

[Lamarque Souza]

Hi,

My review:

. 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.

. you use 0 in same places that you should use nullptr.

. there is no doxygen documentation at all.

. you can optimize

QString("%1:%2").arg(app_id).arg(id)

by doing this:

QString("%1:%2").arg(app_id, id)

since both app_id and id are QStrings.

. in AppChooser::chooseApplication() you do

QString heading;
...
heading = options.value(QLatin1String("heading")).toBool();

shouldn't heading be declared as bool?

. in the same method you do:

if (appDialog->exec()) {
        results.insert(QLatin1String("choice"), appDialog->
selectedApplication());
        appDialog->deleteLater();
        return 0;
}

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().

. in DesktopPortal::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.

Lamarque V. Souza

http://planetkde.org/pt-br

On Fri, May 12, 2017 at 6:23 AM, Jan Grulich <jgrulich at redhat.com> wrote:

> Hi,
>
> it's been now three weeks and nobody either looked at the code or found any
> problem and raised objections. Can we proceed next and move this to place
> where is the rest of Plasma repositories? Or is there a longer period for
> which I have to wait until this can move on?
>
> Regards,
> Jan
>
> On pátek 21. dubna 2017 8:10:36 CEST Jan Grulich wrote:
> > Hi,
> >
> > I would like to request review of xdg-desktop-portal-kde [1]. We would
> like
> > to make it part of Plasma releases, see [2].
> >
> > What is xdg-desktop-portal-kde:
> > It's a KDE implementation of Flatpak portals backend [3], currently with
> > support of AppChooser, FileChooser, Notification and Print portals.
> >
> > One mentioned issue on plasma-devel mailing list was usage of Qt's
> private
> > print API. This will most likely go away if I find out it's useless,
> > otherwise I'll have to keep it as it's used to set CUPS properties which
> > are not available to the outside world.
> >
> > [1] - https://cgit.kde.org/xdg-desktop-portal-kde.git/
> > [2] - https://mail.kde.org/pipermail/plasma-devel/2017-April/069506.html
> > [3] -
> > http://flatpak.org/xdg-desktop-portal/portal-docs.
> html#idm140258860052032
> >
> > Regards,
> > Jan
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20170512/651f2162/attachment.htm>