[Differential] [Updated] D1025: [DrKonqi] Avoid duplicate "Help" button

dfaure added a comment. Yes, KAssistantDialog says buttonBox->setStandardButtons(QDialogButtonBox::Cancel | QDialogButtonBox::Help); If you then call addButton(Help) you get another one. I do wonder if KAssistantDialog should be doing this indeed. It means it offers a button

[Differential] [Commented On] D1497: [kioslave/desktop] Ensure there are no multiple / when creating the file path

dfaure added inline comments. INLINE COMMENTS kioslave/desktop/kio_desktop.cpp:161 Can the path really *not* start with a '/' in this code ? From a QUrl point of view, it can happen with relative URLs ("desktop:foo.txt"), but we very rarely use that in KDE code, if at all (it's unusable wit

[Differential] [Updated] D1497: [kioslave/desktop] Ensure there are no multiple / when creating the file path

dfaure added a comment. I'm pretty sure it should work. What changed in QUrl is that setPath used to normalize the path and doesn't do that anymore. (qtbase commit 2e1de7f3c4ca). So in 5.5 the double slash got simplified to a single slash, while in 5.6 it stays and breaks the test. Not pu

[Differential] [Updated] D1583: fix sycoca init to pass in isolated testbeds without pre-existing sycoca

dfaure added a comment. I'm seeing similar failures in the kservice framework, so I think we should rather fix kservice to emit databaseChanged upon first creation too. REPOSITORY rKDECLITOOLS KDE CLI Utilities REVISION DETAIL https://phabricator.kde.org/D1583 EMAIL PREFERENCES https:

[Differential] [Commented On] D1583: fix sycoca init to pass in isolated testbeds without pre-existing sycoca

dfaure added a comment. Haha, don't assume my code is stupid on purpose, it can also be stupid by oversight :-) I pushed a fix for kservice, can you check if it fixes your issue? http://commits.kde.org/kservice/f82de1626f8efdb56ca810fc827d40bf7eed4601 REPOSITORY rKDECLITOOLS KDE CLI

[Differential] [Commented On] D1583: fix sycoca init to pass in isolated testbeds without pre-existing sycoca

dfaure added a comment. Right. kservicetest was still failing too, but after adding debug output it now passes every time I committed the additional debug output in kdirwatch and enabled kdirwatch debug output in filetypestest. Can you kick a build again and let me know the result?

[Differential] [Requested Changes To] D1583: fix sycoca init to pass in isolated testbeds without pre-existing sycoca

dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed. Fixed in kservice, CI is green, you can discard this patch. Thanks for the analysis of the issue, it really helped! REPOSITORY rKDECLITOOLS KDE CLI Utilities REVISION DETAI

[Differential] [Request, 6 lines] D1826: systemtray container: fix m_internalSystray nullptr crash

dfaure created this revision. dfaure added a reviewer: mart. Restricted Application added a project: Plasma. Restricted Application added a subscriber: plasma-devel. REVISION SUMMARY Not sure how to trigger it, but gdb was clear about the issue. REPOSITORY rPLASMAWORKSPACE Plasma Workspace B

[Differential] [Commented On] D1826: systemtray container: fix m_internalSystray nullptr crash

dfaure added a comment. In https://phabricator.kde.org/D1826#33876, @davidedmundson wrote: > Surely we should have already returned at line 94. m_innerContainment was not null, only m_internalSystray was. > Do you have the bt still? Unfortunately not. REPOSITORY rPLAS

[Differential] [Closed] D1826: systemtray container: fix m_internalSystray nullptr crash

This revision was automatically updated to reflect the committed changes. Closed by commit rPLASMAWORKSPACEb399cd908e53: systemtray container: fix m_internalSystray nullptr crash (authored by dfaure). REPOSITORY rPLASMAWORKSPACE Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.

[Differential] [Request, 8 lines] D1840: Fix runtime warning on startup of digital-clock

dfaure created this revision. Restricted Application added a project: Plasma. Restricted Application added a subscriber: plasma-devel. REVISION SUMMARY org.kde.plasma.digitalclock/contents/ui/main.qml:51: TypeError: Cannot read property 'width' of null REPOSITORY rPLASMAWORKSPACE Plasma Work

[Differential] [Updated] D1840: Fix runtime warning on startup of digital-clock

dfaure added a reviewer: mck182. REPOSITORY rPLASMAWORKSPACE Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D1840 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: dfaure, mck182 Cc: plasma-devel, sebas ___

[Differential] [Closed] D1840: Fix runtime warning on startup of digital-clock

This revision was automatically updated to reflect the committed changes. Closed by commit rPLASMAWORKSPACE820f563cc7f6: Fix runtime warning on startup of digital-clock (authored by dfaure). REPOSITORY rPLASMAWORKSPACE Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D1

[Differential] [Request, 18 lines] D1841: Tasks model: don't load 4 icon sizes to then just use one.

dfaure created this revision. dfaure added a reviewer: hein. Restricted Application added a project: Plasma. Restricted Application added a subscriber: plasma-devel. REPOSITORY rPLASMAWORKSPACE Plasma Workspace BRANCH master REVISION DETAIL https://phabricator.kde.org/D1841 AFFECTED FILES

[Differential] [Commented On] D1841: Tasks model: don't load 4 icon sizes to then just use one.

dfaure added inline comments. INLINE COMMENTS > broulik wrote in xwindowtasksmodel.cpp:662 > Can't you make a property where the applet tells it the size it likes to > have, similar to what you do with the drag pixmap size? No clue, this "fixme" was already there, I'm only optimizing for speed.

[Differential] [Commented On] D1841: Tasks model: don't load 4 icon sizes to then just use one.

dfaure added inline comments. INLINE COMMENTS > hein wrote in xwindowtasksmodel.cpp:665 > Why not use the one from line 654? Oops, didn't see it. I'll adjust the patch. However this makes me wonder, given this code if (!data.icon.name().isEmpty()) { return data.url; } do I still nee

[Differential] [Updated, 12 lines] D1841: Tasks model: don't load 4 icon sizes to then just use one.

dfaure updated this revision to Diff 4400. dfaure added a comment. Simplify as suggested REPOSITORY rPLASMAWORKSPACE Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D1841?vs=4385&id=4400 BRANCH master REVISION DETAIL https://phabricator.kde.org/D1841 AFFECTED

[Differential] [Closed] D1841: Tasks model: don't load 4 icon sizes to then just use one.

dfaure closed this revision. REPOSITORY rPLASMAWORKSPACE Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D1841 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: dfaure, hein Cc: broulik, plasma-devel, sebas

[Differential] [Accepted] D1859: Prepare KSplash for KDED dropping KSplash support

dfaure accepted this revision. dfaure added a comment. This revision is now accepted and ready to land. Thanks! INLINE COMMENTS > splashapp.cpp:106 > { > +//filter out startup events from KDED as they will be removed in a > future release > +if (stage == QLatin1String("kded") || sta

[Differential] [Request, 30 lines] D1865: TasksModel: cache launcherCount().

dfaure created this revision. dfaure added a reviewer: hein. Restricted Application added a project: Plasma. Restricted Application added a subscriber: plasma-devel. REVISION SUMMARY This also allows to only emit launcherCountChanged() when it actually changed. The emit from TasksModel::fil

[Differential] [Updated, 29 lines] D1865: TasksModel: cache launcherCount().

dfaure updated this revision to Diff 4458. dfaure added a comment. Set the new value before emitting the change signal, indeed. REPOSITORY rPLASMAWORKSPACE Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D1865?vs=4451&id=4458 BRANCH master REVISION DETAIL http

[Differential] [Closed] D1865: TasksModel: cache launcherCount().

dfaure closed this revision. REPOSITORY rPLASMAWORKSPACE Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D1865 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: dfaure, hein Cc: broulik, plasma-devel, jensreuterberg, sebas

[Differential] [Accepted] D1898: [Kicker] Use KRun::runService instead of new KRun

dfaure accepted this revision. dfaure added a comment. This revision is now accepted and ready to land. Yes, but please add a comment about porting to KRun::runApplication once https://phabricator.kde.org/D1902 is merged in and you can rely on KF 5.24 :-) REPOSITORY rPLASMADESKTOP Plasma De

[Differential] [Commented On] D1920: Add a LauncherUrlWithoutIcon data role as a speed optimization.

dfaure added inline comments. INLINE COMMENTS > tasksmodel.cpp:357 > || (launcherUrl.isValid() && > launcherUrlsMatch(launcherUrl, > - > launcherIndex.data(AbstractTasksModel::LauncherUrl).toUrl(), > IgnoreQueryItems))) { > +

[Differential] [Accepted] D1921: Use data role that avoids unnecessary work by the source model.

dfaure accepted this revision. This revision is now accepted and ready to land. REPOSITORY rPLASMADESKTOP Plasma Desktop BRANCH master REVISION DETAIL https://phabricator.kde.org/D1921 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: hein, davidedmunds

[Differential] [Accepted] D1920: Add a LauncherUrlWithoutIcon data role as a speed optimization.

dfaure accepted this revision. This revision is now accepted and ready to land. REPOSITORY rPLASMAWORKSPACE Plasma Workspace BRANCH blblblbl (branched from master) REVISION DETAIL https://phabricator.kde.org/D1920 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailprefere

[Differential] [Updated] D1931: Avoid asking model to create string-serialized icon data where possible.

dfaure added a comment. Maybe it should have been the other way around, LauncherUrlWithIcon where needed ;-) (it seems the icon is only rarely needed) INLINE COMMENTS > ContextMenu.qml:225 > } else { > -tasksModel.requestAddLauncher(visualParent.launcherUrl);

[Differential] [Accepted] D1931: Avoid asking model to create string-serialized icon data where possible.

dfaure accepted this revision. This revision is now accepted and ready to land. REPOSITORY rPLASMADESKTOP Plasma Desktop BRANCH master REVISION DETAIL https://phabricator.kde.org/D1931 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: hein, dfaure Cc: p

[Differential] [Updated] D2029: [Task Manager Backend] Honor "NotShowIn" for Jump List Actions

dfaure added a comment. Oh boy we're heading right into a mess similar to web sites and web browsers, if we continue this way. Next we'll need a "really only show in Unity", then we'll start making special cases for some apps, and then ... the world will implode. I am very much against

[Differential] [Accepted] D2029: [Task Manager Backend] Honor "NotShowIn" and "OnlyShowIn" for Jump List Actions

dfaure accepted this revision. dfaure added a comment. This revision is now accepted and ready to land. Thanks! REPOSITORY rPLASMADESKTOP Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D2029 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To:

[Differential] [Accepted] D2054: Fix for bug 364530

dfaure accepted this revision. dfaure added a reviewer: dfaure. dfaure added a comment. This revision is now accepted and ready to land. Yes, this is a much better fix, thank you. REPOSITORY rPLASMAWORKSPACE Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D2054 EMAIL PREFERE

[Differential] [Commented On] D1813: Fix selected name filter with multiple mimetypes

dfaure added inline comments. INLINE COMMENTS > kdeplatformfiledialoghelper.cpp:78 > /* > * Map a KDE filter string into a Qt one. > */ Can you explain and document here what this function does, i.e. input args and return value? It's a bit confusing. "kde" is a mimetype name, e.g. applica

[Differential] [Accepted] D2258: Do not detect kde4 directory but assume it is the same location, this means people who install as non-root with kde-srcbuild can install it

dfaure accepted this revision. dfaure added a comment. This revision is now accepted and ready to land. Thanks. Installing outside the install prefix is against the law. REPOSITORY rPLASMAPA Plasma Audio Volume Applet BRANCH master REVISION DETAIL https://phabricator.kde.org/D2258

[Differential] [Updated] D2365: Add failing test case for selected filter from mimetype

dfaure added a comment. The thing is, mimetype filters should be preferred above name filters. So the bug is in QFileDialog::selectMimeTypeFilter which "falls back" to selectNameFilter. Instead it should call some selectInitiallySelectedMimeTypeFilter in the options, like selectNameFilter d

[Differential] [Commented On] D2365: Add failing test case for selected filter from mimetype

dfaure added a comment. Yes, and yes. REPOSITORY rPLASMAINTEGRATION Integration for Qt applications in Plasma REVISION DETAIL https://phabricator.kde.org/D2365 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: elvisangelaccio, graesslin, dfaure Cc: pla

[Differential] [Request, 21 lines] D2654: AppEntry: load icon on demand, to speed up plasma startup

dfaure created this revision. dfaure added a reviewer: broulik. Restricted Application added a project: Plasma. Restricted Application added a subscriber: plasma-devel. TEST PLAN Starting plasmashell. It still takes too long, but now it's in QML stuff ;) REPOSITORY rPLASMADESKTOP Plasma Deskt

[Differential] [Closed] D2654: AppEntry: load icon on demand, to speed up plasma startup

This revision was automatically updated to reflect the committed changes. Closed by commit rPLASMADESKTOPee2577125c5a: AppEntry: load icon on demand, to speed up plasma startup (authored by dfaure). REPOSITORY rPLASMADESKTOP Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.or

[Differential] [Request, 872 lines] D2914: folderview: port the context menu away from KonqPopupMenu.

dfaure created this revision. dfaure added reviewers: broulik, hein. Restricted Application added a project: Plasma. Restricted Application added a subscriber: plasma-devel. REVISION SUMMARY Many actions were coming from this code anyway, all that was missing was: - Copy To / Move To (pro

[Differential] [Commented On] D2914: folderview: port the context menu away from KonqPopupMenu.

dfaure added a comment. Eike: ping? REPOSITORY rPLASMADESKTOP Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D2914 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: dfaure, broulik, hein Cc: mart, plasma-devel, lesliezhai, ali-mohamed, jens

[Differential] [Closed] D2914: folderview: port the context menu away from KonqPopupMenu.

This revision was automatically updated to reflect the committed changes. Closed by commit rPLASMADESKTOP3cf22b335871: folderview: port the context menu away from KonqPopupMenu. (authored by dfaure). REPOSITORY rPLASMADESKTOP Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.o

[Differential] [Commented On] D2687: WIP: [Icon Widget] Bring back properties dialog

dfaure added inline comments. INLINE COMMENTS > iconapplet.cpp:57 > +if (destroyed()) { > +QFile(m_localPath).remove(); > +} QFile::remove is enough, no need for a QFile instance. > iconapplet.cpp:78 > +if (!m_url.isValid()) { > +// invalid url, use dummy data ("nk >

[Differential] [Updated] D3530: Import plasma-workspace kioslaves

dfaure added a comment. remote:/ sounds very workspace-independent indeed, it sounds useful to have in kio. But desktop:/ and applications:/ make no sense in other workspaces (I'm surprised we even still have applications:/, it's kind of a toy, isn't it?). (ok applications:/ might m

[Differential] [Commented On] D3530: Import plasma-workspace kioslaves

dfaure added a comment. In https://phabricator.kde.org/D3530#65873, @mart wrote: > desktop:/ should be probably conditionally built only on Linux(but in there is quite core), while, may make sense to actually kill applications:/ altogether?? I have no doubt it seems "core on linu

[Differential] [Requested Changes To] D2687: [Icon Widget] Bring back properties dialog

dfaure requested changes to this revision. dfaure added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > iconapplet.cpp:242 > +{ > +new KRun(QUrl::fromLocalFile(m_localPath), nullptr); > +} Just wondering, why is the parent widget nullptr here, while it's QA

[Differential] [Accepted] D2687: [Icon Widget] Bring back properties dialog

dfaure accepted this revision. dfaure added inline comments. This revision is now accepted and ready to land. INLINE COMMENTS > iconapplet.cpp:285 > + > +// otherwise check if the applicaton supports the dropped type > +// TODO should we execute if *any* of the urls are supported,

[Differential] [Commented On] D2687: [Icon Widget] Bring back properties dialog

dfaure added a comment. Shouting or just coping with the file (e.g. displaying raw data, for a text editor) ;) Looks good, thanks. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D2687 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/email

[Differential] [Requested Changes To] D3707: add simple test rig for service runner

dfaure requested changes to this revision. dfaure added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > servicerunnertest.cpp:5 > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU Library General Publ

[Differential] [Commented On] D3707: add simple test rig for service runner

dfaure added a comment. I would say yes, it's worth using LGPL v2+ or v2+v3 or v2+v3+e.V., whichever you prefer, for any new code. Otherwise it's even more work the day we want to relicense away from v2 only (I know because I've been trying to do that for some code for a very long time, and

[Differential] [Commented On] D3707: add simple test rig for service runner

dfaure added inline comments. INLINE COMMENTS > servicerunnertest.cpp:59 > + > +KSycoca::self()->ensureCacheValid(); > +} BTW if this is necessary then there's a bug in ksycoca :-) You can leave it (it helps getting the debug output from ksycoca in the right method) but if it fails without

[Differential] [Accepted] D3796: Fix Plasma-QPA filedialog to show wrong directory with QFileDialog::selectUrl()

dfaure accepted this revision. dfaure added inline comments. INLINE COMMENTS > kdeplatformfiledialoghelper.cpp:365 > + > +// Qt 5 at least until 5.7.0 does not derive the directory from the > passed url > +// and set the initialDirectory option accordingly, also not for known > schemes

[Differential] [Accepted] D3707: add simple test rig for service runner

dfaure accepted this revision. dfaure added inline comments. This revision is now accepted and ready to land. INLINE COMMENTS > servicerunnertest.cpp:49 > +QDir(appsPath).removeRecursively(); > +Q_ASSERT(QDir().mkpath(appsPath)); > +auto fixtureDir = QDir(QFINDTESTDATA("fixtures"));

[Differential] [Commented On] D3796: Fix Plasma-QPA filedialog to show wrong directory with QFileDialog::selectUrl()

dfaure added inline comments. INLINE COMMENTS > kdeplatformfiledialoghelper.cpp:368 > +// like file://, so we have to do it ourselves > +QSharedPointer opt(new > QFileDialogOptions(*options())); > +opt->setInitialDirectory(m_dialog->directory()); This does NOT build for me. qplatfo

[Differential] [Commented On] D3796: Fix Plasma-QPA filedialog to show wrong directory with QFileDialog::selectUrl()

dfaure added a comment. git log says this is qtbase commit 1a42124, from before v5.8.0-alpha1. So I'll check for 5.8 and use clone, thanks for the information. REPOSITORY R135 Integration for Qt applications in Plasma REVISION DETAIL https://phabricator.kde.org/D3796 EMAIL PREFERENCES

[Differential] [Accepted] D3906: [Icon Applet] Fully re-populate when user changes Link URL

dfaure accepted this revision. dfaure added inline comments. This revision is now accepted and ready to land. INLINE COMMENTS > iconapplet.cpp:369 > +QFile::remove(m_localPath); > +setUrl(QUrl(desktopFile.readUrl())); // calls populate() itself > +} else { It look