I'll be AFK for a week for vacation. Will do a proper review once I'm back ;)

Wow, this is quite a radical change :)

Can you try to build another kdeinit app (i.e. khelpcenter) to check if you get the same error?

Btw I think I spotted a regression:

Got the following errore while pushing:

Thanks

In D22420#503573, @AndreyYashkin wrote:

In D22420#503317, @elvisangelaccio wrote:

I'd still like to understand what happend at the change in 4e40fe810d324.

@ngraham Any idea?

@elvisangelaccio, I read the description of change in 4e40fe810d324 and bug 298467, which was fixed in it more carefull and understood that the change was supposed to introduce the feature of setting the focus back to the active view. However, it seems that Ctrl-D or exit command cases were not tested good enough.

Please push to master or wait after August 15th (Applications 19.08 Release day) if you need this in the stable branch too.

Thanks, LGTM now.

In D22444#496089, @elvisangelaccio wrote:

Please use a descriptive commit message: https://community.kde.org/Policies/Commit_Policy#Always_add_descriptive_log_messages

There seems to be another issue: if I run dolphin <some folder> from cmd line I always get Add 'file' to place, no matter what the folder is.

Reverted this commit and then pushed diff 61904.

Note that the commit message will need BUG: (with the colons) in order to automagically close the bug report.

In R318:dc586ada63d1822fbbe38a0dd3449c73b03928e0#3803979, @AndreyYashkin wrote:

@elvisangelaccio, you have forgotten or I did not say clear enough that it is the previous cosmetic cosmetic version(Diff 61904) that was correct! Read my remark again in the revision conversation, please. @hein did not respond and I cannot say even if this one is fully correct for Korean. The one line solution does groupping of some Cyrillic symbols, which cannot be grouped toggether(aleast in Russian dictionary)
This was my fall. Sorry again for it.

I'd still like to understand what happend at the change in 4e40fe810d324.

Thanks, going to push to 19.08 :)

Well, I like that we are replacing 20+ lines of code with only one ;)

In D22594#498890, @ngraham wrote:

As for the feature itself, for the past few years I'd been opposed to this, on the basis that people should just use the built-in Baloo-based search instead. But unfortunately the requests keep coming in

Code looks good, but I spotted a regression: 'Add to Places' is now disabled if you don't select any folder. Currently it is possible to add the current folder as place without the need for any selection.

LGTM. Feel free to push on 19.08.

I like this solution better. I'll push on master only for now, and we'll see if we break something ;)

Can you expland a bit the commit message explaining how does libkgapi use the filename?

Please push to master. Thanks!

I'd go with option 1 ;)

In D22303#497268, @AndreyYashkin wrote:

Previous Diff 61904 was correct.

Sorry for the delay. See the inline comment.

In D22265#491249, @kossebau wrote:

This runs the chance to break some 3rd-party software which also calls the Q_DECLARE_METATYPE(KPluginMetaData) macro.
Any chance this could be moved to the place that needs this for now, and perhaps add a KF6 TODO instead?

See e.g. R36:3aacbbaab50ac6e0557c5d69c430459eb3d71ad7 which was needed to unbreak ark builds.

Thanks for the patch! The fix looks correct, but I have suggestions for the coding style.

Sorry, I missed this one. I'd say let's postpone to 19.12...

In D22303#492247, @AndreyYashkin wrote:

In D22303#491971, @elvisangelaccio wrote:

Thanks for the patch!

Given that the correctness of this code depends on the locale, I'm not confident we won't break some corner cases.
Ideally we'd need more unit tests in KFileItemModelTest::testNameRoleGroups(), but I understand that's a lot to ask.

@cfeck in the bug report suggested to add more letters ranges. @AndreyYashkin Did you try that?

As I understand the puprose of vector "lettersAtoZ" is to put words like "Ottava" and german "Österreich" in one latin "O" group. It seems to be some feature of german language that was taken into account by the author of code. I cannot speak for all who use cyrillic script, but in russian we do not expect anything like this. Thatwhy I discarded the idea of one more range. In addition, it may be wrong to apply such behavior for all languages that use extended latin alphabets.

QCollator thinks that cyrillic symbols are less than latin characters, while chinese and others are bigger. For this reason, it is necessary to put them in a new group without searching them in the vector or they will be putted in latin "A" group.

Please use a descriptive commit message: https://community.kde.org/Policies/Commit_Policy#Always_add_descriptive_log_messages

Thanks :)

This issue was supposed to be fixed by commit 4e40fe810d324 but it seems it got reintroduced in the past months.

Hmm, this feels like a work-around because we initialize m_active(true) everywhere else (DolphinView, DolphinSearchBox).

Thanks for the patch!

In D21878#491061, @aspotashev wrote:

src/settings/services/servicemenuinstaller/servicemenuinstaller.cpp
35–42
Since we are in a Qt program now, we could just open a KMessageBox rather than spawn another process just to show an error dialog (and kdialog might not even be installed).

71–77
Same here. We should use QMimeDatabase rather than run xdg-mime.

This patch is about rewrite to C++. What you suggest are good ideas, but can be done as separate patches, from my POV they are of lower priority.

Besides the two issues you mentioned, the current implementation also has a huge bug that stops many services menus from being installed: the installation scripts are run from a wrong current working dir. I already have a patch to fix it, however I will post it when this one lands because otherwise it's hard to publish the second patch with Phabricator.

To be exact, KMessageBox is not equivalent to kdialog --passivepopup.

Thanks for the patch!

Sure!

@barchiesi This commit broke the kio-gdrive build (CanCreateTeamDrives no longer existing).

+1

In D22009#484596, @barchiesi wrote:

@elvisangelaccio We have a refresh token available that could be used but I couldn't find any information regarding refreshing an account through KAccounts. Do you know if such functionality is available? If not, would you perhaps know who I can reach out to?

I guess go for it.

Sorry but there are a lot of trailing whitespace changes (which unfortunately phabricator ignores).