D29201: KCMUtils: Add option to append service file to list of arguments

[Alexander Lohnau]

alex added a comment.


  > Would it make sense to always pass it? Would it regress in any way?
  
  It would make sense, but it would change the existing behavior. 
  If you use the method setConfigurationArguments then you expect your argument 
list to be the list of arguments you specified + an entry for the metadata.
  So adding an entry by default could break things.

REPOSITORY
  R295 KCMUtils

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

To: alex, #plasma, ngraham, meven, broulik
Cc: apol, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29223: Update Taiwanese holidays

[N. Higa]

nhiga added a comment.


  Some historical arrangements of some holidays (e.g. Children's Day between 
1998-2010) are currently ignored, but I believe this is less important.

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

To: nhiga, winterz, cgiboudeaux, shrapnel
Cc: weisi, #kde_pim, kde-frameworks-devel, shrapnel, LeGast00n, cblack, 
fbampaloukas, dcaliste, michaelh, ngraham, bruns, dvasin, rodsevich, winterz, 
vkrause, mlaurent, knauss, dvratil

D29223: Update Taiwanese holidays

[N. Higa]

nhiga marked 6 inline comments as done.

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

To: nhiga, winterz, cgiboudeaux, shrapnel
Cc: weisi, #kde_pim, kde-frameworks-devel, shrapnel, LeGast00n, cblack, 
fbampaloukas, dcaliste, michaelh, ngraham, bruns, dvasin, rodsevich, winterz, 
vkrause, mlaurent, knauss, dvratil

D29223: Update Taiwanese holidays

[N. Higa]

nhiga updated this revision to Diff 81833.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29223?vs=81321=81833

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

AFFECTED FILES
  holidays/holidays.qrc
  holidays/plan2/holiday_tw_zh
  holidays/plan2/holiday_tw_zh-tw

To: nhiga, winterz, cgiboudeaux, shrapnel
Cc: weisi, #kde_pim, kde-frameworks-devel, shrapnel, LeGast00n, cblack, 
fbampaloukas, dcaliste, michaelh, ngraham, bruns, dvasin, rodsevich, winterz, 
vkrause, mlaurent, knauss, dvratil

D29396: Suppress find_package_handle_standard_args package name mismatch warning.

[Aleix Pol Gonzalez]

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


  This will make our configure steps readable again. Thanks!

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  master

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

To: xuetianweng, #frameworks, #build_system, apol
Cc: apol, kde-frameworks-devel, kde-buildsystem, LeGast00n, cblack, bencreasy, 
michaelh, ngraham, bruns

D29223: Update Taiwanese holidays

[Weisi Dai]

weisi added a comment.


  Thanks! Looks good :)

REPOSITORY
  R175 KHolidays

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

To: nhiga, winterz, cgiboudeaux, shrapnel
Cc: weisi, #kde_pim, kde-frameworks-devel, shrapnel, LeGast00n, cblack, 
fbampaloukas, dcaliste, michaelh, ngraham, bruns, dvasin, rodsevich, winterz, 
vkrause, mlaurent, knauss, dvratil

D29223: Update Taiwanese holidays

[N. Higa]

nhiga added a comment.


  > On the Spring festival 春節 of 2020, the publicholidays.tw page and the 
official source listed on that page both showed Jan 23 to 29 as the days off.
  
  I think both of us can be correct:
  
  - I referred to this document by Ministry of the Interior, R. O. C. (Taiwan) 
for 2020 
.
 I believe that document is showing the "minimum requirements by law", which 
means all employees should be able to haves days off on the listed holidays.
  - For government offices, I believe that the regulations "政府機關調整上班日期處理要點" 
will be applied and civil servants have extra days off in some conditions. For 
example it says " 
四、上班日為星期一或星期五，其後一日或前一日逢星期二或星期四之紀念日及節日之放假，調整該上班日為放假日。農曆除夕前一日為上班日者，調整該上班日為放假日。 ", 
so I think this is why Jan 23 is also a day off for them.
  
  > Would you also indicate the plan for the festivals specified as "2020" in 
this diff for future years?
  
  I plan to follow the format in the `holiday_hk_*` files, so it will be 
something like:
  
:: 應放假之紀念日及節日（以農曆為基礎）

: Chinese New Year's Eve
"農曆除夕"  public cultural on 24 January 2020
: Spring festival
"春節"  public cultural on 25 January 2020 length 
5 days
: Tomb Sweeping Day
"民族掃墓節"public cultural on 4 April 2020
: Dragon Boat Festival
"端午節"public cultural on 25 June 2020
: Mid-Autumn Festival   
"中秋節"public cultural on 1 October 2020

"農曆除夕"  public cultural on DD MM 2021
"春節"  public cultural on DD MM 2021 length Y 
days
"民族掃墓節"public cultural on DD MM 2021
"端午節"public cultural on DD MM 2021
"中秋節"public cultural on DD MM 2021

:: Substitute holidays
:: 補假

"兒童節補假"public on 3 April 2020
"民族掃墓節補假"public on 2 April 2020

"XX補假"public on DD MM 2021
"XX補假"public on DD MM 2021

INLINE COMMENTS

> weisi wrote in holiday_tw_zh-tw:38
> Do you intend to use 3 days here?
> 
> According to 紀念日及節日實施辦法 
> :
> 
> > 下列民俗節日，除春節放假三日外，其餘均放假一日... (3 days off for the Spring festival, 1 day off 
> > otherwise, for the following...)

I intentionally included substitute holidays (Jan 28-29 2020) when writing the 
length, since `holiday_hk_*`also include substitute holidays in the length for 
Spring Festival.

> weisi wrote in holiday_tw_zh-tw:62
> I think this one is more religious than commemorative.

True. Maybe mark it as "religious commemorative"?

REPOSITORY
  R175 KHolidays

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

To: nhiga, winterz, cgiboudeaux, shrapnel
Cc: weisi, #kde_pim, kde-frameworks-devel, shrapnel, LeGast00n, cblack, 
fbampaloukas, dcaliste, michaelh, ngraham, bruns, dvasin, rodsevich, winterz, 
vkrause, mlaurent, knauss, dvratil

D29396: Suppress find_package_handle_standard_args package name mismatch warning.

[Xuetian Weng]

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

REVISION SUMMARY
  cmake introduced a new find_package mismatch check in 3.17, but also allows
  user to suppress the warning by setting a variable (Or parameter, but 
  require new cmake 3.17). Same technique is also used with in cmake,
  e.g. FindGTK2.cmake.

TEST PLAN
  Test with FindXCB.cmake

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  master

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

AFFECTED FILES
  modules/ECMFindModuleHelpers.cmake

To: xuetianweng, #frameworks, #build_system
Cc: kde-frameworks-devel, kde-buildsystem, LeGast00n, cblack, bencreasy, 
michaelh, ngraham, bruns

D29201: KCMUtils: Add option to append service file to list of arguments

[Aleix Pol Gonzalez]

apol added a comment.


  I have the feeling there's a missing piece here.
  
  Would it make sense to always pass it? Would it regress in any way?
  
  At the very least, on the API documentation, explain why one would want that.

REPOSITORY
  R295 KCMUtils

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

To: alex, #plasma, ngraham, meven, broulik
Cc: apol, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29385: Introduce KIO::OpenUrlJob, a rewrite and replacement for KRun

[David Faure]

dfaure updated this revision to Diff 81830.
dfaure marked an inline comment as done.
dfaure added a comment.


  Multiple fixes thanks to review comments; didn't remove the fast path yet (a 
unittest needs fixup)

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29385?vs=81814=81830

BRANCH
  2020_05_openurljob

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/krununittest.cpp
  autotests/openurljobtest.cpp
  autotests/openurljobtest.h
  src/core/kurlauthorized.h
  src/gui/CMakeLists.txt
  src/gui/applicationlauncherjob.cpp
  src/gui/applicationlauncherjob.h
  src/gui/openurljob.cpp
  src/gui/openurljob.h
  src/gui/openurljobhandlerinterface.cpp
  src/gui/openurljobhandlerinterface.h
  src/widgets/CMakeLists.txt
  src/widgets/jobuidelegate.cpp
  src/widgets/kdesktopfileactions.cpp
  src/widgets/kopenwithdialog.cpp
  src/widgets/kopenwithdialog.h
  src/widgets/krun.cpp
  src/widgets/widgetsopenurljobhandler.cpp
  src/widgets/widgetsopenurljobhandler.h

