Re: plasma-framework, kactivities and kactivities-stats: please consider proper de-KF-ication now

[Matthieu Gallien]

On dimanche 5 novembre 2023 18:01:38 CET Nate Graham wrote:
> On 11/5/23 07:42, Kevin Ottens wrote:
> > I was clumsily advocating for this Akademy 2021 or 2022 (can't remember
> > which).
> > 
> > This way it's clearer to application authors when they tie themselves to a
> > given workspace or not.
> > 
> > Also, isn't Elisa able to work without Baloo? It even seems to do the right
> > thing if I trust its CMakeLists.txt. It has Baloo as a recommended but
> > optional dependency, and disable it altogether for Win32 builds.
> 
> Yes, Elisa also includes an internal indexer, for use on Windows and 
> Android, or on *Nix when Baloo isn't installed or is disabled.
> 
> I think the original idea for the app was to delegate all the indexing 
> heavy lifting to Baloo to avoid internal complication, but clearly this 
> has not worked out in practice, since to be truly cross-platform, it 
> can't assume that Baloo is present and active and does need its own 
> indexer. So maybe the best course of action is actually to remove Baloo 
> support entirely and always use the internal indexer, so that we don't 
> have two different code paths.

>From my memory, linking to Baloo was needed to query its database providing 
>much faster discovery of your music files (and the main reason od  a files 
>indexer).
>From a user perspective, reusing the files indexer database makes for a 
>smoother experience.
I would prefer to be able to also leverage files indexer services on other 
platforms rather than tying us to the old way of having each application scans 
files on its own (and monitor file system changes, ...).
That said, maybe there is a way to query baloo database without linking to the 
library.

> Nate
> 

Best

--
Matthieu

signature.asc
Description: This is a digitally signed message part.

D28980: Revert "add Baloo DBus signals for moved or removed files"

[Matthieu Gallien]

mgallien added a comment.


  In D28980#652256 , @bruns wrote:
  
  > In D28980#652102 , @mgallien 
wrote:
  >
  > > To be clear, I am fine with this change going in.
  > >
  > > I would still like to work on a proper solution to allow an application 
to monitor moved files or similar stuff without having to add file system 
watchers itself (duplicate from Baloo owns watchers).
  > >
  > > Do you have an idea ?
  >
  >
  > Elisa would need to register something like a "persistent query". This is 
something also dolphin needs - there definitely already is some notification 
between dolphin/baloo, but that is not working properly.
  >
  > This could be something like "Audio files in ~/Music/", "Audio files in 
~/Audio books/", etc., and you get a signal whenever the query would return a 
different result.
  
  
  That solution would be perfect and rely on working code inside Elisa.
  
  Do you plan to work on it or should I start working on that ? That could take 
several weeks to be done.

REPOSITORY
  R293 Baloo

BRANCH
  config_rework

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

To: bruns, #baloo, mgallien, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D28980: Revert "add Baloo DBus signals for moved or removed files"

[Matthieu Gallien]

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


  To be clear, I am fine with this change going in.
  
  I would still like to work on a proper solution to allow an application to 
monitor moved files or similar stuff without having to add file system watchers 
itself (duplicate from Baloo owns watchers).
  
  Do you have an idea ?

REPOSITORY
  R293 Baloo

BRANCH
  config_rework

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

To: bruns, #baloo, mgallien, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D28980: Revert "add Baloo DBus signals for moved or removed files"

[Matthieu Gallien]

mgallien added a comment.


  The unusual method to get notified is about avoiding signaling moved files if 
nobody is watching. Baloo will only callback if applications have registered 
themselves.

REPOSITORY
  R293 Baloo

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

To: bruns, #baloo, mgallien, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D28980: Revert "add Baloo DBus signals for moved or removed files"

[Matthieu Gallien]

mgallien added a comment.


  Sorry for the lack of time to do a proper review.
  
  I will need to have a thorough look at what Elisa is currently doing and what 
is working (or not).
  
  Is there another working way to get information about files that have moved 
without having an application monitor them itself ?
  
  If I remember correctly, Baloo is not providing information about that even 
though it is monitoring it. Is this still the case ?

REPOSITORY
  R293 Baloo

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

To: bruns, #baloo, mgallien, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D28682: export done signal in filecontentindexer

[Matthieu Gallien]

mgallien added a comment.


  This is the reason why it is so slow in Elisa?

REPOSITORY
  R293 Baloo

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

To: astippich, #baloo, bruns
Cc: mgallien, kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, GB_2, domson, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D21381: use org.freedesktop.appstream-glib package to validate appstream data

[Matthieu Gallien]

mgallien abandoned this revision.

REPOSITORY
  R240 Extra CMake Modules

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

To: mgallien, aacid, yurchor, apol
Cc: bcooksley, kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, 
bencreasy, michaelh, ngraham, bruns

D26054: partial fix for accentuated characters in file name on Windows

[Matthieu Gallien]

This revision was automatically updated to reflect the committed changes.
Closed by commit R286:8c066c834c64: partial fix for accentuated characters in 
file name on Windows (authored by mgallien).

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26054?vs=71684=72019

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

AFFECTED FILES
  src/writers/taglibwriter.cpp

To: mgallien, bruns, astippich
Cc: kde-frameworks-devel, #baloo, hurikhan77, lots0logs, LeGast00n, 
fbampaloukas, GB_2, domson, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D26054: partial fix for accentuated characters in file name on Windows

[Matthieu Gallien]

mgallien updated this revision to Diff 71684.
mgallien added a comment.


  sorry for the mess in the previous patch

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26054?vs=71683=71684

BRANCH
  fixWriterAccentuatedFileName

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

AFFECTED FILES
  src/writers/taglibwriter.cpp

To: mgallien, bruns, astippich
Cc: kde-frameworks-devel, #baloo, hurikhan77, lots0logs, LeGast00n, 
fbampaloukas, GB_2, domson, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D26054: partial fix for accentuated characters in file name on Windows

[Matthieu Gallien]

mgallien created this revision.
mgallien added reviewers: bruns, astippich.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
mgallien requested review of this revision.

REVISION SUMMARY
  partial fix to try to get accentuated file names to work with TagLibWriter

TEST PLAN
  no tests

REPOSITORY
  R286 KFileMetaData

BRANCH
  fixWriterAccentuatedFileName

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

AFFECTED FILES
  CMakeLists.txt
  src/writers/taglibwriter.cpp

To: mgallien, bruns, astippich
Cc: kde-frameworks-devel, #baloo, hurikhan77, lots0logs, LeGast00n, 
fbampaloukas, GB_2, domson, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D25622: partial solution to accept accentuated characters on windows

[Matthieu Gallien]

This revision was automatically updated to reflect the committed changes.
Closed by commit R286:9220e5045687: partial solution to accept accentuated 
characters on windows (authored by mgallien).

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25622?vs=70592=71404

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

AFFECTED FILES
  src/extractors/taglibextractor.cpp

To: mgallien, bruns, #windows
Cc: vonreth, astippich, kde-frameworks-devel, #baloo, hurikhan77, lots0logs, 
LeGast00n, fbampaloukas, GB_2, domson, ashaposhnikov, michaelh, spoorun, 
ngraham, bruns, abrahams

D25622: partial solution to accept accentuated characters on windows

[Matthieu Gallien]

mgallien added a comment.


  I forgot to maybe give some explanations.
  
  The documentation of the constructor of TagLib::FileStream takes a char* and 
"file should be a be a C-string in the local file system encoding".
  
  This means that the proper fix would possibly involve doing changes in TagLib 
itself to better handle non ASCII characters on Windows.

REPOSITORY
  R286 KFileMetaData

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

To: mgallien, bruns, #windows
Cc: kde-frameworks-devel, #baloo, hurikhan77, lots0logs, LeGast00n, 
fbampaloukas, GB_2, domson, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D25622: partial solution to accept accentuated characters on windows

[Matthieu Gallien]

mgallien added a reviewer: bruns.

REPOSITORY
  R286 KFileMetaData

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

To: mgallien, bruns
Cc: kde-frameworks-devel, #baloo, hurikhan77, lots0logs, LeGast00n, 
fbampaloukas, GB_2, domson, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D25622: partial solution to accept accentuated characters on windows

[Matthieu Gallien]

mgallien created this revision.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
mgallien requested review of this revision.

REVISION SUMMARY
  If filename contains accentuated characters, taglib fails to open it on 
windows if the QString is converted to utf8.

TEST PLAN
  Elisa is now able to index files with simple accentuated characters like é, è

REPOSITORY
  R286 KFileMetaData

BRANCH
  master

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

AFFECTED FILES
  src/extractors/taglibextractor.cpp

To: mgallien
Cc: kde-frameworks-devel, #baloo, hurikhan77, lots0logs, LeGast00n, 
fbampaloukas, GB_2, domson, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D24598: Update elisa icon

[Matthieu Gallien]

mgallien accepted this revision.
mgallien added a comment.


  Thanks

REPOSITORY
  R266 Breeze Icons

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

To: astippich, #vdg, ngraham, mgallien, trickyricky26
Cc: trickyricky26, GB_2, kde-frameworks-devel, stuartm, daerny, mfraser, 
mnesbitt, LeGast00n, carneirogustavo, jguidon, ctakano, Tizon, oussemabouaneni, 
ashwind, fbampaloukas, sourabhboss, aureliencouderc, tgraves, hantzv, 
lcmscheid, nhuisman, ursjoss, mykolak, jussiv, michaelh, astippich, James, 
ngraham, bruns, kmf, lemuel, mgallien

