D27224: add Kongress icon

2021-01-31 Thread Ahmad Samir
ahmadsamir added a comment.


  Hello. FWIW, I would also close/abandon this diff :) (nice icon BTW).

REPOSITORY
  R266 Breeze Icons

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

To: mbruchert, dkardarakos, #vdg, ndavis
Cc: ahmadsamir, ndavis, kde-frameworks-devel, LeGast00n, cblack, michaelh, 
ngraham, bruns


Re: RFC: Switching to min Qt version 5.14 for KF on December 14th

2020-12-17 Thread Ahmad Samir

On 17/12/2020 22:06, David Faure wrote:
[...]


Right. That's a reason to fix something indeed, but there are still two ways
to fix that, if it was the only reason : either raise min req to Qt 5.14, or
ask for a Qt 5.13 CI.



Ben said on IRC:
"I used 5.14 as a practical matter as 5.13 is essentially unavailable"

That's why team jumped to 5.14 directly, don't know the details however.

[...]

--
Ahmad Samir


T8349: Improve Places panel usability and presentation

2020-10-18 Thread Ahmad Samir
ahmadsamir added subscribers: sitter, ahmadsamir.
ahmadsamir added a comment.


  ATM, the behaviour is that if you go to /foo/bar/baz/ and "bar" is present in 
the places panel, then while you're at baz or any of its sub-folders then "bar" 
is highlighted in the places panel, i.e the "closest" bookmark is highlighted. 
There is a bug report about changing the behaviour to only highlight the 
"current" folder if it's in the panel.
  
  Posting here to start discussing the issue.
  
  https://bugs.kde.org/show_bug.cgi?id=156678

TASK DETAIL
  https://phabricator.kde.org/T8349

To: ngraham, ahmadsamir
Cc: ahmadsamir, sitter, #frameworks, tomsk, bruns, michaelh, acrouthamel, 
sharvey, mmustac, jtamate, rkflx, #dolphin, ngraham, fabiogomes, waitquietly, 
azyx, dmenig, nikolaik, pberestov, manueljlin, iasensio, Orage, aprcela, 
fprice, cblack, konkinartem, ian, jguidon, Ghost6, jraleigh, fbampaloukas, 
squeakypancakes, alexde, IohannesPetros, GB_2, Codezela, feverfew, 
trickyricky26, meven, crozbo, spoorun, ndavis, navarromorales, firef, 
andrebarros, skadinna, emmanuelp, rdieter, mikesomov, aaronhoneycutt, mbohlender


Re: Dropping the moderation by default flag

2020-10-15 Thread Ahmad Samir

On 2020-07-21 21:16, Albert Astals Cid wrote:

Hi, this list has an unusual setting [for kde mailing lists] inherited from 
kde-core-devel that is that even subscribed people get moderated, and then the 
list moderator can decide to clear the moderate flag for each person one by one.

I'm proposing to change that because:
  * it's non consistent with the rest of kde mailing lists
  * as moderator of this list i don't think i've seen ever any spam coming from 
a subscribed member.

Opinions?

Cheers,
   Albert




Hello. I asked recently on #kde-devel about the kde-core-devel ML, because I sent an email there and 
it was postponed due to moderation.


Given that kde-core-devel and kde-frameworks-devel are similar, could you please set that same 
setting for kde-core-devel too?


Thanks. :)

--
Ahmad Samir


"Approve"ing an MR

2020-10-04 Thread Ahmad Samir
Hello; this is just a reminder that at the moment the "Approve" button in an MR doesn't send an 
email/notification, so please if you approve an MR also add a comment (otherwise it could be some 
while before it actually gets merged by the author of the MR).


This has been reported by bshah at 
https://gitlab.com/gitlab-org/gitlab/-/issues/231351

Have a good day :)

--
Ahmad Samir


Re: KIO on Android Failure

2020-08-18 Thread Ahmad Samir

On 18/08/2020 13:52, Ben Cooksley wrote:

Hi all,

At some point recently functionality was added to KIO which broke the
build on Android.

I'm not sure why we're building KIO on Android, but it appears that
some applications may be using it - and this in turn causes the
Dependency Build jobs to fail.

Could someone take a look please?
https://build.kde.org/job/Administration/job/Docker%20Generate%20AndroidQt5.14%20KF5-SDK/lastFailedBuild/console

Thanks,
Ben



It looks like it's caused by systemd integration in kprocessrunner; also it seems Q_OS_UNIX includes 
Android systems, so we need to guard it with !defind(Q_OS_ANDROID). Should hopefully be fixed by 
https://invent.kde.org/frameworks/kio/-/merge_requests/107 .


--
Ahmad Samir


D28460: Add KCModuleData as base class for plugin

2020-08-14 Thread Ahmad Samir
ahmadsamir closed this revision.
ahmadsamir added a comment.


  Has been moved to 
https://invent.kde.org/frameworks/kcmutils/-/merge_requests/10

REPOSITORY
  R295 KCMUtils

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

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


D28460: Add KCModuleData as base class for plugin

2020-08-14 Thread Ahmad Samir
ahmadsamir added inline comments.

INLINE COMMENTS

> kcmoduleloader.cpp:164
> +
> +QVariantList args2(args.cbegin(), args.cend());
> +

Hello. This breaks the build for Qt 5.12 on the CI, (the min. required version 
in KF is still 5.12):
https://build.kde.org/job/Frameworks/job/kcmutils/job/kf5-qt5%20SUSEQt5.12/155/console

REPOSITORY
  R295 KCMUtils

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

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


D28774: [KFontRequester] Port from QFontDialog to KFontChooserDialog

2020-07-30 Thread Ahmad Samir
ahmadsamir added a comment.


  In D28774#676000 , @volkov wrote:
  
  > Why not add support for `QPlatformTheme::FontDialog` in 
`KdePlatformTheme::createPlatformDialogHelper()`?
  
  
  Sorry about the late reply; I had looked into the platformtheme integration 
stuff, but at the time I didn't think it was a viable solution (nor was it easy 
for me to port); see https://phabricator.kde.org/D27808 for more details.

REPOSITORY
  R236 KWidgetsAddons

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

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


D28880: [KWallet] Port last usage of QRegExp to QRegularExpression