To: dfaure, ahmadsamir, broulik, meven, kossebau, davidedmundson, nicolasfella, 
svuorela
Cc: feverfew, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29385: Introduce KIO::OpenUrlJob, a rewrite and replacement for KRun

[David Faure]

dfaure marked 2 inline comments as done.
dfaure added inline comments.

INLINE COMMENTS

> broulik wrote in openurljob.cpp:261
> I know what you always say when I say what I always say, but why not just 
> always stat/mimetype job?

LOL we're like an old couple, the explicit discussion doesn't actually need to 
happen anymore ;)

OK for everyone else, we're debating whether it's ok to use blocking local-file 
I/O like QFile or QMimeDatabase which reads from the file.

Of course less code paths is a good thing for maintenance, but it seems *so* 
overkill to make two calls to a kioslave just to find out the mimetype of a 
file My main counter argument is performance.

For the whole OpenUrlJob until the mimetype is found:

With KIO

  RESULT : OpenUrlJobTest::takeOverAfterMimeTypeFound():
   0.29 msecs per iteration (total: 75, iterations: 256)

With the local-file optimization

  RESULT : OpenUrlJobTest::takeOverAfterMimeTypeFound():
   0.0986 msecs per iteration (total: 101, iterations: 1024)

That's 3 times faster. Admittedly this is not the kind of things people do in a 
loop.

Well, OK, if nobody objects I can remove the local-files fast paths.
0.2ms is nothing when lharming 2018 PG Demi Lovato Ashley Tisdale Avril 
Lavigne-02052020.mpgaunching an app, or even when opening a URL in a browser.

[More context: QMimeDatabase *might* determine the mimetype from just the 
extension, in which case no I/O happens and we could do that here, or it might 
need to look into the contents of the file. We can ask it to not do that but 
then the mimetype determination will be less good, for some mimetypes; and we 
can't ask it if we would get better information by looking at content, so 
there's no way to split up the work between here and the kioslave. It's "quick 
search" vs "full search", not phase 1, phase 2.]

> broulik wrote in openurljob.cpp:447
> was or should be?

Hehe, see kdesktopfileactions.cpp:141 ;-)

> broulik wrote in openurljob.cpp:506
> While at it, can we clean up/unify those texts? Sometimes it puts the file on 
> a new line, sometimes it's bold, sometines in quotes, etc. Generally I 
> wouldn't really want any HTML formatting in there.

Completely agree. I removed most of the HTML formatting when grabbing the error 
messages from KRun, looks like I forgot this one.

So now it's quotes everywhere.

But indeed sometimes the filename is on its own line. Should I just use quotes 
everywhere?

I guess the problem is when the filename (with full path) is very long.

REPOSITORY
  R241 KIO

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

To: dfaure, ahmadsamir, broulik, meven, kossebau, davidedmundson, nicolasfella, 
svuorela
Cc: feverfew, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29391: Introduce setWindow and CloseWhenWindowActivated

[Nicolas Fella]

nicolasfella added a comment.


  In D29391#662471 , @broulik wrote:
  
  > The `QWidget` one has some additional features, like closing the 
notification when the window is destroyed (cf. its docs), which would be useful?
  
  
  lol, turns out that doesn't even work when using setWidget instead of the 
ctor with a widget because of 30b10492 


REPOSITORY
  R289 KNotifications

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

To: nicolasfella, #frameworks, broulik
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29347: KAuthorized: export method to reload restrictions

[Matthew Dawson]

mdawson added a comment.


  One suggestion for this change:
  Instead of exporting a method that takes no parameters and always loads from 
configuration file, why not make a new method with the implementation that 
takes in a given KConfigGroup.  That way unit tests can pass in a KConfigGroup 
setup appropriately without having to create a normal configuration file in the 
user's home folder (ie use a temporary file).  They can also configure the 
KConfig to not cascade/use the global configuration so they are isolated from 
the environment.
  
  initUrlActionRestrictions can then remain the same, and pass in the 
KConfigGroup that would have been used previously.
  
  It would look something like:
  
KCONFIGCORE_EXPORT void loadUrlActionRestrictions(KConfigGroup cg)
{
// Code as before, without creating the KConfigGroup
}
void initUrlActionRestrictions()
{
KConfigGroup cg(KSharedConfig::openConfig(), "KDE URL Restrictions");
loadUrlActionRestrictions(cg);
}

REPOSITORY
  R237 KConfig

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

To: dfaure, aacid, apol, mdawson
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29390: Respect QIcon::fallbackSearchpaths()

[Friedrich W. H. Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> aacid wrote in kiconloader.cpp:1142
> I just realized that function is private, not really easy to use :/
> 
> anyhow do you think we should remove svgz?
> 
> Also i think using
> 
> const QStringList extensions = { QStringLiteral(".png"), 
> QStringLiteral(".svg"), QStringLiteral(".svgz"), QStringLiteral(".xpm") };
> 
> should be a bit faster

You could make this even a stack-only array, even more fast due to no heap 
alloc ;)

  const QString extensions[] = { QStringLiteral(".png"), 
QStringLiteral(".svg"), QStringLiteral(".svgz") << QStringLiteral(".xpm" };

REPOSITORY
  R302 KIconThemes

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

To: nicolasfella, #plasma, #frameworks
Cc: kossebau, aacid, kde-frameworks-devel, LeGast00n, cblack, michaelh, 
ngraham, bruns

D29391: Introduce setWindow and CloseWhenWindowActivated

[Friedrich W. H. Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> knotification.h:582
>  
> +/** Sets the window associated with this notification.
> + *  This is relevant when using the CloseWhenWindowActivated flag.

And some remarks from a local API docs pedant :)

Please place the text not on the first line after "/**", but the second line 
only (for consistency at least). Also keep indent at one space in the following 
lines then again afte the asterisk.

Also wants a "@since" note, same with the getter :) Them being jealous on the 
one you gave to the flag.

I would also propose to place these methods for the new property before the two 
methods tagged with "@internal", to keep some ordering in the list of methods 
for the human reader of the code.

REPOSITORY
  R289 KNotifications

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

To: nicolasfella, #frameworks, broulik
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29390: Respect QIcon::fallbackSearchpaths()

[Albert Astals Cid]

aacid added inline comments.

INLINE COMMENTS

> aacid wrote in kiconloader.cpp:1142
> Would it make sense trying to use QIconLoader::lookupFallbackIcon ? This way 
> we "upstream" Qt behaviour? For example you're supporting svgz while Qt 
> doesn't. Which would mean different QIcon::fallbackSearchPaths whether you're 
> using KIconLoader or not which doesn't seem totally great?

I just realized that function is private, not really easy to use :/

anyhow do you think we should remove svgz?

Also i think using

const QStringList extensions = { QStringLiteral(".png"), 
QStringLiteral(".svg"), QStringLiteral(".svgz"), QStringLiteral(".xpm") };

should be a bit faster

REPOSITORY
  R302 KIconThemes

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

To: nicolasfella, #plasma, #frameworks
Cc: aacid, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29390: Respect QIcon::fallbackSearchpaths()

[Albert Astals Cid]

aacid added inline comments.

INLINE COMMENTS

> kiconloader.cpp:1142
> +for (const QString  : fallbackPaths) {
> +const QStringList extensions = QStringList() << 
> QStringLiteral(".png") << QStringLiteral(".svg") << QStringLiteral(".svgz") 
> << QStringLiteral(".xpm");
> +

Would it make sense trying to use QIconLoader::lookupFallbackIcon ? This way we 
"upstream" Qt behaviour? For example you're supporting svgz while Qt doesn't. 
Which would mean different QIcon::fallbackSearchPaths whether you're using 
KIconLoader or not which doesn't seem totally great?

REPOSITORY
  R302 KIconThemes

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

To: nicolasfella, #plasma, #frameworks
Cc: aacid, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29385: Introduce KIO::OpenUrlJob, a rewrite and replacement for KRun

[David Faure]

dfaure added a comment.


  In D29385#662439 , @feverfew wrote:
  
  > Ahhh yes correct. Seeming as these jobs are now async (unlike KRun IIRC?), 
it isn't a problem that `resultingArguments` makes several blocking calls to 
the KIOFuse daemon?
  
  
  Some of KRun was async too ("new KRun" was async, the static methods like 
runUrl and others were not).
  The blocking calls in resultingArguments() don't make things worse for 
OpenUrlJob than it was for KRun, but of course the fact that the job is async 
is an opportunity to do everything async.
  The problem is that we can't get rid of KRun just yet
  
  Async on top of sync works -- but still blocks the GUI thread so it's not 
perfect. Sync on top of async requires awful eventloop hacks which are often 
the source of nasty bugs.
  
  Solution 1: we write an async version of DesktopExecParser, keeping the 
existing one for KRun.
  Solution 2: we wait until KRun is gone (KF6) to make DesktopExecParser async.
  
  (I'm skipping non-solutions like event loop hacks in KRun or using the fact 
that nobody seems to really care for the PIDs or "bool success" returned by 
KRun so we could just make KRun "fire and forget" the underlying jobs and 
always return success).
  
  Basically this means rewriting DesktopExecParser as a KJob. Are you up for 
it? ;-)
  I can provide guidance, but I really care about FUSE enough myself to test a 
rewrite if I'd have to write it.
  Or I write, you test ;)

REPOSITORY
  R241 KIO

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

To: dfaure, ahmadsamir, broulik, meven, kossebau, davidedmundson, nicolasfella, 
svuorela
Cc: feverfew, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29391: Introduce setWindow and CloseWhenWindowActivated

[Nicolas Fella]

nicolasfella added a comment.


  In D29391#662471 , @broulik wrote:
  
  > Generally +1
  >  The `QWidget` one has some additional features, like closing the 
notification when the window is destroyed (cf. its docs), which would be useful?
  
  
  I'll take a look
  
  > Not a fan of the timer.
  
  Me neither, but the widgets code has this too, so I kept it. I can remove it 
though if we decide we don't want it
  
  > Also, we might want to deprecate the `QWidget` one now?
  
  That's my plan, but I still need to port some things

REPOSITORY
  R289 KNotifications

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

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

D29391: Introduce setWindow and CloseWhenWindowActivated

[Kai Uwe Broulik]

broulik added a comment.


  Generally +1
  The `QWidget` one has some additional features, like closing the notification 
when the window is destroyed (cf. its docs), which would be useful?
  Not a fan of the timer.
  Also, we might want to deprecate the `QWidget` one now?

REPOSITORY
  R289 KNotifications

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

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

D29021: Remove checks for notification service and fallback to KPassivePopup

[Nicolas Fella]

This revision was automatically updated to reflect the committed changes.
Closed by commit R289:a5f0b1f7c812: Remove checks for notification service and 
fallback to KPassivePopup (authored by nicolasfella).

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29021?vs=80687=81823

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

AFFECTED FILES
  src/notifybypopup.cpp
  src/notifybypopup.h

To: nicolasfella, #frameworks, #plasma, broulik
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29391: Introduce setWindow and CloseWhenWindowActivated

[Nicolas Fella]

nicolasfella created this revision.
nicolasfella added reviewers: Frameworks, broulik.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
nicolasfella requested review of this revision.

REVISION SUMMARY
  This is a replacement for CloseOnWidgetActivated that operates on a QWindow 
instead of QWidget.

TEST PLAN
  PoC port of Yakuake:

REPOSITORY
  R289 KNotifications

BRANCH
  closewin

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

AFFECTED FILES
  src/knotification.cpp
  src/knotification.h

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

D29385: Introduce KIO::OpenUrlJob, a rewrite and replacement for KRun

[Alexander Saoutkin]

feverfew added a comment.


  In D29385#662435 , @dfaure wrote:
  
  > In D29385#662422 , @feverfew 
wrote:
  >
  > > Quick question, how does this affect D23384 
? Previously KRun used 
KIO::DesktopExecParser::resultingArguments() which handled the conversion of 
URLs to local KIOFuse URLs if needed, but now I believe this new API doesn't?
  >
  >
  > No worries, this should still happen.
  >  KRun is actually split into three classes: OpenUrlJob, CommandLauncherJob 
and ApplicationLauncherJob. The last two delegate the work to an internal 
class, KProcessRunner (like KRun did).
  >  So (to roll out the common case of a document-type file), once OpenUrlJob 
finds out which application to start, it calls ApplicationLauncherJob, which 
creates a KProcessRunner, which uses 
KIO::DesktopExecParser::resultingArguments().
  
  
  Ahhh yes correct. Seeming as these jobs are now async (unlike KRun IIRC?), it 
isn't a problem that `resultingArguments` makes several blocking calls to the 
KIOFuse daemon?

REPOSITORY
  R241 KIO

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

To: dfaure, ahmadsamir, broulik, meven, kossebau, davidedmundson, nicolasfella, 
svuorela
Cc: feverfew, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29373: Taiwan: Hardcoding holidays based on the Lunar Calendar; Minor update to the holiday list

[Weisi Dai]

weisi added a comment.


  I agree with the general idea in D29223  
and feel D29223  is in a better shape than 
this diff. However, I think some festivals referred to in this diff (not 
mentioned in the law -  紀念日及節日實施辦法 
 - but are still 
special days of interest that many people celebrate) can be adapted into D29223 
.

REPOSITORY
  R175 KHolidays

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

To: weisi, winterz, cgiboudeaux
Cc: nhiga, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29385: Introduce KIO::OpenUrlJob, a rewrite and replacement for KRun

[David Faure]

dfaure added a comment.


  In D29385#662422 , @feverfew wrote:
  
  > Quick question, how does this affect D23384 
? Previously KRun used 
KIO::DesktopExecParser::resultingArguments() which handled the conversion of 
URLs to local KIOFuse URLs if needed, but now I believe this new API doesn't?
  
  
  No worries, this should still happen.
  KRun is actually split into three classes: OpenUrlJob, CommandLauncherJob and 
ApplicationLauncherJob. The last two delegate the work to an internal class, 
KProcessRunner (like KRun did).
  So (to roll out the common case of a document-type file), once OpenUrlJob 
finds out which application to start, it calls ApplicationLauncherJob, which 
creates a KProcessRunner, which uses 
KIO::DesktopExecParser::resultingArguments().

REPOSITORY
  R241 KIO

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

To: dfaure, ahmadsamir, broulik, meven, kossebau, davidedmundson, nicolasfella, 
svuorela
Cc: feverfew, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29223: Update Taiwanese holidays

[Weisi Dai]

weisi added a comment.


  I prefer this diff to the changes I took in D29373 
. I wish I had seen this diff earlier.
  
  Would you also indicate the plan for the festivals specified as "2020" in 
this diff for future years? For example, could you add the 2021 dates to this 
diff and show the suggested layout of the code? I plan to replicate this in the 
`cn_zh-cn` file once we agree on how to best handle those.
  
  I like the idea of referring to the law as the source, instead of potential 
unreliable third-party websites. But I wonder if this criteria might be too 
restrictive here:
  
  - On the Spring festival 春節 of 2020, the publicholidays.tw page 
 and the official source listed on 
that page  both showed Jan 
23 to 29 as the days off. Or maybe you just meant to use `length 3 days` 
(instead of `length 5 days`) which is what the law says 
? For the future 
years I only put in the day of the Spring festival in D29373 
 but maybe I should have marked `length 3 
days`.
  - 紀念日及節日實施辦法  as 
a law, may not be able to include many interesting dates - you don't really 
expect a law to change too frequently. So I think some festivals can still be 
put in this file - they are still "//day[s] that may have special significance 
to a fairly large number of people//" (quote from the `/DESIGN` file) after 
all. For example, in my opinion, Freedom of Expression Day 言論自由日 and Double 
Ninth Day 重陽節 (and maybe more) deserve to be put in this list.

INLINE COMMENTS

> holiday_tw_zh-tw:17
> +:name"可選 - 預設為國家名"
> +description "中華民國（臺灣）紀念日及節日"
> +

One more space here.

> holiday_tw_zh-tw:37
> +"農曆除夕"  public cultural on 24 January 2020
> +: Spring festival
> +"春節"  public cultural on 25 January 2020 length 
> 5 days

Uppercase `F`.

> holiday_tw_zh-tw:38
> +: Spring festival
> +"春節"  public cultural on 25 January 2020 length 
> 5 days
> +: Tomb Sweeping Day

Do you intend to use 3 days here?

According to 紀念日及節日實施辦法 
:

> 下列民俗節日，除春節放假三日外，其餘均放假一日... (3 days off for the Spring festival, 1 day off 
> otherwise, for the following...)

> holiday_tw_zh-tw:62
> +
> +"佛陀誕辰紀念日"commemorative on 30 April 2020
> +

I think this one is more religious than commemorative.

REPOSITORY
  R175 KHolidays

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

To: nhiga, winterz, cgiboudeaux, shrapnel
Cc: weisi, #kde_pim, kde-frameworks-devel, shrapnel, LeGast00n, cblack, 
fbampaloukas, dcaliste, michaelh, ngraham, bruns, dvasin, rodsevich, winterz, 
vkrause, mlaurent, knauss, dvratil

D29385: Introduce KIO::OpenUrlJob, a rewrite and replacement for KRun

[Alexander Saoutkin]

feverfew added a comment.


  Quick question, how does this affect D23384 
? Previously KRun used 
KIO::DesktopExecParser::resultingArguments() which handled the conversion of 
URLs to local KIOFuse URLs if needed, but now I believe this new API doesn't?

REPOSITORY
  R241 KIO

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

To: dfaure, ahmadsamir, broulik, meven, kossebau, davidedmundson, nicolasfella, 
svuorela
Cc: feverfew, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29390: Respect QIcon::fallbackSearchpaths()

[Nicolas Fella]

nicolasfella created this revision.
nicolasfella added reviewers: Plasma, Frameworks.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
nicolasfella requested review of this revision.

REVISION SUMMARY
  When an icon isn't found within a theme we are supposed to look for it in 
QIcon::fallbackSearchpaths() (https://doc.qt.io/qt-5/qicon.html#fromTheme).
  
  BUG: 405522

TEST PLAN
  Create /home/nico/foo.svg
  Add QIcon::setFallbackSearchPaths(QIcon::fallbackSearchPaths() << 
QStringLiteral("/home/nico/myicons")); to my app
  Use foo icon somewhere

REPOSITORY
  R302 KIconThemes

BRANCH
  fallback

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

AFFECTED FILES
  src/kiconloader.cpp

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

D29372: Taiwan: Use "zh_TW" language code

[Weisi Dai]

weisi added a comment.


  I think the approach in D29223  (renaming 
the file also) is more preferred.

REPOSITORY
  R175 KHolidays

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

To: weisi, winterz
Cc: nhiga, cgiboudeaux, kde-frameworks-devel, LeGast00n, cblack, michaelh, 
ngraham, bruns

D29385: Introduce KIO::OpenUrlJob, a rewrite and replacement for KRun

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> broulik wrote in openurljob.cpp:274
> I know what you always say when I say what I always say but can we use a 
> mimetype job here? :)

Seems I forgot to remove this when I added the comment above instead :)

REPOSITORY
  R241 KIO

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

To: dfaure, ahmadsamir, broulik, meven, kossebau, davidedmundson, nicolasfella, 
svuorela
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29385: Introduce KIO::OpenUrlJob, a rewrite and replacement for KRun

[Kai Uwe Broulik]

broulik added a comment.


  Cool!

INLINE COMMENTS

> openurljob.cpp:261
> +
> +void KIO::OpenUrlJobPrivate::determineLocalMimeType()
> +{

I know what you always say when I say what I always say, but why not just 
always stat/mimetype job?

> openurljob.cpp:274
> +QMimeDatabase db;
> +QMimeType mime = db.mimeTypeForUrl(m_url);
> +//qDebug() << "MIME TYPE is " << mime.name();

I know what you always say when I say what I always say but can we use a 
mimetype job here? :)

> openurljob.cpp:305
> +q->emitResult();
> +} else {
> +if (m_followRedirections) { // Update our URL in case of a 
> redirection

Using an early return here would make the code less nested

> openurljob.cpp:447
> +// X-KDE-LastOpenedWith holds the service desktop entry name that
> +// was should be preferred for opening this URL if possible.
> +// This is used by the Recent Documents menu for instance.

was or should be?

> openurljob.cpp:506
> +q->setError(KJob::UserDefinedError);
> +q->setErrorText(i18n("The file %1 is an executable 
> program. "
> + "For safety it will not be started.", 
> m_url.toDisplayString().toHtmlEscaped()));

While at it, can we clean up/unify those texts? Sometimes it puts the file on a 
new line, sometimes it's bold, sometines in quotes, etc. Generally I wouldn't 
really want any HTML formatting in there.

REPOSITORY
  R241 KIO

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

To: dfaure, ahmadsamir, broulik, meven, kossebau, davidedmundson, nicolasfella, 
svuorela
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

Re: Adding a patch to 5.70

[Filip Fila]

Thank you, David!

On Sun, May 3, 2020 at 7:08 PM David Faure  wrote:

> On Sunday, May 3, 2020 6:31:44 PM CEST Filip Fila wrote:
> > Dear Frameworks maintainers,
> >
> > Would it be possible to add this (https://phabricator.kde.org/D29352)
> patch
> > to the 5.70 release?
> >
> > The change concerns not breaking third-party Plasma themes, and as Nate
> > explained "If this doesn't land in Frameworks 5.70, Plasma 5.19 users run
> > the risk of being hit by it, as 5.70 is the frameworks dependency for
> that
> > Plasma release."
>
> OK, done:
>
> plasma-framework v5.70.0-rc2
> fe45d59e250d1c7f4579f54fec52437ebb0e5d03
> cb8289d495e4df19056ce8814dacd8a0afe93bff1edb0352028c6b6e47364ba5
> sources/plasma-framework-5.70.0.tar.xz
>
>
> --
> David Faure, fa...@kde.org, http://www.davidfaure.fr
> Working on KDE Frameworks 5
>
>
>
>

Re: Adding a patch to 5.70

[Nate Graham]

Thanks everyone!

Nate



On 5/3/20 11:08 AM, David Faure wrote:

On Sunday, May 3, 2020 6:31:44 PM CEST Filip Fila wrote:

Dear Frameworks maintainers,

Would it be possible to add this (https://phabricator.kde.org/D29352) patch
to the 5.70 release?

The change concerns not breaking third-party Plasma themes, and as Nate
explained "If this doesn't land in Frameworks 5.70, Plasma 5.19 users run
the risk of being hit by it, as 5.70 is the frameworks dependency for that
Plasma release."


OK, done:

plasma-framework v5.70.0-rc2
fe45d59e250d1c7f4579f54fec52437ebb0e5d03
cb8289d495e4df19056ce8814dacd8a0afe93bff1edb0352028c6b6e47364ba5  
sources/plasma-framework-5.70.0.tar.xz

D29385: Introduce KIO::OpenUrlJob, a rewrite and replacement for KRun

[David Faure]

dfaure updated this revision to Diff 81814.
dfaure marked 2 inline comments as done.
dfaure added a comment.


  Adjusments based on Friedrich's initial review

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29385?vs=81802=81814

BRANCH
  2020_05_openurljob

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/krununittest.cpp
  autotests/openurljobtest.cpp
  autotests/openurljobtest.h
  src/core/kurlauthorized.h
  src/gui/CMakeLists.txt
  src/gui/applicationlauncherjob.cpp
  src/gui/applicationlauncherjob.h
  src/gui/openurljob.cpp
  src/gui/openurljob.h
  src/gui/openurljobhandlerinterface.cpp
  src/gui/openurljobhandlerinterface.h
  src/widgets/CMakeLists.txt
  src/widgets/jobuidelegate.cpp
  src/widgets/kdesktopfileactions.cpp
  src/widgets/kopenwithdialog.cpp
  src/widgets/kopenwithdialog.h
  src/widgets/krun.cpp
  src/widgets/widgetsopenurljobhandler.cpp
  src/widgets/widgetsopenurljobhandler.h

To: dfaure, ahmadsamir, broulik, meven, kossebau, davidedmundson, nicolasfella, 
svuorela
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29385: Introduce KIO::OpenUrlJob, a rewrite and replacement for KRun

[David Faure]

dfaure marked 5 inline comments as done.
dfaure added a comment.


  In D29385#662336 , @svuorela wrote:
  
  > I've been looking at it for a while, and after trying to decipher the long 
lambdas, which might have been better as named functions, I think it is, beside 
Kossebau's comments, fine.
  
  
  Interesting comment. I thought it was actually more readable to keep that 
code together, rather than having to jump around in the file to find the right 
slots.

INLINE COMMENTS

> kossebau wrote in openurljobtest.cpp:108
> why no simple range-based for loop?

Good point, copy/pasted from another (old) test.

> kossebau wrote in openurljob.h:73
> Makes me wonder if those runflags should not be rather part of KIO namespace, 
> to decouple classes here.

Interesting point, let's discuss it quickly (because if we want to change that, 
I need to redo the kio RC1 tag for 5.70, which has ApplicationLauncherJob).

But I'm not sure we should do that, anyway.

On hindsight, I would rather change this one to be setTemporaryFiles(bool b);

I don't like all the flag-conversion hell that we end up with otherwise. Stuff 
like

  RunFlags flags = tempFile ? KRun::DeleteTemporaryFiles : RunFlags();
  if (runExecutables) {
  flags |= KRun::RunExecutables;
  }

The input data, all the way up, is for sure not in the form of a set of job 
flags.
So I find it more readable to have code like setSomething(conditionForSomething)
than to have a bunch of flag manipulation code.

> kossebau wrote in openurljob.h:96
> Could this and the following bool flags be turned into some flags instead? 
> Just wondering, not looked further.
> 
> Also wondering if there should not be some getter as well, when using a 
> flagset that would be just a single method/symbol.

With this one in particular, since it's on by default, we'd have to either have 
DefaultRunFlags that includes RunExecutables --- or we'd have to make the flag 
DoNotRunExecutables.
I don't like either

Technically yes it could all be flags, but I'd much rather have independent 
setters.

The TempFile flag is a good example why: if it's shared with 
ApplicationLauncherJob by being in the KIO namespace, does that mean we also 
put all of those in there? But then people can write things that don't make 
sense like ApplicationLauncherJob *job = ...; 
job->setFlags(DoNotRunExecutables). What? That flag doesn't apply to that job. 
If there's one thing that job does, it's to run executables :-)   [but via 
desktop files, while that flag is actually about what OpenUrlJob should do when 
opening an executable directly].

Sharing flags between jobs who need a different set, is a problem. It ends up 
with docu like "only these flags make sense here". Not great.

I don't see a use case for getters, but we can certainly add them (either now 
or as needed).

> kossebau wrote in openurljob.h:127
> -> "mimeTypeFound"? would match other casing.

Well spotted, thanks.

> kossebau wrote in openurljobhandlerinterface.h:33
> Please prepend a line
> 
>   * @class OpenUrlJobHandlerInterface openurljobhandlerinterface.h 
> 
> 
> at the top, to trick doxygen into documenting the full CamelCase include.

Ah! I was wondering what that was about...

> kossebau wrote in openurljobhandlerinterface.h:84
> Prevent use of nested Private class here, declare a class 
> OpenUrlJobHandlerInterfacePrivate outside, also use QScopedPointer for 
> consistency and preparedness in case there ever is an actual object.

LOL I was the one arguing against nested Private classes recently. Looks like I 
forgot that when copy/pasting here. Thanks!

Hmm note that using QScopedPointer requires actually defining (in the .cpp) a 
dummy private class, even if there's no "new" anywhere (generated code wants to 
delete it).

REPOSITORY
  R241 KIO

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

To: dfaure, ahmadsamir, broulik, meven, kossebau, davidedmundson, nicolasfella, 
svuorela
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29374: UK, Scotland: Fix syntax error by adding category of Early May Bank Holiday

[David Edmundson]

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

REPOSITORY
  R175 KHolidays

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

To: weisi, winterz, davidedmundson
Cc: jriddell, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29385: Introduce KIO::OpenUrlJob, a rewrite and replacement for KRun

[Sune Vuorela]

svuorela added a comment.


  I've been looking at it for a while, and after trying to decipher the long 
lambdas, which might have been better as named functions, I think it is, beside 
Kossebau's comments, fine.

REPOSITORY
  R241 KIO

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

To: dfaure, ahmadsamir, broulik, meven, kossebau, davidedmundson, nicolasfella, 
svuorela
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

Re: Adding a patch to 5.70

[David Faure]

On Sunday, May 3, 2020 6:31:44 PM CEST Filip Fila wrote:
> Dear Frameworks maintainers,
> 
> Would it be possible to add this (https://phabricator.kde.org/D29352) patch
> to the 5.70 release?
> 
> The change concerns not breaking third-party Plasma themes, and as Nate
> explained "If this doesn't land in Frameworks 5.70, Plasma 5.19 users run
> the risk of being hit by it, as 5.70 is the frameworks dependency for that
> Plasma release."

OK, done:

plasma-framework v5.70.0-rc2
fe45d59e250d1c7f4579f54fec52437ebb0e5d03
cb8289d495e4df19056ce8814dacd8a0afe93bff1edb0352028c6b6e47364ba5  
sources/plasma-framework-5.70.0.tar.xz


-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

Adding a patch to 5.70

[Filip Fila]

Dear Frameworks maintainers,

Would it be possible to add this (https://phabricator.kde.org/D29352) patch
to the 5.70 release?

The change concerns not breaking third-party Plasma themes, and as Nate
explained "If this doesn't land in Frameworks 5.70, Plasma 5.19 users run
the risk of being hit by it, as 5.70 is the frameworks dependency for that
Plasma release."

Sorry to trouble you :/
Filip

D29352: [Plasmoid Heading] Draw the heading only when there is an SVG in the theme

[Filip Fila]

This revision was automatically updated to reflect the committed changes.
Closed by commit R242:1b1018e68703: [Plasmoid Heading] Draw the heading only 
when there is an SVG in the theme (authored by filipf).

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29352?vs=81711=81808

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

AFFECTED FILES
  src/declarativeimports/plasmaextracomponents/qml/PlasmoidHeading.qml

To: filipf, #vdg, #plasma, niccolove, ngraham
Cc: ngraham, niccolove, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

D29352: [Plasmoid Heading] Draw the heading only when there is an SVG in the theme

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.


  If this doesn't land in Frameworks 5.70, Plasma 5.19 users run the risk of 
being hit by it, as 5.70 is the frameworks dependency for that Plasma release.
  
  Frameworks 5.70 just got tagged, so once this lands, it will need to be 
manually added to the tag to ensure it's released with 5.70. Can you email the 
frameworks mailing list and dfaure about it?

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  heading-only-if-svg-exists (branched from master)

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

To: filipf, #vdg, #plasma, niccolove, ngraham
Cc: ngraham, niccolove, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

D29123: Do not mark entry as uninstalled if uninstallation script failed

[Alexander Lohnau]

alex marked 2 inline comments as done.

REPOSITORY
  R304 KNewStuff

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

To: alex, #knewstuff, meven, ngraham, leinir
Cc: leinir, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28745: Skip caching thumbnails on encrypted filesystems

[Thiago Macieira]

thiago added inline comments.

INLINE COMMENTS

> thumbnail.cpp:743
> +const auto mount =  mountsList.findByPath(filePath);
> +allowCache = !(mount->mountType() == 
> QLatin1String("fuse.cryfs") || mount->mountType() == 
> QLatin1String("fuse.encfs"));
> +}

How about encrypted block storage, which is superior to those encrypted fs and 
the recommended solution?

REPOSITORY
  R320 KIO Extras

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

To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns
Cc: thiago, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, 
fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, navarromorales, 
firef, andrebarros, emmanuelp, rdieter, mikesomov

D29197: filenamesearch: Implement stat to display metainfo

[Elvis Angelaccio]

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

REPOSITORY
  R320 KIO Extras

BRANCH
  master

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

To: meven, #dolphin, #frameworks, ngraham, elvisangelaccio
Cc: kde-frameworks-devel, kfm-devel, azyx, nikolaik, pberestov, iasensio, 
aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, Codezela, feverfew, 
meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, rdieter, mikesomov

D26342: Allow overriding to disable auto language detection

[Albert Astals Cid]

This revision was automatically updated to reflect the committed changes.
Closed by commit R246:422fac8a6f92: Allow overriding to disable auto language 
detection (authored by sdepiets, committed by aacid).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D26342?vs=81177=81805#toc

REPOSITORY
  R246 Sonnet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26342?vs=81177=81805

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

AFFECTED FILES
  autotests/test_highlighter.cpp
  src/core/backgroundchecker.cpp
  src/core/backgroundchecker.h
  src/core/backgroundchecker_p.h
  src/ui/highlighter.cpp
  src/ui/highlighter.h

To: sdepiets, #frameworks, cullmann, mlaurent, mludwig, aacid
Cc: aacid, mludwig, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D26342: Allow overriding to disable auto language detection

[Albert Astals Cid]

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


  Noone seems to really disagree.
  
  Let's merge it. @sdepiets I hope this doesn't break everything ;)

REPOSITORY
  R246 Sonnet

BRANCH
  master

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

To: sdepiets, #frameworks, cullmann, mlaurent, mludwig, aacid
Cc: aacid, mludwig, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D29385: Introduce KIO::OpenUrlJob, a rewrite and replacement for KRun

[Friedrich W. H. Kossebau]

kossebau added a comment.


  Quick first nitpicks from initial scan what this patch is about, without 
having looked closer.

INLINE COMMENTS

> openurljobtest.cpp:108
> +{
> +std::for_each(m_filesToRemove.begin(), m_filesToRemove.end(), [](const 
> QString & f) {
> +QFile::remove(f);

why no simple range-based for loop?

> openurljob.h:73
> + */
> +void setRunFlags(ApplicationLauncherJob::RunFlags runFlags);
> +

Makes me wonder if those runflags should not be rather part of KIO namespace, 
to decouple classes here.

> openurljob.h:96
> + */
> +void setRunExecutables(bool allow);
> +

Could this and the following bool flags be turned into some flags instead? Just 
wondering, not looked further.

Also wondering if there should not be some getter as well, when using a flagset 
that would be just a single method/symbol.

> openurljob.h:127
> + */
> +void mimetypeFound(const QString );
> +

-> "mimeTypeFound"? would match other casing.

> openurljobhandlerinterface.cpp:34
> +
> +OpenUrlJobHandlerInterface::~OpenUrlJobHandlerInterface()
> +{

`= default;` instead?

> openurljobhandlerinterface.h:33
> +/**
> + * @brief The OpenUrlJobHandlerInterface class allows OpenUrlJob to
> + * prompt the user about which application to use to open URLs that do not

Please prepend a line

  * @class OpenUrlJobHandlerInterface openurljobhandlerinterface.h 


at the top, to trick doxygen into documenting the full CamelCase include.

> openurljobhandlerinterface.h:54
> + */
> +virtual ~OpenUrlJobHandlerInterface();
> +

override instead?

> openurljobhandlerinterface.h:84
> +private:
> +class Private;
> +Private *const d;

Prevent use of nested Private class here, declare a class 
OpenUrlJobHandlerInterfacePrivate outside, also use QScopedPointer for 
consistency and preparedness in case there ever is an actual object.

> widgetsopenurljobhandler.h:45
> +private:
> +class Private;
> +Private *const d;

Do not use embedded Private class. Also pimp needed here, given not a public 
class?

REPOSITORY
  R241 KIO

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

To: dfaure, ahmadsamir, broulik, meven, kossebau, davidedmundson, nicolasfella, 
svuorela
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29361: KCrash: remove debug output which breaks unittests from using ~/.qttest/config for categorized logging

[Aleix Pol Gonzalez]

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

REPOSITORY
  R285 KCrash

BRANCH
  master

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

To: dfaure, kossebau, davidedmundson, apol
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29385: Introduce KIO::OpenUrlJob, a rewrite and replacement for KRun

[David Faure]

dfaure created this revision.
dfaure added reviewers: ahmadsamir, broulik, meven, kossebau, davidedmundson, 
nicolasfella, svuorela.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
dfaure requested review of this revision.

REVISION SUMMARY
  - KRun::runUrl() is now new OpenUrlJob(url, mimeType).
  - new KRun() is now OpenUrlJob(url), which will first determine the mimeType.
  - the "open with" dialog is provided by KIOWidgets via JobUiDelegate and an 
interface defined in kiogui. When not using JobUiDelegate, we'll fall back to 
QDesktopServices::openUrl, which will call xdg-open, which will call kde-open5, 
which will prompt an open with dialog :-)
  - Running of scripts and executables is off by default, unlike in KRun, since 
this created a few security bugs in unsuspecting applications in the past. File 
managers should enable it, apps that can open attachments or similar things 
should leave it disabled.
  - Instead of BrowserRun/KonqRun deriving from KRun, they can use an 
OpenUrlJob as is, connect to mimeTypeFound and kill the job if they want to 
take over (to embed the document instead of launching an app)
  
  I tried to unittest OpenUrlJob as much as possible. The parts that are
  least unittests are those related to remote protocols though, since they
  require test servers.

TEST PLAN
  Mostly via unittests, for now

REPOSITORY
  R241 KIO

BRANCH
  2020_05_openurljob

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/krununittest.cpp
  autotests/openurljobtest.cpp
  autotests/openurljobtest.h
  src/core/kurlauthorized.h
  src/gui/CMakeLists.txt
  src/gui/applicationlauncherjob.cpp
  src/gui/applicationlauncherjob.h
  src/gui/openurljob.cpp
  src/gui/openurljob.h
  src/gui/openurljobhandlerinterface.cpp
  src/gui/openurljobhandlerinterface.h
  src/widgets/CMakeLists.txt
  src/widgets/jobuidelegate.cpp
  src/widgets/kdesktopfileactions.cpp
  src/widgets/kopenwithdialog.cpp
  src/widgets/kopenwithdialog.h
  src/widgets/krun.cpp
  src/widgets/widgetsopenurljobhandler.cpp
  src/widgets/widgetsopenurljobhandler.h

To: dfaure, ahmadsamir, broulik, meven, kossebau, davidedmundson, nicolasfella, 
svuorela
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29201: KCMUtils: Add option to append service file to list of arguments

[Alexander Lohnau]

alex added a dependent revision: D29384: KCM Runners: Use setAppendServiceFile 
method for plugin selector.

REPOSITORY
  R295 KCMUtils

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

To: alex, #plasma, ngraham, meven, broulik
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29374: UK, Scotland: Fix syntax error by adding category of Early May Bank Holiday

[Jonathan Riddell]

jriddell added a comment.


  thanks :)

REPOSITORY
  R175 KHolidays

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

To: weisi, winterz
Cc: jriddell, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29201: KCMUtils: Add option to append service file to list of arguments

[Alexander Lohnau]

alex updated this revision to Diff 81799.
alex added a comment.


  Little typo :-)

