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

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

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

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

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

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

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

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

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

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

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

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

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 =

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

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

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

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.

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

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

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

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

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

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 > */ > -

D29599: [CopyJob] Try to fix windows build

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

D29599: [CopyJob] Try to fix windows build

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.

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

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

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

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

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

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,

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

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

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

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

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

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

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

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

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

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

D29528: [OpenUrlJob] Improve comments/docs

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

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

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

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

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

D29528: [OpenUrlJob] Improve comments/docs

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

D29528: [OpenUrlJob] Improve comments/docs

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

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

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)

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

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

D29528: [OpenUrlJob] Improve comments/docs

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

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

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

D25339: update lineHeight if boundingRect indicates a larger value.

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

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

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,

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

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

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

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

D29427: [KBookmarkMenu] Assign m_actionCollection early to prevent crash

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

D29427: [KBookmarkMenu] Assign m_actionCollection early to prevent crash

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

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

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

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

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

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

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

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,

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

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

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

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

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

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,

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

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

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

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

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

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)

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

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:

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

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,

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

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

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

D29206: Move check for invalid service from KDesktopFileActions to ApplicationLauncherJob

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

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

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

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

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

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

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

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

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

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

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

D29206: Move check for invalid service from KDesktopFileActions to ApplicationLauncherJob

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,

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

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

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

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

D29219: [KFontChooser] Remove NoFixedCheckBox DisplayFlag, redundant

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

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

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

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

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

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

ahmadsamir added inline comments. INLINE COMMENTS > bruns wrote in fakeopticaldisc.cpp:43 > Thats not a cast but a constructor, see QFlags documentation From opticaldrive.h: Q_DECLARE_FLAGS(MediumTypes, MediumType) AFAIU, MediumTypes is the QFlags, MediumType is the enum. So

D29170: Detect executables without +x permission in $PATH to improve error message

ahmadsamir accepted this revision. ahmadsamir added inline comments. This revision is now accepted and ready to land. INLINE COMMENTS > kprocessrunner.cpp:65 > +const QStringList searchPaths = > QString::fromLocal8Bit(qgetenv("PATH")).split(QDir::listSeparator(), > skipEmptyParts); > +

D29219: [KFontChooser] Remove NoFixedCheckBox DisplayFlag, redundant

ahmadsamir created this revision. ahmadsamir added reviewers: Frameworks, dfaure, cfeck, bport. Herald added a project: Frameworks. ahmadsamir requested review of this revision. REVISION SUMMARY This partially reverts 0004e5c89a248a508a

D29065: [KFontChooser] Add new DisplayFlag; modify how flags are used

ahmadsamir added a comment. I forgot to change the commit message about hiding the show only monospaced fonts checkbox when FixedFontsOnly is set... at least the API docs do mention it :/. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D29065 To:

D29170: Detect executables without +x permission in $PATH to improve error message

ahmadsamir added a comment. In D29170#657689 , @dfaure wrote: > In D29170#657450 , @ahmadsamir wrote: > > > I don't think you need to go out of your way to support custom setups, after all it's

D29170: Detect executables without +x permission in $PATH to improve error message

ahmadsamir added a comment. In D29170#657334 , @dfaure wrote: > Resolve relative executables using the directory of the .desktop file referring to them > > Not useful for /usr/share/applications stuff, but useful for custom setups > where

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

D29170: Detect executables without +x permission in $PATH to improve error message

ahmadsamir added inline comments. INLINE COMMENTS > kprocessrunner.cpp:53 > +// This is a *very* simplified version of > QStandardPaths::findExecutable > +const QStringList searchPaths = > QString::fromLocal8Bit(qgetenv("PATH")).split(QDir::listSeparator(), >

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

ahmadsamir added inline comments. INLINE COMMENTS > bruns wrote in fakeopticaldisc.cpp:43 > Just use `0`. It doesn't make the code more readable; and using QMap::constFind() while iterating is more recognizable/widely-used. REPOSITORY R245 Solid BRANCH l-foreach (branched from master)

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

ahmadsamir updated this revision to Diff 81131. ahmadsamir added a comment. Remove redundant lines... REPOSITORY R245 Solid CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29138?vs=81121=81131 BRANCH l-foreach (branched from master) REVISION DETAIL

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

ahmadsamir added inline comments. INLINE COMMENTS > bruns wrote in fakeopticaldisc.cpp:43 > You forgot to remove the old lines. > > Can also be applied to supportedMedia /* brown paper bag */, removed the lines. I don't see a default a-la-NoContent MediumType member. REPOSITORY R245 Solid

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