2020-06-12 Thread Ahmad Samir
ahmadsamir added a comment.


  @blaze, (I couldn't find your user name at invent.kde.org); FYI:
  https://invent.kde.org/network/falkon/-/merge_requests/7

REPOSITORY
  R311 KWallet

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

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


D29017: WIP: Introduce three new methods that return all "entries" in a folder

2020-06-07 Thread Ahmad Samir
ahmadsamir abandoned this revision.

REPOSITORY
  R311 KWallet

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

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


D29017: WIP: Introduce three new methods that return all "entries" in a folder

2020-06-07 Thread Ahmad Samir
ahmadsamir reopened this revision.
ahmadsamir marked an inline comment as done.
ahmadsamir added a comment.


  Move to gitlab https://invent.kde.org/frameworks/kwallet/-/merge_requests/2

REPOSITORY
  R311 KWallet

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

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


D29810: Don't use the setenv function after fork

2020-06-07 Thread Ahmad Samir
ahmadsamir added a comment.


  In D29810#674824 , @jpalecek wrote:
  
  > In D29810#674815 , @dfaure wrote:
  >
  > > This breaks FreeBSD compilation. Please check: 
https://build.kde.org/job/Frameworks/job/kcrash/job/kf5-qt5%20FreeBSDQt5.14/17/
  >
  >
  > I see. It needs the declaration of `environ`, which is only provided on 
GNU. Should I amend it here?
  
  
  This has been committed already, create a new review request. (Note that KDE 
has moved to Gitlab at https://invent.kde.org, best if you create a merge 
request there; Phabricator still works, but it's planned to become read-only). 
:)

REPOSITORY
  R285 KCrash

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

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


D29017: WIP: Introduce three new methods that return all "entries" in a folder

2020-06-05 Thread Ahmad Samir
This revision was not accepted when it landed; it landed in state "Needs 
Revision".
This revision was automatically updated to reflect the committed changes.
Closed by commit R311:1a19afb6a06d: WIP: Introduce three new methods that 
return all entries in a folder (authored by ahmadsamir).

REPOSITORY
  R311 KWallet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29017?vs=80670=83224

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

AFFECTED FILES
  CMakeLists.txt
  src/api/KWallet/CMakeLists.txt
  src/api/KWallet/kwallet.cpp
  src/api/KWallet/kwallet.h
  src/api/KWallet/org.kde.KWallet.xml
  src/runtime/kwalletd/backend/kwalletbackend.cc
  src/runtime/kwalletd/backend/kwalletbackend.h
  src/runtime/kwalletd/kwalletd.cpp
  src/runtime/kwalletd/kwalletd.h

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


D29017: WIP: Introduce three new methods that return all "entries" in a folder

2020-06-05 Thread Ahmad Samir
ahmadsamir added a comment.


  In D29017#664279 , @blaze wrote:
  
  > Check this commit message 
https://phabricator.kde.org/R32:f56cdda54b7f88b119f2c7cfb2f534b4fe74478f
  
  
  Sorry for the delay. We applied a workable hack in 
https://phabricator.kde.org/D28880 to get rid of the '[^/]' in the string that 
wildcardToRegularExpression returns, and it seemed to work when I tested falkon.
  
  I guess I'll have migrate this to invent.kde and try addressing the review 
comments.

REPOSITORY
  R311 KWallet

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

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


D29362: [KCharSelect] Initially give focus to the search lineedit

2020-05-31 Thread Ahmad Samir
ahmadsamir added a comment.


  Moved to https://invent.kde.org/frameworks/kwidgetsaddons/-/merge_requests/2

REPOSITORY
  R236 KWidgetsAddons

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

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


D29362: [KCharSelect] Initially give focus to the search lineedit

2020-05-31 Thread Ahmad Samir
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 R236:c0c93cc0fcb5: [KCharSelect] Initially give focus to the 
search lineedit (authored by ahmadsamir).

REPOSITORY
  R236 KWidgetsAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29362?vs=81741=83183

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

AFFECTED FILES
  src/kcharselect.cpp

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


D29445: [KOpenWithDialog] When pointing at a non-executable file print more meaningful error

2020-05-29 Thread Ahmad Samir
ahmadsamir added inline comments.

INLINE COMMENTS

> kopenwithdialog.cpp:1009
> +if (QDir::isAbsolutePath(binaryName)) {
> +QFileInfo fi(binaryName);
> +if (fi.exists() && !fi.isExecutable()) {

It's created on the next line. Anyway my initial comment was a nit-pick... it's 
a micro-optimisation for when the code goes through the if block, technically 
DesktopExecParser should get the absolute path in most cases.

REPOSITORY
  R241 KIO

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

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


D29445: [KOpenWithDialog] When pointing at a non-executable file print more meaningful error

2020-05-29 Thread Ahmad Samir
ahmadsamir added inline comments.

INLINE COMMENTS

> broulik wrote in kopenwithdialog.cpp:1008
> Actually, that doesn't exist

https://doc.qt.io/qt-5.15/qfileinfo.html#isAbsolute ?

REPOSITORY
  R241 KIO

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

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


D29782: [StatJob] Make mostLocalUrl ignore remote (ftp, http...etc) URLs

2020-05-23 Thread Ahmad Samir
ahmadsamir added a comment.


  This wasn't landed on master, but rather phabricator is confused by me 
pushing to work/ branch on gitlab :)

REPOSITORY
  R241 KIO

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

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


D29782: [StatJob] Make mostLocalUrl ignore remote (ftp, http...etc) URLs

2020-05-23 Thread Ahmad Samir
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 R241:f2984ff28e72: [StatJob] Make mostLocalUrl ignore remote 
(ftp, http...etc) URLs (authored by ahmadsamir).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D29782?vs=82974=83130#toc

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29782?vs=82974=83130

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

AFFECTED FILES
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/core/statjob.cpp
  src/core/statjob.h
  src/ioslaves/trash/tests/testtrash.cpp
  src/ioslaves/trash/tests/testtrash.h

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


D28765: KSettings::Dialog: add support for KPluginInfos without a KService

2020-05-20 Thread Ahmad Samir
ahmadsamir added a comment.


  I guess a proper fix will need to revert part of this to have the KModuleInfo 
ctor that took a KService not use KPluginInfo internally; better still, of 
course, is having the KPluginInfo ctor work for the case of 
X-KDE-ServiceTypes=SystemSettingsExternalApp, (I couldn't grok KPluginInfo 
stuff fully yet).

REPOSITORY
  R295 KCMUtils

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

To: dfaure, pino, broulik, mart, davidedmundson, ngraham, svuorela
Cc: ahmadsamir, rikmills, wbauer, kossebau, svuorela, cblack, 
kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns


D28765: KSettings::Dialog: add support for KPluginInfos without a KService

2020-05-20 Thread Ahmad Samir
ahmadsamir added a comment.


  systemsettings uses KCModuleInfo::service() to check whether the moduleinfo 
is valid[1], the problem is creating a KPluginInfo from the KService based on a 
.desktop file with X-KDE-ServiceTypes=SystemSettingsExternalApp fails because 
it doesn't seem to have valid metadata. Since the KPluginInfo ctor fails, 
accessing d->pluginInfo.service() causes a crash, I've submitted a proposed fix 
at [2]. This doesn't fix the bug, but merely prevents the crash.
  
  [1] 
https://invent.kde.org/plasma/systemsettings/-/blob/master/core/ModuleView.cpp#L229
  [2] https://invent.kde.org/frameworks/kcmutils/-/merge_requests/1

REPOSITORY
  R295 KCMUtils

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

To: dfaure, pino, broulik, mart, davidedmundson, ngraham, svuorela
Cc: ahmadsamir, rikmills, wbauer, kossebau, svuorela, cblack, 
kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns


D28673: [PackageUrlInterceptor] Make QRegularExpression static

2020-05-19 Thread Ahmad Samir
ahmadsamir commandeered this revision.
ahmadsamir added a reviewer: broulik.
ahmadsamir added a comment.


  Should be handled by 
https://invent.kde.org/frameworks/plasma-framework/-/merge_requests/2

REPOSITORY
  R242 Plasma Framework (Library)

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

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


D28673: [PackageUrlInterceptor] Make QRegularExpression static

2020-05-19 Thread Ahmad Samir
ahmadsamir abandoned this revision.

REPOSITORY
  R242 Plasma Framework (Library)

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

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


D29800: Fix URL being passed as argument when launching a .desktop file

2020-05-17 Thread Ahmad Samir
ahmadsamir added a comment.


  In D29800#672399 , @fvogt wrote:
  
  > Landed to invent - hopefully correctly: 
https://invent.kde.org/frameworks/kio/commit/84e9372f4fa2636f57dc456ac2fa2be271d6a7ec
  
  
  LGTM.

REPOSITORY
  R241 KIO

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

To: fvogt, dfaure, marten
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns


D29558: Add KIO::OpenUrlJob::setShowOpenWithDialog as replacement for KRun::displayOpenWithDialog

2020-05-17 Thread Ahmad Samir
ahmadsamir added a comment.


  In D29558#672395 , @dfaure wrote:
  
  > Not committed after all.
  
  
  It was committed, as I had it when I did git pull, but you lucked out, it 
seemed to have been eaten by the migration to gitlab somehow; I had to 'git 
reset --hard' to be in sync with invent.kde.org :)

REPOSITORY
  R241 KIO

BRANCH
  2020_05_displayOpenWithDialog

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

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


D29787: Fix krununittest

2020-05-17 Thread Ahmad Samir
ahmadsamir added a comment.


  In D29787#672139 , @dfaure wrote:
  
  > Let's hope the CI has xterm installed I doubt it. Maybe we'll need to 
pick "ls" instead, even if that will look weird.
  
  
  The CI seems to have xterm available as the unit test passed (and then after 
the switch to gitlab, a new CI setup will be available, let's hope it has xterm 
too :)).

REPOSITORY
  R241 KIO

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

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


D29787: Fix krununittest

2020-05-16 Thread Ahmad Samir
This revision was automatically updated to reflect the committed changes.
Closed by commit R241:ef7708d1f5ec: Fix krununittest (authored by ahmadsamir).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29787?vs=82975=83011

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

AFFECTED FILES
  autotests/krununittest.cpp

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


D29610: [kio_file] Handle renaming file 'A' to 'a' on FAT32 filesystems

2020-05-16 Thread Ahmad Samir
This revision was automatically updated to reflect the committed changes.
Closed by commit R241:bed09e39fe24: [kio_file] Handle renaming file 
A to a on FAT32 filesystems (authored by ahmadsamir).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29610?vs=82750=83010

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

AFFECTED FILES
  src/core/copyjob.cpp
  src/ioslaves/file/file_unix.cpp

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


D29782: [StatJob] Make mostLocalUrl ignore remote (ftp, http...etc) URLs

2020-05-16 Thread Ahmad Samir
ahmadsamir added a comment.


  I've just realised, we won't get an error with an http url; in that case we 
return the url the statjob was called on as-is and cancel the job.

REPOSITORY
  R241 KIO

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

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


D29782: [StatJob] Make mostLocalUrl ignore remote (ftp, http...etc) URLs

2020-05-16 Thread Ahmad Samir
ahmadsamir added a comment.


  In D29782#672157 , @dfaure wrote:
  
  > Can you add a unittest for a KIO::mostLocalUrl() in testtrash.cpp (which is 
:local, so it should work)
  
  
  I don't see where the trash ioslave sets UDS_LOCAL_PATH; I think it doesn't 
set it, I could be wrong, though.
  
  > and another one in jobtest.cpp for a http URL (e.g. to test which error 
code we're getting, if any?)
  
  I'll look into that next.
  
  > + call mostLocalUrl() on a "normal" StatJob in testtrash.cpp
  
  Same as the first point :)
  
  [...]

REPOSITORY
  R241 KIO

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

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


D29787: Fix krununittest

2020-05-15 Thread Ahmad Samir
ahmadsamir created this revision.
ahmadsamir added reviewers: Frameworks, dfaure.
Herald added a project: Frameworks.
ahmadsamir requested review of this revision.

REVISION SUMMARY
  After 6452a34cf01d03d316 
, 
DesktopExecParser::resultingArguments() needs
  to find the terminal app binary, otherwise it returns an empty QStringList.

TEST PLAN
  krununittest passes again

REPOSITORY
  R241 KIO

BRANCH
  l-terminal (branched from master)

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

AFFECTED FILES
  autotests/krununittest.cpp

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


D29782: [StatJob] Make mostLocalUrl ignore remote (ftp, http...etc) URLs

2020-05-15 Thread Ahmad Samir
ahmadsamir updated this revision to Diff 82974.
ahmadsamir retitled this revision from "[StatJob] Change mostLocalUrl to only 
handle protocols with Class=:local" to "[StatJob] Make mostLocalUrl ignore 
remote (ftp, http...etc) URLs".
ahmadsamir edited the summary of this revision.
ahmadsamir edited the test plan for this revision.
ahmadsamir added a comment.


  Improve

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29782?vs=82959=82974

BRANCH
  l-most-local-url-local (branched from master)

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

AFFECTED FILES
  src/core/statjob.cpp
  src/core/statjob.h

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


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

2020-05-15 Thread Ahmad Samir
ahmadsamir added a comment.


  In D29385#671987 , @dfaure wrote:
  
  > That's the point, a NFS mount *is* a local URL, so we do use QFile for it. 
And then it takes forever because the kernel has to talk over a socket to 
answer us.
  >  Yes this is horrendous. I hate network mounts :-)
  
  
  I don't have *any* network mounts, never have, so I wasn't aware of that. 
