D28214: Add background portal

2020-04-07 Thread Jan Grulich 
jgrulich added inline comments.

INLINE COMMENTS

> davidedmundson wrote in background.cpp:165
> From the docs
> 
>   Commandline to use add when autostarting at login.
>If this is not specified, the Exec line from the
>desktop file will be used.
> 
> I can't see us doing that last part

The commandline is actually the Exec line, it's just named that way from the 
portal specification.

You can see below:

  desktopEntryConfigGroup.writeEntry(QStringLiteral("Exec"), 
KShell::joinArgs(commandline));

REPOSITORY
  R838 Flatpak Support: KDE Portal for XDG Desktop

REVISION DETAIL
  https://phabricator.kde.org/D28214

To: jgrulich, #plasma, davidedmundson, apol
Cc: bcooksley, apol, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D28214: Add background portal

2020-04-07 Thread Jan Grulich 
jgrulich added a comment.


  In D28214#643478 , @bcooksley 
wrote:
  
  > This change, as committed does not appear to compile:
  >  
https://build.kde.org/view/Failing/job/Plasma/job/xdg-desktop-portal-kde/job/kf5-qt5%20FreeBSDQt5.14/lastFailedBuild/console
  
  
  Yes, I know why and didn't realize it. See the mail about PipeWire dependency 
on plasma-devel, I added you to CC list so you don't miss it.

REPOSITORY
  R838 Flatpak Support: KDE Portal for XDG Desktop

REVISION DETAIL
  https://phabricator.kde.org/D28214

To: jgrulich, #plasma, davidedmundson, apol
Cc: bcooksley, apol, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D28214: Add background portal

2020-04-07 Thread David Edmundson 
davidedmundson added inline comments.

INLINE COMMENTS

> background.cpp:165
> +qCDebug(XdgDesktopPortalKdeBackground) << "enable: " << enable;
> +qCDebug(XdgDesktopPortalKdeBackground) << "commandline: " << 
> commandline;
> +qCDebug(XdgDesktopPortalKdeBackground) << "flags: " << flags;

From the docs

  Commandline to use add when autostarting at login.
   If this is not specified, the Exec line from the
   desktop file will be used.

I can't see us doing that last part

> jgrulich wrote in background.cpp:194
> Doesn't need to? I think it just specifies that the desktop file should 
> ignore the exec line and start it through DBus (assuming correct dbus service 
> file is installed etc.). Or am I wrong?

> Doesn't need to? I think it just specifies that the desktop file should 
> ignore the exec line and start it through DBus (assuming correct dbus service 
> file is installed etc.). Or am I wrong?

You are right. We don't support it though.

REPOSITORY
  R838 Flatpak Support: KDE Portal for XDG Desktop

REVISION DETAIL
  https://phabricator.kde.org/D28214

To: jgrulich, #plasma, davidedmundson, apol
Cc: bcooksley, apol, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D28214: Add background portal

2020-04-07 Thread Ben Cooksley 
bcooksley added a comment.


  This change, as committed does not appear to compile:
  
https://build.kde.org/view/Failing/job/Plasma/job/xdg-desktop-portal-kde/job/kf5-qt5%20FreeBSDQt5.14/lastFailedBuild/console

REPOSITORY
  R838 Flatpak Support: KDE Portal for XDG Desktop

REVISION DETAIL
  https://phabricator.kde.org/D28214

To: jgrulich, #plasma, davidedmundson, apol
Cc: bcooksley, apol, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D28214: Add background portal

2020-04-06 Thread Jan Grulich 
This revision was automatically updated to reflect the committed changes.
Closed by commit R838:0cefb5e18db4: Add background portal (authored by 
jgrulich).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D28214?vs=79515=79551#toc

REPOSITORY
  R838 Flatpak Support: KDE Portal for XDG Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28214?vs=79515=79551

REVISION DETAIL
  https://phabricator.kde.org/D28214

AFFECTED FILES
  data/kde.portal
  data/org.freedesktop.impl.portal.desktop.kde.desktop.in
  src/CMakeLists.txt
  src/background.cpp
  src/background.h
  src/desktopportal.cpp
  src/desktopportal.h
  src/waylandintegration.cpp
  src/waylandintegration.h
  src/waylandintegration_p.h

To: jgrulich, #plasma, davidedmundson, apol
Cc: apol, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D28214: Add background portal

2020-04-06 Thread Aleix Pol Gonzalez 
apol accepted this revision.
apol added inline comments.
This revision is now accepted and ready to land.

INLINE COMMENTS

> jgrulich wrote in background.cpp:194
> Doesn't need to? I think it just specifies that the desktop file should 
> ignore the exec line and start it through DBus (assuming correct dbus service 
> file is installed etc.). Or am I wrong?

Ah, alright.

REPOSITORY
  R838 Flatpak Support: KDE Portal for XDG Desktop

BRANCH
  background-portal

REVISION DETAIL
  https://phabricator.kde.org/D28214

To: jgrulich, #plasma, davidedmundson, apol
Cc: apol, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D28214: Add background portal

2020-04-06 Thread Jan Grulich 
jgrulich updated this revision to Diff 79515.
jgrulich marked 5 inline comments as done.
jgrulich added a comment.


  Update to fix review comments

REPOSITORY
  R838 Flatpak Support: KDE Portal for XDG Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28214?vs=78272=79515

BRANCH
  background-portal

REVISION DETAIL
  https://phabricator.kde.org/D28214

