D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

[Friedrich W. H. Kossebau]

kossebau updated this revision to Diff 43516.
kossebau added a comment.


  Updating:
  
  - bump since to 5.53
  - remove todo question about background paiting, assumed not to be required 
task of the  delegate given the current code
  - implement todo about catching destroyed user delegate (not needed by 
kdevelop, done for completeness)
  - implement setting wrappedline info in styleoption for helpevent (not needed 
by kdevelop, done for completeness)
  
  Given we are almost one week into 5.52 code development, and given that
  Dominik might find time to do some more review, I propose to delay now
  to the next release cycle, to hopefully have no objections left/raised to push
  then right after 5.52 tagging.

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8708?vs=42903&id=43516

BRANCH
  addAnnotationItemDelegate

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

AFFECTED FILES
  src/CMakeLists.txt
  src/include/CMakeLists.txt
  src/include/ktexteditor/abstractannotationitemdelegate.h
  src/include/ktexteditor/annotationinterface.h
  src/utils/ktexteditor.cpp
  src/view/kateannotationitemdelegate.cpp
  src/view/kateannotationitemdelegate.h
  src/view/kateview.cpp
  src/view/kateview.h
  src/view/kateviewhelpers.cpp
  src/view/kateviewhelpers.h
  src/view/kateviewinternal.h

To: kossebau, #kate, #kdevelop
Cc: cullmann, dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, ngraham, 
bruns, demsking, sars

D16168: [Tags] Lower default log level to warning, add to categories file

[Stefan Brüns]

This revision was automatically updated to reflect the committed changes.
Closed by commit R293:73430a7ce762: [Tags] Lower default log level to warning, 
add to categories file (authored by bruns).

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16168?vs=43511&id=43515

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

AFFECTED FILES
  baloo.categories
  src/kioslaves/tags/CMakeLists.txt
  src/kioslaves/tags/kio_tags.cpp

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

D16168: [Tags] Lower default log level to warning, add to categories file

[Nathaniel Graham]

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


  Thanks!

REPOSITORY
  R293 Baloo

BRANCH
  debug_cleanup

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

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

D16168: [Tags] Lower default log level to warning, add to categories file

[Stefan Brüns]

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

REVISION SUMMARY
  The tags: ioslave had the old default log level of "all", switch of
  Debug messages by default.
  Add to categories file to allow altering with kdebugsettings.

TEST PLAN
  Watch the journal
  Enter URL tags:/
  -> journal is no longer filled with Debug messages

REPOSITORY
  R293 Baloo

BRANCH
  debug_cleanup

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

AFFECTED FILES
  baloo.categories
  src/kioslaves/tags/CMakeLists.txt
  src/kioslaves/tags/kio_tags.cpp

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

D16135: refactor tests for taglibextractor

[Stefan Brüns]

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


  Looks good, thanks.

REPOSITORY
  R286 KFileMetaData

BRANCH
  refactor_tests

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

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

D16167: Instead of copying and clearing the file list, just move it

[Stefan Brüns]

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

REVISION SUMMARY
  This makes it more explicit the list of files is now owned by the
  Indexer runnable.

TEST PLAN
  make

REPOSITORY
  R293 Baloo

BRANCH
  indexer_cleanup

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

AFFECTED FILES
  src/file/fileindexscheduler.cpp
  src/file/modifiedfileindexer.cpp
  src/file/modifiedfileindexer.h
  src/file/newfileindexer.cpp
  src/file/newfileindexer.h
  src/file/xattrindexer.cpp
  src/file/xattrindexer.h

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

D16166: Pass the FileIndexerConfig as const to the individual indexers

[Stefan Brüns]

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

REVISION SUMMARY
  Also make maxUncommitedFiles() const, used in the FileContentIndexer.
  No functional changes.

TEST PLAN
  make

REPOSITORY
  R293 Baloo

BRANCH
  indexer_cleanup

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

AFFECTED FILES
  src/file/fileindexerconfig.cpp
  src/file/fileindexerconfig.h
  src/file/filtereddiriterator.cpp
  src/file/filtereddiriterator.h
  src/file/modifiedfileindexer.cpp
  src/file/modifiedfileindexer.h
  src/file/newfileindexer.cpp
  src/file/newfileindexer.h
  src/file/unindexedfileindexer.cpp
  src/file/unindexedfileindexer.h
  src/file/unindexedfileiterator.cpp
  src/file/unindexedfileiterator.h
  src/file/xattrindexer.cpp
  src/file/xattrindexer.h

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

