D21584: Add LE Advertising and GATT APIs

2019-06-07 Thread David Rosca 
drosca requested changes to this revision.
drosca added a comment.
This revision now requires changes to proceed.


  After this, it should be good to go.

INLINE COMMENTS

> drosca wrote in gattapplication.h:47
> Maybe it would be best to add constructor that uses default object path. Also 
> please add better documentation as to what object path prefix is.
> 
> `explicit GattApplication(QObject *parent = nullptr);`

Here I meant adding new constructor, because as you did it now, if you want to 
set parent from constructor you would need to copy the default value for the 
objectPathPrefix.

  GattApplication(QObject *parent = nullptr);
  GattApplication(const QString , QObject *parent = nullptr);

> mweichselbaumer wrote in objectmanager.h:47
> QDbusAdaptors can only realize one single DBUS interface.
> This is the base class the org.freedesktop.DBus.ObjectManager adaptor is 
> handling.
> Sure, this could (and should) be reused if other DBUS/BlueZ objects shall 
> realize this interface.
> GattApplication is meant to realize the org.freedesktop.DBus.ObjectManager 
> interface (see gatt-api.txt).

I don't really like this class at all. This is implementation detail, and is of 
no use for the users of the library, so it shouldn't be exported. On top of it, 
there is actually no code, it is just interface class.

Another issue here, although minor, is that the bluezqt_dbustypes.h is not 
installed, so you can't use it. Yes, you could install it, but we don't need it.

As I can see, the only reason for this class is so you can pass it to 
ObjectManagerAdaptor, but there are other ways to achieve the same thing 
without adding `ObjectManager` and deriving from it in GattApplication. If we 
really need it in generic way, we can do it later.

Something like this would work:

  ObjectManagerAdaptor(QObject *parent)
  {
  GattApplication *app = qobject_cast(parent);
  }
  
  DBusManagerStruct GetManagedObjects()
  {
  return app->d->getManagedObjects();
  }

Or for now just make it work only with GattApplication, as this is the only one 
we have now.

Yes, it will not work in the autotest, since you won't have the hook anymore, 
but you can just remove that test.

REPOSITORY
  R269 BluezQt

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

To: mweichselbaumer, drosca
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D14467: Auth Support: Drop privileges if target is not owned by root

2019-06-07 Thread Chinmoy Ranjan Pradhan 
chinmoyr updated this revision to Diff 59319.
chinmoyr added a comment.


  use seteuid to make the logic for rename work

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14467?vs=59290=59319

BRANCH
  arcpatch-D14467

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

AFFECTED FILES
  src/ioslaves/file/file_p.h
  src/ioslaves/file/kauth/filehelper.cpp

To: chinmoyr, dfaure, ngraham, elvisangelaccio, #frameworks, #dolphin
Cc: mgerstner, fvogt, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21584: Add LE Advertising and GATT APIs

2019-06-07 Thread Manuel Weichselbaumer 
mweichselbaumer marked an inline comment as done.
mweichselbaumer added inline comments.

INLINE COMMENTS

> drosca wrote in objectmanager.h:47
> I don't really like this class at all. This is implementation detail, and is 
> of no use for the users of the library, so it shouldn't be exported. On top 
> of it, there is actually no code, it is just interface class.
> 
> Another issue here, although minor, is that the bluezqt_dbustypes.h is not 
> installed, so you can't use it. Yes, you could install it, but we don't need 
> it.
> 
> As I can see, the only reason for this class is so you can pass it to 
> ObjectManagerAdaptor, but there are other ways to achieve the same thing 
> without adding `ObjectManager` and deriving from it in GattApplication. If we 
> really need it in generic way, we can do it later.
> 
> Something like this would work:
> 
>   ObjectManagerAdaptor(QObject *parent)
>   {
>   GattApplication *app = qobject_cast(parent);
>   }
>   
>   DBusManagerStruct GetManagedObjects()
>   {
>   return app->d->getManagedObjects();
>   }
> 
> Or for now just make it work only with GattApplication, as this is the only 
> one we have now.
> 
> Yes, it will not work in the autotest, since you won't have the hook anymore, 
> but you can just remove that test.

I see your point and agree, that ObjectManager is solely used in 
GattApplication.
Just letting you know, that we would need an ObjectManager for the mesh-api as 
well.

Shall we then add constructors for appropriate types to ObjectManagerAdaptor 
(instead of exporting and inheriting from ObjectManager)?

REPOSITORY
  R269 BluezQt

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

To: mweichselbaumer, drosca
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21519: Rename ImageMake and ImageModel properties

2019-06-07 Thread Stefan Brüns 
bruns added a comment.


  In D21519#475821 , @ngraham wrote:
  
  > "Camera Manufacturer"?
  
  
  Well, no ... also "Scanner, Phone, video digitizer, ..."
  
  Digikam just uses "Manufacturer" and "Model", as does Canons DPP

REPOSITORY
  R286 KFileMetaData

BRANCH
  euqipment_properties

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

To: astippich, ngraham, bruns
Cc: kde-frameworks-devel, #baloo, LeGast00n, domson, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D21519: Rename ImageMake and ImageModel properties

2019-06-07 Thread Stefan Brüns 
bruns added a comment.


  In D21519#475781 , @ngraham wrote:
  
  > In D21519#474885 , @astippich 
wrote:
  >
  > > @ngraham what's your take regarding "Manufacturer" vs. "Equipment 
Manufacturer"?
  >
  >
  > "Manufacturer" is shorter, but since the metadata is attached to a photo, 
the longer version might make more sense to cement that it's a property of the 
camera itself, and not the photo!
  
  
  But is it really ambiguous? If it states "Canon", "EOS 70D" or "Nikon", 
"D5000 ", there isn't much room for 
interpretation. Also, "Equipment" is not really fitting very well - what about 
the lense, what about the tripod, the flash(es)? And what/who would a "Photo 
manufacturer" be? We are talking about digital images here.
  
  The more I think about it, the more I dislike the enum renaming - reusing a 
strange name from some widely used standard is one thing, but inventing a new 
one I don't consider a good idea.

REPOSITORY
  R286 KFileMetaData

BRANCH
  euqipment_properties

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

To: astippich, ngraham, bruns
Cc: kde-frameworks-devel, #baloo, LeGast00n, domson, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D21519: Rename ImageMake and ImageModel properties

2019-06-07 Thread Nathaniel Graham 
ngraham added a comment.


  "Camera Manufacturer"?

REPOSITORY
  R286 KFileMetaData

BRANCH
  euqipment_properties

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

To: astippich, ngraham, bruns
Cc: kde-frameworks-devel, #baloo, LeGast00n, domson, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D21519: Rename ImageMake and ImageModel properties

2019-06-07 Thread Nathaniel Graham 
ngraham added a comment.


  In D21519#474885 , @astippich 
wrote:
  
  > @ngraham what's your take regarding "Manufacturer" vs. "Equipment 
Manufacturer"?
  
  
  "Manufacturer" is shorter, but since the metadata is attached to a photo, the 
longer version might make more sense to cement that it's a property of the 
camera itself, and not the photo!

REPOSITORY
  R286 KFileMetaData

BRANCH
  euqipment_properties

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

To: astippich, ngraham, bruns
Cc: kde-frameworks-devel, #baloo, LeGast00n, domson, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D21584: Add LE Advertising and GATT APIs

2019-06-07 Thread David Rosca 
drosca accepted this revision.
drosca added inline comments.
This revision is now accepted and ready to land.

INLINE COMMENTS

> gattapplication.h:85
> + */
> +virtual QDBusObjectPath objectPath() const;
> +

This should be protected

> gattapplication.h:96
> + */
> +DBusManagerStruct getManagedObjects() const;
> +

This doesn't need doc anymore.

> gattcharacteristicadaptor.cpp:62
> +m_gattCharacteristic->writeValue(value);
> +}
> +void GattCharacteristicAdaptor::StartNotify()

newline

> gattservice.h:50
> + */
> +GattService(const QString , bool isPrimary, GattApplication 
> *parent);
> +

explicit

REPOSITORY
  R269 BluezQt

BRANCH
  ble_gatt

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

To: mweichselbaumer, drosca
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21584: Add LE Advertising and GATT APIs

2019-06-07 Thread Nathaniel Graham 
ngraham added a comment.


  What is the net effect of this patch? What does it improve?

REPOSITORY
  R269 BluezQt

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

To: mweichselbaumer, drosca
Cc: ngraham, kde-frameworks-devel, LeGast00n, michaelh, bruns