D24598: Update elisa icon

[Matthieu Gallien]

mgallien added a comment.


  In D24598#546135 , @astippich 
wrote:
  
  > I have no intention on taking over D12992 
 as I would like rather like to code for 
Elisa. I am no designer.
  >  I was unsatisfied with the icon and @mgallien agreed on some slight 
adjustments to the current one.
  >  D12992  has been around for ages. I 
hope that this incremental update can be agreed upon faster and land for next 
frameworks release. Especially I find the "Elisa" text ugly and worth removing 
it quickly.
  
  
  I am in support of the work that is done here. Thanks @astippich

REPOSITORY
  R266 Breeze Icons

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

To: astippich, #vdg, ngraham, mgallien
Cc: GB_2, kde-frameworks-devel, stuartm, daerny, mfraser, mnesbitt, LeGast00n, 
carneirogustavo, jguidon, ctakano, Tizon, oussemabouaneni, ashwind, 
fbampaloukas, sourabhboss, aureliencouderc, tgraves, hantzv, lcmscheid, 
nhuisman, ursjoss, mykolak, jussiv, michaelh, astippich, James, ngraham, bruns, 
kmf, lemuel, mgallien

D24406: Small performance improvements suggested by clang tidy

[Matthieu Gallien]

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


  Thanks

REPOSITORY
  R286 KFileMetaData

BRANCH
  master

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

To: aacid, mgallien
Cc: mgallien, kde-frameworks-devel, #baloo, lots0logs, LeGast00n, fbampaloukas, 
GB_2, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, 
abrahams

D12992: New elisa icon

[Matthieu Gallien]

mgallien added a comment.


  In D12992#533180 , @lshoravi wrote:
  
  > Bumping @abetts
  
  
  Thanks for keeping this on your todo list. This is very much appreciated.
  
  I am trying to move forward the process of having Elisa published on Windows 
Store. It would be nice to be able to have a more distinctive icon before doing 
that.
  
  I have noticed that the current icon is very close to the icon of the Gnome 
podcast application.
  
  Can I help this make progress ?

REPOSITORY
  R266 Breeze Icons

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

To: lshoravi, #vdg, ngraham, #elisa
Cc: ndavis, KonqiDragon, abetts, paullesur, januz, mgallien, alex-l, andreask, 
kde-frameworks-devel, mfraser, mnesbitt, LeGast00n, carneirogustavo, jguidon, 
ctakano, Tizon, oussemabouaneni, ashwind, fbampaloukas, GB_2, sourabhboss, 
aureliencouderc, tgraves, hantzv, lcmscheid, nhuisman, ursjoss, mykolak, 
jussiv, michaelh, astippich, James, ngraham, bruns, kmf, lemuel

D12992: New elisa icon

[Matthieu Gallien]

mgallien added a comment.


  In D12992#486880 , @lshoravi wrote:
  
  > Using @abetts prototype: No blockers, incorporating the given teal and 
green colours given by mgallien is TODO.
  >
  > Otherwise, the blocker would be to find what visual profile Elisa has so 
that we can work around that.
  
  
  Thanks for your help. Let me know if I can help with this.

REPOSITORY
  R266 Breeze Icons

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

To: lshoravi, #vdg, ngraham, #elisa
Cc: abetts, paullesur, januz, mgallien, alex-l, andreask, kde-frameworks-devel, 
mnesbitt, LeGast00n, carneirogustavo, jguidon, ctakano, Tizon, oussemabouaneni, 
ashwind, fbampaloukas, sourabhboss, aureliencouderc, tgraves, hantzv, 
lcmscheid, nhuisman, ursjoss, mykolak, jussiv, michaelh, astippich, James, 
ngraham, bruns, kmf, lemuel

D20526: fix extracting of some properties to match what was writen

[Matthieu Gallien]

mgallien added a comment.


  In D20526#483910 , @astippich 
wrote:
  
  > @mgallien Are you going to update? There are bug reports which will also be 
fixed by this, so it would be nice to get this in.
  >  Or do you mind if I take over?
  
  
  I would like but I am quite busy with real life. If you feel like working on 
this, that would be really great.

REPOSITORY
  R286 KFileMetaData

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

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

D21581: Detect valgrind, avoid database removal when using valgrind

[Matthieu Gallien]

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


  Very nice. Thank you

REPOSITORY
  R293 Baloo

BRANCH
  master

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

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

D21381: use org.freedesktop.appstream-glib package to validate appstream data

[Matthieu Gallien]

mgallien added a comment.


  In D21381#469617 , @apol wrote:
  
  > How about we test against both?
  
  
  I have just pushed a new diff to do that.
  I am not sure if it desirable to keep appstreamcli test given it fails to 
validate files that are validated by flathub and appstream-utils (without 
flathub patches).
  
  > And maybe it should be using `appstream-util validate` instead of forcing 
it through flatpak? 
  >  It will make it easier to be able to run it in build.kde.org.
  
  I would like to also get an extra test to do that on setup where flatpak is 
available given that flathub uses a patched appstream-utils with relaxed 
validation constraints.

REPOSITORY
  R240 Extra CMake Modules

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

To: mgallien, aacid, yurchor, apol
Cc: bcooksley, kde-frameworks-devel, kde-buildsystem, bencreasy, michaelh, 
ngraham, bruns

D21381: use org.freedesktop.appstream-glib package to validate appstream data

[Matthieu Gallien]

mgallien updated this revision to Diff 58628.
mgallien added a comment.


  - use appstream-util and appstreamcli to validate appdata file

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21381?vs=58595=58628

BRANCH
  master

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

AFFECTED FILES
  kde-modules/KDECMakeSettings.cmake

To: mgallien, aacid, yurchor, apol
Cc: bcooksley, kde-frameworks-devel, kde-buildsystem, bencreasy, michaelh, 
ngraham, bruns

D21381: use org.freedesktop.appstream-glib package to validate appstream data

[Matthieu Gallien]

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

REVISION SUMMARY
  should help having the same validation than the one done by flathub
  
  related to D21242  and D21243 


TEST PLAN
  A test with Elisa allows validation tu succeed

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  master

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

AFFECTED FILES
  kde-modules/KDECMakeSettings.cmake
  kde-modules/appstreamtest.cmake

To: mgallien, aacid, yurchor
Cc: kde-frameworks-devel, kde-buildsystem, bencreasy, michaelh, ngraham, bruns

D21219: Don't try to index SQL database dumps

[Matthieu Gallien]

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

REPOSITORY
  R293 Baloo

BRANCH
  dont-try-to-index-database-dumps (branched from master)

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

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

D20526: fix extracting of some properties to match what was writen

[Matthieu Gallien]

mgallien created this revision.
mgallien added reviewers: astippich, bruns.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
mgallien requested review of this revision.

REVISION SUMMARY
  Currently, one can write and read through taglib extractor and writer
  plugins. Unfortunately, some "smart" transformation on the data are
  applied when reading them. This does not allow to read exactly what was
  writen.
  
  This is a work in progress to shed some light on what is needed to get Elisa 
to support reading and writing of properties through KFileMetaData.
  
  This is somehow linked to D19087  and 
D20502  .

TEST PLAN
  New automatic test is OK, no regressions.

REPOSITORY
  R286 KFileMetaData

BRANCH
  master

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

AFFECTED FILES
  autotests/taglibwritertest.cpp
  autotests/taglibwritertest.h
  src/extractors/taglibextractor.cpp

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

D20310: Use nullptr instead of NULL

[Matthieu Gallien]

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


  Thanks

REPOSITORY
  R286 KFileMetaData

BRANCH
  nullptr

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

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

D20219: Propose Stefan Bruns as KFileMetaData maintainer

[Matthieu Gallien]

mgallien added subscribers: bruns, mgallien.
mgallien accepted this revision.
mgallien added a comment.
This revision is now accepted and ready to land.


  Thanks for all the work going on. I am really happy to see all the work done 
by @bruns

REPOSITORY
  R286 KFileMetaData

BRANCH
  master

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

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

D16579: Remove support for non-standard APE tag field names from the test files

[Matthieu Gallien]

mgallien added a comment.


  In D16579#428835 , @smithjd wrote:
  
  > The change was ack'ed in this review, and this review closed by the commit.
  >
  > There was no code that required further review, only binary changes. The 
posted concerns were addressed by D18826 .
  >
  > If you really are that concerned that this review is cleaned up, request 
yourself that the reviewers that requested changes clear their request. I don't 
need to ask them to when the request is no longer valid. Otherwise, please at 
least read the full review closely before complaining about events you weren't 
consulted on or involved with, and have imo little reason to force a re-visit 
of.
  
  
  Hello, the tone of your comment is not appropriate.
  
  Please avoid this.
  
  I agree with @ngraham, you should have asked both commenters if they were in 
agreement with the content and scope of the review before landing it. This is 
the way reviews work.

REPOSITORY
  R286 KFileMetaData

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

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

Re: Power Management and Inhibition by Applications

[Matthieu Gallien]

Hello,

