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
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
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
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"));
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
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
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
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
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
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,
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
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
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
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
>
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
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
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
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
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
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
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
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
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
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
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:
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
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
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);
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
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
dfaure added inline comments.
INLINE COMMENTS
> tasksmodel.cpp:357
> || (launcherUrl.isValid() &&
> launcherUrlsMatch(launcherUrl,
> -
> launcherIndex.data(AbstractTasksModel::LauncherUrl).toUrl(),
> IgnoreQueryItems))) {
> +
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
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
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
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
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
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
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
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
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.
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
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
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
___
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
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.
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
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
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
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?
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
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:
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
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
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
54 matches
Mail list logo