Yep, that is horrendous.

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


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

2020-05-15 Thread Ahmad Samir
ahmadsamir added a comment.


  In D29385#671980 , @dfaure wrote:
  
  > It's 3 times faster on my local SSD.
  >
  > Now think of a NFS mount on a server from another country
  
  
  I was thinking mostly of QFile when url.isLocalFile() is true, but yeah I see 
your point :)

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


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

2020-05-15 Thread Ahmad Samir
ahmadsamir added inline comments.

INLINE COMMENTS

> dfaure wrote in openurljob.cpp:261
> 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.]

> 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.

I know this ship has sailed (well, sunk in this case :)), but if it's 3 times 
faster to use QFile, then is it really a "blocking I/O" operation? it's too 
fast to be "blocking"...

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


D29782: [StatJob] Change mostLocalUrl to only handle protocols with Class=:local

2020-05-15 Thread Ahmad Samir
ahmadsamir planned changes to this revision.

REPOSITORY
  R241 KIO

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

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


D29558: Add KIO::OpenUrlJob::setShowOpenWithDialog as replacement for KRun::displayOpenWithDialog

2020-05-15 Thread Ahmad Samir
ahmadsamir added inline comments.

INLINE COMMENTS

> kossebau wrote in openurljob.h:120
> You meant, according to 
> https://community.kde.org/Frameworks/Frameworks_Documentation_Policy#Document_Public_and_Protected_Members
>  :)

I got the info from a commit in kwidgetsaddons where you "fixed" a previous 
commit of mine :)

Thanks for the link though.

REPOSITORY
  R241 KIO

BRANCH
  2020_05_displayOpenWithDialog

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

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


D29558: Add KIO::OpenUrlJob::setShowOpenWithDialog as replacement for KRun::displayOpenWithDialog

2020-05-15 Thread Ahmad Samir
ahmadsamir accepted this revision.
ahmadsamir added a subscriber: kossebau.
ahmadsamir added inline comments.
This revision is now accepted and ready to land.

INLINE COMMENTS

> openurljob.h:120
> + *
> + * @param b whether to only show the "open with" dialog.
> + */

It seems that we shouldn't end @param with a ".", according to @kossebau 
anyway...

REPOSITORY
  R241 KIO

BRANCH
  2020_05_displayOpenWithDialog

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

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


D29782: [StatJob] Change mostLocalUrl to only handle protocols with Class=:local

2020-05-15 Thread Ahmad Samir
ahmadsamir created this revision.
ahmadsamir added reviewers: Frameworks, dfaure, sitter.
Herald added a project: Frameworks.
ahmadsamir requested review of this revision.

REVISION SUMMARY
  Previously mostLocalUrl would check that !url.isLocalFile(), that meant
  a statjob would be fired for remote urls, such urls will never have a
  mostLocalUrl. Instead check for protocols with Class=:local.
  
  For a list of such protocols: `grep :local /usr/share/kservices5/*.protocol`
  
  Thanks to sitter for figuring it out in the bug report.
  
  BUG: 420985
  FIXED-IN: 5.71

TEST PLAN
  make && ctest

REPOSITORY
  R241 KIO

BRANCH
  l-most-local-url-local (branched from master)

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

AFFECTED FILES
  src/core/statjob.cpp
  src/core/statjob.h

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


D29610: [kio_file] Handle renaming file 'A' to 'a' on FAT32 filesystems

2020-05-15 Thread Ahmad Samir
ahmadsamir added inline comments.

INLINE COMMENTS

> dfaure wrote in file_unix.cpp:1052
> I'm confused. We want to solve renaming 'a' to 'A' when 'A' does *not* exist.
> 
> QFile::rename will not overwrite an existing file, so it will do nothing if 
> dest exists.

const QByteArray dest1 = "/mnt/fat32/A";
  const QByteArray dest2 = "/mnt/fat32/a";
  QT_STATBUF buff_dest;
  qDebug() << QT_LSTAT(dest1, _dest);
  qDebug() << QT_LSTAT(dest2, _dest);

IIUC, from FAT32 POV, 'A' exists if 'a' exists and vice versa.

> dfaure wrote in file_unix.cpp:1074
> Wouldn't it be enough to just call QFile::rename here?
> 
> The whole idea is: if QFile::rename is able to rename a file in all cases, 
> including the a->A special case on FAT, then let's just delegate the renaming 
> to QFile.
> 
> Then we don't need to have any special case in our code.
> 
> QFile::rename does not overwrite, though, so if the dest exists and the 
> Overwrite flag is set, we might have to either delete the dest first (race 
> condition, not sure it matters here), or in *that* case use ::rename() since 
> we know it can't be a FAT32-case-change (FAT32 can't have both a and A).

Too many quirks if we don't use ::rename(). I tested with e.g. in file_unix.cpp:

  bool renamed =  QFile::rename(src, dest);
  if (!renamed) {
  if ((_flags & KIO::Overwrite) || isSrcSymLink) {
  renamed = ::rename(_src.data(), _dest.data()) == 0;
  }
  }

- QFile::rename doesn't overwrite as you said
- renaming symlinks depends on ::rename
- some unit tests are failing due to permissions

  FAIL!  : JobTest::statDetailsBasic() Compared values are not the same
 Actual   (kioItem.permissions()): 420
 Expected (438)  : 438
 Loc: [/home/ahmad/dev/kio/autotests/jobtest.cpp(1464)]
  INFO   : JobTest::statDetailsBasicSetDetails() entering
  FAIL!  : JobTest::statDetailsBasicSetDetails() Compared values are not the 
