D28692: Don't leak DrKonqi dialog / fix crash on wayland

anthonyfieroni added inline comments. INLINE COMMENTS > main.cpp:67 > void openDrKonqiDialog () { > DrKonqiDialog *w = new DrKonqiDialog(); > +QObject::connect(qApp, ::aboutToQuit, w, > ::deleteLater); Adding a qApp as a parent here? REPOSITORY R871 DrKonqi REVISION

D28817: Fix KScreen output identifier position on wayland

anthonyfieroni added a comment. In D28817#647912 , @bport wrote: > I don't think we have a leak, on destructor we delete all view > qDeleteAll(m_views); At that point, when rootObj is nullptr, view is not added to m_views. REPOSITORY

D28817: Fix KScreen output identifier position on wayland

anthonyfieroni added inline comments. INLINE COMMENTS > output_identifier.cpp:58 > if (!rootObj) { > continue; > } view leaks, no? I see it's not a problem in this patch. REPOSITORY R104 KScreen REVISION DETAIL https://phabricator.kde.org/D28817 To: bport,

D28785: Don't request blur when panel is opaque

anthonyfieroni added inline comments. INLINE COMMENTS > panelview.cpp:1015 > > QQuickItem *rootObject = this->rootObject(); > if (rootObject) { You have it as `casted` REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D28785 To: cblack,

D28904: Cleanup dependencies

anthonyfieroni added inline comments. INLINE COMMENTS > CMakeLists.txt:20 > > -find_package(Qt5 ${QT_MIN_VERSION} CONFIG REQUIRED COMPONENTS Widgets DBus > X11Extras) > -find_package(KF5 ${KF5_MIN_VERSION} REQUIRED COMPONENTS Activities Auth > IdleTime Config DBusAddons Solid I18n

D28127: Add some new battery sensors : energy_now, energy_full and power_now.

anthonyfieroni added inline comments. INLINE COMMENTS > acpi.c:147 > if ( maximum > 0) { > -state = charge * 100 / maximum; > +state = charge/(float)(maximum/100);/* to get 0.1% changes */ > } `(float)(var_int/var_int)` does not what you want, it should be

D28074: [kstyle] Avoid invalid iterators in qDeleteAll

anthonyfieroni added a comment. In D28074#628309 , @zzag wrote: > Well, we could do something like this (not sure whether it works though) > > for (KWindowShadow *shadow : _shadows) > disconnect(shadow, nullptr, this, nullptr); >

D28286: [Task Manager] Port backend to ApplicationLauncherJob

anthonyfieroni added a comment. In D28286#641165 , @dfaure wrote: > That wouldn't work either, you need to be able to choose between a Notification delegate, a Dialog delegate (which lives in a different library due to the QtWidgets

D28286: [Task Manager] Port backend to ApplicationLauncherJob

anthonyfieroni added a comment. OK, let's not keep things as they are. Because the best i can make without dedicated function is `new KIO::ApplicationLauncherJob(service, KIO::ApplicationLauncher::WITH_AUTO_ERROR_HANDLED_DELEGATE)` REPOSITORY R119 Plasma Desktop REVISION DETAIL

D28286: [Task Manager] Port backend to ApplicationLauncherJob

anthonyfieroni added a comment. And what about the idea to pass delegate to job constructor? At least it's better than current one. I'm pretty pedantic about duplicate code in plus it makes porting harder. REPOSITORY R119 Plasma Desktop REVISION DETAIL

D28215: [RFC]: WIP: Make mobile broadband actually functional

anthonyfieroni added inline comments. INLINE COMMENTS > main.qml:63-67 > +checked: false > +onEnabledChanged: { > +if (!enabled) > +checked = false > +} This should work enabled: mobileDataCheckbox.enabled checked :

D28286: [Task Manager] Port backend to ApplicationLauncherJob

anthonyfieroni added inline comments. INLINE COMMENTS > backend.cpp:236-239 > +auto *job = new KIO::ApplicationLauncherJob(service); > +auto *delegate = new KNotificationJobUiDelegate; > +delegate->setAutoErrorHandlingEnabled(true); > +

D28286: [Task Manager] Port backend to ApplicationLauncherJob

anthonyfieroni added a comment. In D28286#636668 , @dfaure wrote: > Benefit: available everywhere, unlike a local wrapper function. I think wrapper function in KIO :) REPOSITORY R119 Plasma Desktop REVISION DETAIL

D28286: [Task Manager] Port backend to ApplicationLauncherJob

anthonyfieroni added a comment. Why not? Furthermore they can be easy fixable. REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D28286 To: broulik, #plasma, hein, dfaure Cc: anthonyfieroni, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh,

D28286: [Task Manager] Port backend to ApplicationLauncherJob

anthonyfieroni added a comment. In D28286#636881 , @dfaure wrote: > Why not -> because it just doesn't scale. 30 jobs * 4 delegates = 120 wrapper methods... Even 1200 is not problem to me. REPOSITORY R119 Plasma Desktop REVISION

D28289: Refactor of OverlaySheet

anthonyfieroni added inline comments. INLINE COMMENTS > OverlaySheet.qml:2-19 > +* Copyright (C) 2016 by Marco Martin > +* > +* This program is free software; you can redistribute it and/or modify > +* it under the terms of the GNU Library General Public License as > +* published by the

D29028: feat(wayland): add Wrapland plugin

anthonyfieroni added a comment. In D29028#653194 , @romangg wrote: > In D29028#653192 , @apol wrote: > > > I don't really see why we'd want to support something that is not offering ABI stability

D29807: WIP: Change Chrome API design

anthonyfieroni added reviewers: broulik, Plasma. anthonyfieroni added a comment. Commented code should be removed. INLINE COMMENTS > chromeprofile.h:11 > +public: > +ProfileBookmarks(const QString , const QString ); > +static QList findAllProfiles(const QString > ); You can move

D29616: Fix memory errors caused by using dangling pointers to SensorClients in SensorAgent

anthonyfieroni added inline comments. INLINE COMMENTS > SensorBrowser.cpp:53 > +} > qDeleteAll( mHostInfoMap ); > mHostInfoMap.clear(); When you delete the map content, all agents should loose their connections, no? REPOSITORY R106 KSysguard REVISION DETAIL

D29688: Ignore mount paths that start with '/snap/'

anthonyfieroni added a comment. Should it /var/lib/flatpak (or just /flatpak/) be included as well? REPOSITORY R106 KSysguard BRANCH ignore_snap_partitions REVISION DETAIL https://phabricator.kde.org/D29688 To: ahiemstra, #plasma, davidedmundson Cc: anthonyfieroni, jriddell,

D29506: rejigger lookup of services by exec

anthonyfieroni added inline comments. INLINE COMMENTS > servicerunner.cpp:164 > > -QString finalQuery = QStringLiteral("exist Exec and ( (%1) or (%2) > or (%3) or ('%4' ~~ Exec) or (%5) )") > -.arg(keywordTemplate, genericNameTemplate, nameTemplate, >

D28135: Port away from deprecated QSet/QList methods in some places

anthonyfieroni added inline comments. INLINE COMMENTS > runnermodel.cpp:179 > +if (currRunners != newRunners) { > +m_runners = newRunners; > Here should be `m_runners = runners` to be exactly same as previous. I don't see much benefit of having duplicate items. REPOSITORY R120

D28135: Port away from deprecated QSet/QList methods in some places

anthonyfieroni added inline comments. INLINE COMMENTS > ahmadsamir wrote in runnermodel.cpp:179 > IIUC, the original code used toSet() to remove duplicates from both > "m_runners" and "runners", because QSet doesn't allow duplicate items. But toSet() returns new container m_runners and runners

D28107: Fix overlayIcon sometimes not visible

anthonyfieroni added a comment. Can we just set/clear OverlayIconName depends of icon presence then `overlays: model.OverlayIconName` ? REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D28107 To: davidre, kmaterka, #plasma, broulik, davidedmundson Cc:

D12405: [WIP] Per-screen scale factors on X11 using QT_SCREEN_SCALE_FACTORS

anthonyfieroni added a comment. I test, current situation, i don't think the parch is needed as is. 2 monitors FullHD and 4K (HDMI) now KScreen does not allow different scale factor, to distinct monitor but you can changed config [KScreen] ScaleFactor=1

[Differential] [Commented On] D3923: Make AppstreamQt optional

anthonyfieroni added a comment. In https://phabricator.kde.org/D3923#73375, @mak wrote: > In any case, knowing the distro might be useful to check whether their packaging makes sense ;-) KaOS don't have appstream nor appstreamQt nor Discover (it's a fairly normal when first two

[Differential] [Changed Subscribers] D3738: [Task Manager] Tooltips redesign

anthonyfieroni added inline comments. INLINE COMMENTS > subdiff wrote in ToolTipDelegate.qml:199 > I wanted to use Headings with different levels. The problem is, that they > always automatically create huge margins. Since we wanted to minimize the > size of the tooltips as much as possible, I

[Differential] [Commented On] D3738: [Task Manager] Tooltips redesign

anthonyfieroni added a comment. Maybe we can add configuration for tooltip size? I, personally, like very much biggest ones. REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D3738 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/

<    2   3   4   5   6   7