D16163: refactor taglibextractor to functions specific for metadata type

[Stefan Brüns]

bruns accepted this revision.
bruns added inline comments.
This revision is now accepted and ready to land.

INLINE COMMENTS

> taglibextractor.h:44
> +}
> +
>  namespace KFileMetaData

can also be written as

  namespace TagLib::ID3v2 {
  class Tag;
  }
  namespace TagLib::MP4 {
  class Tag;
  }
  ...

a little bit shorter, but otherwise just a matter of style preference ...

REPOSITORY
  R286 KFileMetaData

BRANCH
  refactor_taglib

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

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

D16165: Don't crash on invalid exiv2 data

[Stefan Brüns]

bruns added a comment.


  Can you enhance the Summary a little bit?
  
  It took me some time to notice `Exiv2::Value::toFloat()` is the same as 
`Exiv2::Value::toFloat(0)` (default argument), and accessing an inexisting 
element (i.e. n >= count()) triggers undefined behaviour, according to the 
Exiv2 documentation.
  
  Can you remove the "tries to look for **two** numbers" from the summary - 
IMHO it is misleading, as although a rational consist of numerator and 
denumerator, it is still 1 element (i.e. `count() >= 1` is sufficient).
  
  Also, `size()` denotes the size in bytes, while we want the number of 
elements, i.e. `count()`, see `Exiv2Extractor::fetchGpsDouble` here and the 
Exiv2 API .

REPOSITORY
  R286 KFileMetaData

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

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

D16165: Don't crash on invalid exiv2 data

[Alexander Stippich]

astippich added a comment.


  I guess bugs
  352856 
  353848
  361259
  will also be fixed by this?

REPOSITORY
  R286 KFileMetaData

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

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

D16165: Don't crash on invalid exiv2 data

[Igor Poboiko]

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

REVISION SUMMARY
  The file from bug 375131 crashes `baloo_file_extractor`. The problem is that 
its EXIF data
  contains a key `Exif.Photo.FocalLength`, whose type is 
`Exiv2::unsignedRational`, and with no value.
  Inside Exiv2, when we try call `toFloat()` on it, it tries to look for two 
numbers inside `value`,
  finds none and crashes.
  This is simple workaround: if we got a property with no value, just return an 
empty QVariant().
  
  (unfortunately, didn't manage to reproduce the hang reported in the bug 
originally)
  
  CCBUG: 375131

TEST PLAN
  `baloo_file_extractor` no longer crashes on the file, it processes the file 
and extracts all the necessary data

REPOSITORY
  R286 KFileMetaData

BRANCH
  dont-crash-invalid-exiv (branched from master)

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

AFFECTED FILES
  src/extractors/exiv2extractor.cpp

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

D15959: Wait for the extraction process to finish before scheduling

[Stefan Brüns]