same
 Actual   (kioItem.permissions()): 420
 Expected (438)  : 438
 Loc: [/home/ahmad/dev/kio/autotests/jobtest.cpp(1503)]

That and ::rename() is used all over the place in file_unix.cpp, I am not that 
comfortable using QFile::rename except for the freaky FAT32 case :)

REPOSITORY
  R241 KIO

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

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


D29610: [kio_file] Handle renaming file 'A' to 'a' on FAT32 filesystems

2020-05-13 Thread Ahmad Samir
ahmadsamir added a comment.


  In D29610#667858 , @dfaure wrote:
  
  > OK for now, to fix the unittests. The *real* fix however is to use 
QFile::rename in kio_file so that this failure to rename doesn't even happen in 
the first place.
  >
  > In this commit can you at least leave a TODO to that effect?
  
  
  Took me a long while "to do" it.
  
  So many quirks, bin/jobtest shows output (qDebug() << 'blah bleh' in e.g. 
file_unix.cpp) that 'ctest -V -R kiocore-jobtest' for some reason doesn't show. 
And of course kdeinit5.
  
  So it's CopyJob -> SimpleJob -> SlaveBase -> FileProtocol, so many 
redirections and pinging from one place in the code to the other, fun stuff... 
:)

INLINE COMMENTS

> dfaure wrote in copyjob.cpp:1965
> Why this check? The next line compares absolute paths -- including the parent 
> dir.
> 
> Hmm OK one could manufacture a special case with /dir/file and /DIR/FILE 
> where the compare() passes but the parent dirs differ. The orig code had that 
> bug... OK :)

Good point, I missed that the original code was checking the file names and the 
paths with the same compare() call.

About /dir/file and /DIR/FILE, the parent dirs is actually one dir since "dir" 
and "DIR" are the same from FAT32 old POV?

REPOSITORY
  R241 KIO

BRANCH
  l-qfile-rename (branched from master)

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

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


D29610: [kio_file] Handle renaming file 'A' to 'a' on FAT32 filesystems

2020-05-13 Thread Ahmad Samir
ahmadsamir updated this revision to Diff 82750.
ahmadsamir retitled this revision from "[CopyJob] Use stricter conditions when 
using QFile::rename in slotResultRenaming" to "[kio_file] Handle renaming file 
'A' to 'a' on FAT32 filesystems".
ahmadsamir edited the summary of this revision.
ahmadsamir edited the test plan for this revision.
ahmadsamir added a comment.


  Move the code to kio_file

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29610?vs=82464=82750

BRANCH
  l-qfile-rename (branched from master)

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

AFFECTED FILES
  src/core/copyjob.cpp
  src/ioslaves/file/file_unix.cpp

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


D29610: [CopyJob] Use stricter conditions when using QFile::rename in slotResultRenaming

2020-05-10 Thread Ahmad Samir
ahmadsamir created this revision.
ahmadsamir added reviewers: Frameworks, dfaure.
Herald added a project: Frameworks.
ahmadsamir requested review of this revision.

REVISION SUMMARY
  The code now uses QFile::rename() only if direct renaming fails and we're
  moving a file/dir e.g. 'A' to 'a' on a case-insensitive filesystem such
  as FAT3. This matches the behaviour from before commit cb89bab36a5aa4e78c 
,
  and also fixes the jobtest moveFileDestAlreadyExists unit test.

TEST PLAN
  jobtest unit test passes again
  Moving a dir 'A' to 'a' on a FAT32 partition still works

REPOSITORY
  R241 KIO

BRANCH
  l-qfile-rename (branched from master)

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

AFFECTED FILES
  src/core/copyjob.cpp

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


D29547: KRun: deprecate all static 'run*' methods, with full porting instructions

2020-05-10 Thread Ahmad Samir
ahmadsamir accepted this revision.
ahmadsamir added inline comments.
This revision is now accepted and ready to land.

INLINE COMMENTS

> dfaure wrote in krun.h:216
> I did, but Friedrich had a less confusing suggestion:
> 
> @deprecated since 5.6. Since 5.71 use ApplicationLauncherJob, otherwise 
> runApplication instead.

Yeah, makes sense; the point is keeping the users of the class informed/aware 
anyway.

REPOSITORY
  R241 KIO

BRANCH
  2020_05_deprecate_KRun_run_methods

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

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


D29547: KRun: deprecate all static 'run*' methods, with full porting instructions

2020-05-10 Thread Ahmad Samir
ahmadsamir added inline comments.

INLINE COMMENTS

> krun.h:216
> + * @deprecated since 5.6, use runApplication instead.
> + * @deprecated since 5.71, use ApplicationLauncherJob instead.
> + * @code

I don't think you want both @deprecated?