ahmadsamir added inline comments. INLINE COMMENTS > bruns wrote in fakeopticaldisc.cpp:43 > These four lines can be written as > `content |= map.value(type, Solid::OpticalDisc::ContentType(0));` Nice. :) I think Solid::OpticalDisc::NoContent is more readable. REPOSITORY R245 Solid BRANCH

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

ahmadsamir updated this revision to Diff 81121. ahmadsamir marked an inline comment as done. ahmadsamir added a comment. Make the code much shorter with map.value(key, defaultvalue), since the ContentType enum has a NoContent member REPOSITORY R245 Solid CHANGES SINCE LAST UPDATE

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

ahmadsamir updated this revision to Diff 81098. ahmadsamir marked an inline comment as done. ahmadsamir added a comment. More const Optimise by iterating over QStringList, and constFind in QMap REPOSITORY R245 Solid CHANGES SINCE LAST UPDATE

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

ahmadsamir marked 4 inline comments as done. ahmadsamir added a comment. In D29138#656140 , @meven wrote: > In D29138#656094 , @dfaure wrote: > > > Wow those iterations over map.keys() were awful.

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

ahmadsamir created this revision. ahmadsamir added reviewers: Frameworks, dfaure, apol, meven. Herald added a project: Frameworks. ahmadsamir requested review of this revision. REVISION SUMMARY src/imports/devices.cpp src/solid/devices/backends/fakehw/* src/solid/devices/backends/fstab/*

D29065: [KFontChooser] Add new DisplayFlag; modify how flags are used

This revision was automatically updated to reflect the committed changes. Closed by commit R236:0004e5c89a24: [KFontChooser] Add new DisplayFlag; modify how flags are used (authored by ahmadsamir). REPOSITORY R236 KWidgetsAddons CHANGES SINCE LAST UPDATE

D29065: [KFontChooser] Add new DisplayFlag; modify how flags are used

ahmadsamir marked an inline comment as done. REPOSITORY R236 KWidgetsAddons BRANCH l-kfontchooser-onlyfixed-display-flag (branched from master) REVISION DETAIL https://phabricator.kde.org/D29065 To: ahmadsamir, #frameworks, dfaure, cfeck, bport Cc: kossebau, kde-frameworks-devel,

D29065: [KFontChooser] Add new DisplayFlag; modify how flags are used

ahmadsamir updated this revision to Diff 81007. ahmadsamir marked an inline comment as done. ahmadsamir added a comment. More docs, and a trailing comma in the enum to ease git diff/blame if more flags are added REPOSITORY R236 KWidgetsAddons CHANGES SINCE LAST UPDATE

D29065: [KFontChooser] Add new DisplayFlag; modify how flags are used

ahmadsamir marked an inline comment as done. ahmadsamir added inline comments. INLINE COMMENTS > dfaure wrote in kfontchooser.h:87 > maybe document that FixedFontsOnly implies NoFixedCheckBox? > > (as in, the widget will behave as if NoFixedCheckBox was set) Yes, will do (less surprises for

D29065: [KFontChooser] Add new DisplayFlag; modify how flags are used

ahmadsamir updated this revision to Diff 80989. ahmadsamir marked 2 inline comments as done. ahmadsamir retitled this revision from "[KFontChooser] Add a DisplayFlag to allow not showing fixedOnly checkbox" to "[KFontChooser] Add new DisplayFlag; modify how flags are used". ahmadsamir edited the

D29065: [KFontChooser] Add a DisplayFlag to allow not showing fixedOnly checkbox

ahmadsamir updated this revision to Diff 80988. ahmadsamir added a comment. - Use a proper flag set, 0/1/2/4/8, from dfaure - When checking a flag is set use bitwise &, not bitwise ^, the latter would fail if another flag was set - As per dfaure's suggestion, when FixedFontsOnly is set,

D29121: Replace foreach with range-for

This revision was automatically updated to reflect the committed changes. Closed by commit R282:b53859b89a13: Replace foreach with range-for (authored by ahmadsamir). REPOSITORY R282 NetworkManagerQt CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29121?vs=80963=80965 REVISION

D29121: Replace foreach with range-for

ahmadsamir created this revision. ahmadsamir added reviewers: Frameworks, jgrulich. Herald added a project: Frameworks. Herald added 1 blocking reviewer(s): jgrulich. ahmadsamir requested review of this revision. REVISION SUMMARY I missed one file before, now it should actually build with

D29093: [NetworkManager-qt] Replace foreach with range for, hopefully last pass

This revision was automatically updated to reflect the committed changes. Closed by commit R282:ee827d8efba2: [NetworkManager-qt] Replace foreach with range for, hopefully last pass (authored by ahmadsamir). REPOSITORY R282 NetworkManagerQt CHANGES SINCE LAST UPDATE

D29091: [NetworkManager-qt] Replace foreach with range/index for loop, third pass

This revision was automatically updated to reflect the committed changes. Closed by commit R282:6d7caf3468ed: [NetworkManager-qt] Replace foreach with range/index for loop, third pass (authored by ahmadsamir). REPOSITORY R282 NetworkManagerQt CHANGES SINCE LAST UPDATE

D29065: [KFontChooser] Add a DisplayFlag to allow not showing fixedOnly checkbox

ahmadsamir added inline comments. INLINE COMMENTS > dfaure wrote in kfontchooser.h:90 > 1/2/4 looked like powers of 2, i.e. a flag set. > > Using 5 here gives is the same value as FixedFontsOnly | ShowDifferences. > > On the other hand it makes sense FixedFontsOnly would automatically hide the

D29061: [KCharSelect] Minor code optimisation

This revision was automatically updated to reflect the committed changes. ahmadsamir marked an inline comment as done. Closed by commit R236:6a9e93c698b8: [KCharSelect] Minor code optimisation (authored by ahmadsamir). REPOSITORY R236 KWidgetsAddons CHANGES SINCE LAST UPDATE

D29061: [KCharSelect] Minor code optimisation

ahmadsamir marked an inline comment as done. ahmadsamir added inline comments. INLINE COMMENTS > dfaure wrote in kcharselectdata.cpp:870 > Shouldn't this be moved to inside the `if (match.hasMatch())` check? Indeed. REPOSITORY R236 KWidgetsAddons REVISION DETAIL

D29061: [KCharSelect] Minor code optimisation

ahmadsamir updated this revision to Diff 80886. ahmadsamir added a comment. Only use captured() if there's a match REPOSITORY R236 KWidgetsAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29061?vs=80781=80886 BRANCH l-general-code-opti (branched from master) REVISION

D29093: [NetworkManager-qt] Replace foreach with range for, hopefully last pass

ahmadsamir added a comment. Depends on D29091 . REPOSITORY R282 NetworkManagerQt REVISION DETAIL https://phabricator.kde.org/D29093 To: ahmadsamir, #frameworks, jgrulich Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29093: [NetworkManager-qt] Replace foreach with range for, hopefully last pass

ahmadsamir updated this revision to Diff 80880. ahmadsamir retitled this revision from "[NetworkManager-qt] Replace foreach with range for" to "[NetworkManager-qt] Replace foreach with range for, hopefully last pass". ahmadsamir edited the summary of this revision. ahmadsamir removed 1 blocking

D29093: [NetworkManager-qt] Replace foreach with range for

ahmadsamir updated this revision to Diff 80879. ahmadsamir added a comment. Get the last ones REPOSITORY R282 NetworkManagerQt CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29093?vs=80877=80879 BRANCH l-foreach-4 (branched from master) REVISION DETAIL

D29093: [NetworkManager-qt] Replace foreach with range for

ahmadsamir created this revision. ahmadsamir added reviewers: Frameworks, jgrulich. Herald added a project: Frameworks. Herald added 1 blocking reviewer(s): jgrulich. ahmadsamir requested review of this revision. TEST PLAN make && ctest REPOSITORY R282 NetworkManagerQt BRANCH l-foreach-4

D29091: [NetworkManager-qt] Replace foreach with range/index for loop, third pass

ahmadsamir created this revision. ahmadsamir added reviewers: Frameworks, jgrulich. Herald added a project: Frameworks. Herald added 1 blocking reviewer(s): jgrulich. ahmadsamir requested review of this revision. TEST PLAN make && ctest REPOSITORY R282 NetworkManagerQt BRANCH l-foreach-3

D29086: [NetworkManager-qt] Replace foreach with range/index for loop

This revision was automatically updated to reflect the committed changes. Closed by commit R282:f0b1c412074a: [NetworkManager-qt] Replace foreach with range/index for loop (authored by ahmadsamir). REPOSITORY R282 NetworkManagerQt CHANGES SINCE LAST UPDATE

D29086: [NetworkManager-qt] Replace foreach with range/index for loop

ahmadsamir updated this revision to Diff 80860. ahmadsamir added a comment. Some missing ones REPOSITORY R282 NetworkManagerQt CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29086?vs=80858=80860 BRANCH l-foreach-2 (branched from master) REVISION DETAIL

<    1   2   3   4   5   6   7   8   9   10   >