D21584: Add LE Advertising and GATT APIs

2019-06-07 Thread Manuel Weichselbaumer 
mweichselbaumer updated this revision to Diff 59346.
mweichselbaumer added a comment.


  Fixed according to comments

REPOSITORY
  R269 BluezQt

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21584?vs=59277=59346

BRANCH
  ble_gatt

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/fakebluez/CMakeLists.txt
  autotests/fakebluez/adapterinterface.cpp
  autotests/fakebluez/adapterinterface.h
  autotests/fakebluez/devicemanager.cpp
  autotests/fakebluez/devicemanager.h
  autotests/fakebluez/gattmanagerinterface.cpp
  autotests/fakebluez/gattmanagerinterface.h
  autotests/fakebluez/leadvertisingmanagerinterface.cpp
  autotests/fakebluez/leadvertisingmanagerinterface.h
  autotests/gattmanagertest.cpp
  autotests/gattmanagertest.h
  autotests/leadvertisingmanagertest.cpp
  autotests/leadvertisingmanagertest.h
  src/CMakeLists.txt
  src/adapter.cpp
  src/adapter.h
  src/adapter_p.cpp
  src/adapter_p.h
  src/gattapplication.cpp
  src/gattapplication.h
  src/gattapplication_p.cpp
  src/gattapplication_p.h
  src/gattcharacteristic.cpp
  src/gattcharacteristic.h
  src/gattcharacteristic_p.cpp
  src/gattcharacteristic_p.h
  src/gattcharacteristicadaptor.cpp
  src/gattcharacteristicadaptor.h
  src/gattmanager.cpp
  src/gattmanager.h
  src/gattmanager_p.cpp
  src/gattmanager_p.h
  src/gattservice.cpp
  src/gattservice.h
  src/gattservice_p.cpp
  src/gattservice_p.h
  src/gattserviceadaptor.cpp
  src/gattserviceadaptor.h
  src/interfaces/org.bluez.GattCharacteristic1.xml
  src/interfaces/org.bluez.GattManager1.xml
  src/interfaces/org.bluez.LEAdvertisement1.xml
  src/interfaces/org.bluez.LEAdvertisingManager1.xml
  src/leadvertisement.cpp
  src/leadvertisement.h
  src/leadvertisement_p.cpp
  src/leadvertisement_p.h
  src/leadvertisementadaptor.cpp
  src/leadvertisementadaptor.h
  src/leadvertisingmanager.cpp
  src/leadvertisingmanager.h
  src/leadvertisingmanager_p.h
  src/mediaendpoint.cpp
  src/objectmanageradaptor.cpp
  src/objectmanageradaptor.h
  src/pendingcall.cpp
  src/pendingcall.h
  src/types.h
  src/utils.cpp
  src/utils.h
  tests/CMakeLists.txt
  tests/leserver.cpp
  tests/leserver.h

To: mweichselbaumer, drosca
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21584: Add LE Advertising and GATT APIs

2019-06-07 Thread Manuel Weichselbaumer 
mweichselbaumer updated this revision to Diff 59349.
mweichselbaumer added a comment.


  Fixed according to comments

REPOSITORY
  R269 BluezQt

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21584?vs=59346=59349

