D29826: [KMainWindow] Invoke QIcon::setFallbackThemeName (later)

[Igor Poboiko]

poboiko added a comment.


  In D29826#674634 , @aacid wrote:
  
  > oh i think  i'm too late :D
  
  
  Whoops, sorry :S
  
  Actually, I thought that if this piece of code is executed with Qt >= 5.15.1 
(or whenever the proper fix is landed), it shouldn't hurt anyone anyways.
  The TODO is more about code maintainability and reducing amount of various 
hacks in the code, and in that case another `#if` won't help much :)
  
  > Let's wait for the fix to actually land and then i'll propose that change
  
  OK! :)

REPOSITORY
  R263 KXmlGui

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

To: poboiko, aacid, mart, broulik
Cc: mart, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29826: [KMainWindow] Invoke QIcon::setFallbackThemeName (later)

[Igor Poboiko]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R263:899c0f7fa457: [KMainWindow] Invoke 
QIcon::setFallbackThemeName (later) (authored by poboiko).

REPOSITORY
  R263 KXmlGui

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29826?vs=83215=83216

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

AFFECTED FILES
  src/kmainwindow.cpp

To: poboiko, aacid, mart, broulik
Cc: mart, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29826: [KMainWindow] Invoke QIcon::setFallbackThemeName (later)

[Igor Poboiko]

poboiko updated this revision to Diff 83215.
poboiko added a comment.


  Right, 5.15.1.

REPOSITORY
  R263 KXmlGui

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29826?vs=83212=83215

BRANCH
  icon-load (branched from master)

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

AFFECTED FILES
  src/kmainwindow.cpp

To: poboiko, aacid, mart, broulik
Cc: mart, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29826: [KMainWindow] Invoke QIcon::setFallbackThemeName (later)

[Igor Poboiko]

poboiko updated this revision to Diff 83212.
poboiko added a comment.


  Seems like the Qt fix was landed
  
  ... another one (https://codereview.qt-project.org/c/qt/qtbase/+/302341), but 
whatever.
  So I've removed link to the old one and added note to remove this after we 
depend on Qt 5.15.
  
  Any objections, should I land this one?

REPOSITORY
  R263 KXmlGui

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29826?vs=83141=83212

BRANCH
  icon-load (branched from master)

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

AFFECTED FILES
  src/kmainwindow.cpp

To: poboiko, aacid, mart, broulik
Cc: mart, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29826: [KMainWindow] Invoke QIcon::setFallbackThemeName (later)

[Igor Poboiko]

poboiko updated this revision to Diff 83141.
poboiko added a comment.


  Sure, it's a temporary workaround anyways :)
  
  Update comment: add TODO mark and link to @aacid Qt patch

REPOSITORY
  R263 KXmlGui

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29826?vs=83129=83141

BRANCH
  icon-load (branched from master)

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

AFFECTED FILES
  src/kmainwindow.cpp

To: poboiko, aacid, mart, broulik
Cc: mart, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29826: [KMainWindow] Invoke QIcon::setFallbackThemeName (later)

[Igor Poboiko]

poboiko added a comment.


  In D29826#673585 , @aacid wrote:
  
  > What about discover for example?
  
  
  Good point...

REPOSITORY
  R263 KXmlGui

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

To: poboiko, aacid, mart, broulik
Cc: mart, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29826: [KMainWindow] Invoke QIcon::setFallbackThemeName (later)

[Igor Poboiko]

poboiko updated this revision to Diff 83129.
poboiko added a comment.


  Don't override if fallbackThemeName is already set

REPOSITORY
  R263 KXmlGui

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29826?vs=83109=83129

BRANCH
  icon-load (branched from master)

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

AFFECTED FILES
  src/kmainwindow.cpp

To: poboiko, aacid, mart, broulik
Cc: mart, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D22488: invoke QIcon::setFallbackThemeName a bit later

[Igor Poboiko]

poboiko added a comment.


  In D22488#673544 , @aacid wrote:
  
  > Wouldn't it make more sense to fix in Qt?
  
  
  It would make more sense.
  I thought as a temporary solution it would be nice to fix it in KF as well, 
since KF releases go out more often.
  
  In D22488#673547 , @aacid wrote:
  
  > What about something like
  >
  > [...]
  
  
  Well, if it serves the purpose - why not? Have you tested it?
  (I just only have Plasma QPA platform installed :( )

REPOSITORY
  R302 KIconThemes

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

To: mart, #frameworks, #plasma
Cc: poboiko, aacid, mlaurent, broulik, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham, bruns

D29826: [KMainWindow] Invoke QIcon::setFallbackThemeName (later)

[Igor Poboiko]

poboiko added a comment.


  Thanks for looking into it! :)
  
  In D29826#673543 , @aacid wrote:
  
  > I don't think moving this code from KIconThemes to kmainwindow makes sense, 
what about all the apps that use KIconThemes but no KMainWindow?
  
  
  I'm just not aware of any. I've looked at some of the most used (by me :]), 
noted that all of them use KMainWindow and thought it might be a good option.

REPOSITORY
  R263 KXmlGui

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

To: poboiko, aacid, mart, broulik
Cc: mart, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29826: [KMainWindow] Invoke QIcon::setFallbackThemeName (later)

[Igor Poboiko]

poboiko added a comment.


  My main motivation here is following: 
https://gerrit.libreoffice.org/c/core/+/94691. 
  If it gets accepted, it would fix long-standing issue that people aren't able 
to use Libreoffice with KF5 integration plugin together with dark color scheme 
(and "breeze-dark" icon theme) out-of-the-box.
  
  See also:
  https://bugs.documentfoundation.org/show_bug.cgi?id=116683
  https://bugs.documentfoundation.org/show_bug.cgi?id=127138
  https://forum.manjaro.org/t/change-libreoffice-look-and-feel/48357
  https://bbs.archlinux.org/viewtopic.php?id=20
  https://www.reddit.com/r/kde/comments/4qqpqr/kde_dark_themes_and_libreoffice/
  
https://askubuntu.com/questions/1026103/libre-office-display-more-visible-icons-on-dark-breeze-kde-theme
  
  In short, people suggest using gtk3 VCL plugin, because it hooks with 
kde-gtk-config which exports KDE icon theme to GTK. Instead of native KF5 VCL 
plugin. What a shame!

REPOSITORY
  R263 KXmlGui

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

To: poboiko, aacid, mart, broulik
Cc: mart, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29826: [KMainWindow] Invoke QIcon::setFallbackThemeName (later)

[Igor Poboiko]

poboiko edited the summary of this revision.

REPOSITORY
  R263 KXmlGui

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

To: poboiko, aacid, mart, broulik
Cc: mart, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29826: [KMainWindow] Invoke QIcon::setFallbackThemeName (later)

[Igor Poboiko]

poboiko created this revision.
poboiko added reviewers: aacid, mart, broulik.
Herald added a project: Frameworks.
poboiko requested review of this revision.

REVISION SUMMARY
  This is alternative approach to D22488: invoke QIcon::setFallbackThemeName a 
bit later  and commit 4214045 
 to 
KIconThemes.
  Okular (and most - if not all - KDE apps inherit KMainWindow, so KDE apps
  should have breeze icons). KMainWindow ctor should be early enough so no icons
  are yet loaded, but late enough so QGuiApplication is already inited.
  
  This should be followed by reverting commit 4214045 
 in 
KIconThemes.
  
  Original problem description (by @mart):
  invoking QIcon::setFallbackThemeName at QCoreApplication ctor
  with Q_COREAPP_STARTUP_FUNCTION breaks the internal status of
  QIconLoader as it instantiates it before the QPlatformTheme,
  but QIconLoader depends from QPlatformTheme to be already instantiated
  otherwise it won't load correctly, thus breaking icon loading
  in QtQuickControls2 styles, such as Material and Fusion
  see https://bugreports.qt.io/browse/QTBUG-74252
  
  CCBUG: 402172

TEST PLAN
  Don't have GTK3 QPA plugin, so cannot test it yet.
  I would appreciate if someone helped me with testing :)

REPOSITORY
  R263 KXmlGui

BRANCH
  icon-load (branched from master)

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

AFFECTED FILES
  src/kmainwindow.cpp

To: poboiko, aacid, mart, broulik
Cc: mart, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29210: [NestedListHelper] Fix indentation of selection, add tests

[Igor Poboiko]

This revision was automatically updated to reflect the committed changes.
Closed by commit R310:7012493410ae: [NestedListHelper] Fix indentation of 
selection, add tests (authored by poboiko).
Herald added a project: Frameworks.

REPOSITORY
  R310 KTextWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29210?vs=81585=81758

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

AFFECTED FILES
  autotests/krichtextedittest.cpp
  autotests/krichtextedittest.h
  src/widgets/krichtextedit.cpp
  src/widgets/nestedlisthelper.cpp
  src/widgets/nestedlisthelper_p.h

