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

[Anthony Fieroni]

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

REPOSITORY
  R297 KDED

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D29815

To: mthw, #frameworks, davidedmundson, anthonyfieroni
Cc: ngraham, anthonyfieroni, broulik, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, bruns

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

[Anthony Fieroni]

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

[Anthony Fieroni]

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

REVISION DETAIL
  https://phabricator.kde.org/D29815

To: mthw, #frameworks, davidedmundson
Cc: anthonyfieroni, broulik, kde-frameworks-devel, LeGast00n, cblack, michaelh, 
ngraham, bruns

D29391: Introduce setWindow and CloseWhenWindowActivated

[Anthony Fieroni]

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
> +
> +d->window = window;
> +connect(window, ::activeChanged, this, [this, window]{

Disconnect previously window to this, if present

> knotification.cpp:586-590
> +connect(window, ::activeChanged, this, [this, window]{
> +if (window->isActive() && (d->flags &  CloseWhenWidgetActivated)) {
> +close();
> +}
> +});

Connect if flag present.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D29391

To: nicolasfella, #frameworks, broulik
Cc: anthonyfieroni, kossebau, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham, bruns

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

[Anthony Fieroni]

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, michaelh, 
ngraham, bruns

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

[Anthony Fieroni]

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 changes.
  That's not cheat since kf5 guarantee backward compatibility.

REPOSITORY
  R285 KCrash

REVISION DETAIL
  https://phabricator.kde.org/D29811

To: jpalecek, #frameworks
Cc: anthonyfieroni, davidedmundson, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham, bruns

D29810: Don't use setenv after fork

[Anthony Fieroni]

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 DETAIL
  https://phabricator.kde.org/D29810

To: jpalecek, #frameworks
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

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

[Anthony Fieroni]

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.

[Anthony Fieroni]

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 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D25339

To: xuetianweng, #ktexteditor, cullmann, dhaumann, #frameworks, rjvbb
Cc: brauch, sars, pshinjo, rjvbb, fakefred, anthonyfieroni, 
kde-frameworks-devel, kwrite-devel, rrosch, LeGast00n, cblack, domson, 
michaelh, ngraham, bruns, demsking, cullmann, dhaumann

D29101: KNewStuff: Fix file path and process call

[Anthony Fieroni]

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, mlaurent, 
leinir, anthonyfieroni
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D29101: KNewStuff: Fix file path and process call

[Anthony Fieroni]

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 exitcode = QProcess::execute(program, args);

The problem is args is modified at argument pass time, then second parameter 
expect it is. That's not guaranteed you should expect compiler to do the right 
thing.

REPOSITORY
  R304 KNewStuff

BRANCH
  bugfix_uninstall (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D29101

To: alex, #knewstuff, ngraham, nicolasfella, elvisangelaccio, meven, mlaurent, 
leinir
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

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

[Anthony Fieroni]

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, bruns, demsking, cullmann, sars, dhaumann

D28669: make CopyJob non-recursive

[Anthony Fieroni]

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 `return;` for now".

So if `m_currentStatSrc` does not point to `m_srcList` end it should add timer 
event to queue returning back here in next queue iteration.

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, 
ngraham, bruns

D28669: make CopyJob non-recursive

[Anthony Fieroni]

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, we'll go and launch a subjob, at which 
> point we go back to the event loop and can handle being killed.

> Which threads? There are no threads involved here.

Yes, i'm afraid of loop that can block the event queue, !isKilled will not work 
in single thread environment.

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, 
ngraham, bruns

D28669: make CopyJob non-recursive

[Anthony Fieroni]

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.

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, 
ngraham, bruns

D28669: make CopyJob non-recursive

[Anthony Fieroni]

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, 
ngraham, bruns

D28535: [KIO-MTP] Fix null pointer dereference

[Anthony Fieroni]

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)

REVISION DETAIL
  https://phabricator.kde.org/D28535

To: feverfew, akrutzler, dfaure, elvisangelaccio
Cc: anthonyfieroni, kde-frameworks-devel, fvogt, kfm-devel, ngraham, nikolaik, 
pberestov, iasensio, fprice, LeGast00n, cblack, fbampaloukas, alexde, GB_2, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, 
andrebarros, bruns, emmanuelp, rdieter, mikesomov

D28535: [KIO-MTP] Fix null pointer dereference

[Anthony Fieroni]

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

REVISION DETAIL
  https://phabricator.kde.org/D28535

To: feverfew, akrutzler, dfaure, elvisangelaccio
Cc: anthonyfieroni, kde-frameworks-devel, fvogt, kfm-devel, ngraham, nikolaik, 
pberestov, iasensio, fprice, LeGast00n, cblack, fbampaloukas, alexde, GB_2, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, 
andrebarros, bruns, emmanuelp, mikesomov

D28535: [KIO-MTP] Fix null pointer dereference

[Anthony Fieroni]

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 take 
care of and return false or nullptr depend of function returning value.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D28535

To: feverfew, akrutzler, dfaure, elvisangelaccio
Cc: anthonyfieroni, kde-frameworks-devel, fvogt, kfm-devel, ngraham, nikolaik, 
pberestov, iasensio, fprice, LeGast00n, cblack, fbampaloukas, alexde, GB_2, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, 
andrebarros, bruns, emmanuelp, mikesomov

D28535: [KIO-MTP] Fix null pointer dereference

[Anthony Fieroni]

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, elvisangelaccio
Cc: anthonyfieroni, kde-frameworks-devel, fvogt, kfm-devel, ngraham, nikolaik, 
pberestov, iasensio, fprice, LeGast00n, cblack, fbampaloukas, alexde, GB_2, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, 
andrebarros, bruns, emmanuelp, mikesomov

D28535: [KIO-MTP] Fix null pointer dereference

[Anthony Fieroni]

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 just a disconnected device and libmtp knows that.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D28535

To: feverfew, akrutzler, dfaure, elvisangelaccio
Cc: anthonyfieroni, kde-frameworks-devel, fvogt, kfm-devel, ngraham, nikolaik, 
pberestov, iasensio, fprice, LeGast00n, cblack, fbampaloukas, alexde, GB_2, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, 
andrebarros, bruns, emmanuelp, mikesomov

D28535: [KIO-MTP] Fix null pointer dereference

[Anthony Fieroni]

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 `MTPDevice::~MTPDevice` sooner or later anyway.
  
  
  I don't think so, libmtp knows device is not available then 
`LIBMTP_Release_Device` (invalid device) will not crash. So i prefer to cache 
device in storage as well so libmtp will just return false on invalid device.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D28535

To: feverfew, akrutzler, dfaure, elvisangelaccio
Cc: anthonyfieroni, kde-frameworks-devel, fvogt, kfm-devel, ngraham, nikolaik, 
pberestov, iasensio, fprice, LeGast00n, cblack, fbampaloukas, alexde, GB_2, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, 
andrebarros, bruns, emmanuelp, mikesomov

D28535: [KIO-MTP] Fix null pointer dereference

[Anthony Fieroni]

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 and will not crash.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D28535

To: feverfew, akrutzler, dfaure, elvisangelaccio
Cc: anthonyfieroni, kde-frameworks-devel, fvogt, kfm-devel, ngraham, nikolaik, 
pberestov, iasensio, fprice, LeGast00n, cblack, fbampaloukas, alexde, GB_2, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, 
andrebarros, bruns, emmanuelp, mikesomov

D28498: [xdgoutput] Explicitly set version of server interface

[Anthony Fieroni]

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
  https://phabricator.kde.org/D28498

To: davidedmundson, #kwin
Cc: anthonyfieroni, apol, kde-frameworks-devel, LeGast00n, cblack, GB_2, 
michaelh, ngraham, bruns

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

[Anthony Fieroni]

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 Solid

REVISION DETAIL
  https://phabricator.kde.org/D28476

To: meven, #frameworks
Cc: anthonyfieroni, sitter, kde-frameworks-devel, LeGast00n, cblack, GB_2, 
michaelh, ngraham, bruns

D21466: Recommend rebooting after installing Samba

[Anthony Fieroni]

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

[Anthony Fieroni]

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
  https://phabricator.kde.org/D27766

To: bruns, #baloo, ngraham
Cc: anthonyfieroni, kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, 
cblack, fbampaloukas, GB_2, domson, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D27766: [ModifiedFileIndexer] Omit BasicIndexingJob run when not required

[Anthony Fieroni]

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 check is redundant now, we here only when `cTimeChanged` is true.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D27766

To: bruns, #baloo, ngraham
Cc: anthonyfieroni, kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, 
cblack, fbampaloukas, GB_2, domson, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

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

[Anthony Fieroni]

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
  https://phabricator.kde.org/D27039

To: davidre, #frameworks, aacid, anthonyfieroni
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

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

[Anthony Fieroni]

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, bruns

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

[Anthony Fieroni]

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 change not too much here.

You should keep devicePixelRatioF calls

REPOSITORY
  R39 KTextEditor

BRANCH
  addmarkinterfacev2

REVISION DETAIL
  https://phabricator.kde.org/D27533

To: kossebau, #kate, #kdevelop, dhaumann
Cc: anthonyfieroni, dhaumann, kwrite-devel, kde-frameworks-devel, rrosch, 
LeGast00n, cblack, GB_2, domson, michaelh, ngraham, bruns, demsking, cullmann, 
sars

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

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> katedocument.h:86
>  class KTEXTEDITOR_EXPORT KTextEditor::DocumentPrivate : public 
> KTextEditor::Document,
> -public 
> KTextEditor::MarkInterface,
> +public 
> KTextEditor::MarkInterfaceV2,
>  public 
> KTextEditor::ModificationInterface,

Why private class is exported, changing parent of an exported class is BIC.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D27533

To: kossebau, #kate, #kdevelop
Cc: anthonyfieroni, dhaumann, kwrite-devel, kde-frameworks-devel, rrosch, 
LeGast00n, cblack, GB_2, domson, michaelh, ngraham, bruns, demsking, cullmann, 
sars

D27504: WIP: RFC: smb faster copy to local

[Anthony Fieroni]

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 allocation?

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D27504

To: sitter, ngraham, cfeck
Cc: anthonyfieroni, asturmlechner, kde-frameworks-devel, kfm-devel, pberestov, 
iasensio, fprice, LeGast00n, cblack, MrPepe, fbampaloukas, alexde, GB_2, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov

D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

[Anthony Fieroni]

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
  https://phabricator.kde.org/D27002

To: fvogt, #frameworks
Cc: anthonyfieroni, meven, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D26957: KateModeMenuList: don't overlap the scroll bar

[Anthony Fieroni]

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, domson, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

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

[Anthony Fieroni]

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()));
> +connect(group, ::triggered, [] (QAction * action) {
> +::activateScheme(action->data().toString());

You can connect to `qApp`

REPOSITORY
  R265 KConfigWidgets

REVISION DETAIL
  https://phabricator.kde.org/D26848

To: davidre, #frameworks
Cc: anthonyfieroni, broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D26801: Really fix the Windows backend for KNotifications

[Anthony Fieroni]

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` notification is not updated by the base, 
> (that is, the notification has all the information in the first invocation 
> itself) but still it has ID=-1 (because it's not updated later by the base, 
> hence the default ID).

So the problem isn't here. notification id should be always valid and not 
calculated here.

> notifybysnore.h:49
> +QHash rev_m_notifications;
> +int counter=0;
>  };

You cannot overlap signed integer, it's undefined behaviour.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D26801

To: brute4s99, vonreth, broulik, #kde_connect
Cc: anthonyfieroni, kde-frameworks-devel, nalvarez, KunalRaghav, ankitbaluni, 
ankit, aliencode, Orage, ritwizsinha, LeGast00n, ewentzel, dshelley, 
pawelkwiecinski, ctakano, vporvaznik, mschroeder, varunp, shivanshukantprasad, 
skymoore, fbampaloukas, GB_2, brute4s99, wistak, anoopv, dvalencia, rmenezes, 
julioc, Leptopoda, timothyc, Danial0_0, johnq, Pitel, domson, adeen-s, 
michaelh, SemperPeritus, daniel.z.tg, jeanv, seebauer, ngraham, bruns, bugzy, 
MayeulC, lemuel, menasshock, mikesomov, tctara, apol

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

[Anthony Fieroni]

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
  https://phabricator.kde.org/D26755

To: SGOrava, #frameworks, dhaumann
Cc: anthonyfieroni, dhaumann, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D26691: Optimize code when dropping files into the desktop

[Anthony Fieroni]

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
> +//choices->exec();
>  } else {
> -dropJob->setApplicationActions(dropActions);
> +//dropJob->setApplicationActions(m_dropActions);
>  }

Remove, don't leave commented code

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D26691

To: trmdi, #plasma, mart
Cc: anthonyfieroni, #plasma, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D26650: Use KService to look for Filelight

[Anthony Fieroni]

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 service ptr life to the process end, which can result in 
memory leak (it's not leak) But since it's a plugin we don't expect a tons of 
objects, but i'm fine to make service a class scope var to not outlive the 
plugin.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D26650

To: shubham, broulik, ngraham
Cc: meven, anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D26650: Use KService to look for Filelight

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> kpropertiesdialog.cpp:1114
> +
> d->m_sizeDetailsButton->setIcon(QIcon::fromTheme(service->icon()));
>  connect(d->m_sizeDetailsButton, ::clicked, this, 
> ::slotSizeDetails);
>  sizelay->addWidget(d->m_sizeDetailsButton, 0);

We don't need to reparse desktop file on every click just make it

  connect(d->m_sizeDetailsButton, ::clicked, this, [this, 
service]() {
  KRun::runApplication(*service, { properties->url() }, 
properties->window());
  });

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D26650

To: shubham, broulik, ngraham
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26650: Use KService to look for Filelight

[Anthony Fieroni]

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() }, properties->window(), QStringLiteral("Filelight"), 
> QStringLiteral("filelight"));
> +KService::Ptr service = 
> KService::serviceByDesktopName(QStringLiteral("org.kde.filelight"));
> +KRun::runApplication(*service, { properties->url() }, 
> properties->window());
>  }

Remove

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D26650

To: shubham, broulik, ngraham
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26650: Use KService to look for Filelight

[Anthony Fieroni]

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"), 
> QStringLiteral("filelight"));
> +KService::Ptr serv = 
> KService::serviceByDesktopName(QStringLiteral("org.kde.filelight"));
> +KRun::runApplication(*serv, { properties->url() }, properties->window());

service can be nullptr, add a check

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D26650

To: shubham, broulik, ngraham
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26117: [solid] Clarify referencing of DeviceInterface

[Anthony Fieroni]

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, LeGast00n, GB_2, michaelh, ngraham, bruns

D26484: Add a new parameter for delaying showing menu

[Anthony Fieroni]

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 = DefaultFlags, bool delayPopup = false);
>  

Do not change signature of exported function, just add another one.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D26484

To: trmdi, #frameworks, davidedmundson, elvisangelaccio, mart, dfaure
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

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

[Anthony Fieroni]

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"
> +<< "withBraces[$e]=file://${HOME}/foo\n"
> +<< "URL[$e]=file://${HOME}/foo\n"

When it has only strings you can use `R"()"`
https://en.cppreference.com/w/cpp/language/string_literal

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D26432

To: mlaurent, dfaure
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

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

[Anthony Fieroni]

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 is not really true. We have that problem in the FreeBSD CI, where 
> there's no /etc/fstab, the mounting is handled by some container stuff I 
> don't really understand...
> 
> You have more reliable ways to know you're on android...

Empty mountPoints means no fstab.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D26407

To: meven, #frameworks, ngraham, broulik, dfaure
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26117: [solid] Clarify referencing of DeviceInterface

[Anthony Fieroni]

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, bruns

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

[Anthony Fieroni]

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) ? Slow : Fast;
> +const auto  = KMountPoint::currentMountPoints();
> +auto mp = mountPoints.findByPath(path);

I think you wanna use `getMountPoints`

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D26407

To: meven, #frameworks, ngraham, broulik, dfaure
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

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

[Anthony Fieroni]

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 KDocTools

REVISION DETAIL
  https://phabricator.kde.org/D26317

To: mlaurent, dfaure
Cc: aacid, anthonyfieroni, kde-frameworks-devel, kde-doc-english, LeGast00n, 
gennad, fbampaloukas, GB_2, michaelh, ngraham, bruns, skadinna

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

[Anthony Fieroni]

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, kde-frameworks-devel, kde-doc-english, LeGast00n, gennad, 
fbampaloukas, GB_2, michaelh, ngraham, bruns, skadinna

D26117: [solid] Clarify referencing of DeviceInterface

[Anthony Fieroni]

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

[Anthony Fieroni]

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"));
> +q->emitResult();
> + }
> +const QByteArray destination = QFile::encodeName(m_dest.toLocalFile());

indentation

> copyxattrjob.cpp:156-158
> +vallen = fgetxattr(bsrc_fd, key.constData(), value.data(), valuelen, 
> 0, 0);
> +#elif HAVE_SYS_EXTATTR
> +vallen = extattr_get_file(m_bsrc, EXTATTR_NAMESPACE_USER, 
> key.constData(), value.data(), valuelen);

valuelen there is no vallen

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17816

To: tmarshall, dfaure, chinmoyr, bruns, #frameworks, cochise
Cc: anthonyfieroni, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, GB_2, michaelh

D26117: [solid] Clarify referencing of DeviceInterface

[Anthony Fieroni]

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

[Anthony Fieroni]

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 0x7f4bf9c42700 (LWP 318852)):
#0  0x7f4c1b898115 in pthread_cond_wait@@GLIBC_2.3.2 () from 
/lib/libpthread.so.0
#1  0x7f4bfac7ae8b in ?? () from /usr/lib/dri/i965_dri.so
#2  0x7f4bfac7aaf7 in ?? () from /usr/lib/dri/i965_dri.so
#3  0x7f4c1b891be4 in start_thread () from /lib/libpthread.so.0
#4  0x7f4c2042033f in clone () from /lib/libc.so.6

Thread 3 (Thread 0x7f4c067a7700 (LWP 318851)):
#0  0x7f4c204122ac in read () from /lib/libc.so.6
#1  0x7f4c17b41e4f in ?? () from /usr/lib/libglib-2.0.so.0
#2  0x7f4c17afc84e in g_main_context_check () from 
/usr/lib/libglib-2.0.so.0
#3  0x7f4c17afcc5a in ?? () from /usr/lib/libglib-2.0.so.0
#4  0x7f4c17afcdcf in g_main_context_iteration () from 
/usr/lib/libglib-2.0.so.0
#5  0x7f4c20dab21b in 
QEventDispatcherGlib::processEvents(QFlags) () 
from /usr/lib/libQt5Core.so.5
#6  0x7f4c20d562eb in 
QEventLoop::exec(QFlags) () from 
/usr/lib/libQt5Core.so.5
#7  0x7f4c20b852ae in QThread::exec() () from /usr/lib/libQt5Core.so.5
#8  0x7f4c1fc3f7f7 in ?? () from /usr/lib/libQt5DBus.so.5
#9  0x7f4c20b8627d in ?? () from /usr/lib/libQt5Core.so.5
#10 0x7f4c1b891be4 in start_thread () from /lib/libpthread.so.0
#11 0x7f4c2042033f in clone () from /lib/libc.so.6

Thread 2 (Thread 0x7f4c0f6fa700 (LWP 318850)):
#0  0x7f4c204165af in poll () from /lib/libc.so.6
#1  0x7f4c1bcb9827 in ?? () from /usr/lib/libxcb.so.1
#2  0x7f4c1bcbb3ba in xcb_wait_for_event () from /usr/lib/libxcb.so.1
#3  0x7f4c124da960 in ?? () from 
/usr/lib/qt5/plugins/platforms/../../../libQt5XcbQpa.so.5
#4  0x7f4c20b8627d in ?? () from /usr/lib/libQt5Core.so.5
#5  0x7f4c1b891be4 in start_thread () from /lib/libpthread.so.0
#6  0x7f4c2042033f in clone () from /lib/libc.so.6

Thread 1 (Thread 0x7f4c245a2840 (LWP 318849)):
[KCrash Handler]
#6  0x7f4c241399dc in QWeakPointer::internalData (this=0x28) 
at /usr/include/qt5/QtCore/qsharedpointer_impl.h:701
#7  0x7f4c24141d88 in QPointer::data (this=0x28) at 
/usr/include/qt5/QtCore/qpointer.h:77
#8  0x7f4c24141c0e in Solid::DeviceInterfacePrivate::backendObject 
(this=0x20) at 
/home/toni/solid/src/solid/devices/frontend/deviceinterface.cpp:110 // 
<- invalid this pointer
#9  0x7f4c24138c99 in Solid::DevicePrivate::~DevicePrivate 
(this=0xe766c0, __in_chrg=) at 
/home/toni/solid/src/solid/devices/frontend/device.cpp:222
#10 0x7f4c24138d48 in Solid::DevicePrivate::~DevicePrivate 
(this=0xe766c0, __in_chrg=) at 
/home/toni/solid/src/solid/devices/frontend/device.cpp:225
#11 0x7f4c2413ab1b in 
Solid::DeviceManagerPrivate::~DeviceManagerPrivate (this=0xde4230, 
__in_chrg=) at 
/home/toni/solid/src/solid/devices/frontend/devicemanager.cpp:58
#12 0x7f4c2413abe6 in 
Solid::DeviceManagerPrivate::~DeviceManagerPrivate (this=0xde4230, 
__in_chrg=) at 
/home/toni/solid/src/solid/devices/frontend/devicemanager.cpp:63
#13 0x7f4c2413f9be in 
qThreadStorage_deleteData (d=0xde4230) at 
/usr/include/qt5/QtCore/qthreadstorage.h:92
#14 0x7f4c2413e6c8 in 
QThreadStorage::deleteData (x=0xde4230) at 
/usr/include/qt5/QtCore/qthreadstorage.h:135
#15 0x7f4c20b8b240 in QThreadStorageData::finish(void**) () from 
/usr/lib/libQt5Core.so.5
#16 0x7f4c20d59819 in QCoreApplicationPrivate::cleanupThreadData() () 
from /usr/lib/libQt5Core.so.5
#17 0x7f4c21348e64 in QGuiApplicationPrivate::~QGuiApplicationPrivate() 
() from /usr/lib/libQt5Gui.so.5
#18 0x7f4c21c0f8b9 in QApplicationPrivate::~QApplicationPrivate() () 
from /usr/lib/libQt5Widgets.so.5
#19 0x7f4c20d888fe in QObject::~QObject() () from 
/usr/lib/libQt5Core.so.5
#20 0x7f4c20d595dc in QCoreApplication::~QCoreApplication() () from 
/usr/lib/libQt5Core.so.5
#21 0x7f4c21c1171e in QApplication::~QApplication() () from 
/usr/lib/libQt5Widgets.so.5
#22 0x0040e645 in ?? ()
#23 0x7f4c2034e2cb in __libc_start_main () from /lib/libc.so.6
#24 0x0040e67a in _start ()
[Inferior 1 (process 318849) detached]

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

[Anthony Fieroni]

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

[Anthony Fieroni]

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 manages its 
backend.

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D26117

AFFECTED FILES
  src/solid/devices/frontend/device.cpp
  src/solid/devices/frontend/device_p.h
  src/solid/devices/frontend/deviceinterface.cpp

To: anthonyfieroni, broulik
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25934: [xattr] Fix passing negative size to QByteArray

[Anthony Fieroni]

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, spoorun, 
ngraham, bruns, abrahams

D25934: [xattr] Fix passing negative size to QByteArray

[Anthony Fieroni]

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

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D25934

AFFECTED FILES
  src/xattr_p.h

To: anthonyfieroni, bruns, astippich
Cc: kde-frameworks-devel, #baloo, hurikhan77, lots0logs, LeGast00n, 
fbampaloukas, GB_2, domson, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

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

[Anthony Fieroni]

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, kde-frameworks-devel, kfm-devel, pberestov, iasensio, 
fprice, LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, 
meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

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

[Anthony Fieroni]

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, iasensio, 
fprice, LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, 
meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

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

[Anthony Fieroni]

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, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, michaelh, 
spoorun, navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, 
mikesomov

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

[Anthony Fieroni]

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, davidedmundson
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

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

[Anthony Fieroni]

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 want different font height in a view.
  
  I don't know which is best.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D25339

To: xuetianweng, #ktexteditor, cullmann, dhaumann
Cc: anthonyfieroni, kde-frameworks-devel, kwrite-devel, LeGast00n, GB_2, 
domson, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D21235: Add handling of fuseiso filesystem type

[Anthony Fieroni]

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.

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D21235

To: hallas, bruns, ngraham
Cc: anthonyfieroni, broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

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

[Anthony Fieroni]

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
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D25308

To: sitter, dfaure, apol
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D25125: Use KListOpenFilesJob for retrieving apps blocking unmount

[Anthony Fieroni]

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 
> QString& errorDetails, const QString );
> -void blockingAppsReady(const QStringList );
>  void clearNotification(const QString );

This is public signal, are you sure none use it?

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D25125

To: hallas, #frameworks, broulik, bruns
Cc: anthonyfieroni, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25039: Fix Clazy performance issues, const

[Anthony Fieroni]

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 QString o = s; // use o instead of s is also fine, using plain s 
is fine too, if you don't touch mutability 
 ...
}

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D25039

To: meven, #frameworks, dfaure
Cc: anthonyfieroni, kossebau, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D25059: KPluginSelector: use new KAboutPluginDialog

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> kpluginselector.cpp:799-803
> +PluginEntry *pluginEntry = model->data(index, 
> PluginEntryRole).value();
> +KPluginMetaData pluginMetaData = pluginEntry->pluginInfo.toMetaData();
> +
> +KAboutPluginDialog aboutPlugin(pluginMetaData, itemView());
>  aboutPlugin.exec();

Can you guard for nullptr?

REPOSITORY
  R295 KCMUtils

REVISION DETAIL
  https://phabricator.kde.org/D25059

To: kossebau, #frameworks, dfaure, apol
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

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

[Anthony Fieroni]

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 on same object which will not be a problem, check it.
  >
  >
  > There are two objects in QPA that live independently:
  >
  > - KDEPlatformSystemTrayIcon (QPlatformSystemTrayIcon), with KSNI instance, 
KSNI and KDEPlatformSystemTrayIcon **are** destroyed on QSystemTrayIcon->hide() 
and new instance (with new KSNI) is created on QSystemTrayIcon->show()
  > - SystemTrayMenu (QPlatformMenu) is **not** destroyed on 
QSystemTrayIcon->hide() and will be reused later on QSystemTrayIcon->show()
  >
  >   kdeplatformsystemtrayicon.cpp#L339 

  >
  >   ``` void KDEPlatformSystemTrayIcon::updateMenu(QPlatformMenu *menu) { 
//... if (SystemTrayMenu *ourMenu = qobject_cast(menu)) { 
m_sni->setContextMenu(ourMenu->menu()); } } ```
  >
  >   About you patch: I understand your idea, but it changes API contract and 
is not backward-compatible. Current documentation says:
  >
  > > The KStatusNotifierItem instance takes ownership of the menu, and will 
delete it upon its destruction.
  >
  > This is quite clear, I want to be really careful here - I don't want to be 
blamed for memory leaks :) I think that we need to keep:
  
  
  First, it will not have memory leaks, we take ownership on parentless menu, 
on menu that has a parent, it will destroy it by parent-child cleanups.
  I want to be more clear why SystemTrayMenu is not destroyed on hide, the idea 
behind that code is to be created a new try menu. On updateMenu you call it by 
same object that's why it's not destroyed, did you have stack strace, that's 
not normal to me.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D24755

To: kmaterka, #frameworks, davidedmundson, broulik, nicolasfella
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

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

[Anthony Fieroni]

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
  https://phabricator.kde.org/D24755

To: kmaterka, #frameworks, davidedmundson, broulik, nicolasfella
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

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

[Anthony Fieroni]

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 (more?) years. It is even documented in the API.
> Your idea will not fix the main issue, we can't set a parent to menu in 
> KDEPlatformSystemTrayIcon. Main purpose of this hack is to prevent deletion 
> of menu when it is *not* possible to set parent.

`Probably KSNI should not own the menu`
Yes, widget that creates the menu should, like in bug report example. Make the 
changes here, then we should find a way to parent the menu, which is the right 
approach.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D24755

To: kmaterka, #frameworks, davidedmundson, broulik, nicolasfella
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

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

[Anthony Fieroni]

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 will delete it. But if there is parent then 
> menu won't be deleted and we will have memory leak. Eventually this menu will 
> be deleted, when parent is destroyed, but current contract is different.
> To make things even worse, associatedWidget can change, so we can't compare 
> the parent of the menu with associatedWidget during the destruction...
> Let's say we will change it to:
> 
>   new QMenu()
> 
> Then it will be removed, because there is no parent. It should not have any 
> important side effects. So far so good.
> 
> What we want to achieve is have an ability to retake ownership after it is 
> passed to setContextMenu. With your approach, it could be done this way:
> QMenu *menu =new QMenu(); // null parent, doesn't matter here
> tray->setContextMenu(menu);
> menu->setParent(parent);
> This way menu won't be deleted. There are two problems with this approach:
> 
> - we don't know if no-one is doing that - most probably not and this can be 
> ignored
> - the parent needs to be a QWidget type. This is serious issue, because there 
> are cases when it is not possible.
> 
> The final goal is to fix KDEPlatformSystemTrayIcon and there is no QWidget to 
> use as a parent :( Exactly:
> kdeplatformsystemtrayicon.cpp:339
> 
>   m_sni->setContextMenu(ourMenu->menu());
> 
> ourMenu can live longer than KStatusNotifierItem *m_sni and can be reused for 
> another KStatusNotifierItem instance. The situation is described in BUG 
> 365105 . In other word, in QPA, 
> menu can and should live independently to system tray icon, which is not the 
> case in KStatusNotifierItem.
> 
> I really like your idea! Maybe I'm missing something obvious and is possible 
> to set parent in KDEPlatformSystemTrayIcon...

That's easy checkable

  if (!qApp->closingDown() && (!d->menu->parent() || d->menu->parent() == 
d->associatedWidget) {
  delete d->menu;
  }

We should not own the menu, that's not tight approach at least.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D24755

To: kmaterka, #frameworks, davidedmundson, broulik, nicolasfella
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

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

[Anthony Fieroni]

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 parent. 
takeOwnership is wrong approach, you can remove it.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D24755

To: kmaterka, #frameworks, davidedmundson, broulik, nicolasfella
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

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

[Anthony Fieroni]

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 don't need to any hacks here. QPointer takes care of 
ownership.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D24755

To: kmaterka, #frameworks, davidedmundson, broulik, nicolasfella
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

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

[Anthony Fieroni]

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: kmaterka, #frameworks, davidedmundson, broulik, nicolasfella
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

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

[Anthony Fieroni]

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 FtpInternal : public QObject
> +{

Should be hidden?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D23579

To: sitter, dfaure
Cc: anthonyfieroni, dfaure, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D24012: Supress mouse events in KCMs causing window moves

[Anthony Fieroni]

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 && ...)` if rc is false event is not handled no?

REPOSITORY
  R295 KCMUtils

REVISION DETAIL
  https://phabricator.kde.org/D24012

To: davidedmundson, #plasma
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

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

[Anthony Fieroni]

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();

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D23420

REVISION DETAIL
  https://phabricator.kde.org/D23420

To: meven, dfaure, #frameworks
Cc: broulik, anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

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

[Anthony Fieroni]

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
  solid-network-fs-check

REVISION DETAIL
  https://phabricator.kde.org/D23420

To: meven, dfaure, #frameworks
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

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

[Anthony Fieroni]

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 it Fast before loop or check whether you change the 
m_slow value.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D23420

To: meven, dfaure, #frameworks
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

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

[Anthony Fieroni]

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;
>  }

