D29815: Fix blurry icons in titlebar appmenu by adding UseHighDpiPixmaps flag

anthonyfieroni accepted this revision. anthonyfieroni added a comment. This revision is now accepted and ready to land. @ngraham i don't think it's needed this for existing reviews, it's double work for nothing. It has 2 weeks testing before 5.71 release just push it, it can be reverted if

D29815: Fix blurry icons in titlebar appmenu by adding UseHighDpiPixmaps flag

anthonyfieroni added a comment. Wait @davidedmundson to accept it. REPOSITORY R297 KDED REVISION DETAIL https://phabricator.kde.org/D29815 To: mthw, #frameworks, davidedmundson Cc: anthonyfieroni, broulik, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29815: Fix blurry icons in titlebar appmenu by adding UseHighDpiPixmaps flag

anthonyfieroni added a reviewer: davidedmundson. anthonyfieroni added a comment. So change should be fine, (https://phabricator.kde.org/source/plasma-workspace/browse/master/appmenu) since KDED loads only modules they don't know where and when qApp is instantiated. REPOSITORY R297 KDED

D29391: Introduce setWindow and CloseWhenWindowActivated

anthonyfieroni added inline comments. INLINE COMMENTS > knotification.cpp:263 > d->needUpdate = true; > d->flags = flags; > if (d->id >= 0) { Flags can be changed, disconnect window, if present and activation flags is not set, connect in opposite. > knotification.cpp:585 > + >

D29815: Fix blurry icons in titlebar appmenu by adding UseHighDpiPixmaps flag

anthonyfieroni added a reviewer: zzag. anthonyfieroni added a comment. Is that KWin that set titlebar menus? REPOSITORY R297 KDED REVISION DETAIL https://phabricator.kde.org/D29815 To: mthw, #frameworks, zzag Cc: anthonyfieroni, broulik, kde-frameworks-devel, LeGast00n, cblack,

D29811: t/simplify-sending-data-through-socket

anthonyfieroni added a comment. > But I'm not quite ready for 5.70 yet, that would probably mean recompiling most of KDE That's not need. Check out master of KCrash, CMakeLists.txt lower kf5 version to match your, compile new patch and test. Then create patch without lower version

D29810: Don't use setenv after fork

anthonyfieroni added inline comments. INLINE COMMENTS > kcrash.cpp:825 > + > +std::array environ_data; //hope it's big enough > +if((unsigned)(environ_end - environ) +2 >= environ_data.size()) { Just use vector, get the size by `environ_end - environ` REPOSITORY R285 KCrash REVISION

D29569: Fix computing display geometry on multi-monitor HiDPI setups on X11

anthonyfieroni added a reviewer: zzag. REPOSITORY R278 KWindowSystem REVISION DETAIL https://phabricator.kde.org/D29569 To: printesoi, davidedmundson, #kwin, zzag Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D25339: update lineHeight if boundingRect indicates a larger value.

anthonyfieroni added inline comments. INLINE COMMENTS > katerenderer.cpp:1192 > +// trigger view update, if any! > +QMetaObject::invokeMethod(m_view, "updateRendererConfig", > Qt::QueuedConnection); > +} Can you use functor here, instead of string. REPOSITORY R39

D29101: KNewStuff: Fix file path and process call

anthonyfieroni accepted this revision. anthonyfieroni added a comment. KNewstuff is framework, it has only master. REPOSITORY R304 KNewStuff BRANCH arcpatch-D29101 REVISION DETAIL https://phabricator.kde.org/D29101 To: alex, #knewstuff, ngraham, nicolasfella, elvisangelaccio, meven,

D29101: KNewStuff: Fix file path and process call

anthonyfieroni added inline comments. INLINE COMMENTS > installation.cpp:610 > +QStringList args = KShell::splitArgs(command); > +int exitcode = QProcess::execute(args.takeFirst(), args); > Get program in exclusive line auto program = args.takeFirst(); int

D25339: KateRenderer: Use representitive character in CJK to estimate the fontHeight.

anthonyfieroni added a comment. +1, looks great. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D25339 To: xuetianweng, #ktexteditor, cullmann, dhaumann Cc: anthonyfieroni, kde-frameworks-devel, kwrite-devel, rrosch, LeGast00n, cblack, domson, michaelh, ngraham,

D28669: make CopyJob non-recursive

anthonyfieroni added inline comments. INLINE COMMENTS > dfaure wrote in copyjob.cpp:814 > Right. If this loop can really block the main thread for a substantial amount > of time, then it should have a condition like "after 100 symlinks, schedule > coming back here (e.g. QTimer singleshot) and

D28669: make CopyJob non-recursive

anthonyfieroni added inline comments. INLINE COMMENTS > dfaure wrote in copyjob.cpp:814 > Which threads? There are no threads involved here. > > There is no need to handling killing here. It wasn't any different in the > orig code with the recursion. > As soon as we find actual work to do,

D28669: make CopyJob non-recursive

anthonyfieroni added inline comments. INLINE COMMENTS > meven wrote in copyjob.cpp:814 > about @anthonyfieroni comment > Add `&& !isKilled()` with a code path to handle it properly. It'll not help, message queue will be blocked and you kill the from other thread, which is not what we want.

D28669: make CopyJob non-recursive

anthonyfieroni added a comment. If you want to kill the job how this loop will be break? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D28669 To: McPain, #frameworks, dfaure, meven, ahmadsamir Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh,

D28535: [KIO-MTP] Fix null pointer dereference

anthonyfieroni added a comment. @elvisangelaccio this peace of code is purely wrong at least `m_storages` is not updated to new device and not only. This code should never exists or try to hide some other bug. REPOSITORY R320 KIO Extras BRANCH fixNullPtr (branched from master)

D28535: [KIO-MTP] Fix null pointer dereference

anthonyfieroni added a comment. Ok that's look good to me. We can move that code in constructor just before loop for storages, then use m_mtpdevice instead of raw device, but it looks like removed code is just a noise and it shouldn't present at all. +1 REPOSITORY R320 KIO Extras

D28535: [KIO-MTP] Fix null pointer dereference

anthonyfieroni added a comment. In D28535#640699 , @feverfew wrote: > So to be succinct, the only correct fix here is to change `getDevice()` to `return m_mtpdevice`? Yes, then check if it's crash, in all other place `LIBMTP_xxx` should

D28535: [KIO-MTP] Fix null pointer dereference

anthonyfieroni added a comment. @feverfew are you gonna try what i'm writing about or i should do it? Just use cached device, do not reopen since it'll return nullptr. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28535 To: feverfew, akrutzler, dfaure,

D28535: [KIO-MTP] Fix null pointer dereference

anthonyfieroni added a comment. I see we don't speak in same language :) `LIBMTP_Open_Raw_Device_Uncached(_rawdevice);` returns nullptr that's normal since device is inaccessible, i mean it does not need to call `LIBMTP_Release_Device` using `m_mtpdevice` is safe it's not nullptr, it's

D28535: [KIO-MTP] Fix null pointer dereference

anthonyfieroni added a comment. In D28535#640672 , @fvogt wrote: > If `getDevice()` returns nullptr, this means that `MTPDevice::getDevice()` returns nullptr. This can only happen if `m_mtpdevice` is nullptr, which will crash in

D28535: [KIO-MTP] Fix null pointer dereference

anthonyfieroni added a comment. You're right about bug report, but it can fail in any other place, just in particular version it happen in `updateStorageInfo` Can we cache `getDevice` in m_device (in constructor) then use it everywhere. I think libmtp has guard against disconnected device

D28498: [xdgoutput] Explicitly set version of server interface

anthonyfieroni added inline comments. INLINE COMMENTS > display.h:296 > + */ > +XdgOutputManagerInterface *createXdgOutputManager(const > XdgOutputInterfaceVersion , QObject *parent = nullptr); > + explicit, also take enum class by value. REPOSITORY R127 KWayland REVISION DETAIL

D28476: Samba: Ensure to differenciate mounts sharing the same source

anthonyfieroni added inline comments. INLINE COMMENTS > fstabhandling.cpp:127 > return fstype + mountpoint; > +} else if (fstype == "cifs") { > +// append mountpoints to samba device name as they don't contain it > in getmntent use `QLatin1String` REPOSITORY R245

D21466: Recommend rebooting after installing Samba

anthonyfieroni added a comment. Just start services, reboot will not do anything else. REPOSITORY R432 File Sharing (Samba) integration REVISION DETAIL https://phabricator.kde.org/D21466 To: ngraham, #vdg, #frameworks, #dolphin, apol Cc: anthonyfieroni, sitter, bruns

D27766: [ModifiedFileIndexer] Omit BasicIndexingJob run when not required

anthonyfieroni added inline comments. INLINE COMMENTS > anthonyfieroni wrote in modifiedfileindexer.cpp:118 > This check is redundant now, we here only when `cTimeChanged` is true. I saw it in other patch, i does not need change. REPOSITORY R293 Baloo REVISION DETAIL

D27766: [ModifiedFileIndexer] Omit BasicIndexingJob run when not required

anthonyfieroni added inline comments. INLINE COMMENTS > modifiedfileindexer.cpp:118 > if (tr.hasDocument(job.document().id())) { > if (cTimeChanged) { > tr.replaceDocument(job.document(), XAttrTerms | DocumentTime > | FileNameTerms | DocumentUrl); This

D27039: [KStyle] Set the color of KMessageWidgets to the correct one from the current color scheme

anthonyfieroni accepted this revision. anthonyfieroni added a comment. This revision is now accepted and ready to land. No, just to not reviewed only by me. REPOSITORY R252 Framework Integration BRANCH messagewidget (branched from master) REVISION DETAIL

D27039: [KStyle] Set the color of KMessageWidgets to the correct one from the current color scheme

anthonyfieroni added a reviewer: aacid. anthonyfieroni added a comment. +1 REPOSITORY R252 Framework Integration REVISION DETAIL https://phabricator.kde.org/D27039 To: davidre, #frameworks, aacid Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham,

D27533: [WIP] Add MarkInterfaceV2, to s/QPixmap/QIcon/g for symbols of marks

anthonyfieroni added inline comments. INLINE COMMENTS > kossebau wrote in kateviewhelpers.cpp:1963 > Possibly `QIcon::paint()` might be also working here as wanted? Needs someone > with HiDPI to check if all things behave as wanted. The old code with all the > `devicePixelRatioF()` made me

D27533: [WIP] Add MarkInterfaceV2, to s/QPixmap/QIcon/g for symbols of marks

anthonyfieroni added inline comments. INLINE COMMENTS > katedocument.h:86 > class KTEXTEDITOR_EXPORT KTextEditor::DocumentPrivate : public > KTextEditor::Document, > -public > KTextEditor::MarkInterface, > +

D27504: WIP: RFC: smb faster copy to local

anthonyfieroni added inline comments. INLINE COMMENTS > kio_smb_dir.cpp:49 > +: segmentSize(static_cast(segmentSize_)) // won't be <0 > +, buf(static_cast(malloc(segmentSize))) > +{ Segment does not free its memory. Why not use QByteArray or similar with automatic storage

D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

anthonyfieroni added inline comments. INLINE COMMENTS > kurlnavigatorplacesselector.cpp:76 > +for(QObject *obj : QObjectList(m_placesMenu->children())) { > +delete qobject_cast(obj); // Noop for nullptr > +} Why cast? REPOSITORY R241 KIO REVISION DETAIL

D26957: KateModeMenuList: don't overlap the scroll bar

anthonyfieroni added a comment. Don't remove default parameter, it'll break other users. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D26957 To: nibags, cullmann, #ktexteditor Cc: anthonyfieroni, kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, GB_2,

D26848: Don't assume the manager and menu have the same lifetime

anthonyfieroni added inline comments. INLINE COMMENTS > kcolorschememanager.cpp:191 > QActionGroup *group = new QActionGroup(menu); > -connect(group, ::triggered, this, [this](QAction * action) > { > -activateScheme(d->model->index(action->data().toInt())); > +

D26801: Really fix the Windows backend for KNotifications

anthonyfieroni added inline comments. INLINE COMMENTS > brute4s99 wrote in notifybysnore.cpp:143 > KNotif objects by default have `d->id = -1` > > `notify()` is invoked by the base with -1 ID and later updated with the > correct ID and the actions. > > Weirdly though, `pairingRequest`

D26755: [WIP] KMessageWidget: Set widget height on resize event

anthonyfieroni added a comment. That's same issue when file is open then externally modified then updated by kmessagewidget button cause a frame to not resize correct? REPOSITORY R236 KWidgetsAddons BRANCH more-proper-height (branched from master) REVISION DETAIL

D26691: Optimize code when dropping files into the desktop

anthonyfieroni added inline comments. INLINE COMMENTS > containmentinterface.cpp:589 > +} > +qDebug() << "clearDataForMimeJob() ends."; > } Don't add uncategorized qDebug, i see it exists in code base but they should be ported as well. > containmentinterface.cpp:816-819 > +

D26650: Use KService to look for Filelight

anthonyfieroni added inline comments. INLINE COMMENTS > meven wrote in kpropertiesdialog.cpp:1114 > service is a QExplicitlySharedDataPointer in fact, I guess it > covers lambda use cases. OK, it can be a problem since we can have many objects of KFilePropsPlugin thus lambda will extend

D26650: Use KService to look for Filelight

anthonyfieroni added inline comments. INLINE COMMENTS > kpropertiesdialog.cpp:1114 > + > d->m_sizeDetailsButton->setIcon(QIcon::fromTheme(service->icon())); > connect(d->m_sizeDetailsButton, ::clicked, this, > ::slotSizeDetails); >

D26650: Use KService to look for Filelight

anthonyfieroni added inline comments. INLINE COMMENTS > kpropertiesdialog.cpp:1452-1457 > void KFilePropsPlugin::slotSizeDetails() > { > // Open the current folder in filelight > -KRun::run((QStandardPaths::findExecutable(QStringLiteral("filelight"))), > { properties->url() },

D26650: Use KService to look for Filelight

anthonyfieroni added inline comments. INLINE COMMENTS > kpropertiesdialog.cpp:1455 > // Open the current folder in filelight > -KRun::run((QStandardPaths::findExecutable(QStringLiteral("filelight"))), > { properties->url() }, properties->window(), QStringLiteral("Filelight"), >

D26117: [solid] Clarify referencing of DeviceInterface

anthonyfieroni abandoned this revision. anthonyfieroni added a comment. https://phabricator.kde.org/R245:3ff3aaa6640c0fb14bba5430110b20237105c203 REPOSITORY R245 Solid REVISION DETAIL https://phabricator.kde.org/D26117 To: anthonyfieroni, broulik, bruns Cc: kde-frameworks-devel,

D26484: Add a new parameter for delaying showing menu

anthonyfieroni added inline comments. INLINE COMMENTS > dropjob.h:129 > */ > -KIOWIDGETS_EXPORT DropJob *drop(const QDropEvent *dropEvent, const QUrl > , JobFlags flags = DefaultFlags); > +KIOWIDGETS_EXPORT DropJob *drop(const QDropEvent *dropEvent, const QUrl > , JobFlags flags =

D26432: Port endl to std::cout/std::cerr or "\n" + flush when necessary. In qt5.15 endl is namespaced.

anthonyfieroni added inline comments. INLINE COMMENTS > kconfigtest.cpp:533-542 > +out << "[Test Group]\n" > +<< "homePath=$HOME/foo\n" > +<< "homePath2=file://$HOME/foo\n" > +<< "withSlash=$WITHSLASH/foo\n" > +<< "withSlash2=$WITHSLASH\n"

D26407: KFileItem: improve isSlow to never block, make SkipMimeTypeFromContent skip only network fs

anthonyfieroni added inline comments. INLINE COMMENTS > kfileitem.cpp:49-50 > + */ > +static KMountPoint::List mountPoints; > +static QDateTime lastMountPointUpdate; > + Put them in `getMountPoints` as local static variables, global static are no-go. > dfaure wrote in kfileitem.cpp:787 > This

D26117: [solid] Clarify referencing of DeviceInterface

anthonyfieroni added a comment. Ping what else to do? If no objectives i'll push it for next framework release. REPOSITORY R245 Solid REVISION DETAIL https://phabricator.kde.org/D26117 To: anthonyfieroni, broulik, bruns Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham,

D26407: KFileItem: improve isSlow to never block, make SkipMimeTypeFromContent skip only network fs

anthonyfieroni added inline comments. INLINE COMMENTS > kfileitem.cpp:782 > if (!path.isEmpty()) { > -const KFileSystemType::Type fsType = > KFileSystemType::fileSystemType(path); > -m_slow = (fsType == KFileSystemType::Nfs || fsType == > KFileSystemType::Smb)

D26317: Port endl to "\n". endl in qt5.15 is namespaced. We don't need to flush as when QFile is deleted it flush data

anthonyfieroni added a comment. In D26317#585315 , @aacid wrote: > Isn't it better to just use `Qt::endl` ? No, it's not. It's namespaced in Qt 5.15 means it should be ifdef guarded which makes things to suck. REPOSITORY R238

D26317: Port endl to "\n". endl in qt5.15 is namespaced. We don't need to flush as when QFile is deleted it flush data

anthonyfieroni added a comment. Will be better if you use QLatin1Char('\n'), "\n" will call strlen on which is unneeded. Also QStringLiteral is missing on some strings. REPOSITORY R238 KDocTools REVISION DETAIL https://phabricator.kde.org/D26317 To: mlaurent, dfaure Cc: anthonyfieroni,

D26117: [solid] Clarify referencing of DeviceInterface

anthonyfieroni added a comment. I've using the patch till now with no issues. REPOSITORY R245 Solid REVISION DETAIL https://phabricator.kde.org/D26117 To: anthonyfieroni, broulik, bruns Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D17816: Support for xattrs on kio copy/move

anthonyfieroni added inline comments. INLINE COMMENTS > copyxattrjob.cpp:98-111 > + const int bsrc_fd = open(m_bsrc, 0); > + if (bsrc_fd < 0) > + { > +q->setErrorText(QLatin1String("failed to obtain file descriptor of > source during xattr copy")); > +

D26117: [solid] Clarify referencing of DeviceInterface

anthonyfieroni added a comment. Ok, is it clear now? REPOSITORY R245 Solid REVISION DETAIL https://phabricator.kde.org/D26117 To: anthonyfieroni, broulik, bruns Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26117: [solid] Clarify referencing of DeviceInterface

anthonyfieroni added a comment. You can't run kinfocenter or what? Application: Info Center (kinfocenter), signal: Segmentation fault Using host libthread_db library "/lib/libthread_db.so.1". [Current thread is 1 (Thread 0x7f4c245a2840 (LWP 318849))] Thread 4 (Thread

D26117: [solid] Clarify referencing of DeviceInterface

anthonyfieroni edited the summary of this revision. anthonyfieroni edited the test plan for this revision. REPOSITORY R245 Solid REVISION DETAIL https://phabricator.kde.org/D26117 To: anthonyfieroni, broulik, bruns Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26117: [solid] Clarify referencing of DeviceInterface

anthonyfieroni created this revision. anthonyfieroni added a reviewer: broulik. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. anthonyfieroni requested review of this revision. REVISION SUMMARY DeviceInterface should be treated as weak object, also let he

D25934: [xattr] Fix passing negative size to QByteArray

anthonyfieroni abandoned this revision. REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D25934 To: anthonyfieroni, bruns, astippich Cc: kde-frameworks-devel, #baloo, hurikhan77, lots0logs, LeGast00n, fbampaloukas, GB_2, domson, ashaposhnikov, michaelh, astippich,

D25934: [xattr] Fix passing negative size to QByteArray

anthonyfieroni created this revision. anthonyfieroni added reviewers: bruns, astippich. Herald added projects: Frameworks, Baloo. Herald added subscribers: Baloo, kde-frameworks-devel. anthonyfieroni requested review of this revision. REVISION SUMMARY CCBUG: 414227 TEST PLAN Not tested

D25902: Fix decrement index and not iterator as discussed with David

anthonyfieroni accepted this revision. This revision is now accepted and ready to land. REPOSITORY R320 KIO Extras BRANCH fix_kioslave_notifier (branched from master) REVISION DETAIL https://phabricator.kde.org/D25902 To: mlaurent, dfaure, anthonyfieroni Cc: anthonyfieroni,

D25902: Fix decrement index and not iterator as discussed with David

anthonyfieroni added inline comments. INLINE COMMENTS > kioslavenotifier.cpp:114 > else > *it++; > } Same here REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D25902 To: mlaurent, dfaure Cc: anthonyfieroni, kde-frameworks-devel, kfm-devel, pberestov,

D25891: fix preview of plain text files when using dark theme

anthonyfieroni accepted this revision. This revision is now accepted and ready to land. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D25891 To: iliakats, #dolphin, anthonyfieroni Cc: kde-frameworks-devel, kfm-devel, pberestov, iasensio, fprice, LeGast00n, MrPepe,

D25219: Only create a session config when actually restoring a session

anthonyfieroni added inline comments. INLINE COMMENTS > kconfiggui.cpp:73 > { > return sessionConfig()->name(); > } sessionConfig() can be nullptr can you add a check. REPOSITORY R237 KConfig REVISION DETAIL https://phabricator.kde.org/D25219 To: ngraham, #frameworks,

D25339: KateRenderer: Use representitive character in CJK to estimate the fontHeight.

anthonyfieroni added a comment. In D25339#563822 , @xuetianweng wrote: > Having different font height for every line? 1. We don't want a bigger lines 2. We don't want a lines that are bigger that other in a view. 3. We don't

D21235: Add handling of fuseiso filesystem type

anthonyfieroni added inline comments. INLINE COMMENTS > fstabdevice.cpp:92 > +if (!isoFilePath.isEmpty()) { > +m_description = ShortenPath(isoFilePath) + QLatin1String(" > on ") + m_product; > +} That's incorrect concatenation of translated strings.

D25308: when kioslave5 couldn't be found in libexec-ish locations try $PATH

anthonyfieroni added inline comments. INLINE COMMENTS > slave.cpp:521 > +// isn't the same as applicationDirPath(). > +QString kioslaveExecutable = > QStandardPaths::findExecutable(QStringLiteral("kioslave5")); > +} Remove QString declaration before. REPOSITORY

D25125: Use KListOpenFilesJob for retrieving apps blocking unmount

anthonyfieroni added a comment. Set QT_PLUGIN_PATH to you local path with plugin after that set the system path for others `QT_PLUGIN_PATH=/local/path:/system/path executable` INLINE COMMENTS > ksolidnotify.h:50 > void notify(Solid::ErrorType solidError, const QString& error, const

D25039: Fix Clazy performance issues, const

anthonyfieroni added a comment. Not using references is not a big problem with QString nor QUrl since they are reference counting objects, say if you don't change their content they are immutable, so const QString s = other; // it's fine void func(QString s) { const

D25059: KPluginSelector: use new KAboutPluginDialog

anthonyfieroni added inline comments. INLINE COMMENTS > kpluginselector.cpp:799-803 > +PluginEntry *pluginEntry = model->data(index, > PluginEntryRole).value(); > +KPluginMetaData pluginMetaData = pluginEntry->pluginInfo.toMetaData(); > + > +KAboutPluginDialog

D24755: [KStatusNotifierItem] Optionaly, do not take menu ownership

anthonyfieroni added a comment. In D24755#551320 , @kmaterka wrote: > In D24755#550415 , @anthonyfieroni wrote: > > > That should be fine, in QPA we have a tray icon per sni, update menu should be

D24755: [KStatusNotifierItem] Optionaly, do not take menu ownership

anthonyfieroni added a comment. F7633806: p.patch That should be fine, in QPA we have a tray icon per sni, update menu should be on same object which will not be a problem, check it. REPOSITORY R289 KNotifications REVISION DETAIL

D24755: [KStatusNotifierItem] Optionaly, do not take menu ownership

anthonyfieroni added inline comments. INLINE COMMENTS > kmaterka wrote in kstatusnotifieritem.cpp:790 > This check is not reliable, assosiatedWidget can change. Anyway, this doesn't > matter here. > Did you read whole comment? Probably KSNI should not own the menu but it is > doing that for 10

D24755: [KStatusNotifierItem] Optionaly, do not take menu ownership

anthonyfieroni added inline comments. INLINE COMMENTS > kmaterka wrote in kstatusnotifieritem.cpp:790 > "associatedWidget" is a KSNI parent (line 780). It might be or might not be > set. If parent is not set, then "associatedWidget" is null and QMenu does not > have parent. This is fine, we

D24755: [KStatusNotifierItem] Optionaly, do not take menu ownership

anthonyfieroni added inline comments. INLINE COMMENTS > kstatusnotifieritem.cpp:454 > Qt::WindowFlags oldFlags = d->menu->windowFlags(); > d->menu->setParent(nullptr); > d->menu->setWindowFlags(oldFlags); Do not take ownership of the menu and delete it when it does not have a

D24755: [KStatusNotifierItem] Optionaly, do not take menu ownership

anthonyfieroni added inline comments. INLINE COMMENTS > kmaterka wrote in kstatusnotifieritemprivate_p.h:158 > Sure, but this is not my code and this is not related. What I want to do here > is more like hack/workaround, so it is better to keep it as simple as > possible. It's related and you

D24755: [KStatusNotifierItem] Optionaly, do not take menu ownership

anthonyfieroni added inline comments. INLINE COMMENTS > kstatusnotifieritemprivate_p.h:158 > > QMenu *menu; > QHash actionCollection; It should be QPointer, no other changes are needed. REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D24755 To:

D23579: WIP: port ftp slave to new error reporting system

anthonyfieroni added inline comments. INLINE COMMENTS > ftp.h:49 > > //=== > -class Ftp : public QObject, public KIO::SlaveBase > { That's BIC we cannot remove, reorder nor insert parent(s) > ftp.h:167 > + > +class

D24012: Supress mouse events in KCMs causing window moves

anthonyfieroni added inline comments. INLINE COMMENTS > kcmoduleqml.cpp:265-268 > +bool rc = KCModule::event(event); > +if (event->type () == QEvent::MouseButtonPress || event->type() == > QEvent::MouseButtonRelease) { > +event->accept(); > +} Should we check `if (rc &&

D23420: Use solid to check if a KFileItem is located on a network mount

anthonyfieroni added inline comments. INLINE COMMENTS > anthonyfieroni wrote in kfileitem.cpp:764 > Get it as `const Solid::StorageAccess`, you can use `auto storageAccess = ...` Let clarify what David mean for (const Solid::Device& device : devices) { auto storageAccess = device.as();

D23420: Use solid to check if a a KFileItem is located on a network mount

anthonyfieroni added inline comments. INLINE COMMENTS > meven wrote in kfileitem.cpp:764 > Can't because of > > Solid::StorageAccess *storageAccess = device.as(); Get it as `const Solid::StorageAccess`, you can use `auto storageAccess = ...` REPOSITORY R241 KIO BRANCH

D23420: Use solid to check if a a KFileItem is located on a network mount

anthonyfieroni added inline comments. INLINE COMMENTS > kfileitem.cpp:767-771 > +m_slow = Slow; > +break; > +} > } > +m_slow = Fast; Wrong logic, when you set it to Slow, after break it will go to line 771 and became Fast again. Set

D23008: [baloo_file_extractor] Be more resistant to multiple QSocketNotifier events

anthonyfieroni added inline comments. INLINE COMMENTS > app.cpp:76-81 > +if (m_tr) { > +// Parent proccess yielded us a new batch while previous have not > yet finished > +// This should not happen, but better play safe > +m_tr->commit(); > +delete m_tr; >

D22155: Add new activities and virtual desktops icons

anthonyfieroni added a comment. In D22155#488593 , @ngraham wrote: > In D22155#488485 , @anthonyfieroni wrote: > > > Renaming the icon is not quite good, if someone use newer framework and old

D22155: Add new activities and virtual desktops icons

anthonyfieroni added a comment. In D22155#488497 , @GB_2 wrote: > We can backport the changes in Plasma to the LTS branch though. We can backport but you cannot increase depends on 5.60 to the LTS branch. So it should have a fallback

D22155: Add new activities and virtual desktops icons

anthonyfieroni added a comment. Renaming the icon is not quite good, if someone use newer framework and old applications / Plasma will not see an icon. That's pretty happen since framework incorporate new features + bugs fix and Plasma offers LTS. REPOSITORY R266 Breeze Icons BRANCH

D22102: Implement apply-on-double-click for all grid view KCM delegates

anthonyfieroni added a comment. Alternatively you can add message widget that explore double click functionality, when double click is applied it can change its message that save is applied. But rater OK/Apply became a bit pointless. REPOSITORY R296 KDeclarative REVISION DETAIL

D21204: Ensure no trailing slash in mountpoint read from fstab file.

anthonyfieroni added inline comments. INLINE COMMENTS > fstabhandling.cpp:231 > +// it will match its eventual mounted device regardless whether or not > it ends with a slash > +for (QString device : fstabDevices) { > +QString deviceName = device; get it by const ref. >

D21721: [WIP] Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

anthonyfieroni added a comment. QList should be QList>, don't leave ownership to consumers. REPOSITORY R304 KNewStuff REVISION DETAIL https://phabricator.kde.org/D21721 To: leinir Cc: anthonyfieroni, pino, ngraham, kde-frameworks-devel, LeGast00n, michaelh, bruns

D21315: Dolphin-style view modes in the file dialog

anthonyfieroni added inline comments. INLINE COMMENTS > kdiroperator.cpp:1955 > > +// View modes that match those of Dolphin > +KToggleAction *iconsViewAction = new KToggleAction(i18n("Icons View"), > this); Can we make them reusable, it should be in framework and then accessed in

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

anthonyfieroni added inline comments. INLINE COMMENTS > batchrenametypes.cpp:98-106 > +QStringList firstCapturedGroups() > +{ > +return capturedGroups; > +} > + > +void clearCapturedGroups() > +{ Make a class instead, free functions with global variable in, is no-go. It's totally miss of

D21280: kioslave: preserve argv[0], to fix applicationDirPath() on non-Linux

anthonyfieroni added inline comments. INLINE COMMENTS > kioslave.cpp:132 > +const int newArgc = argc - 1; > +char ** newArgv = new char*[newArgc]; > +newArgv[0] = argv[0]; Now has a leak, use vector instead. REPOSITORY R241 KIO BRANCH 2019_freebsd_fixed REVISION DETAIL

D20958: New menu of syntax highlighting in the status bar

anthonyfieroni added a comment. dymanic_cast is expensive, prefer qobject_cast if you want to check result against nullptr, if you don't want - static_cast. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D20958 To: nibags, #ktexteditor, #kate, #vdg Cc:

D20958: New menu of syntax highlighting in the status bar

anthonyfieroni added inline comments. INLINE COMMENTS > katemodemenulist.cpp:65 > +m_list->setVerticalScrollBarPolicy(Qt::ScrollBarAlwaysOff); > +m_list->setIconSize(QSize(16, 16)); > +m_list->setResizeMode(QListView::Adjust); As well for icon size. > katemodemenulist.cpp:129 > +

D20748: Fix wrong "Unable to find service type" warnings

anthonyfieroni added inline comments. INLINE COMMENTS > desktopfileparser.cpp:402 > // in cache but we still must make our own copy > m_definitions << *def; > +} else { Where def is deleted? REPOSITORY R244 KCoreAddons BRANCH master REVISION DETAIL

D20693: Remove pixelated border

anthonyfieroni added a comment. Will be good see how it looks in dark theme, especially dark pictures. REPOSITORY R304 KNewStuff BRANCH no-pixelated-border (branched from master) REVISION DETAIL https://phabricator.kde.org/D20693 To: leinir, #knewstuff, ngraham, sitter, #vdg Cc:

D20096: Fill UDSEntry::UDS_CREATION_TIME under linux when glibc >= 2.28

anthonyfieroni added inline comments. INLINE COMMENTS > file.cpp:850-870 > +inline static uint16_t stat_mode(struct statx buf) { return buf.stx_mode; } > +inline static uint32_t stat_dev(struct statx buf) { return > buf.stx_dev_major; } > +inline static uint64_t stat_ino(struct statx buf) {

D5111: Provide demo/preview for checkable menu items

anthonyfieroni removed a reviewer: anthonyfieroni. Herald added a project: Plasma. REPOSITORY R113 Oxygen Theme REVISION DETAIL https://phabricator.kde.org/D5111 To: rjvbb, hpereiradacosta, jriddell, zhigalin Cc: ltoscano, kde-mac, #frameworks, jraleigh, GB_2, ragreen, Pitel, ZrenBot,

D15763: Set correct image attributes on directory thumbnail

anthonyfieroni added a comment. What happen to this? Put it to 19.04 for testing. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D15763 To: broulik, #frameworks, dfaure, anthonyfieroni, jtamate Cc: ngraham, kde-frameworks-devel, kfm-devel, alexde, feverfew,

D19767: Fix malloc/delete mismatch

anthonyfieroni added a reviewer: dfaure. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D19767 To: hallas, #frameworks, dfaure Cc: kde-frameworks-devel, kfm-devel, alexde, feverfew, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp,

D18249: [datamodel] Rework items insert/remove

anthonyfieroni abandoned this revision. REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D18249 To: anthonyfieroni, davidedmundson, broulik, ngraham, mart, #plasma Cc: kde-frameworks-devel, michaelh, ngraham, bruns

  1   2   3   4   5   6   7   >