REPOSITORY
  R295 KCMUtils

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29201?vs=81798=81799

BRANCH
  service_path_append (branched from master)

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

AFFECTED FILES
  src/kpluginselector.cpp
  src/kpluginselector.h
  src/kpluginselector_p.h

To: alex, #plasma, ngraham, meven, broulik
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29201: KCMUtils: Add option to append service file to list of arguments

[Alexander Lohnau]

alex updated this revision to Diff 81798.
alex added a comment.


  Rename method

REPOSITORY
  R295 KCMUtils

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29201?vs=81239=81798

BRANCH
  service_path_append (branched from master)

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

AFFECTED FILES
  src/kpluginselector.cpp
  src/kpluginselector.h
  src/kpluginselector_p.h

To: alex, #plasma, ngraham, meven, broulik
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29372: Taiwan: Use "zh_TW" language code

[N. Higa]

nhiga added a comment.


  I would say that the file should also be renamed.
  
  //My comment from D29223 ://
  
  > The new file name follows the same naming scheme used by the PRC (mainland 
China) holiday file, holiday_cn_zh-cn (holiday_region_Language-Region). It is 
also less ambiguous because zh alone does not indicate the character set being 
used (i.e. Simplified Chinese or Traditional Chinese).

REPOSITORY
  R175 KHolidays

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

To: weisi, winterz
Cc: nhiga, cgiboudeaux, kde-frameworks-devel, LeGast00n, cblack, michaelh, 
ngraham, bruns

D29373: Taiwan: Hardcoding holidays based on the Lunar Calendar; Minor update to the holiday list

[N. Higa]

nhiga added a comment.


  > specified the start year of Peace Memorial Day 228和平紀念日
  
  Great! This is what I have missed in my patch.
  
  > The star * ones are specified using the "lunar new year date" minus/plus 
some days, hoping to make it easier to adapt to additional years in the future.
  
  I appreciate this new idea and I think it is worth discussing which way of 
dealing lunar calendar-based holidays is better.
  I consider my method a no-brainer because:
  
  - In many cases, Lunar calendar-based (public) holidays in Taiwan are 
actually the same as (or similar to) those in Hong Kong, so basically we can 
refer to the dates in `holiday_hk_en-gb` (which contains the dates up to 2030). 
One notable exception is the Mid-Autumn Festival - for Hong Kong the holiday is 
on the day **after** Mid-Autumn Festival.
  - Or, just refer to the document published by the Ministry of the Interior, 
R. O. C. (Taiwan), which is really straightforward.
  
  For your method:
  
  - We can see the patterns for those holidays (i.e. Lunar new year date + XX 
days + extra months or days (leap month 閏月 / leap days 閏日))
  - If you are (very) familiar with lunar calendar, this method may actually be 
better. Otherwise, you will have to be really careful with the leap months/days.
  
  > There are holidays entries regarding specific arrangements (an extra day is 
observed in lieu of an extra working day) that can't be calculated in advance
  
  If you are referring to 補假 (substitute holiday / supplementary holiday), the 
logic is stated in the law (紀念日及節日實施辦法 
):
  
  > 
紀念日及節日之放假日逢例假日應予補假。例假日為星期六者於前一個上班日補假，為星期日者於次一個上班日補假。但農曆除夕及春節放假日逢例假日，均於次一個上班日補假。
  
  I believe that means:
  
  - If a public holiday is on Saturday, the substitute holiday will be the 
previous working day;
  - If a public holiday is on Sunday, the substitute holiday will be the next 
working day;
  - However, for Lunar New Year's eve and Lunar New Year, the substitute 
holiday will always be the previous working day.
  
  > I think there are still issues with the 4 season dates
  
  I have not enabled "Astronomical Events" plugin, but the astronomical seasons 
(especially June solstice, which is not a holiday for Japan and Hong Kong) are 
still automatically displayed for me. And apparently kholidays has the 
astronomical seasons support already in `src/astroseasons.cpp`.
  Some other issues:
  
  - Official holiday names should be used.
  - Using a third-party and non-official site such as `publicholidays.tw` as 
reference seems unreliable. My patch refers to the law and official documents 
to ensure accuracy.
  - Hence, I excluded all non-official holidays, such as Easter, which has been 
included in your patch.
  - But I cannot say I am 100% sure that I have not missed any official 
**commemorative** days. For example, Wikipedia says the Freedom of Expression 
Day (言論自由日) is an official one, but I wonder why it is not included in the law 
紀念日及節日實施辦法. The website I linked above should be displaying the latest (and 
official) revision of the law. I hope someone from Taiwan can provide more 
information on this.

REPOSITORY
  R175 KHolidays

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

To: weisi, winterz, cgiboudeaux
Cc: nhiga, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29369: Use UI marker context in more tr() calls

[Albert Astals Cid]

aacid added a comment.


  +0.5

REPOSITORY
  R294 KBookmarks

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

To: kossebau, #frameworks, #localization
Cc: aacid, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29369: Use UI marker context in more tr() calls

[Friedrich W. H. Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> aacid wrote in kbookmarkmenu.cpp:288
> @title:inmenu?

Nope, this is a normal action/menu entry, the variable name is misleading here 
a bit. See below e.g. for the slot added.

REPOSITORY
  R294 KBookmarks

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

To: kossebau, #frameworks, #localization
Cc: aacid, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29370: Use UI marker context in more tr() calls

[Albert Astals Cid]

aacid added a comment.


  +0.5

REPOSITORY
  R289 KNotifications

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

To: kossebau, #frameworks, #localization, broulik
Cc: aacid, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29369: Use UI marker context in more tr() calls

[Albert Astals Cid]

aacid added inline comments.

INLINE COMMENTS

> kbookmarkmenu.cpp:288
>  
> -QString title = tr("Open Folder in Tabs");
> +const QString title = tr("Open Folder in Tabs", "@action:inmenu");
>  

@title:inmenu?

REPOSITORY
  R294 KBookmarks

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

To: kossebau, #frameworks, #localization
Cc: aacid, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29368: Use UI marker context in more tr() calls

[Albert Astals Cid]

aacid added a comment.


  +0.5

REPOSITORY
  R284 KCompletion

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

To: kossebau, #frameworks, #localization
Cc: aacid, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29367: Use UI marker context in more tr() calls

[Albert Astals Cid]

aacid added a comment.


  +0.5

REPOSITORY
  R276 KItemViews

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

To: kossebau, #frameworks, #localization, davidedmundson
Cc: aacid, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

KDE CI: Frameworks » kdelibs4support » kf5-qt5 FreeBSDQt5.14 - Build # 10 - Still Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/kdelibs4support/job/kf5-qt5%20FreeBSDQt5.14/10/
 Project:
kf5-qt5 FreeBSDQt5.14
 Date of build:
Sun, 03 May 2020 13:16:35 +
 Build duration:
4 min 2 sec and counting
   JUnit Tests
  Name: projectroot Failed: 2 test(s), Passed: 37 test(s), Skipped: 0 test(s), Total: 39 test(s)Failed: projectroot.autotests.kmimetypetestFailed: projectroot.autotests.kstandarddirstest

KDE CI: Frameworks » kdelibs4support » kf5-qt5 FreeBSDQt5.14 - Build # 9 - Still Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/kdelibs4support/job/kf5-qt5%20FreeBSDQt5.14/9/
 Project:
kf5-qt5 FreeBSDQt5.14
 Date of build:
Sun, 03 May 2020 13:11:00 +
 Build duration:
4 min 15 sec and counting
   JUnit Tests
  Name: projectroot Failed: 2 test(s), Passed: 37 test(s), Skipped: 0 test(s), Total: 39 test(s)Failed: projectroot.autotests.kmimetypetestFailed: projectroot.autotests.kstandarddirstest

D29373: Taiwan: Hardcoding holidays based on the Lunar Calendar; Minor update to the holiday list

[N. Higa]

nhiga added a reviewer: cgiboudeaux.

REPOSITORY
  R175 KHolidays

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

To: weisi, winterz, cgiboudeaux
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29223: Update Taiwanese holidays

[N. Higa]

nhiga added a comment.


  @cgiboudeaux 
  Thanks for the heads-up.

REPOSITORY
  R175 KHolidays

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

To: nhiga, winterz, cgiboudeaux, shrapnel
Cc: #kde_pim, kde-frameworks-devel, shrapnel, LeGast00n, cblack, fbampaloukas, 
dcaliste, michaelh, ngraham, bruns, dvasin, rodsevich, winterz, vkrause, 
mlaurent, knauss, dvratil

D29347: KAuthorized: export method to reload restrictions

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> kossebau wrote in kauthorized.cpp:247
> Please also add a note next to this that it is for unit test, so people are 
> not wondering about this random KCONFIGCORE_EXPORT and first have to research 
> commit history.

Good idea, done: 
https://commits.kde.org/kconfig/311e30857e33f9222b28b6d3b841ab6e61558237

REPOSITORY
  R237 KConfig

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

To: dfaure, aacid, apol, mdawson
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29347: KAuthorized: export method to reload restrictions

[Friedrich W. H. Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> kauthorized.cpp:247
>  
> -static void initUrlActionRestrictions()
> +KCONFIGCORE_EXPORT void reloadUrlActionRestrictions()
>  {

Please also add a note next to this that it is for unit test, so people are not 
wondering about this random KCONFIGCORE_EXPORT and first have to research 
commit history.

REPOSITORY
  R237 KConfig

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

To: dfaure, aacid, apol, mdawson
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29347: KAuthorized: export method to reload restrictions

[David Faure]

dfaure closed this revision.

REPOSITORY
  R237 KConfig

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

To: dfaure, aacid, apol, mdawson
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29381: Thumbnail text: use libmagic to detect encoding

[Pino Toscano]

pino added a comment.


  why not KEncodingProber from the KCodecs framework, like also the commented 
bits hint about?

REPOSITORY
  R320 KIO Extras

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

To: meven, #frameworks, sitter, ngraham
Cc: pino, kde-frameworks-devel, kfm-devel, azyx, nikolaik, pberestov, iasensio, 
aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, Codezela, feverfew, 
meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, rdieter, mikesomov

D29198: filenamesearch:/ define a title for the query

[Méven Car]

This revision was automatically updated to reflect the committed changes.
Closed by commit R318:e076d8668f67: filenamesearch:/ define a title for the 
query (authored by meven).

REPOSITORY
  R318 Dolphin

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29198?vs=81330=81788

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

AFFECTED FILES
  src/search/dolphinsearchbox.cpp
  src/search/dolphinsearchbox.h

To: meven, ngraham, elvisangelaccio, #dolphin, #frameworks, iasensio
Cc: iasensio, kfm-devel, azyx, nikolaik, pberestov, aprcela, fprice, 
fbampaloukas, alexde, Codezela, feverfew, meven, spoorun, navarromorales, 
firef, ngraham, andrebarros, emmanuelp, rdieter, mikesomov

D29197: filenamesearch: Implement stat to display metainfo

[Méven Car]

meven added a comment.


  This is a small review

REPOSITORY
  R320 KIO Extras

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

To: meven, #dolphin, #frameworks, ngraham, elvisangelaccio
Cc: kde-frameworks-devel, kfm-devel, azyx, nikolaik, pberestov, iasensio, 
aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, Codezela, feverfew, 
meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, rdieter, mikesomov

D29197: filenamesearch: Implement stat to display metainfo

[Méven Car]

meven added a comment.


  Please review

REPOSITORY
  R320 KIO Extras

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

To: meven, #dolphin, #frameworks, ngraham, elvisangelaccio
Cc: kde-frameworks-devel, kfm-devel, azyx, nikolaik, pberestov, iasensio, 
aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, Codezela, feverfew, 
meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, rdieter, mikesomov

D29381: Thumbnail text: use libmagic to detect encoding

[Méven Car]

meven created this revision.
meven added reviewers: Frameworks, sitter, ngraham.
Herald added projects: Dolphin, Frameworks.
Herald added subscribers: kfm-devel, kde-frameworks-devel.
meven requested review of this revision.

REVISION SUMMARY
  libmagic is the libraray used in `file` utility to search for file encoding 
based on heuristics.
  
  `QTextCodec::codecForUtfText` is limited to detecting UTF-8 BOM encoded files.
  If this fails `QTextCodec::codecForLocale` is used to decode all text files, 
which can be quite incaccurate.
  
  When QTextCodec::codecForUtfText did not find a valid UTF-* file, use 
libmagic to find a TextCodec.
  It can better confirmm UTF-8 presence.
  latin-1 is used as default 8-bit ascii codex when libmagic cannot find a 
precise result.
  
  BUG: 316390
  FIXED-IN: 20.08

TEST PLAN
  In doplhin, see preview of different non utf-8 encoded text files.

REPOSITORY
  R320 KIO Extras

BRANCH
  master

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

AFFECTED FILES
  CMakeLists.txt
  cmake/Findlibmagic.cmake
  thumbnail/CMakeLists.txt
  thumbnail/config-thumbnail.h.cmake
  thumbnail/textcreator.cpp

To: meven, #frameworks, sitter, ngraham
Cc: kde-frameworks-devel, kfm-devel, azyx, nikolaik, pberestov, iasensio, 
aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, Codezela, feverfew, 
meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, rdieter, mikesomov

D29201: KCMUtils: Add option to append service file to list of arguments

[Alexander Lohnau]

alex edited the summary of this revision.
alex added reviewers: meven, broulik.

REPOSITORY
  R295 KCMUtils

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

To: alex, #plasma, ngraham, meven, broulik
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29347: KAuthorized: export method to reload restrictions

[Albert Astals Cid]

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


  a, for using in a different repo, ok.

REPOSITORY
  R237 KConfig

BRANCH
  master

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

To: dfaure, aacid, apol, mdawson
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29372: Taiwan: Use "zh_TW" language code

[Christophe Giboudeaux]

cgiboudeaux added a comment.


  D29223  addresses the issue and needs 
review

REPOSITORY
  R175 KHolidays

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

To: weisi, winterz
Cc: cgiboudeaux, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D29223: Update Taiwanese holidays

[Christophe Giboudeaux]

cgiboudeaux added a comment.


  @nhiga Please have a look at D29372 .

REPOSITORY
  R175 KHolidays

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

To: nhiga, winterz, cgiboudeaux, shrapnel
Cc: #kde_pim, kde-frameworks-devel, shrapnel, LeGast00n, cblack, fbampaloukas, 
dcaliste, michaelh, ngraham, bruns, dvasin, rodsevich, winterz, vkrause, 
mlaurent, knauss, dvratil

D29371: KMainWindow: remove doc paragraph about KGlobal::ref usage

[David Faure]

dfaure added a comment.


  `src/kmainwindow_p.h:59:QEventLoopLocker locker;` so the refcounting 
effect is the same as documented, just with a different "backend".

REPOSITORY
  R263 KXmlGui

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

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

D28745: Skip caching thumbnails on encrypted filesystems

[Méven Car]

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

REPOSITORY
  R320 KIO Extras

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

To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns
Cc: bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, 
Codezela, feverfew, michaelh, spoorun, navarromorales, firef, andrebarros, 
emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Méven Car]

meven added inline comments.

INLINE COMMENTS

> thumbnail.cpp:738
> +#else
> +allowCache = false;
> +#endif

There is an error with the previous line, one of the two should be 
allowDirCached I think.
Here you always ignore result of `sharesFilesystemWithThumbRoot`

> thumbnail.cpp:776
> +struct stat baseStat;
> +if (!lstat(QFile::encodeName(m_thumbBasePath).data(), )) {
> +m_thumbnailDirDeviceId = baseStat.st_dev;

`!= -1`,  Add a warning with errno should it fail.

> thumbnail.cpp:781
> +struct stat fileStat;
> +return m_thumbnailDirDeviceId && !lstat(QFile::encodeName(path).data(), 
> ) && m_thumbnailDirDeviceId == fileStat.st_dev;
> +}

error check here for `!lstat(QFile::encodeName(path).data(), )` is 
important, file might have moved for instance.

REPOSITORY
  R320 KIO Extras

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

To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns
Cc: bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, 
Codezela, feverfew, michaelh, spoorun, navarromorales, firef, andrebarros, 
emmanuelp, rdieter, mikesomov