BRANCH
  ble_gatt

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/fakebluez/CMakeLists.txt
  autotests/fakebluez/adapterinterface.cpp
  autotests/fakebluez/adapterinterface.h
  autotests/fakebluez/devicemanager.cpp
  autotests/fakebluez/devicemanager.h
  autotests/fakebluez/gattmanagerinterface.cpp
  autotests/fakebluez/gattmanagerinterface.h
  autotests/fakebluez/leadvertisingmanagerinterface.cpp
  autotests/fakebluez/leadvertisingmanagerinterface.h
  autotests/gattmanagertest.cpp
  autotests/gattmanagertest.h
  autotests/leadvertisingmanagertest.cpp
  autotests/leadvertisingmanagertest.h
  src/CMakeLists.txt
  src/adapter.cpp
  src/adapter.h
  src/adapter_p.cpp
  src/adapter_p.h
  src/gattapplication.cpp
  src/gattapplication.h
  src/gattapplication_p.cpp
  src/gattapplication_p.h
  src/gattcharacteristic.cpp
  src/gattcharacteristic.h
  src/gattcharacteristic_p.cpp
  src/gattcharacteristic_p.h
  src/gattcharacteristicadaptor.cpp
  src/gattcharacteristicadaptor.h
  src/gattmanager.cpp
  src/gattmanager.h
  src/gattmanager_p.cpp
  src/gattmanager_p.h
  src/gattservice.cpp
  src/gattservice.h
  src/gattservice_p.cpp
  src/gattservice_p.h
  src/gattserviceadaptor.cpp
  src/gattserviceadaptor.h
  src/interfaces/org.bluez.GattCharacteristic1.xml
  src/interfaces/org.bluez.GattManager1.xml
  src/interfaces/org.bluez.LEAdvertisement1.xml
  src/interfaces/org.bluez.LEAdvertisingManager1.xml
  src/leadvertisement.cpp
  src/leadvertisement.h
  src/leadvertisement_p.cpp
  src/leadvertisement_p.h
  src/leadvertisementadaptor.cpp
  src/leadvertisementadaptor.h
  src/leadvertisingmanager.cpp
  src/leadvertisingmanager.h
  src/leadvertisingmanager_p.h
  src/mediaendpoint.cpp
  src/objectmanageradaptor.cpp
  src/objectmanageradaptor.h
  src/pendingcall.cpp
  src/pendingcall.h
  src/types.h
  src/utils.cpp
  src/utils.h
  tests/CMakeLists.txt
  tests/leserver.cpp
  tests/leserver.h

To: mweichselbaumer, drosca
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21584: Add LE Advertising and GATT APIs

2019-06-07 Thread David Rosca 
drosca added a comment.


  In D21584#475914 , @ngraham wrote:
  
  > What is the net effect of this patch? What does it improve?
  
  
  It adds initial support for Bluetooth Low Energy, it's not used anywhere in 
Plasma.

REPOSITORY
  R269 BluezQt

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

To: mweichselbaumer, drosca
Cc: ngraham, kde-frameworks-devel, LeGast00n, michaelh, bruns

D20181: Add Binary Data units (bits, kilobytes, kibibytes ... yottabytes)

2019-06-07 Thread Albert Astals Cid 
aacid added a comment.


  @ngraham what is being requested is using existing code that does this 
instead of reinveinting the wheel.

REPOSITORY
  R292 KUnitConversion

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

To: JJRcop, broulik, #plasma, ngraham
Cc: abetts, cfeck, apol, aacid, ngraham, kde-frameworks-devel, LeGast00n, 
michaelh, bruns

D21584: Add LE Advertising and GATT APIs

2019-06-07 Thread Manuel Weichselbaumer 
mweichselbaumer closed this revision.

REPOSITORY
  R269 BluezQt

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

To: mweichselbaumer, drosca
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21656: Create a breeze icon for kfourinline

2019-06-07 Thread Carl Schwan 
ognarb edited the test plan for this revision.
ognarb added a reviewer: VDG.

REPOSITORY
  R266 Breeze Icons

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

To: ognarb, #vdg
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21659: add .vscode to .gitignore

2019-06-07 Thread Piyush Aggarwal 
brute4s99 created this revision.
brute4s99 added a reviewer: broulik.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
brute4s99 requested review of this revision.

REPOSITORY
  R289 KNotifications

BRANCH
  vscode (branched from master)

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

AFFECTED FILES
  .gitignore

To: brute4s99, broulik
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21659: add .vscode to .gitignore

2019-06-07 Thread Nicolas Fella 
nicolasfella accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R289 KNotifications

BRANCH
  vscode (branched from master)

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

To: brute4s99, broulik, nicolasfella
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21660: simplify conditions and remove audio dependency for win32

2019-06-07 Thread Nicolas Fella 
nicolasfella added inline comments.

INLINE COMMENTS

> CMakeLists.txt:42
> +else ()
> +find_package(Qt5 ${REQUIRED_QT_VERSION} CONFIG REQUIRED DBus)
>  endif()

We don't need DBus on Windows, do we?

REPOSITORY
  R289 KNotifications

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

To: brute4s99, broulik
Cc: nicolasfella, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21659: add .vscode to .gitignore

2019-06-07 Thread Piyush Aggarwal 
This revision was automatically updated to reflect the committed changes.
Closed by commit R289:e4461e0ba978: add .vscode to .gitignore (authored by 
brute4s99).

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21659?vs=59375=59378

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

AFFECTED FILES
  .gitignore

To: brute4s99, broulik, nicolasfella
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D20181: Add Binary Data units (bits, kilobytes, kibibytes ... yottabytes)

2019-06-07 Thread Jonathan Rubenstein 
JJRcop added a comment.


  It does work without any other code, because of the converter addon from R114 
Plasma Addons . The 
screenshots seen in the report were generated after building KUnitConversion 
with this patch on KDE neon dev with no other modifications. I mentioned 
earlier how Frequency and Length, which are both also covered in KFormat, have 
their own duplicate implementations in KUnitConversion like the current patch 
is, and don't currently use KFormat.
  
  I've included all the uncommon unit types to make this complete, but have not 
added them as CommonUnits.

REPOSITORY
  R292 KUnitConversion

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

To: JJRcop, broulik, #plasma, ngraham
Cc: abetts, cfeck, apol, aacid, ngraham, kde-frameworks-devel, LeGast00n, 
michaelh, bruns

D21661: add snoretoast backend for KNotifications on Windows

2019-06-07 Thread Piyush Aggarwal 
brute4s99 marked 5 inline comments as done.
brute4s99 added inline comments.

INLINE COMMENTS

> sredman wrote in CMakeLists.txt:43
> Do you know whether this requires MSVC, or can SnoreToast.exe be built with 
> MSVC and KNotifications be built with an unspecified compiler?

AFAIU it *should work fine with any compiler*, once SnoreToast is installed 
within the system one way or the other. (:

> sredman wrote in notifybysnore.cpp:44
> Did this end up being documented?

Yep! In 
https://piyushagg.home.blog/2019/06/07/gsoc19-project-milestone-1/

REPOSITORY
  R289 KNotifications

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

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21656: Create a breeze icon for kfourinline

2019-06-07 Thread Carl Schwan 
ognarb updated this revision to Diff 59372.
ognarb added a comment.


  Crop shadow

REPOSITORY
  R266 Breeze Icons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21656?vs=59371=59372

BRANCH
  master

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

AFFECTED FILES
  icons-dark/apps/48/kfourinline.svg
  icons/apps/48/kfourinline.svg

To: ognarb, #vdg
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21656: Create a breeze icon for kfourinline

2019-06-07 Thread Carl Schwan 
ognarb edited the test plan for this revision.

REPOSITORY
  R266 Breeze Icons

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

To: ognarb, #vdg
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D20181: Add Binary Data units (bits, kilobytes, kibibytes ... yottabytes)

2019-06-07 Thread Jonathan Rubenstein 
JJRcop added a comment.


  @aacid Are you asking me to use the KFormat functions for the KUnitConversion 
category I've created, or do you not think this should be supported by 
KUnitConversion at all?

REPOSITORY
  R292 KUnitConversion

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

To: JJRcop, broulik, #plasma, ngraham
Cc: abetts, cfeck, apol, aacid, ngraham, kde-frameworks-devel, LeGast00n, 
michaelh, bruns

D20181: Add Binary Data units (bits, kilobytes, kibibytes ... yottabytes)

2019-06-07 Thread Albert Astals Cid 
aacid added a comment.


  A little of both?
  
  First i ideally wouldn't want translators to have to translate this again 
when they're already doing it for KFormat, so using KFormat here if possible 
would be nice.
  
  Second, what's the expected use case of this? I don't see anyone needed a 
function that converts from kibibit to kilobye. Please convince me it's useful 
:)
  
  Does this enable the runner translation without any other code?

REPOSITORY
  R292 KUnitConversion

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

To: JJRcop, broulik, #plasma, ngraham
Cc: abetts, cfeck, apol, aacid, ngraham, kde-frameworks-devel, LeGast00n, 
michaelh, bruns

D21661: add snoretoast backend for KNotifications on Windows

2019-06-07 Thread Piyush Aggarwal 
brute4s99 updated this revision to Diff 59379.
brute4s99 marked 2 inline comments as done.
brute4s99 added a comment.


  updated wrt review.

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21661?vs=59377=59379

BRANCH
  win32 (branched from master)

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

AFFECTED FILES
  src/CMakeLists.txt
  src/knotification.cpp
  src/knotificationmanager.cpp
  src/notifybysnore.cpp
  src/notifybysnore.h
  src/snoretoastactions.h

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21660: simplify conditions and remove audio dependency for win32

2019-06-07 Thread Piyush Aggarwal 
brute4s99 added a dependency: D21657: remove phonon from deps if building for 
win32.

REPOSITORY
  R289 KNotifications

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

To: brute4s99, broulik
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21661: add snoretoast backend for KNotifications on Windows

2019-06-07 Thread Piyush Aggarwal 
brute4s99 added a dependency: D21602: Add NSIS Script for KDE Connect.

REPOSITORY
  R289 KNotifications

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

To: brute4s99
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21661: add snoretoast backend for KNotifications on Windows

2019-06-07 Thread Piyush Aggarwal 
brute4s99 created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
brute4s99 requested review of this revision.

REPOSITORY
  R289 KNotifications

BRANCH
  win32 (branched from master)

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

AFFECTED FILES
  src/CMakeLists.txt
  src/knotification.cpp
  src/knotificationmanager.cpp
  src/notifybysnore.cpp
  src/notifybysnore.h
  src/snoretoastactions.h

To: brute4s99
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21661: add snoretoast backend for KNotifications on Windows

2019-06-07 Thread Piyush Aggarwal 
brute4s99 edited the summary of this revision.
brute4s99 added reviewers: broulik, sredman, vonreth, albertvaka.

REPOSITORY
  R289 KNotifications

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

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21656: Create a breeze icon for kfourinline

2019-06-07 Thread Carl Schwan 
ognarb created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
ognarb requested review of this revision.

REPOSITORY
  R266 Breeze Icons

BRANCH
  master

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

AFFECTED FILES
  icons-dark/apps/48/kfourinline.svg
  icons/apps/48/kfourinline.svg

To: ognarb
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21660: simplify conditions

2019-06-07 Thread Piyush Aggarwal 
brute4s99 created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
brute4s99 requested review of this revision.

REPOSITORY
  R289 KNotifications

BRANCH
  simplify (branched from master)

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

AFFECTED FILES
  CMakeLists.txt

To: brute4s99
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21660: simplify conditions and remove audio dependency for win32

2019-06-07 Thread Piyush Aggarwal 
brute4s99 retitled this revision from "simplify conditions" to "simplify 
conditions and remove audio dependency for win32".
brute4s99 edited the summary of this revision.
brute4s99 added a reviewer: broulik.

REPOSITORY
  R289 KNotifications

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

To: brute4s99, broulik
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21661: add snoretoast backend for KNotifications on Windows

2019-06-07 Thread Simon Redman 
sredman added inline comments.

INLINE COMMENTS

> CMakeLists.txt:43
> +if (WIN32)
> +  if (MSVC)
> +  find_package(LibSnoreToast REQUIRED)

Do you know whether this requires MSVC, or can SnoreToast.exe be built with 
MSVC and KNotifications be built with an unspecified compiler?

> notifybysnore.cpp:44
> +
> +// !DOCUMENT THIS! apps must have shortcut appID same as 
> app->applicationName()
> +NotifyBySnore::NotifyBySnore(QObject* parent) :

Did this end up being documented?

> notifybysnore.cpp:73
> +case SnoreToastActions::Actions::Clicked :{
> +qDebug() << " User clicked on the 
> toast.";
> +break;

Add a category to qDebug, like `qCDebug(LOG_KNOTIFICATIONS) << "Message";`. I 
can't figure out where `LOG_KNOTIFICATIONS` is coming from, but other backends 
seem to use it :)

> notifybysnore.cpp:117
> +if (m_notifications.find(notification->id()) != m_notifications.end() || 
> notification->id() == -1) {
> +qDebug() << "AHAA ! Duplicate for ID: " << notification->id() << 
> " caught!";
> +return;

Maybe this log line should be different? :)

> notifybysnore.cpp:155
> +  << QStringLiteral("-appID") << app->applicationName();
> +;
> +proc->start(program, arguments);

Does this `;` belong to something?

> notifybysnore.h:48
> +QString program = QStringLiteral("SnoreToast.exe");
> +// QProcess *proc;
> +QLocalServer *server;

Delete commented code

REPOSITORY
  R289 KNotifications

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

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns