D21002: Remove kde4 migration agent completely

[Luca Beltrame]

lbeltrame added a comment.


  +1

REPOSITORY
  R311 KWallet

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

To: bruns, #frameworks, cfeck, ngraham
Cc: lbeltrame, kde-frameworks-devel, damjang, LeGast00n, michaelh, ngraham, 
bruns

D21004: [UserMetaData] Shortcut attribute queries for the common case

[Stefan Brüns]

bruns added a comment.


  Ping!

REPOSITORY
  R824 Baloo Widgets

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

To: bruns, #baloo, #frameworks, ngraham, astippich
Cc: domson, ashaposhnikov, astippich, spoorun, abrahams

D20967: [UserMetaData] Add method to query which attributes are set

[Stefan Brüns]

bruns added subscribers: Windows, FreeBSD.

REPOSITORY
  R286 KFileMetaData

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

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

D20967: [UserMetaData] Add method to query which attributes are set

[Stefan Brüns]

bruns added a comment.


  Ping!

REPOSITORY
  R286 KFileMetaData

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

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

D21002: Remove kde4 migration agent completely

[Stefan Brüns]

bruns added a comment.


  Ping!

REPOSITORY
  R311 KWallet

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

To: bruns, #frameworks, cfeck, ngraham
Cc: kde-frameworks-devel, damjang, LeGast00n, michaelh, ngraham, bruns

D21002: Remove kde4 migration agent completely

[Stefan Brüns]

bruns added a reviewer: ngraham.

REPOSITORY
  R311 KWallet

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

To: bruns, #frameworks, cfeck, ngraham
Cc: kde-frameworks-devel, damjang, LeGast00n, michaelh, ngraham, bruns

D21708: [ModifiedFileIndexer] Avoid shadowing XAttr changes by content changes

[Stefan Brüns]

bruns edited the summary of this revision.
bruns added a dependency: D21694: [DocumentUrlDB] Avoid manipulation of the 
whole tree on trivial rename.

REPOSITORY
  R293 Baloo

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

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

D21694: [DocumentUrlDB] Avoid manipulation of the whole tree on trivial rename

[Stefan Brüns]

bruns added a dependent revision: D21708: [ModifiedFileIndexer] Avoid shadowing 
XAttr changes by content changes.

REPOSITORY
  R293 Baloo

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

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

D21709: [PendingFileQueue] Avoid delete + create / create + delete race

[Stefan Brüns]

bruns created this revision.
bruns added reviewers: Baloo, ngraham, astippich, poboiko.
Herald added projects: Frameworks, Baloo.
Herald added a subscriber: kde-frameworks-devel.
bruns requested review of this revision.

REVISION SUMMARY
  When the 'delete' flag is set, the file was removed from the index and
  the item was removed from the pending file queue (`m_cache`). When a
  'created' event arrives before the queue has been processed, the flag
  was merged into the queue item, but never processed (delete + create),
  i.e. the file was omitted from the index.
  
  In case a created/modified file was moved to the `m_pendingFiles` queue
  (files to be reindexed later) and deleted, the pending files timer may
  fire before `process` is run, and the `indexNewFile`/`indexModifiedFile`
  signal is emitted for a no longer existing file. Although this is handled
  in the indexer, this causes unnecessary work.
  
  Emit the `removeFileIndex` immediately and remove the file from all
  queues.
  
  A deleted file/folder is kept in the m_recentlyEmitted list, to avoid
  excessive events when e.g. a temporary file is created/deleted in short
  period of time.

TEST PLAN
  `$> date > testfile; rm testfile; date > testfile`
  After the change, testfile is included in the index
  
  `$> date > testfile; date > testfile2; usleep 2000; rm testfile`
  After the change, `indexNewFile` signal is emitted only for testfile2,
  while for testfile `removeFileIndex` is emitted.

REPOSITORY
  R293 Baloo

BRANCH
  fix_races

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

AFFECTED FILES
  src/file/pendingfile.cpp
  src/file/pendingfilequeue.cpp

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

D21708: [ModifiedFileIndexer] Avoid shadowing XAttr changes by content changes

[Stefan Brüns]

bruns created this revision.
bruns added reviewers: Baloo, ngraham, astippich, poboiko.
Herald added projects: Frameworks, Baloo.
Herald added a subscriber: kde-frameworks-devel.
bruns requested review of this revision.