processNextFile should be called till m_ids ends, why it does not happen, 
probably that's why @bruns reject your patch

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D23008

To: poboiko, #baloo, bruns, ngraham
Cc: anthonyfieroni, broulik, kde-frameworks-devel, LeGast00n, fbampaloukas, 
domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams

D22155: Add new activities and virtual desktops icons

[Anthony Fieroni]

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 applications / Plasma will not see an icon. That's pretty happen since 
framework incorporate new features + bugs fix and Plasma offers LTS.
  >
  >
  > Do you have any examples of distros that do this? To my knowledge, all 
"LTS" style distros freeze all versions of KDE software, not just Plasma and 
apps.
  
  
  IMHO, that should be the *right* scenario, since we don't backport bugfixes 
in framework, distros that provide LTS should stays on Plasma LTS, applications 
numbering through release date e.g. 18.04.3 and top of the framework.
  +1 to be part of Plasma 5.17

REPOSITORY
  R266 Breeze Icons

BRANCH
  add-new-activities-and-virtual-desktops-icons (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D22155

To: GB_2, #vdg, ndavis
Cc: anthonyfieroni, ngraham, ndavis, kde-frameworks-devel, #vdg, LeGast00n, 
michaelh, bruns

D22155: Add new activities and virtual desktops icons

[Anthony Fieroni]

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 code, if new icon isn't found to be used old one.

REPOSITORY
  R266 Breeze Icons

BRANCH
  add-new-activities-and-virtual-desktops-icons (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D22155

To: GB_2, #vdg, ndavis
Cc: anthonyfieroni, ngraham, ndavis, kde-frameworks-devel, #vdg, LeGast00n, 
michaelh, bruns

D22155: Add new activities and virtual desktops icons

[Anthony Fieroni]

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
  add-new-activities-and-virtual-desktops-icons (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D22155

To: GB_2, #vdg, ndavis
Cc: anthonyfieroni, ngraham, ndavis, kde-frameworks-devel, #vdg, LeGast00n, 
michaelh, bruns

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

[Anthony Fieroni]

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
  https://phabricator.kde.org/D22102

To: ngraham, #plasma, #vdg, mart, broulik
Cc: anthonyfieroni, pino, davidedmundson, filipf, mglb, kde-frameworks-devel, 
LeGast00n, michaelh, ngraham, bruns

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

[Anthony Fieroni]

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.

> fstabhandling.cpp:236
> +} else {
> +deviceName = device + '/';
> +}

deviceName.append('/');

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D21204

To: meven, bruns, #frameworks
Cc: anthonyfieroni, ngraham, bruns, apol, kde-frameworks-devel, LeGast00n, 
michaelh

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

[Anthony Fieroni]

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

[Anthony Fieroni]

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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D21315

To: meven, #frameworks, ngraham, #vdg
Cc: anthonyfieroni, kde-frameworks-devel, michaelh, ngraham, bruns

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

[Anthony Fieroni]

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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent
Cc: anthonyfieroni, chinmoyr, mlaurent, asensi, rkflx, dfaure, aacid, ngraham, 
kde-frameworks-devel, michaelh, bruns

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

[Anthony Fieroni]

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
  https://phabricator.kde.org/D21280

To: dfaure, sitter, davidedmundson, apol, adridg
Cc: anthonyfieroni, kde-frameworks-devel, michaelh, ngraham, bruns

D20958: New menu of syntax highlighting in the status bar

[Anthony Fieroni]

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: anthonyfieroni, cullmann, ngraham, loh.tar, kwrite-devel, 
kde-frameworks-devel, domson, michaelh, bruns, demsking, sars, dhaumann

D20958: New menu of syntax highlighting in the status bar

[Anthony Fieroni]

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
> + */
> +const int maxWidthText = m_list->sizeHint().width() - 
> m_scroll->sizeHint().width() - 28;
> +

You can make a static const int iconSize = 16 or something to use it everywhere?

> katemodemenulist.cpp:141
> +if ( !hl->translatedSection.isEmpty() && (prevHlSection == nullptr 
> || hl->translatedSection != *prevHlSection) ) {
> +QPixmap transparent = QPixmap(8, 8);
> +transparent.fill(Qt::transparent);

Here as well, if it's 16/2

> katemodemenulist.cpp:266
> +if (m_selectedItem) {
> +QPixmap emptyIcon(16, 16);
> +emptyIcon.fill(Qt::transparent);

Ditto

> katemodemenulist.cpp:407
> +// set empty icon
> +QPixmap emptyIcon(16, 16);
> +emptyIcon.fill(Qt::transparent);

Ditto

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D20958

To: nibags, #ktexteditor, #kate, #vdg
Cc: anthonyfieroni, cullmann, ngraham, loh.tar, kwrite-devel, 
kde-frameworks-devel, domson, michaelh, bruns, demsking, sars, dhaumann

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

[Anthony Fieroni]

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
  https://phabricator.kde.org/D20748

To: dfaure, mart, arichardson, davidedmundson, mpyne, apol
Cc: anthonyfieroni, aacid, kde-frameworks-devel, michaelh, ngraham, bruns

D20693: Remove pixelated border

[Anthony Fieroni]

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: anthonyfieroni, mmustac, ndavis, kde-frameworks-devel, #knewstuff, 
michaelh, ZrenBot, ngraham, bruns

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

[Anthony Fieroni]

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) { return buf.stx_ino; }
> +inline static uint64_t stat_size(struct statx buf) { return buf.stx_size; }
> +inline static uint32_t stat_uid(struct statx buf) { return buf.stx_uid; }
> +inline static uint32_t stat_gid(struct statx buf) { return buf.stx_gid; }
> +inline static uint64_t stat_atime(struct statx buf) { return 
> buf.stx_atime.tv_sec; }

We can get all stat_xxx functions buf as reference, struct is not needed when 
you use C++.

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D20096

REVISION DETAIL
  https://phabricator.kde.org/D20096

To: meven, #frameworks, dfaure, fvogt, bruns, broulik
Cc: anthonyfieroni, pino, bcooksley, ngraham, kde-frameworks-devel, michaelh, 
bruns

D5111: Provide demo/preview for checkable menu items

[Anthony Fieroni]

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, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15763: Set correct image attributes on directory thumbnail

[Anthony Fieroni]

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, meven, 
michaelh, spoorun, navarromorales, firef, andrebarros, bruns, emmanuelp, 
mikesomov

D19767: Fix malloc/delete mismatch

[Anthony Fieroni]

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, mikesomov

D18249: [datamodel] Rework items insert/remove

[Anthony Fieroni]

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