On mercredi 23 janvier 2019 08:52:26 CET Kai Uwe Broulik wrote:
> Hi,
> 
> > I had thought that would only inhibit the screensaver instead of automatic
> > sleep.
> 
> KScreenLocker (previously KSMServer) that owns the
> org.freedesktop.ScreenSaver interface tells PowerDevil to keep the
> screen on when screensaver is inhibited. It makes no sense to prevent
> the screen from locking (ie. keep the desktop visible) but then allow
> the screen to turn off, having the same effect: hiding the desktop.
> > I did a quick test and in case I inhibit through
> > org.freedesktop.ScreenSaver interface, the battery applet does not
> > indicate any inhibit whereas through
> > org.freedesktop.PowerManagement.Inhibit indicates that an inhibition is
> > valid.
> It should and it does here. Note that PowerDevil only enforces
> inhibitions after five seconds to prevent short transient inhibitions
> (e.g. web browsers block suspend when playing audio which could also
> lead to short notification sounds prolonging the time until suspend)

I have restarted my session and it is now working. Sorry for that.

I also had somehow missed that interface. It looks to me like it is also 
supported by other desktop environments. Do you know if it is really the case 
?

> > I wonder if I should not instead make usage of the Inhibit mechanism from
> > logind coupled with powerdevil. I am not sure if this is desired.
> 
> It is and I would love that, I just haven't had the time to implement it.
> 
> Also I'm a bit tired of adding yet another inhibition interface (we
> already have like three or four of them) of the "but this time, I
> promise, it will be perfect!!" kind. Last time I checked logind didn't
> offer signals for when an inhibition was added/removed, so the "xyz is
> curently blocking PM" in Battery Monitor might not be possible anymore
> this way.
> 
> Cheers
> Kai Uwe

My understanding of the current mechanism in Logind is that except the 
"systemctl suspend" command, nothing prevent suspend when you add an inhibitor 
blocking sleep. I was hoping for this interface to be the "universal" one that 
would avoid the need to support many interfaces in an application.

As far as I understand, each software wanting to suspend has to check itself 
for any blocking inhibition for sleep. I can volunteer to add this to 
Powerdevil if this is deemed useful.

I had checked the dbus interface for logind and there is still no any signals 
to know when an inhibition is added or removed.

Best regards

--
Matthieu Gallien

Re: Power Management and Inhibition by Applications

[Matthieu Gallien]

Hello,

On samedi 19 janvier 2019 12:19:10 CET Elvis Angelaccio wrote:
> On 16/01/19 22:07, Matthieu Gallien wrote:
> > Hello,
> > 
> > I am trying to work on a feature request to add the ability to suppress
> > laptop sleep when playing music.
> > 
> > Currently, Juk is doing it via dbus calls to
> > "org.kde.Solid.PowerManagement".
> > 
> > I also had a look to Solid frameworks and the power classes that add an
> > asynchronous way to do that.
> > Currently, the Solid framework only conditionally provides those features
> > and it seems distributions (at least Debian) do not always build it.
> > 
> > I am also unsure about the current recommended solution that would work
> > for
> > most desktop environments.
> > 
> > What would be the best way forward ?
> 
> Hi,
> the "official" way to achieve that should be a dbus call to the
> org.freedesktop.ScreenSaver interface (methods Inhibit and UnInhibit).

I had thought that would only inhibit the screensaver instead of automatic 
sleep.
I did a quick test and in case I inhibit through org.freedesktop.ScreenSaver 
interface, the battery applet does not indicate any inhibit whereas through 
org.freedesktop.PowerManagement.Inhibit indicates that an inhibition is valid.

I wonder if I should not instead make usage of the Inhibit mechanism from 
logind coupled with powerdevil. I am not sure if this is desired.

I would forward inhibit request to logind and checks existing inhibition 
before allowing powerdecil to suspend.

> > I can also see some other features that could make sense to provide in
> > libraries that music players could use (lyrics fetching, album art
> > fetching, ...). This could be in a dedicated framework to help build
> > media players.
> > 
> > Best regards
> > 
> > --
> > Matthieu Gallien
> 
> Cheers,
> Elvis

Best regards

Power Management and Inhibition by Applications

[Matthieu Gallien]

Hello,

I am trying to work on a feature request to add the ability to suppress laptop 
sleep when playing music.

Currently, Juk is doing it via dbus calls to "org.kde.Solid.PowerManagement".

I also had a look to Solid frameworks and the power classes that add an 
asynchronous way to do that.
Currently, the Solid framework only conditionally provides those features and 
it seems distributions (at least Debian) do not always build it.

I am also unsure about the current recommended solution that would work for 
most desktop environments.

What would be the best way forward ?

I can also see some other features that could make sense to provide in 
libraries that music players could use (lyrics fetching, album art fetching, 
...). This could be in a dedicated framework to help build media players.

Best regards

--
Matthieu Gallien

Power Management and Inhibition by Applications

[Matthieu Gallien]

Hello,

I am trying to work on a feature request to add the ability to suppress laptop 
sleep when playing music.

Currently, Juk is doing it via dbus calls to "org.kde.Solid.PowerManagement".

I also had a look to Solid frameworks and the power classes that add an 
asynchronous way to do that.
Currently, the Solid framework only conditionally provides those features and 
it seems distributions (at least Debian) do not always build it.

I am also unsure about the current recommended solution that would work for 
most desktop environments.

What would be the best way forward ?

I can also see some other features that could make sense to provide in 
libraries that music players could use (lyrics fetching, album art fetching, 
...). This could be in a dedicated framework to help build media players.

Best regards

--
Matthieu Gallien

D17500: Restore mobipocket extractor

[Matthieu Gallien]

mgallien added a comment.


  In D17500#376221 , @aacid wrote:
  
  > In D17500#375753 , @astippich 
wrote:
  >
  > > Oh, thanks for the hint, didn't know that. That makes it a lot more 
complicated than a straight port :(
  > >  Looking at the code of kdegraphics-mobipocket, shouldn't the thumbnail 
extractor actually be part of kio-extras? Seems quite kio-specific, and a quick 
look at lxr didn't reveal any usages of the thumbnailer.
  >
  >
  > Why would it be part of kio-extras? the beauty of plugins is that they can 
live wherever, no?
  >
  > Do I understand that the answer to my "Is it possible to move the extractor 
to kdegraphics-mobipocket instead of having it in kfilemetadata? " question is 
no?
  
  
  There is support for external extractors in KFileMetaData. As far as I know, 
it has not yeet been used.
  That is probably the safest way to do what you suggest.
  
  > If so, that probably needs fixing, the fact that you can't have external 
plugins means that the code is probably not as good as it should

REPOSITORY
  R286 KFileMetaData

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

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

D17046: Use the new FindExiv2 module from ECM.

[Matthieu Gallien]

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


  Please feel free to land this after solving my comment.

INLINE COMMENTS

> CMakeLists.txt:57
> -set_package_properties(Exiv2 PROPERTIES DESCRIPTION "Image Tag reader"
> -   URL "http://www.exiv2.org; TYPE OPTIONAL
> PURPOSE "Support for image metadata")

Maybe, you may not remove the URL. That would be consistent with other 
dependencies.

REPOSITORY
  R286 KFileMetaData

BRANCH
  master

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

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

D16618: add explicit to constructors

[Matthieu Gallien]

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

REPOSITORY
  R286 KFileMetaData

BRANCH
  explicit

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

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

D16618: add explicit to constructors

[Matthieu Gallien]

mgallien added inline comments.

INLINE COMMENTS

> simpleextractionresult.h:42
>  public:
> -SimpleExtractionResult(const QString& url, const QString& mimetype = 
> QString(), const Flags& flags = ExtractEverything);
> +explicit SimpleExtractionResult(const QString& url, const QString& 
> mimetype = QString(), const Flags& flags = ExtractEverything);
>  SimpleExtractionResult(const SimpleExtractionResult& rhs);

Given this class is exported, is this binary compatible ?

REPOSITORY
  R286 KFileMetaData

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

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

D16197: provide a list of supported mimetypes for embeddedimagedata

[Matthieu Gallien]

mgallien added inline comments.

INLINE COMMENTS

> embeddedimagedata.h:62
> + */
> +QStringList readMimetypes() const;
> +

You can make it static because you are returning a static member. It means that 
you should probably return by const reference (even if I do not remember if it 
is always the best answer).

REPOSITORY
  R286 KFileMetaData

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

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

D16163: refactor taglibextractor to functions specific for metadata type

[Matthieu Gallien]

mgallien added a comment.


  In D16163#342046 , @astippich 
wrote:
  
  > To be on the safe side here: I am allowed to modify private member 
functions regarding binary compatibility, right?
  
  
  This page is a very good reference: 
https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B

REPOSITORY
  R286 KFileMetaData

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

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

D8532: [WIP] Restrict file extractor with Seccomp

[Matthieu Gallien]

mgallien added a comment.


  In D8532#289500 , @davidk wrote:
  
  > I was asked in private about the current state of libseccomp integration 
and why there was no progress in a long time.
  >  The current state is, that I have implemented seccomp support in 
kfilemetadata using this API:
  >
  >   bool setProcessReadOnly(uint32_t defaultAction, 
std::vector addionalWhitelist)
  >
  >
  > But there are two blockers, related to external plugins:
  >
  > - External plugins based on interpreters like python/lua/perl etc. need a 
huge whitelist. This is problematic as I want to keep the list of allowed 
syscalls as small as possible (the list would be huge). Additionally, it would 
be difficult to get a list of all needed syscalls. Thus, we would break many 
external plugins.
  > - Baloo is basically unmaintained. Thus, if something breaks, fixing it 
should be as easy as possible. But what if QT requires a new syscall and thus, 
the tests (and deployments) are failing? We need a way to know which syscall 
failed. This works for kfilemetadata plugins, but not for external plugins 
(because they are separate processes). The only way I can image, would be 
running the whole test with strace.
  >
  >   So, if anyone is willing to continue this work, I would be happy to share 
my current state. Otherwise, if everyone agrees that we don't care about 
external plugins (users of external plugins can disable Seccomp support with an 
environment variable), I can finish the patches.
  
  
  Sorry for my late reply
  The external extractors tests of KFileMetaData have always failed and nobody 
ever fixed them. This makes me think that they are not really maintained.
  Is there any use of them apart from the unit tests ?

REPOSITORY
  R293 Baloo

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

To: davidk, apol, ossi, #frameworks, smithjd, bruns
Cc: mgallien, kde-frameworks-devel, michaelh, #baloo, detlefe, ngraham, 
nicolasfella, ashaposhnikov, astippich, spoorun, bruns, abrahams

D15614: remove usage of own TString to QString conversion function

[Matthieu Gallien]

mgallien added a comment.


  In D15614#328722 , @astippich 
wrote:
  
  > The removal of the convertWCharsToQString function is safe, right? It was 
never exported
  
  
  It is a static function. So it is absolutely safe to remove it.

REPOSITORY
  R286 KFileMetaData

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

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

D13700: implement reading of the replaygain tags

[Matthieu Gallien]

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

REPOSITORY
  R286 KFileMetaData

BRANCH
  replaygain

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

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

D13700: implement reading of the replaygain tags

[Matthieu Gallien]

mgallien added a comment.


  Thanks for your hard work. This is a really nice addition to audio tags.
  I no longer have objections. Please finish to take into account feedback from 
@bruns.

REPOSITORY
  R286 KFileMetaData

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

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

D13700: implement reading of the replaygain tags

[Matthieu Gallien]

mgallien requested changes to this revision.
mgallien added a comment.
This revision now requires changes to proceed.


  Some inline comments

INLINE COMMENTS

> taglibextractor.cpp:254-263
> +if(!strcmp( userTextFrame->description().toCString( true ), 
> "replaygain_track_gain" ) ) {
> +data.replayGainTrackGain = 
> convertWCharsToQString(userTextFrame->fieldList().back().toCString(true));
> +}
> +if(!strcmp( userTextFrame->description().toCString( true ), 
> "replaygain_track_peak" ) ) {
> +data.replayGainTrackPeak = 
> convertWCharsToQString(userTextFrame->fieldList().back().toCString(true));
> +}
> +if(!strcmp( userTextFrame->description().toCString( true ), 
> "replaygain_album_gain" ) ) {

I am not a fan of the strcmp usage here. Is it not possible to do without them ?

> taglibextractor.cpp:468
>  
> -// Rating.
>  itMPC = lstMusepack.find("RATING");

This change is unrelated to the patch. Please remove it.

> taglibextractor.cpp:662
>  
> -// Rating.
>  itOgg = lstOgg.find("RATING");

same comment

> taglibextractor.cpp:934
> +}
> +qDebug() << data.replayGainAlbumGain;
> +result->add(Property::ReplayGainAlbumGain, 
> data.replayGainAlbumGain.toDouble());

Can you remove it ?

> taglibextractor.cpp:944
> +/* remove " dB" suffix */
> +if (data.replayGainTrackGain.endsWith(QStringLiteral(" 
> dB"),Qt::CaseInsensitive))
> +{

Could you check if it is not faster to use QLatin1String here ?

> properties.h:327-334
> +/**
> +  * Contains ReplayGain information for audio files
> +  * The album/track gain is given in "dB"
> +  */
> +ReplayGainAlbumPeak,
> +ReplayGainAlbumGain,
> +ReplayGainTrackPeak,

Please document each entry separately otherwise only the first one will have 
documentation (as seen on api.kde.org).

REPOSITORY
  R286 KFileMetaData

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

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

D13906: add missing include

[Matthieu Gallien]

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

REPOSITORY
  R286 KFileMetaData

BRANCH
  add_include

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

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

D15013: balootctl: fix 396535

[Matthieu Gallien]

mgallien added a comment.


  In D15013#313880 , @jausmus wrote:
  
  > In D15013#313867 , 
@anthonyfieroni wrote:
  >
  > > Do not use QDir::separator
  > >
  > >   if (!folder.endsWith(QLatin1Char('/')) {
  > >   folder += QLatin1Char('/');
  > >   }
  > >
  >
  >
  > Does balooctl not need cross platform support?
  
  
  I believe this explains why QDirSeparator should not be used here: 
http://agateau.com/2015/qdir-separator-considered-harmful/

REPOSITORY
  R293 Baloo

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

To: jausmus
Cc: mgallien, anthonyfieroni, kde-frameworks-devel, #baloo, ashaposhnikov, 
michaelh, astippich, spoorun, ngraham, bruns, abrahams

D14131: Add enum alias Property::Language for typo Property::Langauge

[Matthieu Gallien]

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


  Thanks for your work and good idea.

REPOSITORY
  R286 KFileMetaData

BRANCH
  fixLangaugeEnumTypoNow

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

To: kossebau, mgallien
Cc: bcooksley, aacid, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D14131: Add enum alias Property::Language for typo Property::Langauge

[Matthieu Gallien]

mgallien added a comment.


  In D14131#294528 , @aacid wrote:
  
  > Looks good to me. If @mgallien doesn't give you an "accept" in a reasonable 
timeframe i guess you can count this as me accepting it ;)
  
  
  I am in holidays for 3 weeks and unable to properly review anything. Sorry.
  By the way, is there a KDE wide service to indicate vacancies ?

REPOSITORY
  R286 KFileMetaData

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

To: kossebau, mgallien
Cc: aacid, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D12950: add test which checks the property types

[Matthieu Gallien]

mgallien added a comment.


  Sorry for the delay on my side.
  Why are you adding this test ?

REPOSITORY
  R286 KFileMetaData

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

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

D13914: Avoid compiler warnings for taglib headers

[Matthieu Gallien]

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


  Thanks

REPOSITORY
  R286 KFileMetaData

BRANCH
  avoidtaglibheaderwarnings

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

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

D13885: taglibextractor: Restore extracting audio props without tags existing

[Matthieu Gallien]

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


  @kossebau thanks for your work on this patch
  @astippich thanks for your quick reaction and the work on this patch

REPOSITORY
  R286 KFileMetaData

BRANCH
  fix_empty_tags

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

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

D13630: automatic tests: do not embed EmbeddedImageData already in the library

[Matthieu Gallien]

This revision was automatically updated to reflect the committed changes.
Closed by commit R286:aa90123a8c18: automatic tests: do not embed 
EmbeddedImageData already in the library (authored by mgallien).

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13630?vs=36381=36389

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

AFFECTED FILES
  autotests/CMakeLists.txt

To: mgallien, dfaure, bcooksley, bruns, astippich
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D12320: add ability to read embedded cover files

[Matthieu Gallien]

mgallien added a comment.


  D13630  should fix the Windows build. 
Sorry for not noticing this before. I was mostly away from keyboard for the 
last two weeks after having been sick.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien, michaelh, bruns
Cc: bcooksley, dfaure, anthonyfieroni, kde-frameworks-devel, #baloo, bruns, 
ashaposhnikov, michaelh, astippich, spoorun, ngraham, abrahams

D13630: automatic tests: do not embed EmbeddedImageData already in the library

[Matthieu Gallien]

mgallien created this revision.
mgallien added reviewers: dfaure, bcooksley, bruns, astippich.
Restricted Application added projects: Frameworks, Baloo.
Restricted Application added subscribers: Baloo, kde-frameworks-devel.
mgallien requested review of this revision.

REVISION SUMMARY
  should fix build failure due to D12320 

TEST PLAN
  build on Linux and tests are OK

REPOSITORY
  R286 KFileMetaData

BRANCH
  master

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

AFFECTED FILES
  autotests/CMakeLists.txt

To: mgallien, dfaure, bcooksley, bruns, astippich
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D13302: check for needed version of libavcode, libavformat and libavutil

[Matthieu Gallien]

This revision was automatically updated to reflect the committed changes.
Closed by commit R286:461f8ec81b81: check for needed version of libavcode, 
libavformat and libavutil (authored by mgallien).

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13302?vs=35712=35776

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

AFFECTED FILES
  CMakeLists.txt
  cmake/FindFFmpeg.cmake
  src/config-kfilemetadata.h.in
  src/extractors/CMakeLists.txt
  src/extractors/ffmpegextractor.cpp

To: mgallien, romangg, adridg, jriddell
Cc: jriddell, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns

Re: KDE Frameworks 5.47.0

[Matthieu Gallien]

On mercredi 6 juin 2018 23:10:52 CEST Albert Astals Cid wrote:
> El dimecres, 6 de juny de 2018, a les 12:55:15 CEST, Jonathan Riddell va
> 
> escriure:
> > On Sat, Jun 02, 2018 at 09:57:08PM +0200, David Faure wrote:
> > > Dear packagers,
> > > 
> > > KDE Frameworks 5.47.0 has been uploaded to the usual place.
> > 
> > KFileMetaData continues not to compile
> 
> That was already discussed and it's because you're using an old ffmpeg,
> right?
> 
> Matthieu did you not commit that patch to make it clear a newer version is
> needed?

Yes but I failed to understand that the FindFFmpeg.cmake was not able to check 
the version. I have now open https://phabricator.kde.org/D13302 that just uses 
the new API if one has a newer FFmpeg or uses the old API.
This should definitely solve the problem.

I am sorry for not having understood that was the only feasible solution.

> Cheers,
>   Albert
> 
> > Jonathan

Best regards

--
Matthieu Gallien

D12992: New elisa icon

[Matthieu Gallien]

mgallien added a comment.


  In D12992#269849 , @abetts wrote:
  
  > In D12992#269803 , @lshoravi 
wrote:
  >
  > > @abetts Yes, I've been concepting a little on different ideas but not 
really coming anywhere I feel would fit in.
  > >
  > > Looks great!
  > >
  > > As for the background, when I think of soundwaves I think of something 
that would be produced by a visualizer, or a sine wave.
  > >
  > > Did you make the letters yourself or is it a font? I'm asking just in 
case the font is licensed.
  >
  >
  > The letters are from a font from Google Fonts. We should be OK to use it. 
We can also modify a bit if you want. Although I feel it looks good.
  
  
  Your proposal is looking great and it should be easily recognizable. I really 
like it. It also looks quite modern.
  Sorry for the delay for my answer. Thanks a lot for your help.
  
  >> Could you using blue and teal for the accent colours? I'm thinking that's 
what Elisa uses by default.
  > 
  > I sure can! Can you give me a couple of reference colors so that I can 
start from there? I mean a teal and blue HEX numbers.
  
  The blue used by Elisa is the following: #3daee9 .
  The grey used by Elisa is the following: #c4c5c5 .
  
  > 
  > 
  >> (Speaking of that, I can't start Elisa. No error message in terminal or 
anything, it just doesnt start.)
  
  Do you have the possibility to test it from the flathub flatpak repository ? 
At least, the 0.1.1 bug fix version is there. I am not so sure about the status 
of the flatpak nightly builds hosted by KDE. In case, you would need them, we 
should ping people from the flatpak team.
  You can also try to send me any log you may have to try to understand what 
happen.

REPOSITORY
  R266 Breeze Icons

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

To: lshoravi, #vdg, ngraham, #elisa
Cc: abetts, paullesur, januz, astippich, mgallien, alex-l, andreask, 
kde-frameworks-devel, ssteffen, lcmscheid, nhuisman, ursjoss, mykolak, jussiv, 
michaelh, ngraham, bruns, kmf

D13216: Overhaul the file index scheduler.

[Matthieu Gallien]

mgallien added a comment.


  Sounds good to me. I will try to do a proper review as soon as I can. Sorry 
for the delay.

REPOSITORY
  R293 Baloo

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

To: smithjd, bruns, mgallien
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns

D13302: check for needed version of libavcode, libavformat and libavutil

[Matthieu Gallien]

mgallien updated this revision to Diff 35712.
mgallien added a comment.


  - checks if FFmpeg provides the new API and use it only in this case
  
  extend the existing configure header to allow to compile both versions of the 
code in the FFmpeg extractor
  
  in case the new API exists, we prefer it and we also support compilation with 
the old one
  
  kelpt the code checking for the new versions even we do not know how to 
properly check for it (pkg-config is only used to get an hint before searching 
headers and libraries and only on platforms with pkg-config)

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13302?vs=35457=35712

BRANCH
  master

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

AFFECTED FILES
  CMakeLists.txt
  cmake/FindFFmpeg.cmake
  src/config-kfilemetadata.h.in
  src/extractors/CMakeLists.txt
  src/extractors/ffmpegextractor.cpp

To: mgallien, romangg, adridg
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns

D13302: check for needed version of libavcode, libavformat and libavutil

[Matthieu Gallien]

mgallien updated this revision to Diff 35457.
mgallien added a comment.


  - prefer usage of CHECK_STRUCT_HAS_MEMBER instead of having test code
  
  the test is shorter with CHECK_STRUCT_HAS_MEMBER (thanks @adridg for the help)

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13302?vs=35456=35457

BRANCH
  master

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

AFFECTED FILES
  CMakeLists.txt
  cmake/FindFFmpeg.cmake

To: mgallien, romangg, adridg
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns

D13302: check for needed version of libavcode, libavformat and libavutil

[Matthieu Gallien]

mgallien retitled this revision from "[WIP] check for needed version of 
libavcode, libavformat and libavutil" to "check for needed version of 
libavcode, libavformat and libavutil".
mgallien edited the test plan for this revision.

REPOSITORY
  R286 KFileMetaData

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

To: mgallien, romangg, adridg
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns

D13302: [WIP] check for needed version of libavcode, libavformat and libavutil

[Matthieu Gallien]

mgallien updated this revision to Diff 35456.
mgallien added a comment.


  - check AVStrean structure have codecpar member
  
  working check for codecpar member in AVStream structure
  
  should I try to make the result variable be not in the cmake cache ?

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13302?vs=35455=35456

BRANCH
  master

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

AFFECTED FILES
  CMakeLists.txt
  cmake/FindFFmpeg.cmake

To: mgallien, romangg, adridg
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns

D13302: [WIP] check for needed version of libavcode, libavformat and libavutil

[Matthieu Gallien]

mgallien created this revision.
mgallien added reviewers: romangg, adridg.
Restricted Application added projects: Frameworks, Baloo.
Restricted Application added subscribers: Baloo, kde-frameworks-devel.
mgallien requested review of this revision.

REVISION SUMMARY
  check for needed version of libavcode, libavformat and libavutil
  
  modify FindFFmpeg.cmake to better handle the components of ffmpeg and
  allow to check the version of a component
  
  ask for the versions of each component in the 3.1 version of ffmpeg
  
  versions check do not work, so we manually checks that AVstream
  structure has codecpar member

TEST PLAN
  the check for codecpar member in AVStream structure does not work yet

REPOSITORY
  R286 KFileMetaData

BRANCH
  master

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

AFFECTED FILES
  CMakeLists.txt
  cmake/FindFFmpeg.cmake

To: mgallien, romangg, adridg
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns

D12320: [RFC] add ability to read embedded cover files

[Matthieu Gallien]

mgallien added a comment.


  In D12320#272370 , @bruns wrote:
  
  > as the remaining issues are formatting only, this is an `accept` by me save 
the requested changes.
  >  @mgallien - if i am late to accept, can you do it?
  
  
  Yes sure. I have just seen your message. I also believe Alex is mostly away 
from keyboard this weekend.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien, michaelh, bruns
Cc: anthonyfieroni, kde-frameworks-devel, #baloo, bruns, ashaposhnikov, 
michaelh, astippich, spoorun, ngraham

D12320: [RFC] add ability to read embedded cover files

[Matthieu Gallien]

mgallien added a comment.


  In D12320#271300 , @astippich 
wrote:
  
  > any more comments? would be nice to ship it with Kf 5.48
  
  
  @bruns you requested changes. Can you have a look when you have time ?

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien, michaelh, bruns
Cc: anthonyfieroni, kde-frameworks-devel, #baloo, bruns, ashaposhnikov, 
michaelh, astippich, spoorun, ngraham

D12156: implement reading of rating tag

[Matthieu Gallien]

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


  I hope application developers will not complain about the need to support a 
second api to get ratings. I am still unsure this the best way to add this 
support.

REPOSITORY
  R286 KFileMetaData

BRANCH
  rating

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

To: astippich, mgallien, michaelh
Cc: cgilles, kde-frameworks-devel, #baloo, bruns, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham

D12992: New elisa icon

[Matthieu Gallien]

mgallien added a subscriber: paullesur.
mgallien added a comment.


  @januz and @astippich I really understand your point of view about the 
current icon. I share your point of view the need to have an icon easy to 
differentiate from many other players. For example, I would like Elisa be easy 
to spot on this page: https://flathub.org/apps/category/AudioVideo .
  
  At the same time, I had an ex colleague, aged 62 or something like that, 
saying that the old icons was bad for youngsters. That really made me hesitate 
about the current icon. I really doubt people under 20 could understand the 
current icon.
  
  Anyway, the current icon seems to need some more work to really look finished 
as has been pointed out.
  
  I will try to show the current icon and this one to another bunch of people 
to collect feedback (people around 22 and older one). My idea is definitely not 
to run a popularity contest here.
  
  I do like the idea to have some sort of classical music notes in the icon 
while still being easy to associate to music. That would reflect on the fact 
that we made quite some work on supporting this use case.
  
  @lshoravi  I appreciate your offer to help like I appreciate the one 
@paullesur did.

REPOSITORY
  R266 Breeze Icons

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

To: lshoravi, #vdg, ngraham, #elisa
Cc: paullesur, januz, astippich, mgallien, alex-l, andreask, 
kde-frameworks-devel, ssteffen, lcmscheid, nhuisman, ursjoss, mykolak, jussiv, 
michaelh, ngraham, bruns, kmf

D12156: implement reading of rating tag

[Matthieu Gallien]

mgallien added a subscriber: cgilles.
mgallien added a comment.


  @cgilles Are you somehow using KFileMetaData (seems correct if one looks at 
the Debian package dependencies) ?
  This diff is about adding a way to read ratings from "tags" native to a 
specific file format.
  
  This would be in addition to the current ratings that are independent from 
the file types.
  
  Can you comment on this patch with a digikam point of view ?
  
  I am wondering if we should not find a way to make this more transparent for 
users of the API. This is why I am asking here.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien, michaelh
Cc: cgilles, kde-frameworks-devel, #baloo, bruns, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham

D12992: New elisa icon

[Matthieu Gallien]

mgallien added subscribers: astippich, januz.
mgallien added a comment.


  In D12992#266288 , @alex-l wrote:
  
  > F5863162: image.png 
  >
  > ^ I'm for this one without the quaver
  
  
  I also prefer this one without the quaver.
  Is it possible to really simplify the background for the small size icon ? It 
would be probably much easier to read it.
  
  @astippich  and @januz what is your opinion ? You are also working on Elisa 
and should have the rights to speak ;)
  
  > F5862923: image.png 
  > 
  > ^ or this one with white symbol

REPOSITORY
  R266 Breeze Icons

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

To: lshoravi, #vdg, ngraham, #elisa
Cc: januz, astippich, mgallien, alex-l, andreask, kde-frameworks-devel, 
ssteffen, lcmscheid, nhuisman, ursjoss, mykolak, jussiv, michaelh, ognarb, 
ngraham, bruns, kmf

D12578: fix some issues reported by clazy

[Matthieu Gallien]

This revision was automatically updated to reflect the committed changes.
Closed by commit R293:f887468b172a: fix some issues reported by clazy (authored 
by mgallien).

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12578?vs=34389=34390

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

AFFECTED FILES
  autotests/unit/file/pendingfilequeuetest.cpp
  src/engine/transaction.cpp
  src/file/fileindexerconfig.cpp
  src/file/fileindexscheduler.cpp
  src/file/firstrunindexer.cpp
  src/file/indexcleaner.cpp
  src/file/modifiedfileindexer.cpp
  src/file/newfileindexer.cpp
  src/file/xattrindexer.cpp
  src/kioslaves/kded/baloosearchmodule.cpp
  src/lib/query.cpp
  src/lib/searchstore.cpp
  src/lib/taglistjob.cpp
  src/lib/term.cpp

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

D12578: fix some issues reported by clazy

[Matthieu Gallien]

mgallien marked an inline comment as done.
mgallien added a comment.


  @bruns Thanks for the review and sorry for my slowness.

REPOSITORY
  R293 Baloo

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

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

D12578: fix some issues reported by clazy

[Matthieu Gallien]

mgallien updated this revision to Diff 34389.
mgallien added a comment.


  - fix one more issue

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12578?vs=34223=34389

BRANCH
  arcpatch-D12578

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

AFFECTED FILES
  autotests/unit/file/pendingfilequeuetest.cpp
  src/engine/transaction.cpp
  src/file/fileindexerconfig.cpp
  src/file/fileindexscheduler.cpp
  src/file/firstrunindexer.cpp
  src/file/indexcleaner.cpp
  src/file/modifiedfileindexer.cpp
  src/file/newfileindexer.cpp
  src/file/xattrindexer.cpp
  src/kioslaves/kded/baloosearchmodule.cpp
  src/lib/query.cpp
  src/lib/searchstore.cpp
  src/lib/taglistjob.cpp
  src/lib/term.cpp

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

D12578: fix some issues reported by clazy

[Matthieu Gallien]

mgallien updated this revision to Diff 34223.
mgallien added a comment.


  fix some issues

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12578?vs=33247=34223

BRANCH
  arcpatch-D12578

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

AFFECTED FILES
  autotests/unit/file/pendingfilequeuetest.cpp
  src/engine/transaction.cpp
  src/file/fileindexerconfig.cpp
  src/file/fileindexscheduler.cpp
  src/file/firstrunindexer.cpp
  src/file/indexcleaner.cpp
  src/file/modifiedfileindexer.cpp
  src/file/newfileindexer.cpp
  src/file/xattrindexer.cpp
  src/kioslaves/kded/baloosearchmodule.cpp
  src/lib/query.cpp
  src/lib/searchstore.cpp
  src/lib/taglistjob.cpp
  src/lib/term.cpp

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

D12886: check that ffmpeg is at least version 3.1 that introduce the API we require

[Matthieu Gallien]

This revision was automatically updated to reflect the committed changes.
Closed by commit R286:3415015e3d45: check that ffmpeg is at least version 3.1 
that introduce the API we require (authored by mgallien).

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12886?vs=34174=34208

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

AFFECTED FILES
  CMakeLists.txt

To: mgallien, dfaure, michaelh, jriddell, bruns
Cc: bruns, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham

D12578: fix some issues reported by clazy

[Matthieu Gallien]

mgallien added a comment.


  In D12578#262831 , @bruns wrote:
  
  > @mgallien - I think this is trivial to fix up - can you do, so we have one 
less request open?
  
  
  I will do but as I had started working on that as a low priority task, I have 
not yet been able to correctly fix the issues.

REPOSITORY
  R293 Baloo

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

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

D12886: check that ffmpeg is at least version 3.1 that introduce the API we require

[Matthieu Gallien]

mgallien created this revision.
mgallien added reviewers: dfaure, michaelh, jriddell.
Restricted Application added projects: Frameworks, Baloo.
Restricted Application added subscribers: Baloo, kde-frameworks-devel.
mgallien requested review of this revision.

REVISION SUMMARY
  check that ffmpeg is at least version 3.1 that introduce the API we require

TEST PLAN
  checks a working version of ffmpeg

REPOSITORY
  R286 KFileMetaData

BRANCH
  master

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

AFFECTED FILES
  CMakeLists.txt

To: mgallien, dfaure, michaelh, jriddell
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns

Re: kfilemetadata compile failure

[Matthieu Gallien]

On lundi 14 mai 2018 19:57:58 CEST Albert Astals Cid wrote:
> El diumenge, 13 de maig de 2018, a les 23:15:56 CEST, Matthieu Gallien va
> 
> escriure:
> > On dimanche 13 mai 2018 23:03:02 CEST Albert Astals Cid wrote:
> > > El divendres, 11 de maig de 2018, a les 23:22:15 CEST, Jonathan Riddell
> > > va
> > > 
> > > escriure:
> > > > On Thu, Apr 26, 2018 at 08:31:46AM +0200, Kevin Funk wrote:
> > > > > On Wednesday, 25 April 2018 14:34:58 CEST Jonathan Riddell wrote:
> > > > > > kfilemetadata does not compile in KDE neon from git master
> > > > > > currently
> > > > > > 
> > > > > > /workspace/build/src/extractors/ffmpegextractor.cpp:97:15: error:
> > > > > > ‘AVCodecParameters’ does not name a type
> > > > > > 12:27:35  const AVCodecParameters* codec =
> > > > > > stream->codecpar;
> > > > > > 
> > > > > > https://build.neon.kde.org/job/xenial_unstable_kde_kfilemetadata_b
> > > > > > in
> > > > > > _a
> > > > > > md
> > > > > > 64/1 45/console
> > > > > 
> > > > > Raised a concern on the resp. Phab Diff (to ping the responsible
> > 
> > people):
> > > > >   https://phabricator.kde.org/R286:037208a787e0c2412ab616ff1573c323a
> > > > >   23
> > > > >   46
> > > > >   d2
> > > > >   d
> > > 
> > > If phabricator had worked i would have read that.
> > > 
> > > > This compile failure is still in the shortly to be released
> > > > kfilemetadata
> > > > tar
> > > > 
> > > > https://build.neon.kde.org/job/xenial_release_kde_kfilemetadata_bin_am
> > > > d6
> > > > 4/
> > > > 37 /console 14:11:40
> > > > /workspace/build/src/extractors/ffmpegextractor.cpp:97:15: error:
> > > > ‘AVCodecParameters’ does not name a type 14:11:40  const
> > > > AVCodecParameters* codec = stream->codecpar;
> > > 
> > > your avcodec is too old, get a new one.
> > > 
> > > But yes, either the commit should be reverted or the the avcodec cmake
> > > requirement increased.
> > 
> > Sorry, I have completely forgotten to check that during the review. This
> > is
> > my fault.
> > What is the best way to revert for the v5.46.0 release and bump the
> > dependency on master ?
> 
> 5.46.0 was already released, so nothing we can do "to fix it".

I was meaning to have a fix on top of v5.46.0 that may be released as a 
v5.46.1 if needed.

> 
> Cheers,
>   Albert
> 
> > > Cheers,
> > > 
> > >   Albert
> > >   
> > > > Jonathan

phabricator seems to be down. Please find attached a patch to add a check for 
version 3.1 of ffmpeg that has the needed API to fix the build failure.

Sorry again for the time lost by everybody on this issue.

Best regards

--
Matthieu>From cf7c3ca35622778cb1c2a3cea8742f264c68c1dc Mon Sep 17 00:00:00 2001
From: Matthieu Gallien <matthieu_gall...@yahoo.fr>
Date: Mon, 14 May 2018 21:34:16 +0200
Subject: [PATCH] check that ffmpeg is at least version 3.1 that introduce the
 API we require

---
 CMakeLists.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 863f019..a6221fc 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -56,7 +56,7 @@ set_package_properties(Exiv2 PROPERTIES DESCRIPTION "Image Tag reader"
URL "http://www.exiv2.org; TYPE OPTIONAL
PURPOSE "Support for image metadata")
 
-find_package(FFmpeg 1.0)
+find_package(FFmpeg 3.1)
 set_package_properties(FFmpeg PROPERTIES DESCRIPTION "Video Tag reader"
URL "http://ffmpeg.org; TYPE OPTIONAL
PURPOSE "Support for video metadata")
-- 
2.17.0

Re: kfilemetadata compile failure

[Matthieu Gallien]

On dimanche 13 mai 2018 23:03:02 CEST Albert Astals Cid wrote:
> El divendres, 11 de maig de 2018, a les 23:22:15 CEST, Jonathan Riddell va
> 
> escriure:
> > On Thu, Apr 26, 2018 at 08:31:46AM +0200, Kevin Funk wrote:
> > > On Wednesday, 25 April 2018 14:34:58 CEST Jonathan Riddell wrote:
> > > > kfilemetadata does not compile in KDE neon from git master currently
> > > > 
> > > > /workspace/build/src/extractors/ffmpegextractor.cpp:97:15: error:
> > > > ‘AVCodecParameters’ does not name a type
> > > > 12:27:35  const AVCodecParameters* codec = stream->codecpar;
> > > > 
> > > > https://build.neon.kde.org/job/xenial_unstable_kde_kfilemetadata_bin_a
> > > > md
> > > > 64/1 45/console
> > > 
> > > Raised a concern on the resp. Phab Diff (to ping the responsible 
people):
> > >   https://phabricator.kde.org/R286:037208a787e0c2412ab616ff1573c323a2346
> > >   d2
> > >   d
> 
> If phabricator had worked i would have read that.
> 
> > This compile failure is still in the shortly to be released kfilemetadata
> > tar
> > 
> > https://build.neon.kde.org/job/xenial_release_kde_kfilemetadata_bin_amd64/
> > 37 /console 14:11:40
> > /workspace/build/src/extractors/ffmpegextractor.cpp:97:15: error:
> > ‘AVCodecParameters’ does not name a type 14:11:40  const
> > AVCodecParameters* codec = stream->codecpar;
> 
> your avcodec is too old, get a new one.
> 
> But yes, either the commit should be reverted or the the avcodec cmake
> requirement increased.

Sorry, I have completely forgotten to check that during the review. This is my 
fault.
What is the best way to revert for the v5.46.0 release and bump the dependency 
on master ?

> Cheers,
>   Albert
> 
> > Jonathan

D12320: [RFC] add ability to read embedded cover files

[Matthieu Gallien]

mgallien added a comment.


  I am away from keyboard. Can we move forward ?
  Does it make sense to aim for having only one copy of this code and makes the 
kio code depends on KFileMetaData ?

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien, michaelh, bruns
Cc: anthonyfieroni, kde-frameworks-devel, #baloo, bruns, ashaposhnikov, 
michaelh, astippich, spoorun, ngraham

D12156: implement reading of rating tag

[Matthieu Gallien]

mgallien added a comment.


  In D12156#254055 , @astippich 
wrote:
  
  > In D12156#252575 , @mgallien 
wrote:
  >
  > > If I have correctly understood the ideas behind the conception of Baloo, 
we should probably prefer to store the rating with a "native" solution instead 
of the xattr one that is not portable outside of software using KFileMetaData.
  > >
  > > We have to stay compatible with older versions of KFileMetaData. So we 
should push the same rating to both properties and try to read it from both 
properties in the cases where only one exists.
  > >
  > > Could you try to modify your patch to do that ?
  >
  >
  > I don't understand. Which patch to you want me to modify? This one here 
only adds the ability to read the rating embedded in the tags in addition to 
the xattr rating. It's up to the application to decide what to do with the 
information. 
  >  The patch for baloo-widgets just hides this new rating tag to avoid 
duplicate entries in e.g. dolphin, and actually preserves the current behavior 
this way. Baloo caches the embedded rating anyways. 
  >  When KFileMetaData gains the ability to write the tags, one should write 
to both properties, but that is currently not possible.
  
  
  I would like to have only one rating in the KFileMetaData API that would 
transparently use the most appropriate way to store and read the rating.
  That would be:
  
  - music audio file with a rating tag = we read the tag and write both tag and 
xattr attribute to maximize compatibility ;
  - music audio file with only xattr rating attribute = we read the xattr 
attribute and write both tag and xattr attribute to maximize compatibility ;
  - any file with only xattr rating attribute = we read the xattr attribute and 
write the xattr attribute ;
  
  Does this sound reasonable and feasible ?

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien, michaelh
Cc: bruns, #frameworks, ashaposhnikov, michaelh, astippich, spoorun, ngraham

D12197: autotests: Test for multiple values

[Matthieu Gallien]

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

REPOSITORY
  R286 KFileMetaData

BRANCH
  multi-value-test (branched from master)

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

To: michaelh, #baloo, #frameworks, mgallien, bruns
Cc: bruns, ashaposhnikov, michaelh, astippich, spoorun, ngraham

D12320: {RFC] add ability to read embedded cover files

[Matthieu Gallien]

mgallien added a comment.


  Thanks for your work. Please find a few remarks inline.

INLINE COMMENTS

> embeddedimagedata.h:25
> +#include "kfilemetadata_export.h"
> +#include 
> +

You can do a forward declaration of QMimeDatabase because it is only referred 
through a reference.

> embeddedimagedata.h:40
> +class Private;
> +Private *d;
> +

You should be using a std::unique_ptr instead of a raw pointer. You also should 
take care of either forbidding copy (operator= and copy constructor) or 
implement them to perform a deep copy including duplicating the private object.
Currently, it will be implicitly shared between instances.
In many classes in KDE repository you do not need to do it because QObject 
inheritance gives you exactly that.

> embeddedimagedata.h:42
> +
> +QByteArray getFrontCover(const QString , const QString 
> ) const;
> +

The private method can go to the private class.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien, michaelh
Cc: bruns, #frameworks, ashaposhnikov, michaelh, astippich, spoorun

D12578: fix some issues reported by clazy

[Matthieu Gallien]

mgallien created this revision.
mgallien added reviewers: Baloo, Frameworks.
Restricted Application added projects: Frameworks, Baloo.
mgallien requested review of this revision.

REVISION SUMMARY
  fix some issues reported by clazy

TEST PLAN
  tests are OK

REPOSITORY
  R293 Baloo

BRANCH
  master

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

AFFECTED FILES
  CMakeLists.txt
  autotests/unit/file/pendingfilequeuetest.cpp
  src/engine/transaction.cpp
  src/file/fileindexerconfig.cpp
  src/file/fileindexscheduler.cpp
  src/file/firstrunindexer.cpp
  src/file/indexcleaner.cpp
  src/file/modifiedfileindexer.cpp
  src/file/newfileindexer.cpp
  src/file/xattrindexer.cpp
  src/kioslaves/kded/baloosearchmodule.cpp
  src/lib/query.cpp
  src/lib/searchstore.cpp
  src/lib/taglistjob.cpp
  src/lib/term.cpp

To: mgallien, #baloo, #frameworks
Cc: ashaposhnikov, michaelh, astippich, spoorun, bruns

D4911: add Baloo DBus signals for moved or removed files

[Matthieu Gallien]

This revision was automatically updated to reflect the committed changes.
Closed by commit R293:ef0e268c6577: add Baloo DBus signals for moved or removed 
files (authored by mgallien).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D4911?vs=14953=33246#toc

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4911?vs=14953=33246

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

AFFECTED FILES
  autotests/unit/file/CMakeLists.txt
  autotests/unit/file/filewatchtest.cpp
  autotests/unit/file/metadatamovertest.cpp
  autotests/unit/file/metadatamovertest.h
  src/CMakeLists.txt
  src/engine/transaction.cpp
  src/engine/transaction.h
  src/file/CMakeLists.txt
  src/file/filewatch.cpp
  src/file/filewatch.h
  src/file/mainhub.cpp
  src/file/mainhub.h
  src/file/metadatamover.cpp
  src/file/metadatamover.h
  src/file/org.kde.BalooWatcherApplication.xml

To: mgallien, vhanda, dfaure, michaelh, #baloo
Cc: ngraham, cullmann, apol, #frameworks, ashaposhnikov, michaelh, astippich, 
spoorun, bruns

D9408: extractors: Hide warnings from system headers

[Matthieu Gallien]

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

REPOSITORY
  R286 KFileMetaData

BRANCH
  master

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

To: kfunk, mgallien
Cc: #frameworks, ashaposhnikov, michaelh, astippich, spoorun, bruns

D9408: extractors: Hide warnings from system headers

[Matthieu Gallien]

mgallien requested changes to this revision.
mgallien added a comment.
This revision now requires changes to proceed.


  Please have a look at the issue.
  Thanks

INLINE COMMENTS

> CMakeLists.txt:35
> +target_include_directories(kfilemetadata_exiv2extractor SYSTEM PRIVATE 
> ${EXIV2_INCLUDE_DIR})
> +kde_target_enable_exceptions(kfilemetadata_exiv2extractor)
>  target_link_libraries(kfilemetadata_exiv2extractor

I have got an error that I solved by adding PRIVATE after the target. Could you 
fix that ?

REPOSITORY
  R286 KFileMetaData

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

To: kfunk, mgallien
Cc: #frameworks, ashaposhnikov, michaelh, astippich, spoorun, bruns

D12341: fix detection of taglib when compiling for Android

[Matthieu Gallien]

This revision was automatically updated to reflect the committed changes.
Closed by commit R286:d497ffebe202: fix detection of taglib when compiling for 
Android (authored by mgallien).

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12341?vs=32543=33035

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

AFFECTED FILES
  cmake/FindTaglib.cmake

To: mgallien, #frameworks, #baloo, bruns
Cc: bruns, ashaposhnikov, michaelh, astippich, spoorun

D12156: implement reading of rating tag

[Matthieu Gallien]

mgallien requested changes to this revision.
mgallien added a comment.
This revision now requires changes to proceed.


  In D12156#249994 , @astippich 
wrote:
  
  > In D12156#249977 , @michaelh 
wrote:
  >
  > > In D12156#249971 , @astippich 
wrote:
  > >
  > > > In D12156#249951 , @michaelh 
wrote:
  > > >
  > > > > It's for elisa I guess, could you please elaborate how POPM/RATING is 
going to be used and why xattr are not applicable?
  > > >
  > > >
  > > > It will be used as a fallback when there is no xattr rating set or 
available (e.g. on Windows) so that users who have rated their music with other 
players can still see their previous ratings. 
  > > >  It will also useful for managing ratings on other platforms when 
writing support is added.
  > >
  > >
  > > I that case I would suggest to use xattr only on systems that support it, 
and use POPM/RATING on those that don't. I afraid users will find it confusing 
to have two ways of rating a file. Or is rating via dolphin/baloo-widgets not 
planned?
  > >  Because extractors are called in sequence it would also be possible to 
create a dedicate taglibRatingExtractor (much easier to apply build 
conditionals).
  >
  >
  > We still need the ability to read the tag, since users expect to see the 
rating in music players.
  >  For baloo-widgets I created D12157 
  
  
  If I have correctly understood the ideas behind the conception of Baloo, we 
should probably prefer to store the rating with a "native" solution instead of 
the xattr one that is not portable outside of software using KFileMetaData.
  
  We have to stay compatible with older versions of KFileMetaData. So we should 
push the same rating to both properties and try to read it from both properties 
in the cases where only one exists.
  
  Could you try to modify your patch to do that ?

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien, michaelh
Cc: bruns, #frameworks, ashaposhnikov, michaelh, astippich, spoorun

D12320: add ability to read embedded cover files

[Matthieu Gallien]

mgallien added a comment.


  In D12320#249998 , @astippich 
wrote:
  
  > In D12320#249982 , @michaelh 
wrote:
  >
  > > -2
  > >  https://cgit.kde.org/ffmpegthumbs.git/ should be useable, not sure 
though.
  >
  >
  > It is also disqualified by the fact that it is not in frameworks. I think a 
nice solution is to implement a separate "extractor" that is not an extractor 
plugin like taglib, epub, etc. but implemented like the xattr tags 
(usermetadata) as a separate, exported class. This way, baloo doesn't have to 
be changed in any way and still applications using kfilemetadata can query the 
cover files specifically.
  
  
  How does behave Baloo if you add properties of type EmbeddedPicture ?
  Is it not a good idea to fix Baloo to not index everything but only 
searchable properties (like text and numeric properties and ignore binary data) 
?
  
  The current way to manage user rating mush have had a good rationale for its 
current design but I have failed to understand it.
  It would also look quite odd to have a particular way to fetch properties of 
type EmbeddedPicture.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien, michaelh
Cc: bruns, #frameworks, ashaposhnikov, michaelh, astippich, spoorun

D12341: fix detection of taglib when compiling for Android

[Matthieu Gallien]

mgallien created this revision.
mgallien added reviewers: Frameworks, Baloo.
Restricted Application added projects: Frameworks, Baloo.
mgallien requested review of this revision.

REVISION SUMMARY
  fix detection of taglib when cross compiling for Android

TEST PLAN
  tagib is properly found instead of the host version of the library

REPOSITORY
  R286 KFileMetaData

BRANCH
  master

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

AFFECTED FILES
  cmake/FindTaglib.cmake

To: mgallien, #frameworks, #baloo
Cc: ashaposhnikov, michaelh, astippich, spoorun, bruns

D12320: add ability to read embedded cover files

[Matthieu Gallien]

mgallien added a comment.


  Thanks a lot to continue to push this work.
  
  In D12320#249433 , @bruns wrote:
  
  > Baloos DBs should only contain searchable data. So until someone teaches 
baloo how to compare images, it should be kept separate.
  
  
  I am also not convinced that Baloo should index the cover images. They are 
quite a lot of data. At the same time, I am really happy to have support for it 
in KFileMetaData.
  
  It should then be easy to add support in a file preview or in applications.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien, michaelh
Cc: bruns, #frameworks, ashaposhnikov, michaelh, astippich, spoorun

D12108: ffmpegextractor: Silence deprecation warnings

[Matthieu Gallien]

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


  Thanks.
  
  I did try to fix it some months ago but did not finished however I noticed 
that there is no automatic tests for it. Do you have time to work on some ?

REPOSITORY
  R286 KFileMetaData

BRANCH
  ffmpeg_deprecated (branched from master)

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

To: michaelh, #baloo, #frameworks, mgallien
Cc: mgallien, bruns, ashaposhnikov, michaelh, astippich, spoorun

D12156: implement reading of rating tag

[Matthieu Gallien]

mgallien requested changes to this revision.
This revision now requires changes to proceed.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien, michaelh
Cc: #frameworks, ashaposhnikov, michaelh, astippich, spoorun, bruns

D12156: implement reading of rating tag

[Matthieu Gallien]

mgallien added a comment.


  Thanks.
  Fix one issue and we should be good to go.
  Another nice thing is that we should have a portable way to have ratings in 
Elisa that could be read or write somewhere else.

INLINE COMMENTS

> taglibextractor.cpp:222-234
> +if (temp == 0) {
> +data.rating = 0;
> +} else if (temp >= 1 && temp <= 31) {
> +data.rating = 2;
> +} else if (temp >= 32 && temp <= 95) {
> +data.rating = 4;
> +} else if (temp >= 96 && temp <= 159) {

You can and should be using all intermediate values. If I remember correctly, 
in dolphin, you can set all values between 0 and 10. This is done by half blue 
stars.
Can you use modulo instead of embedded if ?

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien, michaelh
Cc: #frameworks, ashaposhnikov, michaelh, astippich, spoorun, bruns

D11489: Make concatenated strings wrappable

[Matthieu Gallien]

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


  You could even push directly for this kind of changes.

REPOSITORY
  R286 KFileMetaData

BRANCH
  wrappable

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

To: michaelh, mgallien, #baloo, #frameworks
Cc: ashaposhnikov, michaelh, astippich, spoorun, bruns

D12197: autotests: Test for multiple values

[Matthieu Gallien]

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


  Sorry, that was a local problem.

REPOSITORY
  R286 KFileMetaData

BRANCH
  multi-value-test

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

To: michaelh, #baloo, #frameworks, mgallien
Cc: bruns, ashaposhnikov, michaelh, astippich, spoorun

D12197: autotests: Test for multiple values

[Matthieu Gallien]

mgallien requested changes to this revision.
mgallien added a comment.
This revision now requires changes to proceed.


  The test for mp3 is failing to complete. I am investigating.

REPOSITORY
  R286 KFileMetaData

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

To: michaelh, #baloo, #frameworks, mgallien
Cc: bruns, ashaposhnikov, michaelh, astippich, spoorun

D10694: epubextractor: Handle multiple subjects better

[Matthieu Gallien]

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


  Thanks

REPOSITORY
  R286 KFileMetaData

BRANCH
  multi-subject

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

To: michaelh, mgallien, dfaure
Cc: bruns, astippich, #frameworks, ashaposhnikov, michaelh, spoorun, 
navarromorales, isidorov, firef, andrebarros, emmanuelp

D12197: autotests: Test for multiple values

[Matthieu Gallien]

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


  Thanks

REPOSITORY
  R286 KFileMetaData

BRANCH
  multi-value-test

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

To: michaelh, #baloo, #frameworks, mgallien
Cc: bruns, ashaposhnikov, michaelh, astippich, spoorun

D12145: autotests: Do not use QScopedPointer for plugins

[Matthieu Gallien]

mgallien added a comment.


  In D12145#245023 , @alexeymin 
wrote:
  
  > Just curious, why is this needed? To avoid dynamic memory allocations?
  
  
  I asked on new code to avoid using an extractor allocated on heap instead of 
on stack. @michaelh just did the modifications in all tests to keep them 
coherent.
  
  To me, the allocation on heap looks absolutely overhead.

REPOSITORY
  R286 KFileMetaData

BRANCH
  plugin-simple (branched from master)

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

To: michaelh, #baloo, #frameworks, mgallien, bruns
Cc: alexeymin, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns