kdereview - xdg-desktop-portal-kde

[Jan Grulich]

Hi,

thank you so much for the review!!

On pátek 12. května 2017 13:21:00 CEST Lamarque Souza wrote:
> 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.

Fixed.

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

Fixed.
 
> . there is no doxygen documentation at all.

Not sure there is reason for a documentation, for implementation of other 
portals there is documentation provided by Flatpak guys, otherwise this is 
something what should be running on the background and users shouldn't even 
know that something like this is running, they don't need to be familiar with 
this 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.
 
Fixed.

> . in AppChooser::chooseApplication() you do
> 
> QString heading;
> ...
> heading = options.value(QLatin1String("heading")).toBool();
> 
> shouldn't heading be declared as bool?

It should be a string so it's a wrong casting.

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

Fixed.

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

Given the dbus signature is given you shouldn't be able to call it with less 
parameters. If you do, you will get an error from dbus that the signature is 
wrong.

Jan

> 
> Lamarque V. Souza
> Linux User #57137 - https://www.linuxcounter.net/user/57137
> 
> 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