To: poboiko, #frameworks, dfaure, mlaurent
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29208: [NestedListHelper] Improve indentation code

[Igor Poboiko]

This revision was automatically updated to reflect the committed changes.
Closed by commit R310:18a2371fe394: [NestedListHelper] Improve indentation code 
(authored by poboiko).

REPOSITORY
  R310 KTextWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29208?vs=81738=81743

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

AFFECTED FILES
  src/widgets/krichtextedit.cpp
  src/widgets/nestedlisthelper.cpp
  src/widgets/nestedlisthelper_p.h

To: poboiko, #frameworks, dfaure, mlaurent
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29208: [NestedListHelper] Improve indentation code

[Igor Poboiko]

poboiko updated this revision to Diff 81738.
poboiko added a comment.


  Improve readability as suggested, also const'ify

REPOSITORY
  R310 KTextWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29208?vs=81256=81738

BRANCH
  unused (branched from master)

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

AFFECTED FILES
  src/widgets/krichtextedit.cpp
  src/widgets/nestedlisthelper.cpp
  src/widgets/nestedlisthelper_p.h

To: poboiko, #frameworks, dfaure, mlaurent
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29210: [NestedListHelper] Fix indentation of selection, add tests

[Igor Poboiko]

poboiko added inline comments.

INLINE COMMENTS

> dfaure wrote in nestedlisthelper.cpp:225
> should the opposite happen? delete the list when decreasing indentation?
> 
> Or is that what the blkFmt.setObjectIndex(-1); is about?
> A comment there would be useful...
> 
> To me setObjectIndex(-1) sounds like it could kill any kind of embedded 
> object there but I might be wrong.

We indeed delete it when decreasing, but here `delta > 0`, which corresponds to 
increasing :)

Regarding `setObjectIndex()`: it does indeed look like magic, and I don't 
totally understand it either. It was there, however, and it is also present in 
the official Qt `TextEdit` example, where it's used when item is removed from 
the list.
I've found more explicit way: just remove the block from list and set it's 
indentation to zero, it seems to be working well.

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

To: poboiko, #frameworks, dfaure, mlaurent
Cc: kde-frameworks-devel

D29210: [NestedListHelper] Fix indentation of selection, add tests

[Igor Poboiko]

poboiko updated this revision to Diff 81585.
poboiko added a comment.


  Make the code that removes the block from the list more obvious (without 
`setObjectIndex` magic)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29210?vs=81267=81585

BRANCH
  change-indent (branched from master)

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

AFFECTED FILES
  autotests/krichtextedittest.cpp
  autotests/krichtextedittest.h
  src/widgets/krichtextedit.cpp
  src/widgets/nestedlisthelper.cpp
  src/widgets/nestedlisthelper_p.h

To: poboiko, #frameworks, dfaure, mlaurent
Cc: kde-frameworks-devel

D29208: [NestedListHelper] Improve indentation code

[Igor Poboiko]

poboiko added inline comments.

INLINE COMMENTS

> dfaure wrote in nestedlisthelper.cpp:87
> So the last block cannot be unindented? How come?

That's the current block being checked, not the next one. I've just checked to 
be sure, last block can be unindented :)

TBH, I don't really know if it's even possible for the current block to be 
invalid (that would probably mean that `textCursor()` returned by `QTextEdit` 
is invalid?). 
I've just borrowed this particular check from the old code...

REPOSITORY
  R310 KTextWidgets

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

To: poboiko, #frameworks, dfaure, mlaurent
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29210: [NestedListHelper] Fix indentation of selection, add tests

[Igor Poboiko]

poboiko created this revision.
poboiko added reviewers: Frameworks, dfaure, mlaurent.
poboiko requested review of this revision.

REVISION SUMMARY
  When we try to increase / decrease an indentation level of a selection, just 
change
  indentation level of each block in the selection, and then make sure all 
styles
  are consistent via `reformatList`.
  
  This actually fixes indentation of selection: previously all blocks had the 
same indentation after this action.
  
  Add a unit test that covers as much weird selections & indentation cases as I 
could imagine.
  
  Slightly overlaps with D29208: [NestedListHelper] Improve indentation code 
, so this one should be applied after that.

TEST PLAN
  `make && ctest`

BRANCH
  change-indent (branched from master)

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

AFFECTED FILES
  autotests/krichtextedittest.cpp
  autotests/krichtextedittest.h
  src/widgets/krichtextedit.cpp
  src/widgets/nestedlisthelper.cpp
  src/widgets/nestedlisthelper_p.h

To: poboiko, #frameworks, dfaure, mlaurent
Cc: kde-frameworks-devel

D29208: [NestedListHelper] Improve indentation code

[Igor Poboiko]

poboiko created this revision.
poboiko added reviewers: Frameworks, dfaure, mlaurent.
Herald added a project: Frameworks.
poboiko requested review of this revision.

REVISION SUMMARY
  The patch includes following improvements:
  
  1. `handleAfterKeyPressEvent` was only used to adjust margins. We don't do it 
anymore, so get rid of it.
  2. Add support for Tab key to increase indentation level (either at the 
beginning of a list item or with multiple lines selected)
  3. Fix `canIndent / canDedent` logic when cursor has a selection. `canIndent` 
should work with `topOfSelection`, and `canDedent` --- with `bottomOfSelection`.
  4. Enclose `handleOnIndentMore / Less` in `beginEditBlock / endEditBlock`, so 
they appear as a single event in Undo stack.
  5. A Return on an empty list element decreases the indentation (so 
double-Return terminates the list)

TEST PLAN
  1. Tab / Return keys now work as explained
  2. IndentMore / Less are undoable with single `Ctrl+Z`
  
  (this leads to cursor jumping around, which is probably beyond the scope of 
this patch)
  
  3. Increase / Decrease Indent actions now become enabled when they should if 
the selection is present
  
  (the behavior of `handleOnIndentMore / Less` when selection is present is 
slightly broken, which is also beyond the scope)

REPOSITORY
  R310 KTextWidgets

BRANCH
  unused (branched from master)

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

AFFECTED FILES
  src/widgets/krichtextedit.cpp
  src/widgets/nestedlisthelper.cpp
  src/widgets/nestedlisthelper_p.h

To: poboiko, #frameworks, dfaure, mlaurent
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28967: [KRichTextEditor] Add support for headings

[Igor Poboiko]

This revision was automatically updated to reflect the committed changes.
Closed by commit R263:2f86040cab5b: [KRichTextEditor] Add support for headings 
(authored by poboiko).

REPOSITORY
  R263 KXmlGui

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28967?vs=80524=80530

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

AFFECTED FILES
  tests/krichtexteditor/krichtexteditorui.rc

To: poboiko, #frameworks, mlaurent
Cc: mlaurent, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28968: [KRichTextEdit] Make sure headings don't mess with undo stack

[Igor Poboiko]

This revision was automatically updated to reflect the committed changes.
Closed by commit R310:7d3a1386bee2: [KRichTextEdit] Make sure headings 
dont mess with undo stack (authored by poboiko).

REPOSITORY
  R310 KTextWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28968?vs=80508=80528

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

AFFECTED FILES
  autotests/krichtextedittest.cpp
  src/widgets/krichtextedit.cpp

To: poboiko, #frameworks, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28968: [KRichTextEdit] Make sure headings don't mess with undo stack

[Igor Poboiko]

poboiko added inline comments.

INLINE COMMENTS

> dfaure wrote in krichtextedit.cpp:572
> Where is the corresponding beginEditBlock()?