> krun.h:229
>   */
> -KIOWIDGETS_DEPRECATED_VERSION(5, 6, "Use KRun::runApplication(const 
> KService &, const QList &, QWidget *, RunFlags, const QString &, const 
> QByteArray &")
> +KIOWIDGETS_DEPRECATED_VERSION(5, 6, "Use KIO::ApplicationLauncherJob, 
> see API docs for a code sample")
>  static bool run(const KService , const QList , 
> QWidget *window,

You meant "5, 71"

> krun.h:369
>   * @return @c true on success, @c false on error
> - * @deprecated since 5.31, use runUrl() with RunFlags instead.
> + * @deprecated since 5.31, use OpenUrlJob instead.
> + * @code

But OpenUrlJob is in 5.71 not 5.31? people will be reading that on api.kde.org 
not only in 5.71 header files, right?
maybe:
@deprecated since 5.31
Since 5.71, use OpenUrlJob instead.

> krun.h:380
>   */
> -KIOWIDGETS_DEPRECATED_VERSION(5, 31, "Use KRun::const QUrl &, const 
> QString &, QWidget *, RunFlags, const QString &, const QByteArray &")
> +KIOWIDGETS_DEPRECATED_VERSION(5, 31, "Use KIO::OpenUrlJob, see API docs 
> for a code sample")
>  static bool runUrl(const QUrl , const QString , QWidget 
> *window,

The same issue 5.31 vs 5.71.

> krun.h:452
>   * @param asn Application startup notification id, if any (otherwise "").
>   * @return @c true on success, @c false on error
> + * @code

Missing @deprecated.

> krun.h:599
>   */
> -void slotTimeout(); // KDE5: rename to slotNextStep() or something like 
> that
> +void slotTimeout();
>  

So, you're keeping the name slotTimout? I guess after 5-10 years, no point 
changing it :)

REPOSITORY
  R241 KIO

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

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


D29599: [CopyJob] Try to fix windows build

2020-05-10 Thread Ahmad Samir
This revision was automatically updated to reflect the committed changes.
Closed by commit R241:d026227574b8: [CopyJob] Try to fix windows build 
(authored by ahmadsamir).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29599?vs=82444=82447

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

AFFECTED FILES
  src/core/copyjob.cpp

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


D29599: [CopyJob] Try to fix windows build

2020-05-10 Thread Ahmad Samir
ahmadsamir created this revision.
ahmadsamir added reviewers: Frameworks, dfaure.
Herald added a project: Frameworks.
ahmadsamir requested review of this revision.

REVISION SUMMARY
  The windows build is failing on the CI because of S_IWUSR; include
  kioglobal_p.h to try and fix that.

REPOSITORY
  R241 KIO

BRANCH
  l-fix-win-build (branched from master)

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

AFFECTED FILES
  src/core/copyjob.cpp

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


D29485: [CopyJob] Check free space for remote urls before copying and other improvements

2020-05-09 Thread Ahmad Samir
ahmadsamir added inline comments.

INLINE COMMENTS

> dfaure wrote in copyjob.cpp:444
> I'm a bit confused. My explanation here points to kio_desktop and kio_remote 
> (and was apparently clear), and the API docs for UDS_LOCAL_PATH say
> 
>   /// A local file path if the ioslave display files sitting
>   /// on the local filesystem (but in another hierarchy, e.g. settings:/ or 
> remote:/)
> 
> which is basically the same information? What is unclear?

> to map URLs from kioslaves-that-wrap-the-local-file-system back to local 
> paths.

That made much more sense to me, reading the docs again now, I know what it 
meant, but I did not before.

REPOSITORY
  R241 KIO

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

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


D29485: [CopyJob] Check free space for remote urls before copying and other improvements

2020-05-09 Thread Ahmad Samir
This revision was automatically updated to reflect the committed changes.
Closed by commit R241:1cac602d9966: [CopyJob] Check free space for remote urls 
before copying and other improvements (authored by ahmadsamir).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29485?vs=82334=82356

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

AFFECTED FILES
  src/core/copyjob.cpp

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


D29485: [CopyJob] Check free space for remote urls before copying and other improvements

2020-05-09 Thread Ahmad Samir
ahmadsamir added a comment.


  @dfaure please don't forget to flesh out the UDS_LOCAL_PATH docs 
https://phabricator.kde.org/D29485#inline-169199

REPOSITORY
  R241 KIO

BRANCH
  l-freespace-remote-2 (branched from master)

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

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


D29485: [CopyJob] Check free space for remote urls before copying and other improvements

2020-05-09 Thread Ahmad Samir
ahmadsamir added inline comments.

INLINE COMMENTS

> dfaure wrote in copyjob.cpp:477
> I'm assuming the TODO was about "What if I'm using a NFS mount and the 
> connection breaks at the time of KDiskFreeSpaceInfo, i.e. what should we do 
> about error handling".
> 
> But I think the current code -- which ignores errors and moves on, both for 
> local and now for remote files, actually makes most sense. This is after all 
> just a preliminary check. The worst that will happen is that there will 
> indeed not be enough room and the copy will fail. But that's better than not 
> trying at all, possibly due to a bug in one of those two classes, or possibly 
> because of intermittent network failures.

That makes sense :)

REPOSITORY
  R241 KIO

BRANCH
  l-freespace-remote-2 (branched from master)

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

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


D29485: [CopyJob] Check free space for remote urls before copying and other improvements

2020-05-09 Thread Ahmad Samir
ahmadsamir updated this revision to Diff 82334.
ahmadsamir added a comment.


  Remove comment

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29485?vs=82305=82334

BRANCH
  l-freespace-remote-2 (branched from master)

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

AFFECTED FILES
  src/core/copyjob.cpp

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


D29485: [CopyJob] Check free space for remote urls before copying and other improvements

2020-05-09 Thread Ahmad Samir
ahmadsamir added inline comments.

INLINE COMMENTS

> dfaure wrote in copyjob.cpp:477
> This comment is now completely out of context. It used to be about NFS/SMB, 
> now this information has gone and one is left wondering why kind of 
> connections we're talking about (connection to the kioslave???)

I meant connection to the remote ulr/host; however e.g Dolphin already reports 
those connection errors in the status bar when it can't connect, so the comment 
is redundant now... (I tried to keep the original code/comments in tact, 
apparently that backfired spectacularly).

REPOSITORY
  R241 KIO

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

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


D29485: [CopyJob] Check free space for remote urls before copying and other improvements

2020-05-08 Thread Ahmad Samir
ahmadsamir updated this revision to Diff 82305.
ahmadsamir marked an inline comment as done.
ahmadsamir added a comment.


  "existingDest" is a better name for the var relating to m_asMethod

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29485?vs=82285=82305

BRANCH
  l-freespace-remote-2 (branched from master)

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

AFFECTED FILES
  src/core/copyjob.cpp

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


D29485: [CopyJob] Check free space for remote urls before copying and other improvements

2020-05-08 Thread Ahmad Samir
ahmadsamir marked an inline comment as done.
ahmadsamir added inline comments.

INLINE COMMENTS

> dfaure wrote in copyjob.cpp:430
> This is the same as "actualDest" too, so its definition could be moved 
> further up and shared with this too.
> 
> (Not that the name is perfect --- when copying ~/file.txt to ~/file2.txt, the 
> actual destination is ~/file2.txt.
> Or copying ~/dir1 as ~/dir2, then the actual destination is ~/dir2. So even a 
> name like destDir isn't perfect...)
> 
> existingDest maybe. ~ exists. ~/dir2 not yet.

> This is the same as "actualDest" too, so its definition could be moved 
> further up and shared with this too.

The code handling UDS_LOCAL_PATH is in between them, it may change m_dest.

And "existingDest" it is.

REPOSITORY
  R241 KIO

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

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


D29528: [OpenUrlJob] Improve comments/docs

2020-05-08 Thread Ahmad Samir
This revision was automatically updated to reflect the committed changes.
Closed by commit R241:1b9b239eb262: [OpenUrlJob] Improve comments/docs 
(authored by ahmadsamir).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29528?vs=82286=82304

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

AFFECTED FILES
  src/gui/openurljob.cpp
  src/gui/openurljob.h
  src/gui/openurljobhandlerinterface.h
  src/widgets/kdesktopfileactions.cpp
  src/widgets/kopenwithdialog.cpp
  src/widgets/krun.cpp
  src/widgets/widgetsopenurljobhandler.h

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


D29537: [CopyJob] Get rid of an old TODO and use QFile::rename()

2020-05-08 Thread Ahmad Samir
This revision was automatically updated to reflect the committed changes.
Closed by commit R241:cb89bab36a5a: [CopyJob] Get rid of an old TODO and use 
QFile::rename() (authored by ahmadsamir).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29537?vs=82288=82303

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

AFFECTED FILES
  src/core/copyjob.cpp

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


D29537: [CopyJob] Get rid of an old TODO and use QFile::rename()

2020-05-08 Thread Ahmad Samir
ahmadsamir created this revision.
ahmadsamir added reviewers: Frameworks, dfaure.
Herald added a project: Frameworks.
ahmadsamir requested review of this revision.

TEST PLAN
  Renaming a dir A to a on a FAT32 partition still works

REPOSITORY
  R241 KIO

BRANCH
  l-qfile-rename (branched from master)

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

AFFECTED FILES
  src/core/copyjob.cpp

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


D29528: [OpenUrlJob] Improve comments/docs

2020-05-08 Thread Ahmad Samir
ahmadsamir updated this revision to Diff 82286.
ahmadsamir marked an inline comment as done.
ahmadsamir added a comment.


  Address comments

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29528?vs=82260=82286

BRANCH
  l-late (branched from master)

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

AFFECTED FILES
  src/gui/openurljob.cpp
  src/gui/openurljob.h
  src/gui/openurljobhandlerinterface.h
  src/widgets/kdesktopfileactions.cpp
  src/widgets/kopenwithdialog.cpp
  src/widgets/krun.cpp
  src/widgets/widgetsopenurljobhandler.h

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


D29528: [OpenUrlJob] Improve comments/docs

2020-05-08 Thread Ahmad Samir
ahmadsamir marked 2 inline comments as done.
ahmadsamir added inline comments.

INLINE COMMENTS

> dfaure wrote in openurljob.cpp:590
> That one was on purpose. I find this version less readable, mixing a test and 
> an actual action (with error handling).

Fair point.

REPOSITORY
  R241 KIO

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

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


D29485: [CopyJob] Check free space for remote urls before copying and other improvements

2020-05-08 Thread Ahmad Samir
ahmadsamir updated this revision to Diff 82285.
ahmadsamir marked 2 inline comments as done.
ahmadsamir added a comment.


  Address comments

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29485?vs=82113=82285

BRANCH
  l-freespace-remote-2 (branched from master)

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

AFFECTED FILES
  src/core/copyjob.cpp

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


D29485: [CopyJob] Check free space for remote urls before copying and other improvements

2020-05-08 Thread Ahmad Samir
ahmadsamir marked 2 inline comments as done.
ahmadsamir added inline comments.

INLINE COMMENTS

> dfaure wrote in copyjob.cpp:430
> Here you kept a comment that said "we want to check", but the check already 
> happened.
> I'd say just remove the two lines of comments.
> The code is clearer without them.

Copy-paste-comment logic mistake.

> dfaure wrote in copyjob.cpp:433
> That is not a path, for remote URLs. I suggest using 
> dest.toDisplayString(QUrl::PreferLocalFiles) and just inlining that in the 
> setErrorText call.

path as in "destination dir", but indeed technically it's not the 'path' part 
for remote urls. And toDisplayString will remove any passwords (not sure there 
may be passwords here, but still, safer that way).

> dfaure wrote in copyjob.cpp:444
> I think this check is too early.
> 
> UDS_LOCAL_PATH is also set by kio_desktop and kio_remote, for instance.
> It's the main point of that entry: to map URLs from 
> kioslaves-that-wrap-the-local-file-system back to local paths.
> 
> AFAICS the old code would use UDS_LOCAL_PATH also for non-local-file URLs.

Excellent points; I was scratching my head trying to figure out what use 
UDS_LOCAL_PATH is when, while testing with qDebug(), it was _always_ empty, the 
only time it wasn't empty was in testtrash unit test, some code there set it, 
used it or something. So I resorted to grep'ing the code and I found the 
KCoreDirLister::cachedItemForUrl instance (which answers your next comment). So 
someone who knows all the use-cases of UDS_LOCAL_PATH (you in this case) 
should/must update the UDS_LOCAL_PATH docs :D

> dfaure wrote in copyjob.cpp:478
> The lambda is called after going back to the event loop, so this will have 
> happened anyway, no?
> I don't get it.
> 
> On the other hand I'm fine if this is done here, I'm just not sure why the 
> comment says it has to be so.
> 
> (Easy solution is to remove that comment, especially given the suggestion 
> below it won't even seem weird to do things in this order)

"The lambda is called after going back to the event loop, so this will have 
happened anyway, no?"
Good point (thanks for the info, I didn't think of the event loop POV).

However, right after the connect(), 'return' is called, so as not to call 
statCurrentSrc() on line 502, I want it called from the lambda _after_ the free 
space check job finishes. In one of the iterations working on this patch I got 
a race, where it would sometimes work and error out if there isn't enough free 
space at a remote dest, but some other times it would start copying; hence the 
"must" part; I don't recall the exact details though (I've been working on it 
for a couple of days, I tried so many stuff, the details of the trial and error 
are mushed all together at this point sorry :/)

So, I'll remove the comment or improve it to convey the idea in a clearer way.

> copyjob.cpp:479
> +// Must do this here before statCurrentSrc() is called in the lambda 
> connected to
> +// FileSystemFreeSpaceJob below
>  q->removeSubjob(job);

s/below/below, because return is called right after it, so that statCurrentSrc 
is called from the lambda/

does that sound better?

> dfaure wrote in copyjob.cpp:485
> Maybe you can move the if (m_dest.isLocalFile()) block here, and use `else`.
> It'll be clearer that free space check happens in both cases.

Looking at it again, removeSubjob can be called before the free disk space 
check, because it's statCurrentSrc that adds a new subJob (I was worried of 
calling removeSubjob too early).

> dfaure wrote in copyjob.cpp:486
> ... and this line is the same in both code paths, so it could be extracted to 
> before the if().
> 
> To avoid confusion between dest and m_dest, maybe rename this var to 
> destToCheck ?

actualDest sounds nicer.

REPOSITORY
  R241 KIO

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

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


D29528: [OpenUrlJob] Improve comments/docs

2020-05-08 Thread Ahmad Samir
ahmadsamir created this revision.
ahmadsamir added reviewers: Frameworks, dfaure.
Herald added a project: Frameworks.
ahmadsamir requested review of this revision.

REPOSITORY
  R241 KIO

BRANCH
  l-late (branched from master)

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

AFFECTED FILES
  src/gui/openurljob.cpp
  src/gui/openurljob.h
  src/gui/openurljobhandlerinterface.h
  src/widgets/kdesktopfileactions.cpp
  src/widgets/kopenwithdialog.cpp
  src/widgets/krun.cpp
  src/widgets/widgetsopenurljobhandler.h

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


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

2020-05-08 Thread Ahmad Samir
ahmadsamir added a comment.


  In D29385#664552 , @dfaure wrote:
  
  > -void KIO::OpenUrlJob::setRunFlags(KIO::ApplicationLauncherJob::RunFlags 
runFlags)
  >  +void KIO::OpenUrlJob::setDeleteTemporaryFile(bool b)
  >
  > The more I think about it, the least I like the use of flags here.
  >
  > 1. they are from the wrong class as Kai-Uwe pointed out, but more 
importantly:
  > 2. the other bool setters here are for unrelated concerns, better keep them 
separate.
  >
  >   Are we doing lineEdit->setDragEnabled(true); 
lineEdit->setClearButtonEnabled(true); lineEdit->setReadOnly(true); or are we 
doing lineEdit->setFlags(QLineEdit::DragEnabled | QLineEdit::ClearButtonEnabled 
| QLineEdit::ReadOnly)?
  
  
  I vote:
  lineEdit->setDragEnabled(true); lineEdit->setClearButtonEnabled(true); 
lineEdit->setReadOnly(true);
  it's:
  
  - more readable, and easier to use (similar methods are used through out Qt 
code)
  - avoids the case you talked about, of the user setting nonsensical, from the 
code POV, flags.
  
  I was too slow working my way through this short story^W^W review, so I've 
gathered my other comments (mostly code style, docs changes...etc) in a diff, 
that I'll submit shortly.

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


D25339: update lineHeight if boundingRect indicates a larger value.

2020-05-07 Thread Ahmad Samir
ahmadsamir added a comment.


  In D25339#665827 , @xuetianweng 
wrote:
  
  >
  
  
  [...]
  
  > As for higher line, it might not as bad as you thought as it actually might 
improve readability for many people.
  
  I agree with this statement :). Thanks to this diff I found out where the 
line height can be manipulated; the way the code computed it, fontHeight was 34 
(IIRC), I've hardcoded it to 40 and I very much prefer reading text that way.
  
  Note that Konsole tries and compute a sane line height to accommodate CJK 
characters... etc, but it also has a config option to change the line height 
(Settings -> Appearance -> Miscellaneous)... the code can be smart, but it 
can't know how a user prefers to read text, there is no one-size fits all.

REPOSITORY
  R39 KTextEditor

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

To: xuetianweng, #ktexteditor, cullmann, dhaumann, #frameworks, rjvbb
Cc: ahmadsamir, brauch, sars, pshinjo, rjvbb, fakefred, anthonyfieroni, 
kde-frameworks-devel, kwrite-devel, rrosch, LeGast00n, cblack, domson, 
michaelh, ngraham, bruns, demsking, cullmann, dhaumann


D29485: [CopyJob] Check free space for remote urls before copying and other improvements

2020-05-06 Thread Ahmad Samir
ahmadsamir added a comment.


  I couldn't seem to test the m_privilegeExecutionEnabled stuff, i.e. using 
dolphin, the paste action is disabled if the dir isn't owned by me.

REPOSITORY
  R241 KIO

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

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


D29485: [CopyJob] Check free space for remote urls before copying and other improvements

2020-05-06 Thread Ahmad Samir
ahmadsamir created this revision.
ahmadsamir added reviewers: Frameworks, dfaure, meven, sitter.
Herald added a project: Frameworks.
ahmadsamir requested review of this revision.

REVISION SUMMARY
  Use KIO::FileSystemFreeSpaceJob to check free space for remote urls.
  
  Thanks to sitter for pin-pointing the responsible code in the bug report,
  and to kbroulik for pointing out the existence of KIO::FileSystemFreeSpaceJob.
  
  Also use UDSEntry to check writability for both local and remote urls.
  
  BUG: 418443
  
  FIXED-IN: 5.71

TEST PLAN
  testtrash unit test fails (again...)
  
  - Find/create a local partition with a small capacity, copying a file/dir 
that is bigger than the partition will show the error message as before
  - Do the same for a remote url (I tested with sftp://), an error message 
about not having enough free space is shown

REPOSITORY
  R241 KIO

BRANCH
  l-freespace-remote-2 (branched from master)

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

AFFECTED FILES
  src/core/copyjob.cpp

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


D29445: [KOpenWithDialog] When pointing at a non-executable file print more meaningful error

2020-05-05 Thread Ahmad Samir
ahmadsamir added inline comments.

INLINE COMMENTS

> dfaure wrote in kopenwithdialog.cpp:1006
> Right, hence the isEmpty() here. This check passes if 1) it doesn't exist, or 
> 2) it's not executable.

I was wondering how the QString returned by 
KIO::DesktopExecParser::executablePath() would work if it returned /usr/bin/foo 
(as it doesn't strip the path), so how does findExecutable() work in that case? 
... so I tested and it turns out, findExecutable() will work with:

- foo, if it can find it in $PATH and it's executable
- /usr/bin/foo, if it's executable (though I have to wonder how that qualifies 
as "executableName"?)
- /some/path/foo, as long as "foo" is executable, the docs don't say anything 
about this behaviour, however the implementation does indeed support this

This change covers the use case of an absolute path to a file that _exists_ but 
isn't _executable_.

And again, findExecutable() would be more useful if it reported some sort of 
error saying "I found it, but it's not executable".

> kopenwithdialog.cpp:1008
> +// Maybe it exists but isn't executable
> +if (QDir::isAbsolutePath(binaryName)) {
> +QFileInfo fi(binaryName);

Why not QFileInfo::isAbsolute()?

REPOSITORY
  R241 KIO

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

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


D29427: [KBookmarkMenu] Assign m_actionCollection early to prevent crash

2020-05-04 Thread Ahmad Samir
This revision was automatically updated to reflect the committed changes.
Closed by commit R294:8a282319bc9e: [KBookmarkMenu] Assign m_actionCollection 
early to prevent crash (authored by ahmadsamir).

REPOSITORY
  R294 KBookmarks

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29427?vs=81934=81950

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

AFFECTED FILES
  src/kbookmarkmenu.cpp
  src/kbookmarkmenu.h

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


D29427: [KBookmarkMenu] Assign m_actionCollection early to prevent crash

2020-05-04 Thread Ahmad Samir
ahmadsamir added a reviewer: nicolasfella.

REPOSITORY
  R294 KBookmarks

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

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


D29427: [KBookmarkMenu] Assign m_actionCollection early to prevent crash

2020-05-04 Thread Ahmad Samir
ahmadsamir added a reviewer: kossebau.

REPOSITORY
  R294 KBookmarks

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

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


D29427: [KBookmarkMenu] Assign m_actionCollection early to prevent crash

2020-05-04 Thread Ahmad Samir
ahmadsamir added a subscriber: rikmills.

REPOSITORY
  R294 KBookmarks

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

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


D29427: [KBookmarkMenu] Assign m_actionCollection early to prevent crash

2020-05-04 Thread Ahmad Samir
ahmadsamir created this revision.
ahmadsamir added reviewers: Frameworks, dfaure.
Herald added a project: Frameworks.
ahmadsamir requested review of this revision.

REVISION SUMMARY
  The deprecated ctor that took a KActionCollection param called the new
  ctor (that doesn't take an actionCollection before) m_actionCollection was
  assigned. This caused the menu actions never to get added to the
  actionCollection as it was still nullptr. This caused crashes in
  applications that still use the deprecated ctor, e.g. this bug in
  konsole https://bugs.kde.org/show_bug.cgi?id=420820.
  
  Since we can't assign m_actionCollection in the initializer list because
  then the constructor delegation would follow a member initializer
  (info courtsey of the compiler), introduce a helper init method to
  reduce code duplication between the two ctor's.
  
  Thanks to Friedrich W. H. Kossebau for figuring it out
  https://phabricator.kde.org/D28800#663274.
  
  CCBUG: 420820

TEST PLAN
  make && ctest

REPOSITORY
  R294 KBookmarks

BRANCH
  l-bookmarkmenu-assign-actioncoll (branched from master)

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

AFFECTED FILES
  src/kbookmarkmenu.cpp
  src/kbookmarkmenu.h

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


D29362: [KCharSelect] Initially give focus to the search lineedit

2020-05-02 Thread Ahmad Samir
ahmadsamir created this revision.
ahmadsamir added reviewers: Frameworks, cfeck.
Herald added a project: Frameworks.
ahmadsamir requested review of this revision.

REVISION SUMMARY
  If the search lineedit widget is added, give it initial focus, so that you
  launch the dialogue and start typing to find the character you want;
  (much like a web browser).

TEST PLAN
  make && ctest

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  l-searchline-focus-initial (branched from master)

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

AFFECTED FILES
  src/kcharselect.cpp

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


D29254: [RenameDialog] Add an arrow indicating direction from src to dest

2020-05-02 Thread Ahmad Samir
This revision was automatically updated to reflect the committed changes.
Closed by commit R241:baa38ebee32e: [RenameDialog] Add an arrow indicating 
direction from src to dest (authored by ahmadsamir).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29254?vs=81628=81728

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

AFFECTED FILES
  src/widgets/renamedialog.cpp

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


D29254: [RenameDialog] Add an arrow indicating direction from src to dest

2020-04-30 Thread Ahmad Samir
ahmadsamir edited the summary of this revision.

REPOSITORY
  R241 KIO

BRANCH
  l-srt-to-dest (branched from master)

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

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


D29254: [RenameDialog] Add an arrow indicating direction from src to dest

2020-04-30 Thread Ahmad Samir
ahmadsamir updated this revision to Diff 81628.
ahmadsamir edited the summary of this revision.
ahmadsamir added a reviewer: Dolphin.
ahmadsamir removed subscribers: safaalfulaij, hpereiradacosta, pino, ngraham.
ahmadsamir added a comment.


  Verbatim

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29254?vs=81627=81628

BRANCH
  l-srt-to-dest (branched from master)

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

AFFECTED FILES
  src/widgets/renamedialog.cpp

To: ahmadsamir, #frameworks, dfaure, meven, ngraham, #dolphin
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns, 
safaalfulaij, hpereiradacosta, pino


D29254: [RenameDialog] Add an arrow indicating direction from src to dest

2020-04-30 Thread Ahmad Samir
ahmadsamir updated this revision to Diff 81627.
ahmadsamir added a comment.


  Take RTL layout into consideration when setting the arrow icon by using
  qApp->isRightToLeft()

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29254?vs=81492=81627

BRANCH
  l-srt-to-dest (branched from master)

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

AFFECTED FILES
  src/widgets/renamedialog.cpp

To: ahmadsamir, #frameworks, dfaure, meven, ngraham
Cc: safaalfulaij, hpereiradacosta, pino, ngraham, kde-frameworks-devel, 
LeGast00n, cblack, michaelh, bruns


D29254: [RenameDialog] Add an arrow indicating direction from src to dest

2020-04-30 Thread Ahmad Samir
ahmadsamir added a comment.


  OK, thank you :)
  
  I'll adjust the diff accordingly.

REPOSITORY
  R241 KIO

BRANCH
  l-srt-to-dest (branched from master)

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

To: ahmadsamir, #frameworks, dfaure, meven, ngraham
Cc: safaalfulaij, hpereiradacosta, pino, ngraham, kde-frameworks-devel, 
LeGast00n, cblack, michaelh, bruns


D29254: [RenameDialog] Add an arrow indicating direction from src to dest

2020-04-30 Thread Ahmad Samir
ahmadsamir marked an inline comment as done.
ahmadsamir added inline comments.

INLINE COMMENTS

> pino wrote in renamedialog.cpp:299
> this is not right-to-left aware; please use the layout direction of the 
> widget to use "go-next" or "go-previous"

@pino, right; dolphin isn't rtl-aware (or if it is, I couldn't find out how to 
switch it to rtl). But I agree the code here should account for rtl anyway.

@ngraham: I think we should stick to the icon naming spec[1], so that it works 
with themes other than breeze/oxygen; so it has to be go-previous.

[1] 
https://specifications.freedesktop.org/icon-naming-spec/icon-naming-spec-latest.html

REPOSITORY
  R241 KIO

BRANCH
  l-srt-to-dest (branched from master)

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

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


D29254: [RenameDialog] Add an arrow indicating direction from src to dest

2020-04-29 Thread Ahmad Samir
ahmadsamir marked an inline comment as done.
ahmadsamir added inline comments.

INLINE COMMENTS

> meven wrote in renamedialog.cpp:143
> You don't need a member variable for this.

Indeed, it's only used in one place (too much copy/paste...).

REPOSITORY
  R241 KIO

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

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


D29254: [RenameDialog] Add an arrow indicating direction from src to dest

2020-04-29 Thread Ahmad Samir
ahmadsamir updated this revision to Diff 81492.
ahmadsamir added a comment.


  Don't use a member var for an object that's only used in one function

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29254?vs=81431=81492

BRANCH
  l-srt-to-dest (branched from master)

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

AFFECTED FILES
  src/widgets/renamedialog.cpp

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


D29254: [RenameDialog] Add an arrow indicating direction from src to dest

2020-04-28 Thread Ahmad Samir
ahmadsamir added a comment.


  I meant it worked with Arabic, the direction was the same, just the text is 
translated, so the renamedialog and dolphin aren't RTL AFAICS.

REPOSITORY
  R241 KIO

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

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


D29254: [RenameDialog] Add an arrow indicating direction from src to dest

2020-04-28 Thread Ahmad Samir
ahmadsamir added a comment.


  I tried with Arabic, and the rename dialog had some Arabic text, but the 
layout was still LTR (can it switch to RTL?).

REPOSITORY
  R241 KIO

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

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


D29254: [RenameDialog] Add an arrow indicating direction from src to dest

2020-04-28 Thread Ahmad Samir
ahmadsamir edited the summary of this revision.

REPOSITORY
  R241 KIO

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

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


D29254: [RenameDialog] Add an arrow indicating direction from src to dest

2020-04-28 Thread Ahmad Samir
ahmadsamir created this revision.
ahmadsamir added reviewers: Frameworks, dfaure, meven.
Herald added a project: Frameworks.
ahmadsamir requested review of this revision.

REVISION SUMMARY
  This adds a visual indication to show the direction of the copy/move..etc
  operation pointing from source to destination.
  
  See the screenshot.
  
  BUG: 268600

REPOSITORY
  R241 KIO

BRANCH
  l-srt-to-dest (branched from master)

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

AFFECTED FILES
  src/widgets/renamedialog.cpp

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


D29206: Move check for invalid service from KDesktopFileActions to ApplicationLauncherJob

2020-04-27 Thread Ahmad Samir
ahmadsamir accepted this revision.
ahmadsamir added inline comments.
This revision is now accepted and ready to land.

INLINE COMMENTS

> dfaure wrote in kprocessrunner.cpp:55
> I see what you're saying.
> 
> It's the name of the spec, though.
> https://specifications.freedesktop.org/desktop-entry-spec/latest/
> 
> Each file is an "entry for the desktop", I guess.

Well, what can we do... that ship has sailed.

REPOSITORY
  R241 KIO

BRANCH
  2020_04_invalid_service

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

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


D29239: [KPlotting] foreach is gone, build with -DQT_NO_FOREACH

2020-04-27 Thread Ahmad Samir
This revision was automatically updated to reflect the committed changes.
Closed by commit R277:a6c269a6b2ce: [KPlotting] foreach is gone, build with 
-DQT_NO_FOREACH (authored by ahmadsamir).

REPOSITORY
  R277 KPlotting

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29239?vs=81381=81383

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

AFFECTED FILES
  CMakeLists.txt

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


D29233: [Solid] Port foreach to range/index for

2020-04-27 Thread Ahmad Samir
This revision was automatically updated to reflect the committed changes.
Closed by commit R245:dd878b1c2933: [Solid] Port foreach to range/index for 
(authored by ahmadsamir).

REPOSITORY
  R245 Solid

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29233?vs=81359=81382

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

AFFECTED FILES
  src/solid/devices/backends/upower/upowermanager.cpp
  src/solid/devices/backends/win/winblock.cpp
  src/solid/devices/backends/win/windevicemanager.cpp
  src/solid/devices/backends/win/winopticaldrive.cpp
  src/solid/devices/backends/win/winprocessor.cpp
  src/tools/solid-hardware/solid-hardware.cpp

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


D29239: [KPlotting] foreach is gone, build with -DQT_NO_FOREACH

2020-04-27 Thread Ahmad Samir
ahmadsamir created this revision.
ahmadsamir added reviewers: Frameworks, dfaure.
Herald added a project: Frameworks.
ahmadsamir requested review of this revision.

TEST PLAN
  make && ctest

REPOSITORY
  R277 KPlotting

BRANCH
  l-no-foreach (branched from master)

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

AFFECTED FILES
  CMakeLists.txt

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


D29229: [KPlotting] Port foreach (deprecated) to range for

2020-04-27 Thread Ahmad Samir
This revision was automatically updated to reflect the committed changes.
Closed by commit R277:8281cb629ffa: [KPlotting] Port foreach (deprecated) to 
range for (authored by ahmadsamir).

REPOSITORY
  R277 KPlotting

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29229?vs=81339=81380

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

AFFECTED FILES
  src/kplotobject.cpp
  src/kplotwidget.cpp

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


D29221: [FakeCdrom] Add a new UnknownMediumType enumerator to MediumType

2020-04-27 Thread Ahmad Samir
This revision was automatically updated to reflect the committed changes.
Closed by commit R245:c4e8755ac324: [FakeCdrom] Add a new UnknownMediumType 
enumerator to MediumType (authored by ahmadsamir).

REPOSITORY
  R245 Solid

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29221?vs=81316=81379

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

AFFECTED FILES
  src/solid/devices/backends/fakehw/fakecdrom.cpp
  src/solid/devices/frontend/opticaldrive.h

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


D29206: Move check for invalid service from KDesktopFileActions to ApplicationLauncherJob

2020-04-27 Thread Ahmad Samir
ahmadsamir added a comment.


  Otherwise makes sense.

INLINE COMMENTS

> kprocessrunner.cpp:55
> +if (!service->isValid()) {
> +emitDelayedError(i18n("The desktop entry file\n%1\nis not valid.", 
> service->entryPath()));
> +return;

IMHO, the word "entry" here is confusing, the first thing that comes to mind is 
that an entry (i.e. a line) in the file is invalid.

REPOSITORY
  R241 KIO

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

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


D29233: [Solid] Port foreach to range/index for

2020-04-27 Thread Ahmad Samir
ahmadsamir created this revision.
ahmadsamir added reviewers: Frameworks, dfaure, bruns, meven.
Herald added a project: Frameworks.
ahmadsamir requested review of this revision.

TEST PLAN
  make && ctest

REPOSITORY
  R245 Solid

BRANCH
  l-foreach-3 (branched from master)

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

AFFECTED FILES
  src/solid/devices/backends/upower/upowermanager.cpp
  src/solid/devices/backends/win/winblock.cpp
  src/solid/devices/backends/win/windevicemanager.cpp
  src/solid/devices/backends/win/winopticaldrive.cpp
  src/solid/devices/backends/win/winprocessor.cpp
  src/tools/solid-hardware/solid-hardware.cpp

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


D29229: [KPlotting] Port foreach (deprecated) to range for

2020-04-27 Thread Ahmad Samir
ahmadsamir created this revision.
ahmadsamir added reviewers: Frameworks, dfaure, apol, meven.
Herald added a project: Frameworks.
ahmadsamir requested review of this revision.

TEST PLAN
  make && ctest

REPOSITORY
  R277 KPlotting

BRANCH
  l-foreach (branched from master)

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

AFFECTED FILES
  src/kplotobject.cpp
  src/kplotwidget.cpp

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


D29219: [KFontChooser] Remove NoFixedCheckBox DisplayFlag, redundant

2020-04-27 Thread Ahmad Samir
This revision was automatically updated to reflect the committed changes.
Closed by commit R236:5d803a237955: [KFontChooser] Remove NoFixedCheckBox 
DisplayFlag, redundant (authored by ahmadsamir).

REPOSITORY
  R236 KWidgetsAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29219?vs=81291=81317

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

AFFECTED FILES
  src/kfontchooser.cpp
  src/kfontchooser.h
  tests/kfontchooserdialogtest.cpp

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


D29221: [FakeCdrom] Add a new UnknownMediumType enumerator to MediumType

2020-04-27 Thread Ahmad Samir
ahmadsamir created this revision.
ahmadsamir added reviewers: Frameworks, dfaure, meven, bruns.
Herald added a project: Frameworks.
ahmadsamir requested review of this revision.

REVISION SUMMARY
  As was discussed in https://phabricator.kde.org/D29138 and on IRC (with
  frinring and vkrause), add a new enumerator to MediumType with value 0
  to indicate no-op. Change one for-loop to use the new enumerator with
  QMap::value(key, defaultValue).

TEST PLAN
  make && ctest

REPOSITORY
  R245 Solid

BRANCH
  l-mediumtypes (branched from master)

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

AFFECTED FILES
  src/solid/devices/backends/fakehw/fakecdrom.cpp
  src/solid/devices/frontend/opticaldrive.h

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


D29138: [Solid] Replace foreach (deprecated) with range/index for

2020-04-27 Thread Ahmad Samir
This revision was automatically updated to reflect the committed changes.
Closed by commit R245:b9abef40855e: [Solid] Replace foreach (deprecated) with 
range/index for (authored by ahmadsamir).

REPOSITORY
  R245 Solid

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29138?vs=81131=81314

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

AFFECTED FILES
  src/imports/devices.cpp
  src/solid/devices/backends/fakehw/fakecdrom.cpp
  src/solid/devices/backends/fakehw/fakedevice.cpp
  src/solid/devices/backends/fakehw/fakemanager.cpp
  src/solid/devices/backends/fakehw/fakeopticaldisc.cpp
  src/solid/devices/backends/fakehw/fakeprocessor.cpp
  src/solid/devices/backends/fakehw/fakestorageaccess.cpp
  src/solid/devices/backends/fstab/fstabdevice.cpp
  src/solid/devices/backends/fstab/fstabmanager.cpp

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


  1   2   3   4   5   6   7   8   9   10   >