REVISION SUMMARY
  The ModifiedFileIndexer updates the timestamps, which has the effect
  the XAttr indexer will not pick up any XAttr changes later when both
  happen at approximately the same time.
  
  In case the file metadata has changed, also update everything associated
  to the ctime (XAttrs, filename). This also has the nice side effect
  of getting rid of a separate BasicIndexingJob run later.

TEST PLAN
  1. touch testfile; sleep 10
  2. setfattr -n baloo.file.rating -v 4 ; touch testfile
  
  After the change, `balooshow testfile` shows the rating.

REPOSITORY
  R293 Baloo

BRANCH
  fix_races

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

AFFECTED FILES
  src/file/modifiedfileindexer.cpp

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

D21694: [DocumentUrlDB] Avoid manipulation of the whole tree on trivial rename

[Stefan Brüns]

bruns added a dependent revision: D21707: [XAttrIndexer] Update DocumentTime 
when XAttrs are updated.

REPOSITORY
  R293 Baloo

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

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

D21707: [XAttrIndexer] Update DocumentTime when XAttrs are updated

[Stefan Brüns]

bruns created this revision.
bruns added reviewers: Baloo, ngraham, astippich, poboiko.
Herald added projects: Frameworks, Baloo.
Herald added a subscriber: kde-frameworks-devel.
bruns requested review of this revision.

REVISION SUMMARY
  After modifying a files attributes (like ownership, permissions, tags,
  ...) the timestamp in the DB would be different to the filesystem
  timestamp. The change would only be picked up on next login by the
  UnindexedFileIndexer.
  
  Also update the FileNameTerms and DocumentUrl, as renames also change
  the ctime. As the WriteTransaction checks for changes, setting the
  flags in excess is cheap, but avoids getting the filename out of sync.
  
  Depends on D21694 

REPOSITORY
  R293 Baloo

BRANCH
  fix_races

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

AFFECTED FILES
  src/engine/documenturldb.h
  src/file/xattrindexer.cpp

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

D21706: [ModifiedFileIndexer] Use correct mimetype for folders, delay until needed

[Stefan Brüns]

bruns created this revision.
bruns added reviewers: Baloo, ngraham, astippich, poboiko.
Herald added projects: Frameworks, Baloo.
Herald added a subscriber: kde-frameworks-devel.
bruns requested review of this revision.

REVISION SUMMARY
  mimeTypeForFile is quite expensive (much more than QFileInfo and fetching
  the timestamps from the DB), and yields the wrong type for folders.

REPOSITORY
  R293 Baloo

BRANCH
  cleanup

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

AFFECTED FILES
  src/file/modifiedfileindexer.cpp

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

D21705: [NewFileIndexer] Use correct mimetype for folders, check excludeFolders

[Stefan Brüns]

bruns created this revision.
bruns added reviewers: Baloo, ngraham, astippich, poboiko.
Herald added projects: Frameworks, Baloo.
Herald added a subscriber: kde-frameworks-devel.
bruns requested review of this revision.

REVISION SUMMARY
  mimeTypeForFile yields the wrong type for folders, hardcode it.
  
  In case a newly added folder is an excluded folder, avoid adding it to
  the index.