`joinPreviousEditBlock` in a way acts as `beginEditBlock` (see 
https://doc.qt.io/qt-5/qtextcursor.html#joinPreviousEditBlock).

The intent here was following: if user pressed `Return`, a new line (empty 
`QTextBlock`) is inserted somewhere inside `QTextEdit::keyPressEvent`. I want 
to also change the cursor style, and I want this action to be treated together 
with this `QTextBlock` insertion as a single action in undo stack. That's what 
`joinPreviousEditBlock / endEditBlock` combo does (semantically, this "previous 
edit block" is `QTextBlock` insertion inside `QTextEdit::keyPressEvent`)

REPOSITORY
  R310 KTextWidgets

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

To: poboiko, #frameworks, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28966: [KRichTextEdit] Fix scroll jumping around when horizontal rule is added

[Igor Poboiko]

This revision was automatically updated to reflect the committed changes.
Closed by commit R310:0e14353610fa: [KRichTextEdit] Fix scroll jumping around 
when horizontal rule is added (authored by poboiko).

REPOSITORY
  R310 KTextWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28966?vs=80505=80525

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

AFFECTED FILES
  autotests/krichtextedittest.cpp
  autotests/krichtextedittest.h
  src/widgets/krichtextedit.cpp

To: poboiko, #frameworks, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28967: [KRichTextEditor] Add support for headings

[Igor Poboiko]

poboiko updated this revision to Diff 80524.
poboiko added a comment.


  Update version

REPOSITORY
  R263 KXmlGui

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28967?vs=80507=80524

BRANCH
  heading (branched from master)

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

AFFECTED FILES
  tests/krichtexteditor/krichtexteditorui.rc

To: poboiko, #frameworks, mlaurent
Cc: mlaurent, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28968: [KRichTextEdit] Make sure headings don't mess with undo stack

[Igor Poboiko]

poboiko created this revision.
poboiko added reviewers: Frameworks, dfaure.
Herald added a project: Frameworks.
poboiko requested review of this revision.

REVISION SUMMARY
  This patch ensures that everything related to headings is undoable
  with a single `Ctrl+Z`. It also adds tests for the following use cases
  (ensuring those are undoable with a single `undo` command):
  
  1. Make line a heading
  2. Creating a newline after a heading
  3. Merging a heading and non-heading line with `Delete` key
  4. The same, with `Backspace` key

TEST PLAN
  `make && ctest`

REPOSITORY
  R310 KTextWidgets

BRANCH
  heading-undo-stack (branched from master)

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

AFFECTED FILES
  autotests/krichtextedittest.cpp
  src/widgets/krichtextedit.cpp

To: poboiko, #frameworks, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28967: [KRichTextEditor] Add support for headings

[Igor Poboiko]

poboiko created this revision.
poboiko added a reviewer: Frameworks.
Herald added a project: Frameworks.
poboiko requested review of this revision.

REVISION SUMMARY
  Add new actions introduced in D28854: [KRichTextWidget] Add support for 
headings  to the KRichTextEditor test app.

TEST PLAN
  Run `krichtexteditor`, observe actions in menu / toolbar (which work)

REPOSITORY
  R263 KXmlGui

BRANCH
  heading (branched from master)

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

AFFECTED FILES
  tests/krichtexteditor/krichtexteditorui.rc

To: poboiko, #frameworks
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28966: [KRichTextEdit] Fix scroll jumping around when horizontal rule is added

[Igor Poboiko]

poboiko created this revision.
poboiko added reviewers: Frameworks, dfaure.
Herald added a project: Frameworks.
poboiko requested review of this revision.

REVISION SUMMARY
  Due to Qt bug 83605, it's not a good idea to `setTextCursor` while the cursor
  is inside `beginEditBlock / endEditBlock` (scrollbar might jump to the top),
  see KDE bug 195828. It's sufficient to move `setTextCursor` outside to fix it.
  (it's never too late... :)
  
  This patch also adds a test for this case (which currently fails due to
  regression, see D28819: [KRichTextEdit] Always treat key press as single 
modification in undo stack  and D28964: 
[KRichTextWidget] Remove ancient workaround and fix regression (commit 1d1eb6f) 
 for the fix).
  
  BUG: 195828

TEST PLAN
  1. Apply D28964: [KRichTextWidget] Remove ancient workaround and fix 
regression (commit 1d1eb6f) 
  2. `ctest`

REPOSITORY
  R310 KTextWidgets

BRANCH
  dont-scroll-rule (branched from master)

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

AFFECTED FILES
  autotests/krichtextedittest.cpp
  autotests/krichtextedittest.h
  src/widgets/krichtextedit.cpp

To: poboiko, #frameworks, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28964: [KRichTextWidget] Remove ancient workaround and fix regression (commit 1d1eb6f)

[Igor Poboiko]

This revision was automatically updated to reflect the committed changes.
Closed by commit R310:0494fc4a2838: [KRichTextWidget] Remove ancient workaround 
and fix regression (commit 1d1eb6f) (authored by poboiko).

REPOSITORY
  R310 KTextWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28964?vs=80502=80506

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

AFFECTED FILES
  src/widgets/krichtextedit.cpp
  src/widgets/nestedlisthelper.cpp
  src/widgets/nestedlisthelper_p.h

To: poboiko, #frameworks, dfaure, ahmadsamir
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28964: [KRichTextWidget] Remove ancient workaround and fix regression (commit 1d1eb6f)

[Igor Poboiko]

poboiko created this revision.
poboiko added reviewers: Frameworks, dfaure, ahmadsamir.
Herald added a project: Frameworks.
poboiko requested review of this revision.

REVISION SUMMARY
  This patch removes the workaround, which (according to comment) comes from Qt4
  times. Although I wasn't even able to find the original bug (only the title,
  "Remove implicit margin on bullet lists when exporting to HTML"), it would 
seem
  like the workaround is no longer actual (i.e. margins are consistent).
  
  This is an alternative approach to D28819: [KRichTextEdit] Always treat key 
press as single modification in undo stack 
, as it removes the code which
  actually cluttered the undo stack. The latter patch introduced couple
  regressions unfortunately
  (e.g. ability to "undo" a written word, not just the last letter)
  
  However, it slightly changes the behavior: it removes margins around a bullet
  list, which are probably not needed anyways (e.g. LibreOffice doesn't have it)

TEST PLAN
  1. Create a bullet list in `krichtexteditor` with couple of lines
  2. Try to undo -> a single undo is sufficient
  3. Press enter to add bunch of empty new lines, so that scroll appears
  4. Scroll still follows the cursor

REPOSITORY
  R310 KTextWidgets

BRANCH
  remove-workaround (branched from master)

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

AFFECTED FILES
  src/widgets/krichtextedit.cpp
  src/widgets/nestedlisthelper.cpp
  src/widgets/nestedlisthelper_p.h

To: poboiko, #frameworks, dfaure, ahmadsamir
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28819: [KRichTextEdit] Always treat key press as single modification in undo stack

[Igor Poboiko]

poboiko added a comment.


  In D28819#651007 , @dfaure wrote:
  
  > The *ideal* way of proceeding is to submit a Qt bug report with your 
testcase (after searching for an existing Qt bug report for the same issue), 
then turn it into a Qt autotest and make a gerrit merge request for Qt with 
both the test and a fix for it. And then, and only then, add a workaround in 
krichtextedit with a Qt version ifdef and a reference to the Qt bug report.
  >
  > The less ideal way is to only do the Qt bug report and the workaround 
here...
  
  
  Thanks for your comment! 
  I already did the first part (that is, Qt bugreport 
).
  I can play around Qt code too, and see if I can prepare a test & patch.
  Although, to be honest, I'm a bit afraid to touch it. Most likely there was 
some reasoning behind this "dirty" value, which I can't yet guess by looking at 
the code (it lacks documentation / comments) :(

REPOSITORY
  R310 KTextWidgets

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

To: poboiko, #frameworks, mlaurent, dfaure
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D28854: [KRichTextWidget] Add support for headings

[Igor Poboiko]

poboiko closed this revision.

REPOSITORY
  R310 KTextWidgets

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

To: poboiko, #frameworks, mlaurent, ahmadsamir, dfaure
Cc: abstractdevelop, kde-frameworks-devel, LeGast00n, cblack, michaelh, 
ngraham, bruns

D28819: [KRichTextEdit] Always treat key press as single modification in undo stack

[Igor Poboiko]

poboiko added a comment.


  @dfaure There seem to be a regression caused by this patch: if I reach the 
end of current view, and then press Enter so it has to scroll down, it scrolls 
to the very beginning of the document instead.
  
  This is the same issue as in ancient bug 195828 
, and it seems like it is Qt issue.
  
  Here's the simplest MWE:
  
#include 
#include 
#include 
#include 
#include 

int main(int argc, char *argv[])
{
QApplication a(argc, argv);
auto w = new QWidget();
auto edit = new QTextEdit();
auto button = new QPushButton(QStringLiteral("Insert new line"));
button->connect(button, ::clicked, [edit](){
auto cursor = edit->textCursor();
cursor.beginEditBlock();
cursor.insertBlock();
edit->setTextCursor(cursor);
cursor.endEditBlock();
});
auto layout = new QVBoxLayout(w);
layout->addWidget(edit);
layout->addWidget(button);
w->show();
return a.exec();
}
  
  Seems like the following code in `QTextCursorPrivate::setX` causes the 
trouble:
  
if (priv->isInEditBlock() || priv->inContentsChange) {
x = -1; // mark dirty
return;
}
  
  and seems like this "dirty" value is treated literally afterwards.
  
  For this example, switching `endEditBlock` and `setTextCursor` lines fixes 
the issue (and same with `Insert Rule`).
  I've also found a (dirty) workaround for the original patch: if I call 
`setTextCursor(textCursor())` right after `endEditBlock()`, it works like a 
charm. It seems like this line (although semantically is a NOOP) triggers the 
update of this "dirty" value.
  
  How should I proceed?

REPOSITORY
  R310 KTextWidgets

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

To: poboiko, #frameworks, mlaurent, dfaure
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D28854: [KRichTextWidget] Add support for headings

[Igor Poboiko]

poboiko marked 3 inline comments as done.
poboiko added a comment.


  In D28854#649189 , 
@abstractdevelop wrote:
  
  > Hey, thanks for this @poboiko  I used to have to implement this myself, so 
this will be very useful in my app, O20.Word. ;)
  
  
  Sure, you're welcome! I like doing useful stuff :)

INLINE COMMENTS

> dfaure wrote in krichtextedit.cpp:346
> If boundedLevel is 6, the size adjustement will be -1?
> Does this mean a heading that's smaller than normal text?

Yes, that's true. I agree it's a bit confusing, but that's the case with HTML 
too, which I had in mind (see screenshot, it's Chrome)
F8241667: scr1.png 

For markdown, it seems like there are only 5 levels, 5th having the same size 
as normal text (6th level gets ignored, that's Okular)
F8241670: scr2.png 

I guess we probably can follow the Markdown way and bound it with 5, just to 
avoid confusion.

> dfaure wrote in krichtextedit.cpp:375
> This seems to duplicate what happened with selectCursor already. Why are two 
> cursors necessary to change the style of one paragraph?

I used `selectCursor` to select a line (block) under cursor and then change the 
style of the selection with `mergeCharFormat` .

`cursor` is for actual users cursor, which also has to change the style, so 
newly typed text will have the same style and heading level too (I use 
`mergeBlockCharFormat` for that reason - if I use `mergeCharFormat` here, the 
cursor will remain big after I press `Enter` after a title. However, it will 
change its size to the smaller one immediately after I start typing). But I 
don't want to mess with users selection, that's why I'm keeping both.

REPOSITORY
  R310 KTextWidgets

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

To: poboiko, #frameworks, mlaurent, ahmadsamir, dfaure
Cc: abstractdevelop, kde-frameworks-devel, LeGast00n, cblack, michaelh, 
ngraham, bruns

D28854: [KRichTextWidget] Add support for headings

[Igor Poboiko]

poboiko updated this revision to Diff 80202.
poboiko added a comment.


  Added a unit-test testing for everything I could come up with 
  (at least 4 "behavior nuances" from the commit message)
  
  Address other feedback as well (const'ify, @since version)

REPOSITORY
  R310 KTextWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28854?vs=80193=80202

BRANCH
  textwidget-heading (branched from master)

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

AFFECTED FILES
  autotests/krichtextedittest.cpp
  autotests/krichtextedittest.h
  src/widgets/krichtextedit.cpp
  src/widgets/krichtextedit.h
  src/widgets/krichtextwidget.cpp
  src/widgets/krichtextwidget.h

To: poboiko, #frameworks, mlaurent, ahmadsamir, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28854: [KRichTextWidget] Add support for headings

[Igor Poboiko]

poboiko edited the test plan for this revision.

REPOSITORY
  R310 KTextWidgets

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

To: poboiko, #frameworks, mlaurent, ahmadsamir, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28854: [KRichTextWidget] Add support for headings

[Igor Poboiko]

poboiko created this revision.
poboiko added reviewers: Frameworks, mlaurent, ahmadsamir, dfaure.
Herald added a project: Frameworks.
poboiko requested review of this revision.

REVISION SUMMARY
  This patch adds support of different headings (essentially, HTML h1..h6 tags).
  Those might be pretty useful when formatting a rich text (I'm having KJots - 
note taking app - in mind)
  
  `QTextBlockFormat` supports `headingLevel` since Qt 5.12; however, as stated 
in the documentation,
  `setHeadingLevel` does not adjust style (font size, etc), so we have to take 
care of it on our own.
  
  It also adds a `KSelectAction` to choose between different headings.
  
  There are few behavior nuances:
  
  1. If user presses `Enter` at the end of heading line, cursor should switch 
to `basic text`
  2. If user presses `Enter` in the middle of the heading, line just breaks 
into two headings
  3. If user presses `Backspace` at the beginning of a line after the heading, 
the line is merged with the previous one (and thus becomes heading itself)
  4. The same with `Delete` at the end of heading line: the next line should 
become heading too.

TEST PLAN
  See the video of `KRichTextEditor` from `KXmlGui` (I had to add 
`format_heading_level` to rc-file).

REPOSITORY
  R310 KTextWidgets

BRANCH
  textwidget-heading (branched from master)

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

AFFECTED FILES
  src/widgets/krichtextedit.cpp
  src/widgets/krichtextedit.h
  src/widgets/krichtextwidget.cpp
  src/widgets/krichtextwidget.h

To: poboiko, #frameworks, mlaurent, ahmadsamir, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28819: [KRichTextEdit] Always treat key press as single modification in undo stack

[Igor Poboiko]

This revision was automatically updated to reflect the committed changes.
Closed by commit R310:1d1eb6feb7f8: [KRichTextEdit] Always treat key press as 
single modification in undo stack (authored by poboiko).

REPOSITORY
  R310 KTextWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28819?vs=80084=80164

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

AFFECTED FILES
  src/widgets/krichtextedit.cpp

To: poboiko, #frameworks, mlaurent, dfaure
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D28819: [KRichTextEdit] Always treat key press as single modification in undo stack

[Igor Poboiko]

poboiko edited the summary of this revision.

REPOSITORY
  R310 KTextWidgets

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

To: poboiko, #frameworks, mlaurent
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28819: [KRichTextEdit] Always treat key press as single modification in undo stack

[Igor Poboiko]

poboiko created this revision.
poboiko added reviewers: Frameworks, mlaurent.
Herald added a project: Frameworks.
poboiko requested review of this revision.

REVISION SUMMARY
  If my cursor is on a bullet list, and I press Enter to create a new element,
  it registers as 5-7 actions in an undo stack, mostly margins adjustments. So 
to
  rewind it, user has to press `Ctrl+Z` 5+ times, which is quite frustrating.
  
  Instead this patch suggests to treat _any_ change that is a result of a single
  key press as a single event, so user has to press `Ctrl+Z` just once to undo 
it

TEST PLAN
  1. Open KMail -> New Mail, enable Rich Text mode (or, say, open KJots)
  2. Create a bullet list with couple of elements
  3. Put cursor on the last element, press Enter to create a new element
  4. Try to undo this action with Ctrl+Z
  5. (without patch) One has to press Ctrl+Z 5-7 times to undo it.
  6. (with patch) A single Ctrl+Z is enough

REPOSITORY
  R310 KTextWidgets

BRANCH
  single-undo (branched from master)

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

AFFECTED FILES
  src/widgets/krichtextedit.cpp

To: poboiko, #frameworks, mlaurent
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D23787: [baloo_file_extractor] Improve handling of large plain-text files

[Igor Poboiko]

poboiko edited the summary of this revision.

REPOSITORY
  R293 Baloo

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

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

D23787: [baloo_file_extractor] Improve handling of large plain-text files

[Igor Poboiko]

poboiko added a comment.


  @bruns: I've missed D16593: [ExtractorCollection] Use only best matching 
extractor plugin , and had in mind previous 
situation where we've matched all extractors based on inheritance. In that 
case, "Secondly" part indeed does not seem to apply anymore.
  (as for my previous answer: I misunderstood you, thought you were asking 
about the case where `PlainTextExtractor` did not match & matched afterwards)
  
  > Your script is wrong. E.g. SVG inherits from text/plain, but has its own 
extractor, thus is not fed to the PlaintextExtractor. Dito for anything 
inheriting from XML.
  
  I'm not claiming the list to be comprehensive, it's just a first 
approximation.
  I'm claiming just that there is plethora of plain-text-based types (and might 
be even more in the future), some of which **in principle** might cause an 
issue.
  
  There were plenty of situations in the past when users first encountered 
Baloo choking on some files (see git log of `fileexcludefilters.cpp` - SQL 
dumps, genome data, etc.), which made Baloo unusable for them.
  Luckily for us, they reported it, and we blacklisted it. But I think it's 
unlikely we will manage to cover all the problematic cases that way (not all 
users report issues, and we're not familiar with all possible mimetypes).
  This patch should serve as a preventive measure, reducing the probabilty of 
Baloo choking on it in the first place.

REPOSITORY
  R293 Baloo

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

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

D23787: [baloo_file_extractor] Improve handling of large plain-text files

[Igor Poboiko]

poboiko added a comment.


  Ping?

REPOSITORY
  R293 Baloo

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

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

D23787: [baloo_file_extractor] Improve handling of large plain-text files

[Igor Poboiko]

poboiko updated this revision to Diff 67318.
poboiko added a comment.


  Minor optimization: change check order (filesize / extractor property)
  
  Also, it should be better check by "Id" instead of "Name"

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23787?vs=66805=67318

BRANCH
  improve-large-text-files (branched from master)

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

AFFECTED FILES
  src/file/extractor/app.cpp

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

D23787: [baloo_file_extractor] Improve handling of large plain-text files

[Igor Poboiko]

poboiko marked an inline comment as done.

REPOSITORY
  R293 Baloo

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

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

D23787: [baloo_file_extractor] Improve handling of large plain-text files

[Igor Poboiko]

poboiko added inline comments.

INLINE COMMENTS

> bruns wrote in app.cpp:185
> Users will love us for spammig the logs ...

I though users might actually want to know if file was excluded (and the 
reasoning behind it).
I can make its severity less, i.e. `qCDebug`. Or you think it should be 
completely removed?

REPOSITORY
  R293 Baloo

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

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

D23787: [baloo_file_extractor] Improve handling of large plain-text files

[Igor Poboiko]

poboiko added a comment.


  In D23787#537891 , @bruns wrote:
  
  > Can you please provide an example which:
  >
  > - is currently indexed though it should be skipped due to size
  > - is skipped after this change
  
  
  Sure. Any mimetype inherited from "text/plain", but starting with "text/" 
counts. I've made an actual list:
  F7515259: list.txt 
  (using simple python script, which iterates over 
`QMimeDatabase().allMimeTypes()`, checks if `type.inherits("text/plain")` and 
is not already excluded by default Baloo config from 
`file/fileexcludefilters.cpp`)
  
  By looking at list, I see that some of them might be pretty heavy (and 
useless to index). For example, `application/x-valgrind-massif`, or 
`application/sql` (I know, SQL dumps are excluded by extension `*.sql`, but 
someone might simply use another extension like `.dump`). It's also pretty easy 
to imagine large Wolfram Mathematica file, i.e. containing pictures (that 
corresponds to `application/mathematica` from the list; although on my computer 
those are detected as `application/vnd.wolfram.nb`, which for some reason do 
not inherit `text/plain`, although it's plaintext-based).
  
  We can do our best to exclude undesired types, but I'm not sure we will be 
able to cover all of them. And some files might be of desirable type, but 
simply too large (RSS feeds `application/rss+xml`, LyX files for some books 
`application/x-lyx`, mailboxes `message/rfc822` or `application/mbox`).
  
  > and another example which:
  > 
  > - is currently skipped though it should be indexed
  > - is indexed after this change
  
  There shouldn't be any. I mean, "PlaintextExtractor" should be inside 
`exList` for anything that starts with `text/`...

REPOSITORY
  R293 Baloo

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

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

D23787: [baloo_file_extractor] Improve handling of large plain-text files

[Igor Poboiko]

poboiko marked an inline comment as done.

REPOSITORY
  R293 Baloo

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

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

D23787: [baloo_file_extractor] Improve handling of large plain-text files

[Igor Poboiko]

poboiko updated this revision to Diff 66805.
poboiko added a comment.


  Address raised issue: fetch file size only once

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23787?vs=65646=66805

BRANCH
  improve-large-text-files (branched from master)

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

AFFECTED FILES
  src/file/extractor/app.cpp

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

D23787: [baloo_file_extractor] Improve handling of large plain-text files

[Igor Poboiko]

poboiko created this revision.
poboiko added reviewers: Baloo, bruns, ngraham.
Herald added projects: Frameworks, Baloo.
poboiko requested review of this revision.

REVISION SUMMARY
  First of all, not all plain text-based mimetypes starts with `text/`:
  i.e. `application/sql` for SQL dumps (already handled in FileExcludeFilters),
  or `application/postscript` for PS images. There are most likely to be more.
  Alternative solution would be using `QMimeType::inherits` instead.
  
  Secondly, not all extractors are bad with large files: for example, if it is
  a PS image, then PostScriptDSExtractor still might extract useful information.
  Issues are mostly caused by PlainTextExtractor, which generates just too much
  terms.
  
  This patch aims at tackling both issues: it just skips PlaintextExtractor for
  large files, utilizing extractor metadata introduced in D19109: [Extractor] 
Add metadata to extractors .

TEST PLAN
  1. Create large `.txt` file (>10Mb)
  2. `baloo_file_extractor` still skips it.

REPOSITORY
  R293 Baloo

BRANCH
  improve-large-text-files (branched from master)

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

AFFECTED FILES
  src/file/extractor/app.cpp

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

D23200: Fixes a crash in Peruse triggered by baloo

[Igor Poboiko]

poboiko added a comment.


  AFAIK `canonicalFilePath()` cannot return path without slash. The result is 
either empty (if path does not exist; this is covered by first `if`), or a 
proper path. The second check is redundant.

REPOSITORY
  R293 Baloo

BRANCH
  fixCrashWithEmptyFolder

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

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

D23008: [baloo_file_extractor] Be more resistant to multiple QSocketNotifier events

[Igor Poboiko]

poboiko added a comment.


  In D23008#508782 , @bruns wrote:
  
  > @poboiko - is the problem you describe purely theoretical, or did you 
actually see it happen?
  >
  > Multiple triggers of `slotNewInput` are **not** a problem, the code flow 
reaches the `m_tr = new Transaction(...)` only iff a batch has been read. And 
batches are only issued when the  previous one has been signaled completed with 
the 'B' batch end marker.
  
  
  To be completely honest, it //is// theoretical. I do see it should not 
happen. Provided everything works as expected.
  There were just too many examples (even inside baloo), when critical things 
didn't work as expected, and got silently ignored due to asserts. I thought it 
won't hurt getting rid of those, replacing them with proper checks. Even if 
it's checks of stuff that's impossible.
  
  I did see extractor hanging though, without reliable way to reproduce it, and 
didn't have time to debug it when it happened.
  That's why I've started lurking around extractors code in the first place, 
looking for stuff like that.

REPOSITORY
  R293 Baloo

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

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

D23008: [baloo_file_extractor] Be more resistant to multiple QSocketNotifier events

[Igor Poboiko]

poboiko added inline comments.

INLINE COMMENTS

> anthonyfieroni wrote in app.cpp:76-81
> processNextFile should be called till m_ids ends, why it does not happen, 
> probably that's why @bruns reject your patch

Well, if that happens, something went wrong, and we have to do something.
As far as I can see, the options are:

1. Ignore new batch (that would be simply `return`)
2. Ignore old batch (that is done now - committing something that was extracted 
from the old one if there is any)
3. Try to merge batches and process everything (that would be `m_ids.append(new 
batch)` and do not create a new transaction). This might require some 
additional housekeeping though, as we do not want resulting transaction to be 
larger than `batchSize`. And we probably do not want to do our own splitting 
here - as it would duplicate work done in the parent `baloo_file` process.

However, since ignored batch do not get removed from `ContentIndexingDB`, our 
parent process `baloo_file` will retry those documents eventually. I believe it 
should be pretty safe to just drop one of the batches here.

> broulik wrote in app.h:50
> It doens't have an EXPORT macro to it, so I suppose not?

It's not. It's only used inside `main.cpp` of `baloo_file_extractor`.

REPOSITORY
  R293 Baloo

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

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

D22942: [kded/baloosearchmodule] Remove useless qDebug messages

[Igor Poboiko]

poboiko closed this revision.

REPOSITORY
  R293 Baloo

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

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

D23008: [baloo_file_extractor] Be more resistant to multiple QSocketNotifier events

[Igor Poboiko]

poboiko created this revision.
poboiko added reviewers: Baloo, bruns, ngraham.
Herald added projects: Frameworks, Baloo.
poboiko requested review of this revision.

REVISION SUMMARY
  When a new batch arrives at stdin (as notified by QSocketNotifier), extractor
  creates a new transaction. If code, which reacts to notification, gets 
executed
  multiple times, we end up having two write transaction, which is a bad idea.
  
  Current implementation disables `QSocketNotifier`, so that we won't receive 
new
  notifications. It blocks further notifications, but it does not eliminate the
  possibility when several notifications already appeared in the event queue
  (which can happen due to race condition).
  It shouldn't happen, normally, but the only guard for that case currently is a
  single `Q_ASSERT`, which gets silently ignored in the non-debug build.
  
  This patch replaces assert with proper check, which instead commits & deletes
  the transaction if there is any (just in case).
  It also removes attempts to open the database multiple times (for each batch),
  which is a bad idea, as LMDB documentation suggests. These attempts will be
  ignored anyway (because of the check inside Database::open), yet semantically
  it's better to move the code outside, to the main(), as it is in `baloo_file`.

TEST PLAN
  I didn't manage to cause such race condition "in the laboratory environment", 
  so I've only checked it does not break anything

REPOSITORY
  R293 Baloo

BRANCH
  extractor-transaction (branched from master)

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

AFFECTED FILES
  src/file/extractor/app.cpp
  src/file/extractor/app.h
  src/file/extractor/main.cpp

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

D22942: [kded/baloosearchmodule] Remove useless qDebug messages

[Igor Poboiko]

poboiko created this revision.
poboiko added reviewers: Baloo, bruns, ngraham.
Herald added projects: Frameworks, Baloo.
poboiko requested review of this revision.

REVISION SUMMARY
  Those messages are not informative anyway; they are categorized as "kdeinit5";
  and they tend to spam logs during indexing with filenames of indexed files.

TEST PLAN
  make

REPOSITORY
  R293 Baloo

BRANCH
  no-qdebug (branched from master)

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

AFFECTED FILES
  src/kioslaves/kded/baloosearchmodule.cpp

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

D22557: [IndexCleaner] ignore non-existent entries inside config

[Igor Poboiko]

poboiko closed this revision.

REPOSITORY
  R293 Baloo

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

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

D22502: [FileIndexerConfig] skip invalid entries from included/excludedFolders

[Igor Poboiko]

poboiko edited the summary of this revision.

REPOSITORY
  R293 Baloo

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

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

D22557: [IndexCleaner] ignore non-existent entries inside config

[Igor Poboiko]

poboiko created this revision.
poboiko added reviewers: Baloo, bruns, ngraham.
Herald added projects: Frameworks, Baloo.
poboiko requested review of this revision.

REVISION SUMMARY
  If a folder was added to include/excludeFolders and then removed,
  there will be a stale entry in the config, for which `filePathToId`
  yields 0. When passed to `removeRecursively`, it might reinterpret it
  as tree root, which is not something we want (although there are
  checks for it inside `WriteTransaction`)
  
  See also D22502: [FileIndexerConfig] skip invalid entries from 
included/excludedFolders 

REPOSITORY
  R293 Baloo

BRANCH
  no-indexcleaner-assert (branched from master)

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

AFFECTED FILES
  src/file/indexcleaner.cpp

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

D22502: [FileIndexerConfig] skip invalid entries from included/excludedFolders

[Igor Poboiko]

poboiko added a comment.


  In D22502#496812 , @bruns wrote:
  
  > The correct fix is to check the returned/calculated ID in the IndexCleaner, 
otherwise its racy.
  
  
  I think both can be implemented. 
  I had in mind the case when folder was removed while baloo wasn't running, 
and thus encountering this issue on the next run.

REPOSITORY
  R293 Baloo

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

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

D22502: [FileIndexerConfig] skip invalid entries from included/excludedFolders

[Igor Poboiko]

poboiko created this revision.
poboiko added reviewers: Baloo, bruns, ngraham.
Herald added projects: Frameworks, Baloo.
poboiko requested review of this revision.

REVISION SUMMARY
  It is possible to create an invalid entry inside includedFolders, for example:
  by creating some folder, adding it to config and then deleting it.
  
  If such entry appears inside config, for example, `IndexCleaner` will go mad:
  it calculates the id of each entry from the config (which for non-existent
  file resolves to 0), and then calls `tr.removeRecursively(0, shouldDelete)`.
  Which either crashes due to assert (if it's a debug build) or silently runs
  `removeRecursively` for the whole tree.
  
  This patch omits invalid entries when building folders cache inside config,
  and adds unit test for such case

TEST PLAN
  ctest

REPOSITORY
  R293 Baloo

BRANCH
  removed-include-folder (branched from master)

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

AFFECTED FILES
  autotests/unit/file/fileindexerconfigtest.cpp
  autotests/unit/file/fileindexerconfigtest.h
  src/file/fileindexerconfig.cpp

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

D21427: Always skip trailing slashes in FilderedDirIterator

[Igor Poboiko]

poboiko added a comment.


  In D21427#494174 , @bruns wrote:
  
  > In D21427#494010 , @poboiko 
wrote:
  >
  > > Ping!
  > >
  > > Apparently, it does fix bug 409257, which is pretty serious one (db 
corruption, after all).
  >
  >
  > The DB corruption is already fixed in KF5.60.
  
  
  I saw it in master and was wondering if it ended up in 5.60 (and whether it's 
still possible to corrupt it in any other way).
  
  OK, DB corruption apart, I believe this patch is still relevant - we rely on 
paths not having trailing slash in different parts of the code anyways. And it 
clearly doesn't work as expected.

REPOSITORY
  R293 Baloo

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

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

D22392: [balooctl/baloo_file_extractor] Consolidate code that performs actual indexing

[Igor Poboiko]

poboiko created this revision.
poboiko added reviewers: Baloo, bruns, ngraham.
Herald added projects: Frameworks, Baloo.
poboiko requested review of this revision.

REVISION SUMMARY
  The file contents can be indexed in two ways: either by `baloo_file_extractor`
  after being added to `ContentIndexngDB`, or by calling `balooctl index fname`.
  The code is duplicated, yet with slight difference, between two places.
  
  This patch introduces and `Indexer` class, which is shared between extractor
  and `balooctl`, and does all the necessary routine in a unified way.
  
  It also removes checks if the file is already inside `ContentIndexingDB` from
  `balooctl index` command, and performs indexing even if it is (removing it
  from `ContentIndexingDB` afterwards), which is useful for debugging 
extractors.
  
  Note that as it unifies behavior of `extractor` and `balooctl`, it also now
  explicitly forbids indexing of files that are excluded or has invalid
  mimetypes via `balooctl index`; which is useless anyways, because such files
  will be removed from the index by `IndexCleaner`.
  Now it prints "failed" message to the user in that case

TEST PLAN
  - It compiles.
  - `balooctl index fname` no longer tells useless "the file is already 
scheduled for indexing" message, reindexing file anyways.

REPOSITORY
  R293 Baloo

BRANCH
  indexer-consolidate (branched from master)

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

AFFECTED FILES
  src/file/CMakeLists.txt
  src/file/extractor/CMakeLists.txt
  src/file/extractor/app.cpp
  src/file/extractor/app.h
  src/file/indexer.cpp
  src/file/indexer.h
  src/tools/balooctl/CMakeLists.txt
  src/tools/balooctl/indexer.cpp
  src/tools/balooctl/indexer.h
  src/tools/balooctl/main.cpp

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

D21427: Always skip trailing slashes in FilderedDirIterator

[Igor Poboiko]

poboiko added a comment.


  Ping!
  
  Apparently, it does fix bug 409257, which is pretty serious one (db 
corruption, after all).

REPOSITORY
  R293 Baloo

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

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

D21427: Always skip trailing slashes in FilderedDirIterator

[Igor Poboiko]

poboiko edited the summary of this revision.

REPOSITORY
  R293 Baloo

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

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

D22166: [AdvancedQueryParser] Introduce support for phrase queries

[Igor Poboiko]

poboiko added a comment.


  Note that it somewhat duplicates work done inside `QueryParser`. Which is 
//almost// not used anywhere - most of the parsing is done by 
`AdvancedQueryParser`.
  
  I feel somewhat weird having two separate parsers, as well as two different 
entities which store parsed query - i.e. `EngineQuery` and `Term`. 
  Do we really need them both?

REPOSITORY
  R293 Baloo

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

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

D22166: [AdvancedQueryParser] Introduce support for phrase queries

[Igor Poboiko]

poboiko created this revision.
poboiko added reviewers: Baloo, ngraham, bruns.
Herald added projects: Frameworks, Baloo.
poboiko requested review of this revision.

REVISION SUMMARY
  AdvancedQueryParser's lexxer can actually handle double quotes:
  for query `a "b c" d`, it returns three tokens (`a`, `b c`, `d`).
  However, when building `Term`, all of them end up having `Auto` comparator,
  which for strings then resolves to `Contains`.
  Finally, inside `SearchStore::constructContainsQuery`, we simply split such
  multi-word term and build an `EngineQuery::StartsWith` query for `b` and `c`.
  
  This patch sets `Equal` comparator for those terms that are inside double 
quotes.
  For that we call `SearchStore::constructEqualsQuery`, which treats it as
  `EngineQuery::Phrase`, which is precicely what we want.
  
  Code-wise, it's more convenient to create a separate class `Token` (instead of
  treating tokens as `QString`), which is aware if this token is inside double 
quotes.
  This patch also moves some of the token-processing routine inside this class
  (see `toVariant`, `toComp`, `toOp` methods)
  
  Also, introduce various tests with double-quoted queries.

TEST PLAN
$ ctest
$ echo "some unique phrase" > ~/test
$ baloosearch '"some unique phrase"'
  
  (only `~/test` should pop up)

REPOSITORY
  R293 Baloo

BRANCH
  phrasesearch

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

AFFECTED FILES
  autotests/unit/lib/advancedqueryparsertest.cpp
  src/lib/advancedqueryparser.cpp

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

D21427: Always skip trailing slashes in FilderedDirIterator

[Igor Poboiko]

poboiko edited the summary of this revision.

REPOSITORY
  R293 Baloo

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

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

D21427: Always skip trailing slashes in FilderedDirIterator

[Igor Poboiko]

poboiko added a reviewer: ngraham.
poboiko added a comment.


  Not entirely sure, but bug 409257 
 might be caused by that (at least 
in my case, it looked the same).

REPOSITORY
  R293 Baloo

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

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

D21427: Always skip trailing slashes in FilderedDirIterator

[Igor Poboiko]

poboiko added a comment.


  Ping.
  
  I've found a way to reproduce a related issue:
  
$ mkdir ~/test
$ balooctl config add includeFolders ~/test
$ balooctl stop

$ balooctl start
  
  This prints an error:
  
replace called with invalid arguments, docId:  url: "~/test/"
  
  The problem is the same: `DocumentUrlDB` returns a path without trailing 
slash, but `FilteredDirIterator` returns a path with one.
  `WriteTransaction` thinks the path has changed, tries to replace it, calls 
`DocumentUrlDB::replace`, which fails because it doesn't want to work with path 
which has trailing slash.
  
  In general, it's not a serious issue: we have problems only for folders that 
are inside `includeFolders`. If such folder wasn't renamed, then we don't care 
about DocUrlDB::replace failure anyway.
  If it was renamed, most likely it's not inside `includeFolders` anymore. 
However, we can do something like
  
$ mkdir ~/test1
$ mkdir ~/test2
$ balooctl config add includeFolders ~/test{1,2}
$ touch ~/test1/somefile
$ balooctl stop
$ rm -rf ~/test2
$ mv ~/test2 ~/test1
$ balooctl start
  
  the rename then gets silently ignored (file `somefile` doesn't pop up in 
searches; if we do `balooshow -x `, it returns an invalid path).

REPOSITORY
  R293 Baloo

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

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

D21509: [UnIndexedFileIteratorTest] Add tests

[Igor Poboiko]

poboiko retitled this revision from "[baloo_file] Index renamed folders inside 
UnindexedFileIndexer" to "[UnIndexedFileIteratorTest] Add tests".
poboiko edited the summary of this revision.

REPOSITORY
  R293 Baloo

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

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

D21509: [baloo_file] Index renamed folders inside UnindexedFileIndexer

[Igor Poboiko]

poboiko updated this revision to Diff 59951.
poboiko added a comment.


  Rebase on master.
  
  Since we now reindex folder always when cTime changes, revert `nameChanged()` 
routine
  Cover the case when we feed `UnIndexedFileIterator` with path that ends with 
`/`

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21509?vs=59099=59951

BRANCH
  unindexed-renamed

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

AFFECTED FILES
  autotests/unit/file/unindexedfileiteratortest.cpp

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

D21839: [TermGenerator] Use UTF-8 ByteArray for termList

[Igor Poboiko]

poboiko added a comment.


  > As the limit is somewhat arbitrary, maybe we can just limit the QString? I 
don't think this has any serious side effects.
  
  Yep, that's what I've suggested (if I understood you correctly).
  I guess, we can put the trimming part right to `termList()`, it will further 
reduce code repetition. Something like
  
str = str.left(maxTermSize); 
list << str.toUtf8();

REPOSITORY
  R293 Baloo

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

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

D21839: [TermGenerator] Use UTF-8 ByteArray for termList

[Igor Poboiko]

poboiko added a comment.


  Actually, there is an issue with that code right now, which I wanted to fix, 
but forgot.
  The trimming part `finalArr = finalArr.mid(0, maxTermSize);` actually should 
be performed on `QString` instead of `QByteArray` - unicode symbols inside term 
can consist of two bytes, and cutting at `maxTermSize` bytes can actually cut 
half of symbols. I end up with terms like `тождественно�` inside `balooshow -x`.
  Not to mention that russian terms end up being pretty small.

REPOSITORY
  R293 Baloo

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

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

D21673: [FileIndexScheduler] Ensure indexer is not run in suspended state

[Igor Poboiko]

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


  Apart from small nitpick, I think it's fine.

INLINE COMMENTS

> fileindexscheduler.h:131
>  bool m_isGoingIdle;
> +bool m_isSuspended = false;
>  };

I think it's inconvenient to scatter default values all around various places - 
just initialize it in the constructor like everything else.

REPOSITORY
  R293 Baloo

BRANCH
  scheduler

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

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

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

[Igor Poboiko]

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


  Can we also cover this case with tests?

REPOSITORY
  R293 Baloo

BRANCH
  cleanup

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

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

D21671: [FileIndexScheduler] Stop the indexer when quit() is called via DBus

[Igor Poboiko]

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


  That's a nice catch!

REPOSITORY
  R293 Baloo

BRANCH
  scheduler

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

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

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

[Igor Poboiko]

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

REPOSITORY
  R293 Baloo

BRANCH
  basicindexingjob

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

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

D21704: [FirstRunIndexer] Use correct mimetype for folders

[Igor Poboiko]

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

REPOSITORY
  R293 Baloo

BRANCH
  cleanup

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

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

D21698: Move invariant IndexingLevel out of the loop

[Igor Poboiko]

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

REPOSITORY
  R293 Baloo

BRANCH
  invariants

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

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

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

[Igor Poboiko]

poboiko added inline comments.

INLINE COMMENTS

> modifiedfileindexer.cpp:68
>  QFileInfo fileInfo(filePath);
> -
> -// A folders mtime is updated when a new file is added / removed / 
> renamed
> -// we don't really need to reindex a folder when that happens
> -// In fact, we never need to reindex a folder
> -if (timeInfo.mTime && fileInfo.isDir()) {
> +if (fileInfo.isSymLink()) {
>  continue;

Does it also changes behavior of ModifiedFileIndexer? What happened to symlinks 
before?

REPOSITORY
  R293 Baloo

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

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

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

[Igor Poboiko]

poboiko added a comment.


  I thought about it myself. I googled it a bit (i.e. here 
) and saw 
that there might be some quite unwanted runtime overhead because of using 
`std::function`. It might be negligible (since we're doing some costly DB 
operations inside anyways), but I'd prefer if we did some profiling to make 
sure it's OK.

REPOSITORY
  R293 Baloo

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

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

D21672: [PowerStateMonitor] Be conservative when determining power state

[Igor Poboiko]

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


  Makes sense to me

REPOSITORY
  R293 Baloo

BRANCH
  scheduler

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

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

D21579: [FilteredDirIterator] Avoid RegExp overhead for exact matches

[Igor Poboiko]

poboiko added inline comments.

INLINE COMMENTS

> regexpcache.cpp:60
> +}
>  f.replace(QLatin1Char('.'), QStringLiteral("\\."));
>  f.replace(QLatin1Char('?'), QLatin1Char('.'));

A quick note for future. There is 
`QRegularExpression::wildcardToRegularExpression()`, which apparently does 
precicely this.
It is there since Qt 5.12 though, while we depend on Qt 5.10. But Plasma 
already bumped dependency to 5.12, so so we can change it when Frameworks will 
follow this bump.

REPOSITORY
  R293 Baloo

BRANCH
  submit_unindexed

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

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

D21578: [UnindexedFileIterator] Delay mimetype determination until it is needed

[Igor Poboiko]

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

REPOSITORY
  R293 Baloo

BRANCH
  submit_unindexed

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

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

D21577: [UnindexedFileIndexer] Skip filetime checks for new files

[Igor Poboiko]

poboiko added a comment.


  I didn't know `QFileInfo` fetches information on demand (and caches it). That 
is the reason for this change, right?
  I think it would be nice to elaborate on that in summary, or maybe as a brief 
comment in the code.

INLINE COMMENTS

> unindexedfileiterator.cpp:109
> +} else {
> +timeInfo = m_transaction->documentTimeInfo(fileId);
> +if (timeInfo.mTime != fileInfo.lastModified().toSecsSinceEpoch()) {

Why do we need it here?

REPOSITORY
  R293 Baloo

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

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

D21576: [UnindexedFileIndexer] Do not try to add nonexistant file to index

[Igor Poboiko]

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

REPOSITORY
  R293 Baloo

BRANCH
  submit_unindexed

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

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

D21509: [baloo_file] Index renamed folders inside UnindexedFileIndexer

[Igor Poboiko]

poboiko added inline comments.

INLINE COMMENTS

> bruns wrote in unindexedfileiteratortest.cpp:100
> Make this plain members, not pointers.
> Also, one temporary dir is enough, you can put the db and the test tree 
> side-by-side.

I wanted both DB and directory tree to be recreated from scratch for each test, 
since tests modify DB or rename folders, and thus can affect each other. That's 
why I made them to be pointers.

> unindexedfileiterator.cpp:118
>  auto fileMTime = fileInfo.metadataChangeTime().toTime_t();
>  if (timeInfo.cTime != fileMTime) {
>  m_cTimeChanged = true;

I wanted to note that this line is messed up a bit: either `fileMTime` should 
be renamed to `fileCTime`, or even removed, just like three lines above. 
(it looks like the variable was introduced some time ago inside `#ifdef QT < 
5.10.0`, which was removed recently with Qt dependency bump).

Since that's just some cosmetic change, I can change it in this patch as well. 
Or should I make a new one?

> bruns wrote in unindexedfileiterator.cpp:122
> thats wrong:
> 
>   $:/tmp> touch dir
>   $:/tmp> stat dir
> File: dir
> Size: 0   Blocks: 0  IO Block: 4096   regular empty 
> file
>   Device: 3fh/63d Inode: 1892961 Links: 1
>   Access: (0644/-rw-r--r--)  Uid: ( 1000/ sbruens)   Gid: (  100/   users)
>   Access: 2019-06-03 19:21:57.764626372 +0200
>   Modify: 2019-06-03 19:21:57.764626372 +0200
>   Change: 2019-06-03 19:21:57.764626372 +0200
>Birth: 2019-06-03 19:21:57.764626372 +0200
>   $:/tmp> mv dir  dir_renamed
>   $:/tmp> stat dir_renamed 
> File: dir_renamed
> Size: 0   Blocks: 0  IO Block: 4096   regular empty 
> file
>   Device: 3fh/63d Inode: 1892961 Links: 1
>   Access: (0644/-rw-r--r--)  Uid: ( 1000/ sbruens)   Gid: (  100/   users)
>   Access: 2019-06-03 19:21:57.764626372 +0200
>   Modify: 2019-06-03 19:21:57.764626372 +0200
>   Change: 2019-06-03 19:22:07.096601727 +0200
>Birth: 2019-06-03 19:21:57.764626372 +0200

Right, sorry, I've mixed them up. Damn russian locale.

> bruns wrote in unindexedfileiterator.cpp:130
> The comment does not match - the mtime changes when something **in** the 
> directory is modified, when the directory is renamed, the ctime changes.

The comment is not mine - but that's precisely what it says. We don't want to 
reindex a directory every time something inside it modifies.

REPOSITORY
  R293 Baloo

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

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

D21509: [baloo_file] Index renamed folders inside UnindexedFileIndexer

[Igor Poboiko]

poboiko updated this revision to Diff 59099.
poboiko marked 5 inline comments as done.
poboiko added a comment.


  Use single temp dir
  
  Fixed `mTime -> cTime`

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21509?vs=59071=59099

BRANCH
  unindexed-renamed

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

AFFECTED FILES
  autotests/unit/file/unindexedfileiteratortest.cpp
  src/file/unindexedfileindexer.cpp
  src/file/unindexedfileiterator.cpp
  src/file/unindexedfileiterator.h

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

D21509: [baloo_file] Index renamed folders inside UnindexedFileIndexer

[Igor Poboiko]

poboiko updated this revision to Diff 59071.
poboiko added a comment.


  Fixed comment with directory structure

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21509?vs=59070=59071

BRANCH
  unindexed-renamed

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

AFFECTED FILES
  autotests/unit/file/unindexedfileiteratortest.cpp
  src/file/unindexedfileindexer.cpp
  src/file/unindexedfileiterator.cpp
  src/file/unindexedfileiterator.h

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

D21509: [baloo_file] Index renamed folders inside UnindexedFileIndexer

[Igor Poboiko]

poboiko added inline comments.

INLINE COMMENTS

> bruns wrote in unindexedfileiterator.cpp:126
> This whole block belongs into a separate block, executed `if 
> (m_cTimeChanged)`.

Did you mean `m_mTimeChanged`?

REPOSITORY
  R293 Baloo

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

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

D21509: [baloo_file] Index renamed folders inside UnindexedFileIndexer

[Igor Poboiko]

poboiko updated this revision to Diff 59070.
poboiko marked 3 inline comments as done.
poboiko added a comment.


  Moved `m_nameChanged` check inside separate block

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21509?vs=59068=59070

BRANCH
  unindexed-renamed

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

AFFECTED FILES
  autotests/unit/file/unindexedfileiteratortest.cpp
  src/file/unindexedfileindexer.cpp
  src/file/unindexedfileiterator.cpp
  src/file/unindexedfileiterator.h

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

D21509: [baloo_file] Index renamed folders inside UnindexedFileIndexer

[Igor Poboiko]

poboiko updated this revision to Diff 59068.
poboiko added a comment.


  Split test to three separate test functions, which cover different test cases.
  
  Renamed folders to something more meaningful; put all the names as static 
  consts in the very beginning, for further reuse inside test functions.
  
  Removed useless mimetype detection and use "text/plain" explicitly

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21509?vs=58934=59068

BRANCH
  unindexed-renamed

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

AFFECTED FILES
  autotests/unit/file/unindexedfileiteratortest.cpp
  src/file/unindexedfileindexer.cpp
  src/file/unindexedfileiterator.cpp
  src/file/unindexedfileiterator.h

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

D21509: [baloo_file] Index renamed folders inside UnindexedFileIndexer

[Igor Poboiko]

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

REVISION SUMMARY
  If some folder was renamed while `baloo_file` was not running, it should pop 
up in `UnIndexedFileIterator`.
  However, it does not, because previously it was thought that folder should 
never be reindexed.
  It is not correct: it should be reindexed if its name was changed.
  As a consequence, we end up with invalid entries inside DB (DocumentUrl is 
invalid).
  
  Also, added unit test for `UnIndexedFileIterator`, which covers basic cases 
(including this one).
  Right now it fails due to D21427: Always skip trailing slashes in 
FilderedDirIterator  (one of the folders 
has trailing slash, while it should not)
  
  Also, since we need to check if name was changed already inside iterator, we 
can export this information
  and reuse it from `UnindexedFileIndexer`.

TEST PLAN
  1. Apply also patch from D21427: Always skip trailing slashes in 
FilderedDirIterator .
  2. ctest

REPOSITORY
  R293 Baloo

BRANCH
  master

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

AFFECTED FILES
  autotests/unit/file/unindexedfileiteratortest.cpp
  src/file/unindexedfileindexer.cpp
  src/file/unindexedfileiterator.cpp
  src/file/unindexedfileiterator.h

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

D21440: Start baloo_file later

[Igor Poboiko]

poboiko added a comment.


  In D21440#471144 , @broulik wrote:
  
  > The idle tracking is only in the extractor process, not in baloo_file 
itself. On startup it runs the `UnindexedFileIndexer` and iterates all the 
folders looking for files to re-index, consuming a considerable amount of CPU 
time, spending most of its time doing regexp matching, mime type determination, 
and date time processing. Only after that it may run the extractor process when 
there's new files to be indexed.
  >  So I think starting `baloo_file` later is safe since it checks all the 
files anyway? Otherwise/additionally, we should look into making the 
`UnindexedFileIndexer` start delayed.
  
  
  I think it should be pretty safe to start `baloo_file` later. 
  The very reason to add `UnindexedFileIndexer` was to make sure we index those 
files which were changed/added when Baloo wasn't running (as well as 
`IndexCleaner` to take care of files which were removed).

REPOSITORY
  R293 Baloo

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

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

D21427: Always skip trailing slashes in FilderedDirIterator

[Igor Poboiko]

poboiko edited the summary of this revision.

REPOSITORY
  R293 Baloo

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

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

D21427: Always skip trailing slashes in FilderedDirIterator

[Igor Poboiko]

poboiko edited the summary of this revision.

REPOSITORY
  R293 Baloo

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

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

D21427: Always skip trailing slashes in FilderedDirIterator

[Igor Poboiko]

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

REVISION SUMMARY
  I've encountered weird regression. Here is the description:
  
  1. Since some time, `config->includeFolders()` contains folders with trailing 
slashes.
  2. First entry that `QDirIterator::path()` returns matches exactly its 
argument, i.e. if we feed it with path with trailing slash,
  
  it will return exactly it. All the other paths don't have trailing slashes 
though.
  
  3. Because of that, inside `UnindexedFileIndexer::run()`, when we check 
whether path has changed, we perform comparison `it.filePath() == 
tr.documentUrl(id)`,
  
  and it fails becaue `documentUrl()` always returns path without trailing 
slash.
  
  4. So we call `DocumentUrlDB::replace()`, which removes old entry and calls 
`DocumentUrlDB::put()`.
  5. Finally, for some reason, `IdTreeDB` don't want to work with paths with 
trailing slashes.
  
  So in the end of the day we get entry for `includeFolder` removed from DB. 
Which corrupts `IdTreeDB` --- it can no longer resolve paths.
  The simplest solution proposed here is to make sure our `DirIterator` always 
returns paths without trailing slash, including first call.

TEST PLAN
  It compiles. As far as I can see, it also fixes the regression.
  Although, for some reason, this regression it's not always reproducible, so I 
cannot be completely sure. Would appreciate if someone else looked into it.

REPOSITORY
  R293 Baloo

BRANCH
  trailingSlash

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

AFFECTED FILES
  src/file/filtereddiriterator.cpp

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

D15983: React to config updates inside indexer

[Igor Poboiko]

poboiko closed this revision.

REPOSITORY
  R293 Baloo

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

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

D15983: React to config updates inside indexer

[Igor Poboiko]

poboiko added a comment.


  In D15983#434247 , @ngraham wrote:
  
  > Can you rebase this on master again? Sorry for the radio silence. :(
  
  
  Nevermind, I also kinda forgot about this :)
  Seems like this patch still applies to current master:
  
# git rebase master
Current branch arcpatch-D15983 is up to date.

REPOSITORY
  R293 Baloo

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

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