bruns added a comment.


  In D15959#341722 , @poboiko wrote:
  
  > In D15959#339523 , @bruns wrote:
  >
  > > This works for me:
  > >
  > >void FileIndexScheduler::scheduleIndexing()
  > >{
  > >   -if (m_threadPool.activeThreadCount() || m_indexerState == 
Suspended) {
  > >   +if (m_indexerState == Suspended) {
  > >return;
  > >}
  > >
  >
  >
  > That might require some changes in the scheduling logic. Right now it will 
behave weirdly.
  >  If we do this change, there might be situations where bunch of i.e. 
`ModifiedFileIndexer` gets pushed to queue in a row. For example, we were 
inside `FirstRunIndexer`, and 10 files were modified.
  
  
  Yes, this could happen ...
  
  > Even worse, if will have a single new file and then 10 modified files - it 
will push 10 `NewFileIndexer`s.
  
  No, this will not happen. The single entry in `m_newFiles` will essentially 
be moved into the the `NewFileIndexer` (Source) 

 and afterwards `m_newFiles` is `empty()`.
  
  But you are right, this should be improved.

REPOSITORY
  R293 Baloo

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

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

D16163: refactor taglibextractor to functions specific for metadata type

[Alexander Stippich]

astippich updated this revision to Diff 43500.
astippich added a comment.


  - use forward declaration

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16163?vs=43486&id=43500

BRANCH
  refactor_taglib

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

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

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

D16137: Add an option to KConfigDialog to fit page contents horizontally

[Tim __]

kadabash added a comment.


  Sorry for the duplicate updates, I messed up my Arcanist commands.
  
  Thanks for your help so far, I tried to follow your suggestions in the above 
change.

REPOSITORY
  R265 KConfigWidgets

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

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

D16137: Add an option to KConfigDialog to fit page contents horizontally

[Tim __]

kadabash updated this revision to Diff 43499.
kadabash added a comment.


  Restore binary compatibility and introduce alignment flags

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16137?vs=43498&id=43499

BRANCH
  config-dialog-content-fit (branched from master)

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

AFFECTED FILES
  src/kconfigdialog.cpp
  src/kconfigdialog.h

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

D16137: Add an option to KConfigDialog to fit page contents horizontally

[Tim __]

kadabash updated this revision to Diff 43498.
kadabash added a comment.


  Restore binary compatibility and introduce alignment flags

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16137?vs=43413&id=43498

BRANCH
  config-dialog-content-fit (branched from master)

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

AFFECTED FILES
  src/kconfigdialog.cpp
  src/kconfigdialog.h

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

D16163: refactor taglibextractor to functions specific for metadata type

[Alexander Stippich]

astippich added a comment.


  In D16163#342057 , @mgallien wrote:
  
  > 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
  
  
  Thanks, I already knew that side, but I tend to ask explicitly in case I 
misunderstood something. I really don't want to mess up frameworks :)
  
  In D16163#342061 , @bruns wrote:
  
  > In D16163#342046 , @astippich 
wrote:
  >
  > > To be on the safe side here: I am allowed to modify private member 
functions regarding binary compatibility, right?
  >
  >
  > Non-virtual methods do not affect the class layout or the vtable layout, so 
you are safe here for sure. `private` or not does not matter.
  
  
  Thanks for the explanation!

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

D16135: refactor tests for taglibextractor

[Alexander Stippich]

astippich updated this revision to Diff 43490.
astippich added a comment.


  - rebase

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16135?vs=43409&id=43490

BRANCH
  refactor_tests

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

AFFECTED FILES
  autotests/taglibextractortest.cpp
  autotests/taglibextractortest.h

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

D16162: harmonize test data for vorbis comment tags

[Alexander Stippich]

This revision was automatically updated to reflect the committed changes.
Closed by commit R286:1aab09a628ae: harmonize test data for vorbis comment tags 
(authored by astippich).

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16162?vs=43485&id=43489

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

AFFECTED FILES
  autotests/samplefiles/test.flac
  autotests/samplefiles/test.ogg
  autotests/samplefiles/test.opus
  autotests/taglibextractortest.cpp

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

D16163: refactor taglibextractor to functions specific for metadata type

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> taglibextractor.h:31
> +#include 
>  #include 
>  

Keep the includes in the implementation and just do a forward declaration of 
the types (possible as you only use these as pointer types in the header).

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

D16163: refactor taglibextractor to functions specific for metadata type

[Stefan Brüns]

bruns 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?
  
  
  Non-virtual methods do not affect the class layout or the vtable layout, so 
you are safe here for sure. `private` or not does not matter.

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

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

D16162: harmonize test data for vorbis comment tags

[Stefan Brüns]

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


  Thx

REPOSITORY
  R286 KFileMetaData

BRANCH
  harmonize_vorbis

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

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

D16163: refactor taglibextractor to functions specific for metadata type

[Alexander Stippich]

astippich added a comment.


  To be on the safe side here: I am allowed to modify private member functions 
regarding binary compatibility, right?

REPOSITORY
  R286 KFileMetaData

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

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

D16163: refactor taglibextractor to functions specific for metadata type

[Alexander Stippich]

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

REVISION SUMMARY
  Refactor the specific extraction funtions in taglibextractor
  so that they can be re-used for files of different mimetype,
  but with the same tag types for their metadata.
  No functional change intended.

REPOSITORY
  R286 KFileMetaData

BRANCH
  refactor_taglib

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

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

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

D16135: refactor tests for taglibextractor

[Alexander Stippich]

astippich added a comment.


  In D16135#341549 , @bruns wrote:
  
  > Can you split this up in two patches, the first one updating the test files 
and fixing the values in the tests, and the second one doing the refactoring?
  
  
  see D16162 . I will rebase afterwards

REPOSITORY
  R286 KFileMetaData

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

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

D16162: harmonize test data for vorbis comment tags

[Alexander Stippich]

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

REVISION SUMMARY
  harmonize the test data for all files with vorbis comments
  so that the test can be converted to a data-driven test 
  in a following patch

REPOSITORY
  R286 KFileMetaData

BRANCH
  harmonize_vorbis

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

AFFECTED FILES
  autotests/samplefiles/test.flac
  autotests/samplefiles/test.ogg
  autotests/samplefiles/test.opus
  autotests/taglibextractortest.cpp

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

D16137: Add an option to KConfigDialog to fit page contents horizontally

[Albert Astals Cid]

aacid added a comment.


  In D16137#341984 , @kadabash wrote:
  
  > I have to admit, I don't know how to implement a version with flags 
correctly.
  
  
  Look at kcmodule.h for example you'll see some flags defined there, then 
there's a setButtons function, this would be the same but to the addPage 
function
  
  > Did I understand you correctly in that an overload with signature
  > 
  >   addPage(QWidget *page, const QString &itemName, const QString &pixmapName 
= QString(), const QString &header = QString(), bool manage = true, bool 
fitContentHorizontally = false)
  > 
  > would be ok as well? This would then be called by the overload without the 
last option (i.e. the `addPage` that existed before this diff).
  
  I'm not really the kconfigwidgets maintainer so i can't say if that would be 
acceptable API wise, but it would be ok binary compatibility wise, yes.
  
  It would though not compile because would mean you have two functions
  
addPage(QWidget *page, const QString &itemName, const QString &pixmapName = 
QString(), const QString &header = QString(), bool manage = true)
addPage(QWidget *page, const QString &itemName, const QString &pixmapName = 
QString(), const QString &header = QString(), bool manage = true, bool 
fitContentHorizontally = false)
  
  which one would be called when i call
  
addPage(someWidget, "myItemName")
  
  the first or the second?
  
  To solve that you need to make your new function not have default values, but 
as said i'm not sure if that's ok API wise, personally i think using flags is 
nicer

REPOSITORY
  R265 KConfigWidgets

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

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

D16137: Add an option to KConfigDialog to fit page contents horizontally

[Tim __]

kadabash added a comment.


  I have to admit, I don't know how to implement a version with flags correctly.
  
  Did I understand you correctly in that an overload with signature
  
addPage(QWidget *page, const QString &itemName, const QString &pixmapName = 
QString(), const QString &header = QString(), bool manage = true, bool 
fitContentHorizontally = false)
  
  would be ok as well? This would then be called by the overload without the 
last option (i.e. the `addPage` that existed before this diff).
  
  For the second version
  
addPage(QWidget *page, KCoreConfigSkeleton *config, const QString 
&itemName, const QString &pixmapName = QString(), const QString &header = 
QString(), bool fitContentHorizontally = false)
  
  I would then do the same.

REPOSITORY
  R265 KConfigWidgets

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

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

D16154: fix Service(Type)Browser documentation

[Harald Sitter]

sitter updated this revision to Diff 43468.
sitter added a comment.


  use QStringLiteral

REPOSITORY
  R272 KDNSSD

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16154?vs=43465&id=43468

BRANCH
  master

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

AFFECTED FILES
  src/servicebrowser.h
  src/servicetypebrowser.h

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

D16154: fix Service(Type)Browser documentation

[Harald Sitter]

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

REVISION SUMMARY
  servicebrowser code sample:
  
  - now uses our coding style
  - correctly references RemoteService::Ptr from outside the namespace
  - uses functor based connection syntax for compiler assisted validation
  
  servicetypebrowser:
  
  - no longer uses incorrect doxygen references prentending to be 
ServiceBrowser (this resulted in a) the STB not showing up on api.kde.org AND 
b) the STB's docs being merged into the SB's)
  - documentation is clearly on what it browses exactly

TEST PLAN
  kapidox_generate seems to generate sane docs now

REPOSITORY
  R272 KDNSSD

BRANCH
  master

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

AFFECTED FILES
  src/servicebrowser.h
  src/servicetypebrowser.h

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

D15985: [balooctl] Print current state & indexing file when monitor starts

[Igor Poboiko]

poboiko added a comment.


  That's weird. I caught it only once and thought that it is because I 
trivially forgot the endl. But now I cannot reproduce it anymore.
  Seems like it should be fine, since we do emit finished signal, which will 
write the 'OK' part.
  Sorry for the noise.

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

To: poboiko, #baloo, #frameworks, bruns, ngraham
Cc: ngraham, bruns

D15985: [balooctl] Print current state & indexing file when monitor starts

[Stefan Brüns]

bruns added a comment.


  Also, is your running baloo_file using current master? Otherwise, it may 
return a file finished long ago, see D15994 
.

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

To: poboiko, #baloo, #frameworks, bruns, ngraham
Cc: ngraham, bruns

D15985: [balooctl] Print current state & indexing file when monitor starts

[Stefan Brüns]

bruns added a comment.


  In D15985#341854 , @poboiko wrote:
  
  >   Indexing /some/file1Indexing /some/file2: OK
  >
  
  
  Are you sure its file1...file2 and not the same file twice? If it is the 
same, we can set m_currentFile in 71 and suppress the duplicated output in 
startedIndexingFile.

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

To: poboiko, #baloo, #frameworks, bruns, ngraham
Cc: ngraham, bruns

D15985: [balooctl] Print current state & indexing file when monitor starts

[Stefan Brüns]

bruns added a comment.


  In D15985#341854 , @poboiko wrote:
  
  > In D15985#341811 , @bruns wrote:
  >
  > > No, definitely not, see D15995 .
  >
  >
  > I don't see how it is a problem. The problem is not that it's not written 
to terminal (it is), but that it just doesn't have a trailing newline, i.e. 
output looks like
  >
  >   Indexing /some/file1Indexing /some/file2: OK
  >   Indexing /some/file3: OK
  >   ...
  >
  
  
  So apparently the indexer has not yet processed the registration request and 
does not send us a finished signal?
  
  Can you try to find out the exact sequence of messages using `dbus-monitor 
interface='org.kde.fileindexer'`?

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

To: poboiko, #baloo, #frameworks, bruns, ngraham
Cc: ngraham, bruns

D15985: [balooctl] Print current state & indexing file when monitor starts

[Igor Poboiko]

poboiko added a comment.


  In D15985#341811 , @bruns wrote:
  
  > No, definitely not, see D15995 .
  
  
  I don't see how it is a problem. The problem is not that it's not written to 
terminal (it is), but that it just doesn't have a trailing newline, i.e. output 
looks like
  
Indexing /some/file1Indexing /some/file2: OK
Indexing /some/file3: OK
...
  
  A flush shouldn't hurt.
  
  Just in case, I suggest not to do it inside `startedIndexingFile`, but right 
here, after line 73 (that's because we have a special case of 
`startedIndexingFile` without `finsihedIndexingFile`).

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

To: poboiko, #baloo, #frameworks, bruns, ngraham
Cc: ngraham, bruns

D15985: [balooctl] Print current state & indexing file when monitor starts

[Stefan Brüns]

bruns added a comment.


  In D15985#341751 , @poboiko wrote:
  
  > Woops! `startedIndexingFile` does not print a newline. I guess I can just 
add `m_out << endl`...
  
  
  No, definitely not, see D15995 .

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

To: poboiko, #baloo, #frameworks, bruns, ngraham
Cc: ngraham, bruns

KDE CI: Frameworks » baloo » kf5-qt5 WindowsMSVCQt5.11 - Build # 33 - Fixed!

[CI System]

BUILD SUCCESS
 Build URL
https://build.kde.org/job/Frameworks/job/baloo/job/kf5-qt5%20WindowsMSVCQt5.11/33/
 Project:
kf5-qt5 WindowsMSVCQt5.11
 Date of build:
Fri, 12 Oct 2018 09:36:44 +
 Build duration:
2 min 33 sec and counting

D16141: Disable unmount option for / or /home

[Thomas Surrel]

thsurrel added a comment.


  In D16141#341553 , @ngraham wrote:
  
  > However, I'm afraid this doesn't actually work for me:
  
  
  That's strange, it is the same code than in Dolphin (and it works for me). 
Could you have a look at the placesModel->url(index) != 
QUrl::fromLocalFile(QDir::rootPath()) comparison at tell me what each side 
contains ?

REPOSITORY
  R241 KIO

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

To: thsurrel, #frameworks, ngraham
Cc: dfaure, broulik, ngraham, kde-frameworks-devel, michaelh, bruns

D16141: Disable unmount option for / or /home

[Thomas Surrel]

thsurrel added a comment.


  In D16141#341738 , @dfaure wrote:
  
  > This whole method could put placesModel->url(index) into a local variable 
to avoid calling it so many times, though.
  
  
  Should I do that here or in a different patch ?

REPOSITORY
  R241 KIO

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

To: thsurrel, #frameworks, ngraham
Cc: dfaure, broulik, ngraham, kde-frameworks-devel, michaelh, bruns

D15985: [balooctl] Print current state & indexing file when monitor starts

[Igor Poboiko]

poboiko added a comment.


  Woops! `startedIndexingFile` does not print a newline. I guess I can just add 
`m_out << endl`...

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

To: poboiko, #baloo, #frameworks, bruns, ngraham
Cc: ngraham, bruns

D15983: React to config updates inside indexer

[Igor Poboiko]

poboiko marked 3 inline comments as done.

REPOSITORY
  R293 Baloo

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

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

D15983: React to config updates inside indexer

[Igor Poboiko]

poboiko updated this revision to Diff 43446.
poboiko added a comment.
Herald added projects: Frameworks, Baloo.
Herald added a subscriber: kde-frameworks-devel.


  Address raised issues

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15983?vs=42971&id=43446

BRANCH
  update-config

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

AFFECTED FILES
  src/file/fileindexscheduler.cpp
  src/file/fileindexscheduler.h
  src/file/indexcleaner.cpp
  src/file/mainhub.cpp

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

D16141: Disable unmount option for / or /home

[David Faure]

dfaure added a comment.


  +1
  
  This whole method could put placesModel->url(index) into a local variable to 
avoid calling it so many times, though.

REPOSITORY
  R241 KIO

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

To: thsurrel, #frameworks, ngraham
Cc: dfaure, broulik, ngraham, kde-frameworks-devel, michaelh, bruns

D15826: [Balooshow] Avoid out-of-bounds access when accessing corrupt db data

[Igor Poboiko]

poboiko added a comment.


  I'm not entirely sure how translating system works, but won't it pop up as 4 
identical lines in i.e. `Lokalize`, causing frustration to our translators? 
  It would also mean that if we would want to change the message, we would have 
to do it in 4 different lines.
  
  I suggest introducing something like an `QString errorMessage` and printing 
it only once.

REPOSITORY
  R293 Baloo

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

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

D16141: Disable unmount option for / or /home

[Thomas Surrel]

thsurrel updated this revision to Diff 43443.
thsurrel added a comment.


  Moved includes

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16141?vs=43427&id=43443

BRANCH
  arc_unmount (branched from master)

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

AFFECTED FILES
  src/filewidgets/kfileplacesview.cpp

To: thsurrel, #frameworks, ngraham
Cc: broulik, ngraham, kde-frameworks-devel, michaelh, bruns

D15939: Perform checks for unindexed files and stale index entries on startup

[Igor Poboiko]

poboiko added a comment.


  In D15939#341094 , @smithjd wrote:
  
  > https://phabricator.kde.org/D11529 was already up for review, implemented 
the index cleaner and checked for removeable volumes before removing index 
entries. Exporting the storagedevices object was required: 
https://phabricator.kde.org/D15047.  The alternative, implementing a path 
lookup function is here: https://phabricator.kde.org/D15843.
  
  
  Sorry, I was not aware of those patches (didn't do my homework). I'll look at 
those!

REPOSITORY
  R293 Baloo

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

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

D16145: Adapt autotest to new menu structure

[gregormi]

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

REPOSITORY
  R304 KNewStuff

BRANCH
  fix

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

To: nicolasfella, gregormi, dhaumann
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D15959: Wait for the extraction process to finish before scheduling

[Igor Poboiko]

poboiko added a comment.


  In D15959#339523 , @bruns wrote:
  
  > This works for me:
  >
  >void FileIndexScheduler::scheduleIndexing()
  >{
  >   -if (m_threadPool.activeThreadCount() || m_indexerState == Suspended) 
{
  >   +if (m_indexerState == Suspended) {
  >return;
  >}
  >
  
  
  That might require some changes in the scheduling logic. Right now it will 
behave weirdly.
  If we do this change, there might be situations where bunch of i.e. 
`ModifiedFileIndexer` gets pushed to queue in a row. For example, we were 
inside `FirstRunIndexer`, and 10 files were modified.
  Even worse, if will have a single new file and then 10 modified files - it 
will push 10 `NewFileIndexer`s.

REPOSITORY
  R293 Baloo

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

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

D16141: Disable unmount option for / or /home

[Thomas Surrel]

thsurrel added a comment.


  If logged in as root, wouldn't QDir::homePath() return /root and not 
/home/ ? You should be able to unmount /home in that case.

REPOSITORY
  R241 KIO

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

To: thsurrel, #frameworks, ngraham
Cc: broulik, ngraham, kde-frameworks-devel, michaelh, bruns

D16141: Disable unmount option for / or /home

[Thomas Surrel]

thsurrel edited the summary of this revision.

REPOSITORY
  R241 KIO

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

To: thsurrel, #frameworks, ngraham
Cc: broulik, ngraham, kde-frameworks-devel, michaelh, bruns