TEST PLAN
  $> balooctl config add ~/test/excluded
  $> mkdir ~/test
  $> mkdir ~/test/excluded; mkdir ~/test/included.log/
  $> balooshow -x ~/test/*
  
  'excluded' should not be in the index, 'included.log' should have
  mimetype inode/directory (terms 'Minode Mdirectory').

REPOSITORY
  R293 Baloo

BRANCH
  cleanup

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

AFFECTED FILES
  src/file/newfileindexer.cpp

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

D21704: [FirstRunIndexer] Use correct mimetype for folders

[Stefan Brüns]

bruns created this revision.
bruns added reviewers: Baloo, ngraham, astippich, poboiko.
Herald added projects: Frameworks, Baloo.
Herald added a subscriber: kde-frameworks-devel.
bruns requested review of this revision.

REVISION SUMMARY
  mimeTypeForFile yields an incorrect mimetype for folders, just hardcode
  "inode/directory".

REPOSITORY
  R293 Baloo

BRANCH
  cleanup

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

AFFECTED FILES
  src/file/firstrunindexer.cpp

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

D21703: [Transaction] Replace template for functor with std::function

[Stefan Brüns]

bruns created this revision.
bruns added reviewers: Baloo, ngraham, astippich, poboiko.
Herald added projects: Frameworks, Baloo.
Herald added a subscriber: kde-frameworks-devel.
bruns requested review of this revision.

REVISION SUMMARY
  The callback parameter has a specific type, it accepts a document ID and
  returns bool, while the template is overly broad.

REPOSITORY
  R293 Baloo

BRANCH
  cleanup

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

AFFECTED FILES
  src/engine/transaction.h
  src/engine/writetransaction.cpp
  src/engine/writetransaction.h

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

D21661: add snoretoast backend for KNotifications on Windows

[Nicolas Fella]

nicolasfella added inline comments.

INLINE COMMENTS

> knotificationmanager.cpp:140
>  if (action == QLatin1String("Popup")) {
> -#ifndef Q_OS_ANDROID
> -if (d->portalDBusServiceExists) {
> -plugin = new NotifyByPortal(this);
> -} else {
> -plugin = new NotifyByPopup(this);
> -}
> -#else
> -plugin = new NotifyByAndroid(this);
> -#endif
> -
> +#if defined(Q_OS_ANDROID)
> +plugin = new NotifyByAndroid(this);

No space before macro

> brute4s99 wrote in notifybysnore.cpp:44
> Yep! In 
> https://piyushagg.home.blog/2019/06/07/gsoc19-project-milestone-1/

This should be documented somewhere else, too. Either the API dox or the KDE 
wiki

REPOSITORY
  R289 KNotifications

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

[Hannah von Reth]

vonreth added inline comments.

INLINE COMMENTS

> albertvaka wrote in notifybysnore.h:45
> Isn't LibSnore abstracting this? Why do we need to call the exe instead of 
> using some method in LibSnore?

Libsnore is something different, yes its an abstract "notification framework" 
but ... its basically unmaintained and the windows 8+ notifications use exactly 
this executable

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

D21698: Move invariant IndexingLevel out of the loop

[Stefan Brüns]

bruns created this revision.
bruns added reviewers: Baloo, ngraham, astippich, poboiko.
Herald added projects: Frameworks, Baloo.
Herald added a subscriber: kde-frameworks-devel.
bruns requested review of this revision.

REVISION SUMMARY
  The IndexingLevel is constant, no need to fetch it on each loop
  iteration.

REPOSITORY
  R293 Baloo

BRANCH
  invariants

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

AFFECTED FILES
  src/file/firstrunindexer.cpp
  src/file/modifiedfileindexer.cpp
  src/file/newfileindexer.cpp
  src/file/xattrindexer.cpp

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

D21697: [BasicIndexingJob] Skip lookup of baloo document type for directories

[Stefan Brüns]

bruns created this revision.
bruns added reviewers: Baloo, ngraham, astippich, poboiko.
Herald added projects: Frameworks, Baloo.
Herald added a subscriber: kde-frameworks-devel.
bruns requested review of this revision.

REVISION SUMMARY
  typesForMimeType("inode/directory") returns an empty list, so there is
  no need to do the lookup.

REPOSITORY
  R293 Baloo

BRANCH
  basicindexingjob

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

AFFECTED FILES
  src/file/basicindexingjob.cpp

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

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

[Albert Astals Cid]

aacid added a comment.


  Yes i know you've mentioned it earlier.
  
  In my opinion, that someone is doing things wrong is not a reason to not try 
to aim and do things better.

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

[Albert Vaca Cintora]

albertvaka added inline comments.

INLINE COMMENTS

> notifybysnore.h:45
> +QMap> m_notifications;
> +QString program = QStringLiteral("SnoreToast.exe");
> +QLocalServer *server;

Isn't LibSnore abstracting this? Why do we need to call the exe instead of 
using some method in LibSnore?

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

D21695: Add FindTaglib.cmake

[Heiko Becker]

heikobecker added a comment.


  I'm not entirely sure about taglib-config on Windows and Android (can't test 
there), but similar to pkg-config I omitted the special casing. Tried to test 
this by moving taglib-config out of the way on Linux and a taglib install in 
default locations, which worked fine.

REPOSITORY
  R240 Extra CMake Modules

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

To: heikobecker, kde-buildsystem, kde-frameworks-devel
Cc: LeGast00n, bencreasy, michaelh, ngraham, bruns

D21695: Add FindTaglib.cmake

[Heiko Becker]

heikobecker created this revision.
heikobecker added reviewers: kde-buildsystem, kde-frameworks-devel.
Herald added projects: Frameworks, Build System.
heikobecker requested review of this revision.

REVISION SUMMARY
  The old one from KDELibs4 times is used by several projects from
  Frameworks, KDE Applications and Extragear.
  Modernized according the current cmake coding style but it also
  keeps compatibility with the old find module for now and should be
  a drop-in replacement.

TEST PLAN
  Removed the bundled FindTaglib.cmake from kfilemetadata, kio-extras,
  juk, amarok and rename and built them successfully.

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  master

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

AFFECTED FILES
  find-modules/FindTaglib.cmake

To: heikobecker, kde-buildsystem, kde-frameworks-devel
Cc: LeGast00n, bencreasy, michaelh, ngraham, bruns

D21694: [DocumentUrlDB] Avoid manipulation of the whole tree on trivial rename

[Stefan Brüns]

bruns retitled this revision from "[DocuementUrlDB] Avoid manipulation of the 
whole tree on trivial rename" to "[DocumentUrlDB] Avoid manipulation of the 
whole tree on trivial rename".

REPOSITORY
  R293 Baloo

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

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

D21693: [DocumentUrlDB] Catch invalid URLs early

[Stefan Brüns]

bruns updated this revision to Diff 59451.
bruns edited the summary of this revision.
bruns edited the test plan for this revision.
bruns added a comment.


  split replace into private helper and wrapper

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21693?vs=59449&id=59451

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

AFFECTED FILES
  src/engine/documenturldb.h

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

D21692: [DocumentUrlDB] Remove unused 'rename' method

[Stefan Brüns]

bruns added a comment.


  In D21692#476804 , @shubham wrote:
  
  > Does this function had any use when it was first implemented? Or may it 
come to use later?
  
  
  Its last use was removed with commit 23f9a65c215b068dc87dddf7e81a74415d5cfb10 

  
  Problem with it is it only copes with renames, not moves (i.e. changing the 
parent).
  
  Also see D21694 .

REPOSITORY
  R293 Baloo

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

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

D21692: [DocumentUrlDB] Remove unused 'rename' method

[Shubham]

shubham added a comment.


  Does this function had any use when it was first implemented? Or may it come 
to use later?

REPOSITORY
  R293 Baloo

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

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

D21694: [DocuementUrlDB] Avoid manipulation of the whole tree on trivial rename

[Stefan Brüns]

bruns created this revision.
bruns added reviewers: Baloo, ngraham, astippich, poboiko.
Herald added projects: Frameworks, Baloo.
Herald added a subscriber: kde-frameworks-devel.
bruns requested review of this revision.

REVISION SUMMARY
  In case a file or folder is renamed, only the "name" in the IdFilenameDB
  has to be updated, the IdTreeDB can be left unmodified.
  
  Currently, a rename has the same effect as removing the file/folder,
  eventually removing also its parents, and later adding the exact same
  items to the tree again (a rename does not effect the document IDs).

TEST PLAN
  1. disable indexer
  2. rename file/folder
  3. reenable indexer

REPOSITORY
  R293 Baloo

BRANCH
  documenturldb

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

AFFECTED FILES
  src/engine/documenturldb.h

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

D21693: [DocumentUrlDB] Catch invalid URLs early

[Stefan Brüns]

bruns created this revision.
bruns added reviewers: Baloo, ngraham, astippich, poboiko.
Herald added projects: Frameworks, Baloo.
Herald added a subscriber: kde-frameworks-devel.
bruns requested review of this revision.

REVISION SUMMARY
  DocumentUrlDB::replace calls DocumentUrlDB::put, which silently
  discards invalid URLs. In case of an invalid URL, the old URL/name
  is removed from the DB, but the new one is never inserted.

REPOSITORY
  R293 Baloo

BRANCH
  documenturldb

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

AFFECTED FILES
  src/engine/documenturldb.h

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

D21692: [DocumentUrlDB] Remove unused 'rename' method

[Stefan Brüns]

bruns created this revision.
bruns added reviewers: Baloo, ngraham, astippich, poboiko.
Herald added projects: Frameworks, Baloo.
Herald added a subscriber: kde-frameworks-devel.
bruns requested review of this revision.

REVISION SUMMARY
  It is not used anywhere, so remove it.

TEST PLAN
  still compiles

REPOSITORY
  R293 Baloo

BRANCH
  documenturldb

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

AFFECTED FILES
  src/engine/documenturldb.cpp
  src/engine/documenturldb.h

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

D21661: add snoretoast backend for KNotifications on Windows

[Hannah von Reth]

vonreth added inline comments.

INLINE COMMENTS

> notifybysnore.cpp:46
> +iconDir = new QTemporaryDir();
> +server = new QLocalServer();
> +server->listen(qApp->applicationName());

@brute4s99 Have you heard of the how Qt manages the life time of QObjects?
 If you pass this class as the parent object, the server will automatically get 
deleted once this class gets deleted

> notifybysnore.cpp:47
> +server = new QLocalServer();
> +server->listen(qApp->applicationName());
> +

I think the server should be a bit more unique,  what if two process of that 
name exist?
How about the full application path as a qHash?

> pino wrote in notifybysnore.cpp:56
> QMap -> QHash

same here, for this small amount of pairs a map is ok

> notifybysnore.cpp:153
> +[=](int exitCode, QProcess::ExitStatus exitStatus){ 
> +proc->close();
> +QFile file(iconPath);

deleteLater()

> notifybysnore.cpp:155
> +QFile file(iconPath);
> +file.remove();
> +});

No need to remove the icon at all, we use a tmp dir so we could just keep them 
around until the application exits and they get deleted automatically?

> notifybysnore.cpp:180
> +[=](int exitCode, QProcess::ExitStatus exitStatus){ 
> +proc->close();
> +});

Use proc->deleteLater() which will call the destructor 
https://doc.qt.io/qt-5/qprocess.html#dtor.QProcess

> pino wrote in notifybysnore.h:44
> QMap -> QHash

The amount of notifications should be small in which case a map has a better 
performance.

> notifybysnore.h:46
> +QString program = QStringLiteral("SnoreToast.exe");
> +QLocalServer *server;
> +QTemporaryDir *iconDir;

I guess you could use both, server and icon dir as non pointer members, that 
way they would exist as long as NotifyBySnore  exists.

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

[Hannah von Reth]

vonreth added a comment.


  In D21661#476607 , @sredman wrote:
  
  > 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.
  
  
  So the location doesn't matter, but as far as I know its only possible to 
register the internal com server in an executable. We could make it a static 
lib and link it in all KDE applications, but to make the action center 
integration work, we would need to to also compile a class into the executable 
using a compile time uuid.
  
  The solution with a external process isn't optimal, but the only I was able 
to come up with.
  
  The used header is meant to help with parsing the response, the cmake target 
is a INTERFACE lib, it only provides the include path..
  
  > 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

KDE CI: Frameworks » purpose » kf5-qt5 SUSEQt5.12 - Build # 49 - Still Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/purpose/job/kf5-qt5%20SUSEQt5.12/49/
 Project:
kf5-qt5 SUSEQt5.12
 Date of build:
Sun, 09 Jun 2019 10:08:08 +
 Build duration:
12 min and counting
   BUILD ARTIFACTS
  acc/KF5Purpose-5.60.0.xml
   JUnit Tests
  Name: (root) Failed: 0 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 1 test(s)Name: projectroot Failed: 1 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 2 test(s)Failed: projectroot.autotests.alternativesmodeltest
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report21%
(5/24)26%
(14/54)26%
(14/54)20%
(448/2243)17%
(166/996)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsautotests100%
(2/2)100%
(2/2)90%
(141/156)49%
(49/100)src100%
(8/8)100%
(8/8)68%
(223/329)49%
(88/180)src.externalprocess0%
(0/2)0%
(0/2)0%
(0/137)0%
(0/98)src.fileitemactionplugin0%
(0/1)0%
(0/1)0%
(0/24)0%
(0/16)src.plugins.bluetooth0%
(0/1)0%
(0/1)0%
(0/33)0%
(0/8)src.plugins.email0%
(0/1)0%
(0/1)0%
(0/63)0%
(0/24)src.plugins.imgur0%
(0/2)0%
(0/2)0%
(0/184)0%
(0/63)src.plugins.kdeconnect0%
(0/1)0%
(0/1)0%
(0/31)0%
(0/6)src.plugins.kdeconnect_sms0%
(0/1)0%
(0/1)0%
(0/16)0%
(0/2)src.plugins.ktp-sendfile0%
(0/1)0%
(0/1)0%
(0/28)0%
(0/6)src.plugins.nextcloud0%
(0/3)0%
(0/3)0%
(0/80)0%
(0/22)src.plugins.pastebin0%
(0/1)0%
(0/1)0%
(0/54)0%
(0/23)src.plugins.phabricator0%
(0/3)0%
(0/3)0%
(0/220)0%
(0/76)src.plugins.phabricator.quick0%
(0/5)0%
(0/5)0%
(0/93)0%
(0/48)src.plugins.phabricator.tests0%
(0/1)0%
(0/1)0%
(0/60)0%
(0/22)src.plugins.reviewboard0%
(0/3)0%
(0/3)0%
(0/229)0%
(0/70)src.plugins.reviewboard.quick0%
(0/7)0%
(0/7)0%
(0/153)0%
(0/80)src.plugins.saveas100%
  

KDE CI: Frameworks » kcrash » kf5-qt5 WindowsMSVCQt5.11 - Build # 44 - Aborted!

[CI System]

BUILD ABORTED
 Build URL
https://build.kde.org/job/Frameworks/job/kcrash/job/kf5-qt5%20WindowsMSVCQt5.11/44/
 Project:
kf5-qt5 WindowsMSVCQt5.11
 Date of build:
Sat, 08 Jun 2019 09:37:58 +
 Build duration:
1 day 0 hr and counting

KDE CI: Frameworks » purpose » kf5-qt5 SUSEQt5.10 - Build # 54 - Fixed!

[CI System]

BUILD SUCCESS
 Build URL
https://build.kde.org/job/Frameworks/job/purpose/job/kf5-qt5%20SUSEQt5.10/54/
 Project:
kf5-qt5 SUSEQt5.10
 Date of build:
Sun, 09 Jun 2019 10:08:08 +
 Build duration:
3 min 28 sec and counting
   BUILD ARTIFACTS
  acc/KF5Purpose-5.60.0.xml
   JUnit Tests
  Name: (root) Failed: 0 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 1 test(s)Name: projectroot Failed: 0 test(s), Passed: 2 test(s), Skipped: 0 test(s), Total: 2 test(s)
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report21%
(5/24)26%
(14/54)26%
(14/54)20%
(458/2243)18%
(175/996)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsautotests100%
(2/2)100%
(2/2)97%
(151/156)58%
(58/100)src100%
(8/8)100%
(8/8)68%
(223/329)49%
(88/180)src.externalprocess0%
(0/2)0%
(0/2)0%
(0/137)0%
(0/98)src.fileitemactionplugin0%
(0/1)0%
(0/1)0%
(0/24)0%
(0/16)src.plugins.bluetooth0%
(0/1)0%
(0/1)0%
(0/33)0%
(0/8)src.plugins.email0%
(0/1)0%
(0/1)0%
(0/63)0%
(0/24)src.plugins.imgur0%
(0/2)0%
(0/2)0%
(0/184)0%
(0/63)src.plugins.kdeconnect0%
(0/1)0%
(0/1)0%
(0/31)0%
(0/6)src.plugins.kdeconnect_sms0%
(0/1)0%
(0/1)0%
(0/16)0%
(0/2)src.plugins.ktp-sendfile0%
(0/1)0%
(0/1)0%
(0/28)0%
(0/6)src.plugins.nextcloud0%
(0/3)0%
(0/3)0%
(0/80)0%
(0/22)src.plugins.pastebin0%
(0/1)0%
(0/1)0%
(0/54)0%
(0/23)src.plugins.phabricator0%
(0/3)0%
(0/3)0%
(0/220)0%
(0/76)src.plugins.phabricator.quick0%
(0/5)0%
(0/5)0%
(0/93)0%
(0/48)src.plugins.phabricator.tests0%
(0/1)0%
(0/1)0%
(0/60)0%
(0/22)src.plugins.reviewboard0%
(0/3)0%
(0/3)0%
(0/229)0%
(0/70)src.plugins.reviewboard.quick0%
(0/7)0%
(0/7)0%
(0/153)0%
(0/80)src.plugins.saveas100%
(1/1)100%
  

D21681: ECMPackageConfigHelpers -> CMakePackageConfigHelpers

[Albert Astals Cid]

aacid closed this revision.

REPOSITORY
  R495 Purpose Library

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

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

D21681: ECMPackageConfigHelpers -> CMakePackageConfigHelpers

[Volker Krause]

vkrause accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R495 Purpose Library

BRANCH
  master

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

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