AFFECTED FILES
  data/kde.portal
  data/org.freedesktop.impl.portal.desktop.kde.desktop.in
  src/CMakeLists.txt
  src/background.cpp
  src/background.h
  src/desktopportal.cpp
  src/desktopportal.h
  src/waylandintegration.cpp
  src/waylandintegration.h
  src/waylandintegration_p.h

To: jgrulich, #plasma, davidedmundson
Cc: apol, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D28214: Add background portal

2020-04-06 Thread Jan Grulich 
jgrulich added inline comments.

INLINE COMMENTS

> apol wrote in background.cpp:69
> I don't really understand why we're exposing this. Is it for contained apps?

This is actually not API exposed to the clients, this is purely for 
xdg-desktop-portal needs, while the client interface (xdg-desktop-portal) has 
limited stuff available, see 
https://github.com/flatpak/xdg-desktop-portal/blob/master/data/org.freedesktop.portal.Background.xml.

> apol wrote in background.cpp:194
> Will dbus look in autostart though?

Doesn't need to? I think it just specifies that the desktop file should ignore 
the exec line and start it through DBus (assuming correct dbus service file is 
installed etc.). Or am I wrong?

REPOSITORY
  R838 Flatpak Support: KDE Portal for XDG Desktop

REVISION DETAIL
  https://phabricator.kde.org/D28214

To: jgrulich, #plasma, davidedmundson
Cc: apol, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D28214: Add background portal

2020-04-06 Thread Aleix Pol Gonzalez 
apol added inline comments.

INLINE COMMENTS

> background.cpp:69
> +qCDebug(XdgDesktopPortalKdeBackground) << "GetAppState called: no 
> parameters";
> +return m_appStates;
> +}

I don't really understand why we're exposing this. Is it for contained apps?

> background.cpp:133
> +
> +QVariantMap map;
> +map.insert(QStringLiteral("result"), static_cast(result));

const QVariantMap map = { {QStringLiteral("result"), static_cast(result)} 
};

> background.cpp:194
> +if (autostartFlags.testFlag(AutostartFlag::Activatable)) {
> +   desktopEntryConfigGroup.writeEntry(QStringLiteral("DBusActivatable"), 
> true);
> +}

Will dbus look in autostart though?

> background.cpp:207
> +
> +connect(window, ::Client::PlasmaWindow::activeChanged, this, 
> [=] () {
> +setActiveWindow(window->appId(), window->isActive());

context should be [this]

> background.cpp:210
> +});
> +connect(window, ::Client::PlasmaWindow::unmapped, this, [=] () {
> +uint windows = 0;

[this]

> background.h:79
> +uint m_notificationCounter = 0;
> +QList m_windowList;
> +QVariantMap m_appStates;

I'd call it windows, it's better not include the type. It will change to 
QVector with Qt6 anyway.

REPOSITORY
  R838 Flatpak Support: KDE Portal for XDG Desktop

REVISION DETAIL
  https://phabricator.kde.org/D28214

To: jgrulich, #plasma, davidedmundson
Cc: apol, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D28214: Add background portal

2020-03-23 Thread Jan Grulich 
jgrulich updated this revision to Diff 78272.
jgrulich added a comment.


  Avoid potential crash when captured variable goes out of scope

REPOSITORY
  R838 Flatpak Support: KDE Portal for XDG Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28214?vs=78271=78272

BRANCH
  background-portal

REVISION DETAIL
  https://phabricator.kde.org/D28214

AFFECTED FILES
  data/kde.portal
  data/org.freedesktop.impl.portal.desktop.kde.desktop.in
  src/CMakeLists.txt
  src/background.cpp
  src/background.h
  src/desktopportal.cpp
  src/desktopportal.h
  src/waylandintegration.cpp
  src/waylandintegration.h
  src/waylandintegration_p.h

To: jgrulich, #plasma, davidedmundson
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28214: Add background portal

2020-03-23 Thread Jan Grulich 
jgrulich updated this revision to Diff 78271.
jgrulich added a comment.


  Replace my custom function for quoting of arguments with KShell

REPOSITORY
  R838 Flatpak Support: KDE Portal for XDG Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28214?vs=78270=78271

BRANCH
  background-portal

REVISION DETAIL
  https://phabricator.kde.org/D28214

AFFECTED FILES
  data/kde.portal
  data/org.freedesktop.impl.portal.desktop.kde.desktop.in
  src/CMakeLists.txt
  src/background.cpp
  src/background.h
  src/desktopportal.cpp
  src/desktopportal.h
  src/waylandintegration.cpp
  src/waylandintegration.h
  src/waylandintegration_p.h

To: jgrulich, #plasma, davidedmundson
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28214: Add background portal

2020-03-23 Thread Jan Grulich 
jgrulich created this revision.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
jgrulich requested review of this revision.

REVISION SUMMARY
  Implements Background portal, providing APIS related to applications 
  running in the background.
  
  Implemented according to: 
https://github.com/flatpak/xdg-desktop-portal/blob/master/data/org.freedesktop.impl.portal.Background.xml

REPOSITORY
  R838 Flatpak Support: KDE Portal for XDG Desktop

BRANCH
  background-portal

REVISION DETAIL
  https://phabricator.kde.org/D28214

AFFECTED FILES
  data/kde.portal
  data/org.freedesktop.impl.portal.desktop.kde.desktop.in
  src/CMakeLists.txt
  src/background.cpp
  src/background.h
  src/desktopportal.cpp
  src/desktopportal.h
  src/waylandintegration.cpp
  src/waylandintegration.h
  src/waylandintegration_p.h

To: jgrulich
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart