D22727: allow kio-extras to build with mingw on win32 and remove unnecessary includes

[Simon Redman]

sredman added a comment.


  In D22727#513842 , @dfaure wrote:
  
  > The truth is stronger than "I would not recommend".
  >  put() in SlaveBase-derived classes is called by the KIO library 
(TransferJob), so you CANNOT change the meaning of the arguments. It's part of 
the API/ABI for all slaves, and this cannot change until KF6.
  
  
  +1
  
  But it is worth making a note of this as something to change (or discuss 
changing) for KF6 since it seems like using the Qt constants would be a better 
design

REPOSITORY
  R320 KIO Extras

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

To: brute4s99, vonreth, dfaure, pino
Cc: sredman, pino, kde-frameworks-devel, kfm-devel, aprcela, vmarinescu, 
fprice, LeGast00n, MrPepe, fbampaloukas, alexde, Codezela, feverfew, meven, 
michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

D23196: have the app icon as fallback icon in Snore

[Simon Redman]

sredman accepted this revision.
sredman added a comment.
This revision is now accepted and ready to land.


  Since KNotifications on Windows is so far only used by you anyway, I say it's 
fine to merge. @broulik, any objections?

INLINE COMMENTS

> notifybysnore.cpp:157
> +notification->pixmap().save(iconPath, "PNG");
> +arguments   << QStringLiteral("-p") << iconPath;
> +} else if (!qApp->windowIcon().isNull()){

Whitespace

> notifybysnore.cpp:160
> +QIcon app_icon = qApp->windowIcon();
> +// We limit the icon size to 1024x1024 as it is the highest supported by 
> Windows
> +QPixmap pixmap = app_icon.pixmap(app_icon.actualSize(QSize(1024, 
> 1024)));

Indentation

REPOSITORY
  R289 KNotifications

BRANCH
  arcpatch-D23196

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

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

D22727: allow kio-extras to build with mingw on win32 and remove unnecessary includes

[Simon Redman]

sredman added a comment.


  In D22727#510698 , @brute4s99 
wrote:
  
  > In D22727#505540 , @dfaure wrote:
  >
  > > But, wait, this code is mixing "int permissions" (*) with the QFileDevice 
enum, that doesn't make any sense to me.
  > >
  > > (*) this comes from KIO::put, which takes unix permissions on unix, not 
sure what it takes on Windows...
  > >
  > > This doesn't match: unix permissions are octal (e.g. group read is 040 in 
octal), QFileDevice enum is hex (0x040).
  >
  >
  > Okay. I'm still unable to understand where/ how sftp::put() is called, or I 
would change everywhere it is called, to Qt way of permission extraction.
  
  
  I would recommend not making API or ABI changes. It's annoying that they're 
not compatible, but you can probably find some way to convert from Qt 
permissions to the expected values in order to not change users

REPOSITORY
  R320 KIO Extras

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

To: brute4s99, vonreth, dfaure, pino
Cc: sredman, pino, kde-frameworks-devel, kfm-devel, aprcela, vmarinescu, 
fprice, LeGast00n, MrPepe, fbampaloukas, alexde, Codezela, feverfew, meven, 
michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

D21661: add snoretoast backend for KNotifications on Windows

[Simon Redman]

sredman accepted this revision.
sredman added a comment.
This revision is now accepted and ready to land.


  I noticed a couple of places where the cmake file could be simplified so make 
those changes and then let's land this monster

INLINE COMMENTS

> CMakeLists.txt:76
> +if (NOT WIN32)
> +if (ANDROID)
> +find_package(Qt5 ${REQUIRED_QT_VERSION} CONFIG REQUIRED 
> AndroidExtras)

Move this `if(Android)` block up to where the other *Extras are, lines 63-68 
(then remove the connected `else` and then fix the indenting)

> CMakeLists.txt:98
> +endif()
> +else()
> +find_package(LibSnoreToast REQUIRED)

Logically this `else` is pretty separate from the `if` that it is connected to. 
Could you change it to `if(NOT WIN32)` to make it easier to read?

REPOSITORY
  R289 KNotifications

BRANCH
  arcpatch-D21661

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

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

D21661: add snoretoast backend for KNotifications on Windows

[Simon Redman]

sredman added a comment.


  In D21661#476571 , @pino wrote:
  
  > In D21661#476156 , @pino wrote:
  >
  > > how is snoretoast actually used here? you are requiring the library for 
building and linking, but then:
  > >
  > > - `snoretoastactions.h`, which is part of the headers of snoretoast, is 
copied here
  > > - the snoretoast library is never used, as the utilities of it are 
invoked instead If the library does all the work already, then I'd prefer to 
use it directly instead of spawning executables all the time...
  >
  >
  > Piyush, what about this? It seems the snoretoast library provides a 
`SnoreToasts` class to do this instead of spawning an helper tool, what about 
using it instead?
  
  
  @vonreth can probably explain better, but basically the situation as I 
understand it is on Windows you need to be installed in a special place and 
registered with the OS in order to show notifications. Since KNotifications is 
a library, an app using it can't (feasibly) be properly registered with the OS. 
It is possible we could come up with some complicated solution which would 
require every KNotification-using app to do some special and probably difficult 
to understand change to support Windows. Or we can have SnoreNotify.exe take 
care of all that nonsense for us. Note that, up to this point, there have been 
no special KNotifications changes to the generic KDE Connect codebase to make 
this work, just some tweaks to the Windows installer to pull in SnoreToast.
  
  Spawning the process of a small, helper .exe is what other big cross-platform 
apps, like say Chromium/Google Chrome, do.
  
  @pino Thank you very much for your reviews up to this point. It is very 
appreciated :). Supporting notifications on Windows is part of a GSoC project 
to release KDE Connect on Windows. We have a weekly VoIP meeting to talk about 
KDE Connect GSoC progress. If you would like to join those meetings, I can give 
you the information

REPOSITORY
  R289 KNotifications

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

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

D21661: add snoretoast backend for KNotifications on Windows

[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

D21369: [WIP] Add AbstractContact properties for KContact::PhoneNumber objects

[Simon Redman]

sredman added a comment.


  In D21369#469389 , @apol wrote:
  
  > Seems like a clear layer break to spill KContacts over KPeople :P.
  >  Let's discuss it at the sprint? Or earlier. Let's discuss it? ^^'
  
  
  Sounds good

REPOSITORY
  R307 KPeople

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

To: sredman, apol
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D21369: [WIP] Add AbstractContact properties for KContact::PhoneNumber objects

[Simon Redman]

sredman edited the summary of this revision.

REPOSITORY
  R307 KPeople

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

To: sredman, apol
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D21369: [WIP] Add AbstractContact properties for KContact::PhoneNumber objects

[Simon Redman]

sredman updated this revision to Diff 58565.
sredman added a comment.


  - Hack on QMetaType support

REPOSITORY
  R307 KPeople

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21369?vs=58561=58565

BRANCH
  kcontacts-phonenumber

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

AFFECTED FILES
  CMakeLists.txt
  src/CMakeLists.txt
  src/backends/CMakeLists.txt
  src/backends/abstractcontact.cpp
  src/backends/abstractcontact.h
  src/persondata.cpp
  src/persondata.h

To: sredman, apol
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D21369: [WIP] Add AbstractContact properties for KContact::PhoneNumber objects

[Simon Redman]

sredman added a dependent revision: D21374: WIP: Add support for 
KContacts::PhoneNumber-related custom data fields.

REPOSITORY
  R307 KPeople

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

To: sredman, apol
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D21369: [WIP] Add AbstractContact properties for KContact::PhoneNumber objects

[Simon Redman]

sredman updated this revision to Diff 58561.
sredman added a comment.


  - Add link_library target to backends as well

REPOSITORY
  R307 KPeople

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21369?vs=58556=58561

BRANCH
  kcontacts-phonenumber

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

AFFECTED FILES
  CMakeLists.txt
  src/CMakeLists.txt
  src/backends/CMakeLists.txt
  src/backends/abstractcontact.cpp
  src/backends/abstractcontact.h
  src/persondata.cpp
  src/persondata.h

To: sredman, apol
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D21369: [WIP] Add AbstractContact properties for KContact::PhoneNumber objects

[Simon Redman]

sredman added a reviewer: apol.

REPOSITORY
  R307 KPeople

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

To: sredman, apol
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D21369: [WIP] Add AbstractContact properties for KContact::PhoneNumber objects

[Simon Redman]

sredman updated this revision to Diff 58556.
sredman added a comment.


  - Move KF5::Contacts to PUBLIC library

REPOSITORY
  R307 KPeople

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21369?vs=58553=58556

BRANCH
  kcontacts-phonenumber

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

AFFECTED FILES
  CMakeLists.txt
  src/CMakeLists.txt
  src/backends/abstractcontact.cpp
  src/backends/abstractcontact.h
  src/persondata.cpp
  src/persondata.h

To: sredman
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D21369: [WIP] Add AbstractContact properties for KContact::PhoneNumber objects

[Simon Redman]

sredman created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
sredman requested review of this revision.

REVISION SUMMARY
  - Add phone number getters to PersonData
  - Add KContact::PhoneNumber properties to AbstractContact
  
Does not currently compile because the compiler can't find `#include 
`. Help please!

REPOSITORY
  R307 KPeople

BRANCH
  kcontacts-phonenumber

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

AFFECTED FILES
  CMakeLists.txt
  src/CMakeLists.txt
  src/backends/abstractcontact.cpp
  src/backends/abstractcontact.h
  src/persondata.cpp
  src/persondata.h

To: sredman
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D16692: A QApplication object needs to be instantiated for kio-kdeconnect to work on KDE Neon

[Simon Redman]

sredman added a comment.


  This is going to be our cleanest patch ever once we've had everybody and 
their grandmother look at it. I guess this is what happens when you fix a hot 
bug! 
  
  In D16692#354783 , @jriddell wrote:
  
  > I can recreate the problem with 1.3 branch build of kdeconnect-kde in a 
virtualmachine install of KDE neon user edition
  >  The problem remains when building and installing it with the D16692 
 patch
  >
  > Building 1.3 branch on KDE neon developer edition I can not recreate the 
problem.  So the problem may be in some other area such as kio framework.
  
  
  On my KDE Neon user edition VM, this patch solves the issue 

INLINE COMMENTS

> CMakeLists.txt:23
> +install(TARGETS kio_kdeconnect DESTINATION ${KDE_INSTALL_PLUGINDIR}/kf5/kio)
> +#install(FILES kdeconnect.protocol DESTINATION ${SERVICES_INSTALL_DIR})

Is there some reason to keep this line as a comment?

REPOSITORY
  R224 KDE Connect

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

To: eduisters, #kde_connect, #frameworks, albertvaka
Cc: sredman, apol, jriddell, nicolasfella, albertvaka, kdeconnect, 
shivanshukantprasad, skymoore, wistak, dvalencia, rmenezes, julioc, Leptopoda, 
timothyc, jdvr, yannux, Danial0_0, johnq, Pitel, adeen-s, SemperPeritus, 
daniel.z.tg, jeanv, seebauer, bugzy, MayeulC, menasshock, tctara