Re: Need xcb/xkb help for severe kglobalaccel_x11 issue

2021-02-21 Thread Fabian Vogt
Moin,

Am Samstag, 6. Februar 2021, 10:08:43 CET schrieb David Faure:
> Problem mostly fixed by 
> https://invent.kde.org/plasma/plasma-desktop/-/merge_requests/339
> but still seeing 15 notifications instead of 1 (already better than 145...).
> 
> Indeed a single call to
> /usr/bin/setxkbmap -layout us,fr -option -option compose:caps
> sends 12 XCB_XKB_NEW_KEYBOARD_NOTIFY events,
> all with changed & XCB_XKB_NKN_DETAIL_KEYCODES, which makes kglobalaccel
> ungrab+regrab all keys.

Here setxkbmap doesn't cause any XCB_XKB_NEW_KEYBOARD_NOTIFY events, only
XCB_XKB_MAP_NOTIFY. I tried this with Xwayland though, maybe that behaves
differently...

> Should I add another compression timer in kglobalaccel, or do you think this 
> is
> fixable in setxkbmap?

It's probably fixable somewhere in either setxkbmap/xkb or the X server, but a
compression timer would work even with older versions and also for other
causes like xmodmap, which can be much worse than just 15 events.

Cheers,
Fabian




Re: Need xcb/xkb help for severe kglobalaccel_x11 issue

2021-01-30 Thread Fabian Vogt
Hi,

Am Samstag, 30. Januar 2021, 18:32:32 CET schrieb David Faure:
> For years, I've noticed that when resuming a laptop from sleep, kglobalaccel 
> and X11 
> use 100% CPU for a few minutes, making everything crawl for a while.
> 
> I finally debugged why: kglobalaccel grabs and ungrabs all global shortcuts 
> many many times in a row.
> 
> KGlobalAccelImpl::nativeEventFilter event->response_type= 85 xkbEvent= 1 
> calling x11MappingNotify()
> KGlobalAccelImpl::x11MappingNotify Got XMappingNotify event
> KGlobalAccelInterface::ungrabKeys 
> KGlobalAccelInterface::grabKeys 
> KGlobalAccelImpl::nativeEventFilter event->response_type= 85 xkbEvent= 1 
> calling x11MappingNotify()
> KGlobalAccelImpl::x11MappingNotify Got XMappingNotify event
> KGlobalAccelInterface::ungrabKeys 
> KGlobalAccelInterface::grabKeys 
> and so on, and so on...
> 
> The reason why x11MappingNotify() does ungrabKeys+grabKeys is apparently 
> "Maybe the X modifier map has been changed."
> ... which is not the case at all...
>
> What's an XCB_XKB_MAP_NOTIFY anyway?  
> http://manpages.ubuntu.com/manpages/xenial/en/man3/xcb_xkb_map_notify_event_t.3.html
> is very much incomplete...

It's just the XCB equivalent of XkbMapNotify. If you search for the latter, you
should get more hits.

> Is it event really such an event that we're getting?
> The code says
> 
> } else if (m_xkb_first_event && responseType == m_xkb_first_event) {
> const uint8_t xkbEvent = event->pad0;
> switch (xkbEvent) {
> case XCB_XKB_MAP_NOTIFY:
> qDebug() << "event->response_type=" << event->response_type 
> << "xkbEvent=" << xkbEvent << "calling x11MappingNotify()";
> x11MappingNotify();
> break;
> 
> What sense does it make that this code is checking pad0, which looks like 
> some padding member? To avoid a downcast to a more specific event type maybe?

Yep. That's what an earlier revision did, but I was recommended to use pad0
instead: 
https://phabricator.kde.org/D16434?vs=44241=44247=ignore-most#toc
It's equivalent to xkbType, which indicates the exact type of this xkb event.

> I'm not sure:
> * if we're really getting a stream of XCB_XKB_MAP_NOTIFY events or if this 
> code misunderstands that

I don't think that the code would misunderstand that. AFAICT the check is
strict enough to not catch other events, and if it's wrong instead it wouldn't
catch the correct events either. If this was the cause of the issue, I would
expect misbehaviour outside of the post-resume state. I can't rule it out
though.

> * if so, why is X11 sending such a high number of those

I suspect that something else is triggering keyboard layout reloads or similar
in a loop. The keyboard kded module listens for new keyboards and mouse events
and configures them, which is likely to be called on resume when devices get
reenumerated. You could try to disable that before suspend/resume.

If I run "xmodmap .Xmodmap" here with "xev" open, I get quite a high count of
mapping change events, presumably for every assignment in the file. If the kded
calls xmodmap with many assignments, that would be enough to explain the issue.
Maybe xmodmap could be optimized to upload everything at once, like setxkbmap.

I actually had the issue that calling xmodmap here basically froze the whole
session for a while, which was probably caused by that behaviour.

> * why the code reacts the same to XCB_XKB_MAP_NOTIFY and to 
> XCB_MAPPING_NOTIFY, isn't one enough?

When an X client indicates that it uses Xkb instead of the X core protocol,
then it will only receive XKB events. In the case of kglobalaccel, the decision
is made by Qt (which is why it broke at some point). The XCB_MAPPING_NOTIFY
case is probably unused now, which could be ensured by just doing Xkb stuff
explicitly on initialization.

> Without outside help I guess I would just compress those events using a 
> timer, but that would be a "fix without really understanding the code", never 
> good.

Agreed. If those events are caused by something we can't fix, then this might
turn out to be the best option though.

Cheers,
Fabian

> I just noticed https://phabricator.kde.org/D16434 so CC'ing Fabian :)




D29800: Fix URL being passed as argument when launching a .desktop file

2020-05-17 Thread Fabian Vogt
fvogt marked an inline comment as done.
fvogt added a comment.


  Landed to invent - hopefully correctly: 
https://invent.kde.org/frameworks/kio/commit/84e9372f4fa2636f57dc456ac2fa2be271d6a7ec

REPOSITORY
  R241 KIO

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

To: fvogt, dfaure, marten
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns


D29800: Fix URL being passed as argument when launching a .desktop file

2020-05-17 Thread Fabian Vogt
fvogt closed this revision.

REPOSITORY
  R241 KIO

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

To: fvogt, dfaure, marten
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns


D29800: Fix URL being passed as argument when launching a .desktop file

2020-05-16 Thread Fabian Vogt
fvogt created this revision.
fvogt added reviewers: dfaure, marten.
Herald added a project: Frameworks.
fvogt requested review of this revision.

REVISION SUMMARY
  When a .desktop file is executed directly, it doesn't receive a parameter.
  BUG: 421364

TEST PLAN
  Test passes, applications open normally again.

REPOSITORY
  R241 KIO

BRANCH
  execfi

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

AFFECTED FILES
  autotests/openurljobtest.cpp
  autotests/openurljobtest.h
  src/gui/openurljob.cpp

To: fvogt, dfaure, marten
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns


D29503: Pixel align children of GridViewInternal

2020-05-07 Thread Fabian Vogt
fvogt added a comment.


  In D29503#665612 , @ngraham wrote:
  
  > I can't reproduce it, but I wonder if this could fix or help 
https://bugs.kde.org/show_bug.cgi?id=417488?
  
  
  Yup, that's exactly what I was seeing before this change: F8293538: 
8sMM0Gq.png  vs F8293533: 
Screenshot_20200507_153104.png 

REPOSITORY
  R296 KDeclarative

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

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


D29503: Pixel align children of GridViewInternal

2020-05-07 Thread Fabian Vogt
This revision was automatically updated to reflect the committed changes.
Closed by commit R296:9725a21bcd0e: Pixel align children of GridViewInternal 
(authored by fvogt).

REPOSITORY
  R296 KDeclarative

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29503?vs=82183=82184

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

AFFECTED FILES
  src/qmlcontrols/kcmcontrols/qml/private/GridViewInternal.qml

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


D29503: Pixel align children of GridViewInternal

2020-05-07 Thread Fabian Vogt
fvogt created this revision.
fvogt added reviewers: Frameworks, broulik, mart.
Herald added a project: Frameworks.
fvogt requested review of this revision.

REVISION SUMMARY
  The scroll bar size can be odd (for breeze it's 21), which causes leftMargin
  to be 12.5. This causes every delegate inside to be blurred.

TEST PLAN
  Monkeypatched, now kcm_style is no longer blurred.

REPOSITORY
  R296 KDeclarative

BRANCH
  master

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

AFFECTED FILES
  src/qmlcontrols/kcmcontrols/qml/private/GridViewInternal.qml

To: fvogt, #frameworks, broulik, mart
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns


D28856: Save disabling of desktop file components in kglobalshortcutsrc

2020-04-15 Thread Fabian Vogt
fvogt requested changes to this revision.
fvogt added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> globalshortcutsregistry.cpp:274
> +auto disabledComponents = KConfigGroup(&_config, 
> "disabledComponents").readEntry("disabled", QStringList());
>  for (const QString  : groupList)
>  {

`disabledComponents` is the group name, right? It would also be part of 
`groupList`, so it would try to load it as shortcut...

> globalshortcutsregistry.cpp:333
>  for (const QString  : lstDesktopFiles) {
> -if (_components.contains(desktopFile)) {
> +if (_components.contains(desktopFile) || 
> disabledComponents.contains(desktopFile)) {
>  continue;

Is `desktopFile` the equivalent to `component->uniqueName()`? I would assume 
no, so this check might need to be moved after the `KServiceActionComponent` 
construction

REPOSITORY
  R268 KGlobalAccel

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

To: davidre, davidedmundson, fvogt, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns


D24956: Consider desktop files with NoDisplay attribute

2020-04-15 Thread Fabian Vogt
fvogt added a comment.


  In D24956#648968 , @davidedmundson 
wrote:
  
  > > [14:12]  DavidRedondo1: my understanding is that a system might 
ship "konsole opens with control+t". The UI allows you to remove that. This 
would remove the entry in kglobalshortcutsrc, but because it's still  in the 
system defaults file as soon as you log in again it'll add it back
  >
  > [14:25]  d_ed, fvogt Apparently the runtime writes the 
hidden thing when a component is cleanedUp 
https://cgit.kde.org/kglobalaccel.git/tree/src/runtime/kserviceactioncomponent.cpp#n135
  >  [14:27]  Does that fail or something when the file is not 
writeable?
  >  [14:31]  I think it fails
  >  [14:31]  I just tested it
  >
  > if it is indeed broken...then we may as well just merge this.
  
  
  It might not be broken for imported desktop files, in which case this would 
be a noticable regression. Not tested though.

REPOSITORY
  R268 KGlobalAccel

BRANCH
  master

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

To: meven, mart, #plasma, fvogt, apol, davidedmundson
Cc: davidedmundson, davidre, kde-frameworks-devel, LeGast00n, cblack, michaelh, 
ngraham, bruns


D24956: Consider desktop files with NoDisplay attribute

2020-04-15 Thread Fabian Vogt
fvogt added a comment.


  In D24956#648905 , @davidedmundson 
wrote:
  
  > kglobalshortcutseditor.cpp
  >  needs updating to match
  >
  > I think you're right with your reasoning about NoDisplay, but we do want 
something to be able to mask system files. From the spec should we be checking 
Hidden= ?
  
  
  `Hidden=true` is equivalent to not having the file at all according to the 
spec, so it would make sense. There's also D25088 
 open.

REPOSITORY
  R268 KGlobalAccel

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

To: meven, mart, #plasma, fvogt, apol
Cc: davidedmundson, davidre, kde-frameworks-devel, LeGast00n, cblack, michaelh, 
ngraham, bruns


D28535: [KIO-MTP] Fix null pointer dereference

2020-04-03 Thread Fabian Vogt
fvogt added a comment.


  I assume there is a reason why `MTPDevice::getDevice()` has code for handling 
this very specific case, so I wouldn't just remove it without figuring out why: 
../https://i.redd.it/hfnl7xo8yovy.gif
  
  If not, that would indeed be the best option.

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

2020-04-03 Thread Fabian Vogt
fvogt added a comment.


  In D28535#640682 , @feverfew wrote:
  
  > In D28535#640674 , @fvogt wrote:
  >
  > > What you're suggesting is to change `MTPDevice::getDevice` to return the 
old device if reopening fails - but reopening without releasing might not work.
  >
  >
  > This seems to be a robust solution IMO, why do you suspect this might not 
work?
  
  
  Because there can only be a single libusb session per device. So you have to 
release the old one before opening again.

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

2020-04-03 Thread Fabian Vogt
fvogt added a comment.


  In D28535#640680 , @anthonyfieroni 
wrote:
  
  > 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.
  
  
  Yes, and until that point everything is fine.
  Only after `m_mtpdevice = LIBMTP_Open_Raw_Device_Uncached(_rawdevice);`, 
which sets `m_mtpdevice` to nullptr it goes down the path I outlined.

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

2020-04-03 Thread Fabian Vogt
fvogt added a comment.


  There is no such thing as an "invalid device" at that point anymore. There's 
only nullptr.
  
LIBMTP_mtpdevice_t *MTPDevice::getDevice()
{
if (!m_mtpdevice->storage) {
qCDebug(LOG_KIOD_KMTPD) << "no storage found: reopen mtpdevice";
LIBMTP_Release_Device(m_mtpdevice);
m_mtpdevice = LIBMTP_Open_Raw_Device_Uncached(_rawdevice);
}

return m_mtpdevice;
}
  
  What you're suggesting is to change `MTPDevice::getDevice` to return the old 
device if reopening fails - but reopening without releasing might not work.

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

2020-04-03 Thread Fabian Vogt
fvogt added a comment.


  In D28535#640656 , @anthonyfieroni 
wrote:
  
  > 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.
  
  
  `MTPDevice` already does that.
  
  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.
  
  So this patch will at most just delay the crash.
  
  AFAICT `MTPDevice` is supposed to be destroyed in `KMTPd::deviceRemoved` on 
disconnection, but this is obviously racy.

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


D27863: Fix "Make it compile against last qt5.15 without deprecated method. QProcess::execute(QString) is deprecated"

2020-03-05 Thread Fabian Vogt
This revision was automatically updated to reflect the committed changes.
Closed by commit R268:8e14750977c6: Fix Make it compile against last 
qt5.15 without deprecated method. QProcess… (authored by fvogt).

REPOSITORY
  R268 KGlobalAccel

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27863?vs=77022=77026

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

AFFECTED FILES
  src/runtime/kserviceactioncomponent.cpp

To: fvogt, #frameworks, mlaurent
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns


D27863: Fix "Make it compile against last qt5.15 without deprecated method. QProcess::execute(QString) is deprecated"

2020-03-05 Thread Fabian Vogt
fvogt marked an inline comment as done.

REPOSITORY
  R268 KGlobalAccel

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

To: fvogt, #frameworks, mlaurent
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns


D27863: Fix "Make it compile against last qt5.15 without deprecated method. QProcess::execute(QString) is deprecated"

2020-03-05 Thread Fabian Vogt
fvogt edited the test plan for this revision.

REPOSITORY
  R268 KGlobalAccel

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

To: fvogt, #frameworks, mlaurent
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns


D27863: Revert "Make it compile against last qt5.15 without deprecated method. QProcess::execute(QString) is deprecated"

2020-03-05 Thread Fabian Vogt
fvogt updated this revision to Diff 77022.
fvogt added a comment.


  Do it differently, just like it's done below

REPOSITORY
  R268 KGlobalAccel

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27863?vs=77009=77022

BRANCH
  somefix

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

AFFECTED FILES
  src/runtime/kserviceactioncomponent.cpp

To: fvogt, #frameworks, mlaurent
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns


D27863: Fix "Make it compile against last qt5.15 without deprecated method. QProcess::execute(QString) is deprecated"

2020-03-05 Thread Fabian Vogt
fvogt retitled this revision from "Revert "Make it compile against last qt5.15 
without deprecated method. QProcess::execute(QString) is deprecated"" to "Fix 
"Make it compile against last qt5.15 without deprecated method. 
QProcess::execute(QString) is deprecated"".

REPOSITORY
  R268 KGlobalAccel

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

To: fvogt, #frameworks, mlaurent
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns


D27863: Revert "Make it compile against last qt5.15 without deprecated method. QProcess::execute(QString) is deprecated"

2020-03-05 Thread Fabian Vogt
fvogt added a comment.


  In D27863#622655 , @mlaurent wrote:
  
  > if splitting is already done why this code re-call 
"m_desktopFile.desktopGroup().readEntry(QStringLiteral("Exec"), QString())" ? 
  >  => QProcess::startDetached(commands, parts) no ?
  
  
  I would assume that works, yes.

REPOSITORY
  R268 KGlobalAccel

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

To: fvogt, #frameworks, mlaurent
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns


D27863: Revert "Make it compile against last qt5.15 without deprecated method. QProcess::execute(QString) is deprecated"

2020-03-05 Thread Fabian Vogt
fvogt added a comment.


  The split arguments are already available as `parts` above, as used in the 
klauncher call AFAICT.

REPOSITORY
  R268 KGlobalAccel

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

To: fvogt, #frameworks, mlaurent
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns


D27863: Revert "Make it compile against last qt5.15 without deprecated method. QProcess::execute(QString) is deprecated"

2020-03-05 Thread Fabian Vogt
fvogt added a reviewer: mlaurent.

REPOSITORY
  R268 KGlobalAccel

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

To: fvogt, #frameworks, mlaurent
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns


D27863: Revert "Make it compile against last qt5.15 without deprecated method. QProcess::execute(QString) is deprecated"

2020-03-05 Thread Fabian Vogt
fvogt created this revision.
fvogt added a reviewer: Frameworks.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
fvogt requested review of this revision.

REVISION SUMMARY
  This port is broken AFAICT - it tries to run the full Exec= line as binary, 
without
  splitting arguments first.
  
  This reverts commit 59cbea835502428f30c1495abe4a1b3d133103e3 
.

TEST PLAN
  Not tested at all.

REPOSITORY
  R268 KGlobalAccel

BRANCH
  somefix

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

AFFECTED FILES
  src/runtime/kserviceactioncomponent.cpp

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


D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

2020-02-04 Thread Fabian Vogt
fvogt added a comment.


  In D27002#605999 , @ngraham wrote:
  
  > Could this be the fix for https://bugs.kde.org/show_bug.cgi?id=398908, or 
part of it?
  
  
  This leak presented itself by steadily growing memory use while something 
still unknown triggered solid's `onMtabChanged` excessively.
  If that's also the case for the reporter of that bug, this should've made a 
big difference, otherwise only a small one.
  
  The output of heaptrack would tell for sure.

REPOSITORY
  R241 KIO

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

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


D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

2020-02-04 Thread Fabian Vogt
This revision was automatically updated to reflect the committed changes.
Closed by commit R241:213ed50634c0: Fix memory leak in 
KUrlNavigatorPlacesSelector::updateMenu (authored by fvogt).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27002?vs=74645=74989

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

AFFECTED FILES
  src/filewidgets/kurlnavigatorplacesselector.cpp

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


D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

2020-02-04 Thread Fabian Vogt
fvogt added a comment.


  In D27002#605860 , @meven wrote:
  
  > User feedback: "so far so good, 160 MB Memory usage"
  >  Does not sound reassuring, I guess the user meant 160 MB compared to 200MB 
or similar prior to patch.
  
  
  Updated - should be clearer now :-)

REPOSITORY
  R241 KIO

BRANCH
  noleak

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

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


D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

2020-02-04 Thread Fabian Vogt
fvogt edited the test plan for this revision.

REPOSITORY
  R241 KIO

BRANCH
  noleak

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

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


D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

2020-02-04 Thread Fabian Vogt
fvogt added a comment.


  I'll land tomorrow if no objections.

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


D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

2020-01-30 Thread Fabian Vogt
fvogt marked 3 inline comments as done.

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


D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

2020-01-30 Thread Fabian Vogt
fvogt edited the test plan for this revision.

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


D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

2020-01-30 Thread Fabian Vogt
fvogt added inline comments.

INLINE COMMENTS

> anthonyfieroni wrote in kurlnavigatorplacesselector.cpp:76
> Why cast?

To only delete submenus, not anything else.

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


D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

2020-01-30 Thread Fabian Vogt
fvogt added inline comments.

INLINE COMMENTS

> meven wrote in kurlnavigatorplacesselector.cpp:75
> Shouldn't it be done before the call to `m_placesMenu->clear();`

How would that make a difference?

REPOSITORY
  R241 KIO

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

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


D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

2020-01-30 Thread Fabian Vogt
fvogt updated this revision to Diff 74645.
fvogt added a comment.


  Make a copy, QObject::children returns const & for some reason, so gets 
modified during iteration.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27002?vs=74611=74645

BRANCH
  noleak

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

AFFECTED FILES
  src/filewidgets/kurlnavigatorplacesselector.cpp

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


D21606: RFC: ThreadWeaver Job Decorators not used properly and have no effect

2020-01-30 Thread Fabian Vogt
fvogt abandoned this revision.
fvogt added a comment.


  D22758  got merged

REPOSITORY
  R308 KRunner

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

To: fvogt
Cc: apol, davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns


D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

2020-01-29 Thread Fabian Vogt
fvogt created this revision.
fvogt added a reviewer: Frameworks.
Herald added a project: Frameworks.
fvogt requested review of this revision.

REVISION SUMMARY
  This method gets called each time solid notices a change, which can in some
  setups be very frequent. It leaked memory as the submenus and their actions
  were not deallocated properly.

TEST PLAN
  Builds, waiting for user feedback.

REPOSITORY
  R241 KIO

BRANCH
  noleak

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

AFFECTED FILES
  src/filewidgets/kurlnavigatorplacesselector.cpp

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


D25856: Show IOSlaves that return local files when in local file mode

2020-01-14 Thread Fabian Vogt
fvogt added a comment.


  In D25856#594088 , @meven wrote:
  
  > In D25856#575125 , @fvogt wrote:
  >
  > > In D25856#575083 , @ngraham 
wrote:
  > >
  > > > In D25856#575044 , @fvogt 
wrote:
  > > >
  > > > > IMO this should be done in KIO, so that all users benefit.
  > > >
  > > >
  > > > How would you do it in KIO?
  > >
  > >
  > > Add code to KFileDialog to allow specific protocols/slaves if the `file` 
scheme is supported.
  >
  >
  > I would guess something in the slave interface sort of like 
`KDE-KIO-Protocols` in json/.protocol files expect it would be about supported 
output scheme instead of input/handled.
  >  Something like `KDE-KIO-Output-Protocols`
  >  Then there would be an equivalent to `KProtocolInfoFactory::findProtocol` 
to get those, like `KProtocolInfoFactory::findOutputProtocol`
  >  Basically ioslaves `desktop` `file` `recentlyused` `trash` and `tags` 
would have `"file"` set in there.
  
  
  IMO an overgeneralization: I can't come up with any use for that new key 
other than `file`.
  There's the `mostLocalUrl()` method already, so maybe it should just be 
indicated that `mostLocalUrl()` returns `file://`?
  
  > And also `recentdocuments` `timeline` for those who use those.
  
  AFAIK those can also have non-local URLs inside, so it's not possible to 
guarantee that only locally-reachable files are shown that way.
  
  > What do you think ?
  
  It seems like there's no easy way to fully implement this :-/

REPOSITORY
  R229 KDialog

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

To: ngraham, #frameworks, #dolphin
Cc: fvogt, broulik, meven


D26191: Add support for FileJob->truncate() in smb/sftp slaves

2020-01-08 Thread Fabian Vogt
fvogt accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R320 KIO Extras

BRANCH
  arcpatch-D26191

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

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


D26191: Add support for FileJob->truncate() in smb/sftp slaves

2019-12-23 Thread Fabian Vogt
fvogt requested changes to this revision.
fvogt added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> kio_sftp.cpp:1480
> +if (!attr) {
> +sftp_attributes_free(attr);
> +int errorCode = toKIOError(sftp_get_error(mSftp));

This entire block is duplicated - can that be improved?

> kio_sftp.cpp:1492
> +} else {
> +sftp_attributes_free(attr);
> +int errorCode = toKIOError(sftp_get_error(mSftp));

AFAICT, this has to be done in every case.

REPOSITORY
  R320 KIO Extras

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

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


D26157: Port QRegExp to QRegularExpression

2019-12-22 Thread Fabian Vogt
fvogt accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R299 KDESu

BRANCH
  l-qregularexpression (branched from master)

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

To: ahmadsamir, sitter, fvogt, jriddell
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns


D25699: [PC3 ToolButton] Have the label take into account complementary color schemes

2019-12-12 Thread Fabian Vogt
fvogt added a comment.


  This fixed the button label, but the menu itself is unsuable due to a black 
text on dark background: 
https://openqa.opensuse.org/tests/1110939#step/start_wayland_plasma5/21

REPOSITORY
  R242 Plasma Framework (Library)

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

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


D25699: [PC3 ToolButton] Have the label take into account complementary color schemes

2019-12-11 Thread Fabian Vogt
fvogt added a comment.


  In D25699#575278 , @davidedmundson 
wrote:
  
  > Please don't link external sites (GitHub) in the committed message.
  >
  > RE: Menu
  >  There is nothing in QQC2::Button to add a menu
  >
  > If we want that it would have to be a custom button subclass, rather than 
something we support in the style.
  
  
  Or as a simple hack for this case, adding "▾" to the button's label.

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  ok-text-colo (branched from master)

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

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


D25856: Show IOSlaves that return local files when in local file mode

2019-12-10 Thread Fabian Vogt
fvogt added a comment.


  In D25856#575083 , @ngraham wrote:
  
  > In D25856#575044 , @fvogt wrote:
  >
  > > IMO this should be done in KIO, so that all users benefit.
  >
  >
  > How would you do it in KIO?
  
  
  Add code to KFileDialog to allow specific protocols/slaves if the `file` 
scheme is supported.

REPOSITORY
  R229 KDialog

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

To: ngraham, #frameworks, #dolphin
Cc: fvogt, broulik, meven


D25856: Show IOSlaves that return local files when in local file mode

2019-12-10 Thread Fabian Vogt
fvogt added a comment.


  IMO this should be done in KIO, so that all users benefit.
  
  In D25856#575011 , @broulik wrote:
  
  > Isn't that what `KProtocolInfo::protocolClass() == ":local"` is for?
  
  
  Almost, but not quite - it just means that the resource is on the same system 
and therefore there's no hostname in the URL.
  Protocols like `man`, `tar`, `zip`, etc. are all `:local` as well, but can't 
be translated to `file:///...`.
  I'd hope there is some other way to query for that property.

REPOSITORY
  R229 KDialog

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

To: ngraham, #frameworks, #dolphin
Cc: fvogt, broulik, meven


D25699: [PC3 ToolButton] Have the label take into account complementary color schemes

2019-12-07 Thread Fabian Vogt
fvogt added a comment.


  The check for the prefix was added in 
`bf1d1cc6b2ad37cb586f44b56fa2438ed3a5dbfc`, while the `control.flat` one got 
added much earlier.
  
  The labels are visible again with just the `control.flat` condition, but the 
prefix one might be needed as well for non-breeze themes.
  
  There's a part missing though, the triangle (visible on 
https://openqa.opensuse.org/tests/1105226#step/start_wayland_plasma5/21) is 
gone. That seems to be a feature lost with PC3 :-(

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  ok-text-colo (branched from master)

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

To: filipf, #plasma, ngraham
Cc: fvogt, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns


D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

2019-12-04 Thread Fabian Vogt
fvogt added a comment.


  In D23384#572198 , @ngraham wrote:
  
  > I just tested writing today, for files opened in 3rd-party apps that get 
the FUSE mount path. Results:
  >  ...
  
  
  Was that with or without the KIO::open merge request?

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: alexde, broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, 
LeGast00n, GB_2, michaelh, bruns


D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

2019-12-02 Thread Fabian Vogt
fvogt added a comment.


  In D23384#571276 , @ngraham wrote:
  
  > I'm afraid that even with that change, the issue is still present. I 
honestly don't think it would be the worst thing in the world if we always 
handed the kio-fuse paths to apps that don't use ioslaves.
  
  
  It would be. I like to have links like http://kde.org opened properly in the 
web browser, ftp://some/where opened in an FTP client and so on...
  Media players know more about the format and streaming it than kio-fuse ever 
could, so avoiding layers in between if possible is definitely an advantage.

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, 
GB_2, michaelh, bruns


D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

2019-12-02 Thread Fabian Vogt
fvogt added a comment.


  In D23384#570830 , @ngraham wrote:
  
  > In D23384#570735 , @fvogt wrote:
  >
  > > > Clicking a http link in my chat app now kiofuses it and then has the 
browser desperately try to open /run/user/1000/kio-fuse-bla/http/kde.org/ - it 
only gets the first character in the file, so I usually just see < for a HTML 
page
  > >
  > > Ignoring the elephant in the room which is that this diff forces 
everything through `mountUrl`, that's the expected behavior with a plain HTTP 
URL as the size isn't known until the file is downloaded. So `stat` reports a 
size of `1` until the file is actually opened.
  > >  This is unavoidable, otherwise every `ls` would trigger a download of 
all files. If handling this better is important, `HTTPProtocol::stat` could use 
a `HEAD` request to get the `Content-Length`, but that doesn't work in all 
cases either.
  >
  >
  > Well, we need to fix this or else it's a very serious regression that 
breaks a huge part of the desktop. Opening links in a web browser is pretty 
basic functionality.
  
  
  That issue is not in kio-fuse:
  
  > Ignoring the elephant in the room which is that this diff forces everything 
through `mountUrl`, ...

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, 
GB_2, michaelh, bruns


D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

2019-12-02 Thread Fabian Vogt
fvogt added a comment.


  In D23384#570331 , @ngraham wrote:
  
  > In D23384#570118 , @fvogt wrote:
  >
  > > Please try both of the following:
  >
  >
  > Done. Here are the log files:
  >
  > F7793378: kio-fuse.log 
  
  
  Just as expected:
  
unique: 4248, opcode: READ (15), nodeid: 13, insize: 80, pid: 24522
org.kde.kio.fuse: Read "Dean plays with Winnie.mov" 0 65536
org.kde.kio.fuse: Fetching cache for "Dean plays with Winnie.mov"
kf5.kio.core: KLocalSocket(0x212cfd0) Jumbo packet of 65534 bytes
kf5.kio.core: KLocalSocket(0x212cfd0) Jumbo packet of 65534 bytes
   unique: 4248, success, outsize: 65552
kf5.kio.core: KLocalSocket(0x212cfd0) Jumbo packet of 65534 bytes
unique: 4250, opcode: GETATTR (3), nodeid: 13, insize: 56, pid: 24522
   unique: 4250, success, outsize: 120
unique: 4252, opcode: READ (15), nodeid: 13, insize: 80, pid: 24522
org.kde.kio.fuse: Read "Dean plays with Winnie.mov" 153833472 36864
  
  It first reads the header and then jumps to the file's end to read a few KiB 
there.
  Nothing kio-fuse can do about that, except trying to use `KIO:open` in the 
future, but that has some other drawbacks...
  
  > F7793379: strace output.log 

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, 
GB_2, michaelh, bruns


D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

2019-12-02 Thread Fabian Vogt
fvogt added a comment.


  > Clicking a http link in my chat app now kiofuses it and then has the 
browser desperately try to open /run/user/1000/kio-fuse-bla/http/kde.org/ - it 
only gets the first character in the file, so I usually just see < for a HTML 
page
  
  Ignoring the elephant in the room which is that this diff forces everything 
through `mountUrl`, that's the expected behavior with a plain HTTP URL as the 
size isn't known until the file is downloaded. So `stat` reports a size of `1` 
until the file is actually opened.
  This is unavoidable, otherwise every `ls` would trigger a download of all 
files. If handling this better is important, `HTTPProtocol::stat` could use a 
`HEAD` request to get the `Content-Length`, but that doesn't work in all cases 
either.

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, 
GB_2, michaelh, bruns


D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

2019-12-01 Thread Fabian Vogt
fvogt added a comment.


  Unfortunately the `kio-fuse -d` output is incomplete, probably because Qt was 
too smart and logged to the journal instead...
  It's visible that there are multiple processes reading the file, maybe 
thumbnailing is in progress?
  Can you try with thumbnails in dolphin disabled?
  
  Please try both of the following:
  
  Add this to kio-fuse and rebuild:
  
diff --git a/kiofusevfs.cpp b/kiofusevfs.cpp
index 7755b56..a3a4c72 100644
--- a/kiofusevfs.cpp
+++ b/kiofusevfs.cpp
@@ -859,6 +859,7 @@ void KIOFuseVFS::read(fuse_req_t req, fuse_ino_t ino, 
size_t size, off_t off, fu
return;
case KIOFuseNode::NodeType::RemoteFileNode:
{
+   qDebug(KIOFUSE_LOG) << "Read" << node->m_nodeName << off << 
size;
auto remoteNode = 
std::dynamic_pointer_cast(node);
that->awaitBytesAvailable(remoteNode, off + size, [=](int 
error) {
if(error != 0 && error != ESPIPE)
  
  then
  
QT_LOGGING_RULES=*.debug=true QT_FORCE_STDERR_LOGGING=1 
~/kde/usr/lib64/libexec/kio-fuse -d $yourfavlocation &>somelog.file
  
  and to get just the syscalls which totem makes:
  
strace -fefile totem $yourfavlocation/where/the/video.is

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, GB_2, 
michaelh, bruns


D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

2019-11-30 Thread Fabian Vogt
fvogt added a comment.


  **Issue #1:**
  
  That happens because the .desktop file sets 
`X-KDE-Protocols=ftp,http,https,mms,rtmp,rtsp,sftp,smb`:
  https://code.videolan.org/videolan/vlc/blob/master/share/vlc.desktop.in#L124
  
  **Issue #2:**
  
  The only explanation I have for that is that totem for some reason starts 
reading the file from the end for some reason.
  If you start `kio-fuse -d` manually (kill the other kio-fuse process first) 
and then use totem again, what's the debug output?
  
  If totem behaves like this, only `KIO::open` support might help a bit, with 
the cost of increased latency.
  That needs support for something like `KIO::truncate` though, so might take a 
while.
  
  **Issue #3:**
  
  Everything is in a separate process and async already. When/how does it 
happen?

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, GB_2, 
michaelh, bruns


D25107: Add install dir for systemd units

2019-11-25 Thread Fabian Vogt
fvogt accepted this revision.
fvogt added inline comments.
This revision is now accepted and ready to land.

INLINE COMMENTS

> KDEInstallDirs.cmake:245
>  set(_LIBDIR_DEFAULT "lib")
> -# Override this default 'lib' with 'lib64' iff:
> +# Override this default 'lib' with 'lib64' if:
>  #  - we are on a Linux, kFreeBSD or Hurd system but NOT cross-compiling

IMO the `iff` here can stay

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  master

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

To: davidedmundson, apol, fvogt
Cc: cgiboudeaux, fvogt, kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, 
bencreasy, michaelh, ngraham, bruns


D25107: Add install dir for systemd units

2019-11-25 Thread Fabian Vogt
fvogt reopened this revision.
fvogt added a comment.
This revision is now accepted and ready to land.


  AFAICT this breaks if `LIBDIR` != "lib". `systemd` only looks in `/usr/lib` 
AFAICT, so hardcoding to `$prefix/lib/systemd` might be better.

REPOSITORY
  R240 Extra CMake Modules

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

To: davidedmundson, apol
Cc: fvogt, kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, bencreasy, 
michaelh, ngraham, bruns


D25107: Add install dir for systemd units

2019-11-25 Thread Fabian Vogt
fvogt requested changes to this revision.
This revision now requires changes to proceed.

REPOSITORY
  R240 Extra CMake Modules

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

To: davidedmundson, apol, fvogt
Cc: fvogt, kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, bencreasy, 
michaelh, ngraham, bruns


D25159: Fix linking to libssh 0.9.1

2019-11-05 Thread Fabian Vogt
fvogt requested changes to this revision.
fvogt added a comment.
This revision now requires changes to proceed.


  The libssh maintainer is likely reverting the change, so this should not be 
necessary.

REPOSITORY
  R320 KIO Extras

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

To: arojas, apol, sitter, fvogt
Cc: fvogt, sitter, asn, apol, asturmlechner, 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


D24245: Add support for passing Unix file descriptors

2019-09-26 Thread Fabian Vogt
fvogt requested changes to this revision.
fvogt added a comment.
This revision now requires changes to proceed.


  Unfortunately, this breaks public API and ABI by modifying KAuth::ActionReply.
  
  I'm not sure whether there has to be any compatibility for the DBus API, but 
I guess not (logging out and back in should suffice to make sure the backend 
used is the same).
  
  I wonder why you made a wrapper around `QDBusUnixFileDescriptor`, does it not 
work inside a `QVariant` OOTB?

REPOSITORY
  R283 KAuth

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

To: volkov, fvogt, chinmoyr, cfeck, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns


D23692: kdesu: set kernel flags to prevent ptrace instead of relying on setgid

2019-09-20 Thread Fabian Vogt
fvogt added a reviewer: Frameworks.

REPOSITORY
  R299 KDESu

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

To: maltek, adridg, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns


D23694: Add support for sshfs to the fstab backend

2019-09-03 Thread Fabian Vogt
fvogt added a comment.


  `fuse.sshfs` is used by kdeconnect as well, does that cause some kind of 
conflict?
  
  If not, LGTM.
  
  Could be improved by adding other filesystems (curlftpfs?) as well and using 
something like
  
  `QStringList{"nfs", "nfs4", "smbfs", "cifs", "fuse.sshfs"}.contains(fstype);` 
instead.

REPOSITORY
  R245 Solid

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

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


D23454: Fixing bug where MTP slave does not return error in stat()/mimetype()

2019-08-26 Thread Fabian Vogt
This revision was automatically updated to reflect the committed changes.
Closed by commit R320:8c42ec63200f: Fixing bug where MTP slave does not return 
error in stat()/mimetype() (authored by feverfew, committed by fvogt).

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23454?vs=64614=64648

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

AFFECTED FILES
  mtp/kio_mtp.cpp

To: feverfew, chinmoyr, akrutzler, elvisangelaccio, dfaure
Cc: kde-frameworks-devel, kfm-devel, fvogt, aprcela, vmarinescu, fprice, 
LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, 
michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov


D22758: Add a done signal to FindMatchesJob instead of using QObjectDecorator wrongly

2019-08-21 Thread Fabian Vogt
This revision was not accepted when it landed; it landed in state "Needs 
Revision".
This revision was automatically updated to reflect the committed changes.
Closed by commit R308:54e18b0d9b0d: Add a done signal to FindMatchesJob instead 
of using QObjectDecorator wrongly (authored by fvogt).

REPOSITORY
  R308 KRunner

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22758?vs=62583=64191

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

AFFECTED FILES
  src/runnerjobs.cpp
  src/runnerjobs_p.h
  src/runnermanager.cpp

To: fvogt, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns


D23029: Fix the attica pkgconfig file.

2019-08-08 Thread Fabian Vogt
fvogt added inline comments.

INLINE COMMENTS

> libKF5Attica.pc.cmake:3
> +exec_prefix=${BIN_INSTALL_DIR}
>  libdir=${LIB_INSTALL_DIR}
> +includedir=${KF5_INCLUDE_INSTALL_DIR}

In ECM and Qt this is absolute, `${prefix}/lib64`, same for `exec_prefix`.

REPOSITORY
  R235 Attica

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

To: cgiboudeaux
Cc: fvogt, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns


D22979: Security: remove support for $(...) in config keys with [$e] marker.

2019-08-07 Thread Fabian Vogt
fvogt added a comment.


  In D22979#508493 , @kives wrote:
  
  > Does anyone think this can be easily backported to previous versions of KDE 
in upstream distros such as Kubuntu, etc.?
  
  
  I backported this down to KConfig 5.20 and KDELibs 4.14.18, differences were 
trivial to resolve.

REPOSITORY
  R237 KConfig

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

To: dfaure, mdawson, aacid, broulik, davidedmundson, kossebau, apol, sitter, 
security-team
Cc: kives, ZaWertun, rikmills, fvogt, ngraham, kde-frameworks-devel, LeGast00n, 
michaelh, bruns


D22723: Fix RunnerManager::queryFinished()

2019-07-27 Thread Fabian Vogt
fvogt added a comment.


  > QObjects live in their own thread and shouldn't be used outside.
  >  https://doc.qt.io/qt-5/qobject.html#thread
  > 
  > In your patch we are emitting the signal from the run thread instead of the 
actual object's thread. This is wrong.
  
  Needs a `Qt::QueuedConnection` indeed. Or the job has to be moved to a 
different thread, which means that Qt makes the connection a queued one 
automatically.
  
  > Also I'd say there's value in using tools (i.e. Threadweaver) as it's meant 
to be used, despite you don't liking its semantics.
  
  Yes, improving the Threadweaver API is the best option here.

REPOSITORY
  R308 KRunner

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

To: apol, #frameworks, fvogt, davidedmundson
Cc: aacid, kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, bruns


D22723: Fix RunnerManager::queryFinished()

2019-07-26 Thread Fabian Vogt
fvogt added a comment.


  In D22723#502365 , @aacid wrote:
  
  > I honestly don't see the problem with this patch, one may argue that the 
ThreadWeaver API is awkward, ok, but this is using it correctly AFAICS, i.e. 
have a ThreadWeaver::QObjectDecorator, give it a ThreadWeaver::Job on its 
constructor, and go on from there.
  
  
  Correctly as in "it's supposed to work" yes, but it's not as it was intended 
to be AFAICT.
  This now adds a custom private and friend class (ugh) which means that now 
there's a sandwich out of three classes:
  `FindMatchesJob -(inherits)> QObjectDecorator -(uses)> 
FindMatchesJobInternal` while only a single one would do the job.
  I did a PoC of dropping use of `QObjectDecorator`: 
https://phabricator.kde.org/D22758
  
  Here, one may argue that it reinvents the wheel, but if the current iteration 
of the wheel is square I'd say it's allowed.
  It also gets rid of a heap allocation.
  
  What do you think?
  
  If you don't like it I won't object landing this internal class, but please 
add a long comment explaining why it was done like this.

REPOSITORY
  R308 KRunner

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

To: apol, #frameworks, fvogt, davidedmundson
Cc: aacid, kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, bruns


D22758: Add a done signal to FindMatchesJob instead of using QObjectDecorator wrongly

2019-07-26 Thread Fabian Vogt
fvogt created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
fvogt requested review of this revision.

REVISION SUMMARY
  Currently KRunner uses QObjectDecorator wrongly and changing the
  design to fix that would not only be a lot of work, but also include downsides
  like having two custom Job classes, making the code harder to read.
  So merge the part of QObjectDecorator that would've been used here into the
  FindMatchesJob class and drop use of QObjectDecorator.

TEST PLAN
  Added a debug statement in the jobDone slot, it's called.

REPOSITORY
  R308 KRunner

BRANCH
  betterhack

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

AFFECTED FILES
  src/runnerjobs.cpp
  src/runnerjobs_p.h
  src/runnermanager.cpp

To: fvogt
Cc: kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, bruns


D22723: Fix RunnerManager::queryFinished()

2019-07-25 Thread Fabian Vogt
fvogt added a comment.


  In D22723#501907 , @apol wrote:
  
  > In D22723#501690 , @fvogt wrote:
  >
  > > Looks like a hack still, with two Job objects for each job...
  > >
  > > What about just merging `QObjectDecorator` into `FindMatchesJobInternal` 
by basically just adding a custom `done` signal and ignoring the entire 
"decorators which are actually wrappers" business?
  > >
  > > IMO this new `FindMatchesJobInternal` class makes it even less obvious 
what's actually going on.
  >
  >
  > This is how ThreadWeaver and especially QObjectDecorator is meant to be 
used.
  
  
  The way `ThreadWeaver::QObjectDecorator` is apparently meant to be used is to 
wrap the custom job inside a `QObjectDecorator` and use only the wrapper from 
there on:
  
  
https://github.com/KDE/threadweaver/blob/239cf8fffe687c0a758f5170a40b26ae0acef7b0/autotests/QueueTests.cpp#L157
  
autoDeleteJob = new QObjectDecorator(new AppendCharacterJob(QChar('a'), 
));
[...]
QVERIFY(autoDeleteJob != nullptr);
QVERIFY(connect(autoDeleteJob, SIGNAL(done(ThreadWeaver::JobPointer)),
SLOT(deleteJob(ThreadWeaver::JobPointer;
  
  which is not great, to say the least.
  
  What this patch does on the surface is wrap the `QObjectDecorator` inside an 
object that fakes being the custom job itself.
  That's actually a slightly better design than the code above does as now the 
pointer passed from the `done` signal is not the `QObjectDecorator` pointer but 
the custom class.
  
  Still, IMO much better would be to just merge the `QObjectDecorator` into the 
custom job as that would both avoid creating two job objects per job and make 
the code clearer and shorter.
  
  > I don't really know why you say it's confusing. The confusing part so far 
was that jobDone slot was never called.
  
  Which likely happened because the author was confused by the code...

REPOSITORY
  R308 KRunner

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

To: apol, #frameworks, fvogt, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, bruns


D22723: Fix RunnerManager::queryFinished()

2019-07-24 Thread Fabian Vogt
fvogt added a reviewer: davidedmundson.
fvogt added a comment.


  Looks like a hack still, with two Job objects for each job...
  
  What about just merging `QObjectDecorator` into `FindMatchesJobInternal` by 
basically just adding a custom `done` signal and ignoring the entire 
"decorators which are actually wrappers" business?
  
  IMO this new `FindMatchesJobInternal` class makes it even less obvious what's 
actually going on.

REPOSITORY
  R308 KRunner

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

To: apol, #frameworks, fvogt, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, bruns


D22555: [RFC] Add a kded module to manage various available fuse mount services

2019-07-19 Thread Fabian Vogt
fvogt added inline comments.

INLINE COMMENTS

> mountservicemanager.cpp:42
> +{
> +KConfigGroup cfg = 
> KConfigGroup(KSharedConfig::openConfig(QStringLiteral("fusemanagerrc")), 
> QStringLiteral("Fuse Services"));
> +return cfg.readEntry(url.scheme(), QString());

This line is the only place "fuse" is ever mentioned, so maybe this should be 
renamed?

REPOSITORY
  R241 KIO

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

To: chinmoyr, fvogt
Cc: kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, bruns


D22147: Better use of Qt APIs in Plasma::Theme

2019-07-01 Thread Fabian Vogt
fvogt resigned from this revision.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: apol, #plasma, #frameworks
Cc: bruns, tcanabrava, fvogt, broulik, kde-frameworks-devel, LeGast00n, 
michaelh, ngraham


D22147: Better use of Qt APIs in Plasma::Theme

2019-07-01 Thread Fabian Vogt
fvogt added inline comments.

INLINE COMMENTS

> apol wrote in svg.cpp:317
> Please note this is only to make sure the regex was properly compiled. It 
> isn't matching there yet.

It really does not look that way as you're immediately using captures after 
that.

If that's really what you want (which I doubt), it should be 
`sizeHintedKeyExpr.isValid()` instead and be done before the foreach.

Currently it would just add `QSize(0, 0)` to `elementsWithSizeHints` even for 
"öoiawze9pv5z2p3v4" as key.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: apol, #plasma, #frameworks, fvogt
Cc: tcanabrava, fvogt, broulik, kde-frameworks-devel, LeGast00n, michaelh, 
ngraham, bruns


D22147: Better use of Qt APIs in Plasma::Theme

2019-07-01 Thread Fabian Vogt
fvogt requested changes to this revision.
fvogt added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> svg.cpp:317
> +const auto match = sizeHintedKeyExpr.match(key);
> +if (match.isValid()) {
> +QString baseElementId = match.captured(3);

`isValid` is always true, you probably want to use `hasMatch` instead.

This is not obvious, I only noticed this because I debugged this error before 
(https://phabricator.kde.org/D17359)

REPOSITORY
  R242 Plasma Framework (Library)

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

To: apol, #plasma, #frameworks, fvogt
Cc: fvogt, broulik, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns


D22080: [Fstab] Show mounted "overlay" filesystems

2019-06-25 Thread Fabian Vogt
fvogt accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R245 Solid

BRANCH
  submit

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

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


D22081: [Fstab] Select appropriate icon for home or root directory

2019-06-25 Thread Fabian Vogt
fvogt accepted this revision.
fvogt added a comment.
This revision is now accepted and ready to land.


  With this and D22080  applied, there's an 
entry for / now:
  
  F6932935: Screenshot_20190625_092256.png 

  
  On a regular system, nothing seems to have changed, so the changes seem to be 
fine.

REPOSITORY
  R245 Solid

BRANCH
  fstab_overlayfs

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

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


D15739: [Places panel] Don't show Root by default

2019-06-24 Thread Fabian Vogt
fvogt added a comment.


  In D15739#486043 , @bruns wrote:
  
  > In D15739#486020 , @fvogt wrote:
  >
  > > In D15739#486009 , @bruns 
wrote:
  > >
  > > > It actually already does, via the fstab backend (currently network 
(SMB/NFS) and various fuse mounts). Fstab in this case also includes everything 
in MTAB, should be quite easy to extend this.
  > > >
  > > > What is the output of `cat /proc/self/mounts` (fell free to remove 
anything irrelevant, like cgroups)?
  > >
  > >
  > > The entries involved in / are these:
  > >
  > >   /dev/sr0 /run/initramfs/live iso9660 ro,relatime 0 0
  > >   /dev/loop0 /run/initramfs/squashfs_container squashfs ro,relatime 0 0
  > >   /dev/loop1 /run/rootfsbase ext4 ro,relatime,data=ordered 0 0
  > >   LiveOS_rootfs / overlay 
rw,relatime,lowerdir=/run/rootfsbase,upperdir=/run/overlayfs/rw,workdir=/run/overlayfs/work
 0 0
  > >
  >
  >
  > Hm, loop1 is the `lowerdir` of the overlay - how are loop0 and sr0 
involved, are these the backing files?
  
  
  /dev/sr0 contains a squashfs, which is visible as /dev/loop0.
  /dev/loop0 contains an ext4 image, which is visible as /dev/loop1.
  
  So it's doubly indirect.
  
  > Though, the relevant part is `mntent.mnt.type == "overlay"` and 
`mntent.mnt_dir == "/"`. Matching for "overlay" is probably sufficient.
  
  If adding such a special case makes sense, yes. I'd even argue about 
'mntent.mnt_dir == "/" && isKnownFilesystem(mntent)' or something like that to 
ensure that an entry for `/` is always provided.

REPOSITORY
  R241 KIO

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

To: meven, #dolphin, #vdg, tcanabrava, ngraham, elvisangelaccio
Cc: andriusr, fvogt, meven, elvisangelaccio, Codezela, davidc, tcanabrava, 
ndavis, romangg, bruns, davidedmundson, abetts, svenmauch, broulik, 
acrouthamel, kde-frameworks-devel, LeGast00n, michaelh, ngraham


D15739: [Places panel] Don't show Root by default

2019-06-24 Thread Fabian Vogt
fvogt added a comment.


  In D15739#486009 , @bruns wrote:
  
  > In D15739#485983 , @fvogt wrote:
  >
  > > Even if all (block) devices and their mountpoints were shown in the 
devices view, there would be no equivalent of "/". One loop device provides the 
read-only base for /, but that's actually more confusing than the current state 
as it looks like /, but is only a read-only view of "the past".
  > >
  > > I guess solid needs to gain support for mountpoints not backed by devices?
  >
  >
  > It actually already does, via the fstab backend (currently network 
(SMB/NFS) and various fuse mounts). Fstab in this case also includes everything 
in MTAB, should be quite easy to extend this.
  >
  > What is the output of `cat /proc/self/mounts` (fell free to remove anything 
irrelevant, like cgroups)?
  
  
  The entries involved in / are these:
  
/dev/sr0 /run/initramfs/live iso9660 ro,relatime 0 0
/dev/loop0 /run/initramfs/squashfs_container squashfs ro,relatime 0 0
/dev/loop1 /run/rootfsbase ext4 ro,relatime,data=ordered 0 0
LiveOS_rootfs / overlay 
rw,relatime,lowerdir=/run/rootfsbase,upperdir=/run/overlayfs/rw,workdir=/run/overlayfs/work
 0 0

REPOSITORY
  R241 KIO

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

To: meven, #dolphin, #vdg, tcanabrava, ngraham, elvisangelaccio
Cc: andriusr, fvogt, meven, elvisangelaccio, Codezela, davidc, tcanabrava, 
ndavis, romangg, bruns, davidedmundson, abetts, svenmauch, broulik, 
acrouthamel, kde-frameworks-devel, LeGast00n, michaelh, ngraham


D15739: [Places panel] Don't show Root by default

2019-06-24 Thread Fabian Vogt
fvogt added a comment.


  In D15739#485979 , @ngraham wrote:
  
  > In D15739#485978 , @fvogt wrote:
  >
  > > Well, the only mount of interest is the overlay mount of /, which is not 
backed by any (block) device but rather by two filesystems. AFAICT udisks 
doesn't care about this kind of mount, not sure what solid does.
  > >
  > > Showing an overlay mount as "Device" is also somewhat wrong IMO.
  >
  >
  > I'm saying that regardless of the technical details of the backend, it 
never makes sense to not show any devices. From the user's perspective, there 
is always a device of some sort, regardless of its underlying configuration. 
There can't not be a device. That doesn't make sense; software requires 
hardware.
  
  
  Even if all (block) devices and their mountpoints were shown in the devices 
view, there would be no equivalent of "/". One loop device provides the 
read-only base for /, but that's actually more confusing than the current state 
as it looks like /, but is only a read-only view of "the past".
  
  I guess solid needs to gain support for mountpoints not backed by devices?

REPOSITORY
  R241 KIO

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

To: meven, #dolphin, #vdg, tcanabrava, ngraham, elvisangelaccio
Cc: fvogt, meven, elvisangelaccio, Codezela, davidc, tcanabrava, ndavis, 
romangg, bruns, davidedmundson, abetts, svenmauch, broulik, acrouthamel, 
kde-frameworks-devel, LeGast00n, michaelh, ngraham


D15739: [Places panel] Don't show Root by default

2019-06-24 Thread Fabian Vogt
fvogt added a comment.


  In D15739#485976 , @ngraham wrote:
  
  > In D15739#485975 , @fvogt wrote:
  >
  > > No, there's in fact no devices section at all as it would be empty. The 
live cd itself is marked as ignored as there's nothing useful on there and it 
can't be ejected and the other layers are mounted from loop devices which 
udisks apparently ignores as well.
  >
  >
  > That seems like a bug in the way the devices list is populated or 
displayed. There can't be no devices, that's silly.
  
  
  Well, the only mount of interest is the overlay mount of /, which is not 
backed by any (block) device but rather by two filesystems. AFAICT udisks 
doesn't care about this kind of mount, not sure what solid does.
  
  Showing an overlay mount as "Device" is also somewhat wrong IMO.

REPOSITORY
  R241 KIO

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

To: meven, #dolphin, #vdg, tcanabrava, ngraham, elvisangelaccio
Cc: fvogt, meven, elvisangelaccio, Codezela, davidc, tcanabrava, ndavis, 
romangg, bruns, davidedmundson, abetts, svenmauch, broulik, acrouthamel, 
kde-frameworks-devel, LeGast00n, michaelh, ngraham


D15739: [Places panel] Don't show Root by default

2019-06-24 Thread Fabian Vogt
fvogt added a comment.


  In D15739#485960 , @ngraham wrote:
  
  > In D15739#485949 , @fvogt wrote:
  >
  > > I just noticed this in openQA runs from a live medium. There is no 
single-click way to get to the root directory anymore, as / is mounted as an 
overlayfs of a tmpfs and a squashfs container.
  > >
  > > Not sure what the best way to improve this is, any idea?
  >
  >
  > So there's no entry in the Devices section that goes to `/`?
  
  
  No, there's in fact no devices section at all as it would be empty. The live 
cd itself is marked as ignored as there's nothing useful on there and it can't 
be ejected and the other layers are mounted from loop devices which udisks 
apparently ignores as well.

REPOSITORY
  R241 KIO

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

To: meven, #dolphin, #vdg, tcanabrava, ngraham, elvisangelaccio
Cc: fvogt, meven, elvisangelaccio, Codezela, davidc, tcanabrava, ndavis, 
romangg, bruns, davidedmundson, abetts, svenmauch, broulik, acrouthamel, 
kde-frameworks-devel, LeGast00n, michaelh, ngraham


D15739: [Places panel] Don't show Root by default

2019-06-24 Thread Fabian Vogt
fvogt added a comment.


  I just noticed this in openQA runs from a live medium. There is no 
single-click way to get to the root directory anymore, as / is mounted as an 
overlayfs of a tmpfs and a squashfs container.
  
  Not sure what the best way to improve this is, any idea?

REPOSITORY
  R241 KIO

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

To: meven, #dolphin, #vdg, tcanabrava, ngraham, elvisangelaccio
Cc: fvogt, meven, elvisangelaccio, Codezela, davidc, tcanabrava, ndavis, 
romangg, bruns, davidedmundson, abetts, svenmauch, broulik, acrouthamel, 
kde-frameworks-devel, LeGast00n, michaelh, ngraham


D21607: Don't delay emission of matchesChanged indefinitely

2019-06-19 Thread Fabian Vogt
This revision was automatically updated to reflect the committed changes.
Closed by commit R308:a07027cd4f22: Dont delay emission of matchesChanged 
indefinitely (authored by fvogt).

REPOSITORY
  R308 KRunner

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21607?vs=59252=60049

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

AFFECTED FILES
  src/runnermanager.cpp

To: fvogt, #frameworks, broulik, ngraham
Cc: ngraham, bruns, kde-frameworks-devel, LeGast00n, michaelh


D21607: Don't delay emission of matchesChanged indefinitely

2019-06-18 Thread Fabian Vogt
fvogt added a comment.


  Before I land this, I'd like if someone other than me tries krunner with this 
patch applied and judges the result with several runners. The difference is 
very noticable with the appstream runner as it does not batch results.

REPOSITORY
  R308 KRunner

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

To: fvogt, #frameworks, broulik
Cc: ngraham, bruns, kde-frameworks-devel, LeGast00n, michaelh


D8532: [WIP] Restrict file extractor with Seccomp

2019-06-11 Thread Fabian Vogt
fvogt added a comment.


  In D8532#478224 , @bruns wrote:
  
  > I totally agree with fvogt here - the extractors should just receive a 
readonly file descriptor.
  >
  > For this, there are several steps required:
  >
  > 1. let the extractors work with file descriptors (KFileMetaData)
  > 2. make sure the extractor plugins are fully initialized before receiving 
file descriptors
  > 3. actually feed file descriptors to the extractor
  >
  >   (1.) is trivial for some extractors (e.g. taglib), for others it may be 
hard. (2.) depends on several things - the plugins must be instantiated early 
(which clashes with the lazy loading), and the plugin may not load any external 
resources later on.
  >
  >   Using file descriptors has another benefit - currently, the file is 
stat'ed and so on, and then the corresponding path is fed to the extractor. It 
would be much better to open the file, use fstatat and friends, run the 
extractor and close the file again.
  
  
  What could also be done as an intermediate step is to whitelist opening 
read-only fds for metadata extractions. That way something like plugin loading 
is also covered and not many changes are required.
  The sandbox could be opt-in for plugins which just specify that they support 
sandboxing using the specified whitelist, with plugins which don't support 
sandboxing disabled by default.
  I used this approach in a (private so far) branch for sandboxing the 
thumbnail kio slave and it works well.

REPOSITORY
  R293 Baloo

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

To: davidk, apol, ossi, #frameworks, smithjd, bruns
Cc: fvogt, mgallien, kde-frameworks-devel, michaelh, #baloo, detlefe, ngraham, 
nicolasfella, LeGast00n, domson, ashaposhnikov, astippich, spoorun, bruns, 
abrahams


D21607: Don't delay emission of matchesChanged indefinitely

2019-06-06 Thread Fabian Vogt
fvogt updated this revision to Diff 59252.
fvogt added a comment.


  New algorithm with no delay if not necessary.

REPOSITORY
  R308 KRunner

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21607?vs=59203=59252

BRANCH
  master

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

AFFECTED FILES
  src/runnermanager.cpp

To: fvogt, #frameworks, broulik
Cc: bruns, kde-frameworks-devel, LeGast00n, michaelh, ngraham


D21607: Don't delay emission of matchesChanged indefinitely

2019-06-06 Thread Fabian Vogt
fvogt added a comment.


  In D21607#474772 , @fvogt wrote:
  
  > So for the stable branches I'd like to keep the current version of the diff 
with a latency of [100,599] while for master something like the above with a 
latency of [0,500] can be tried.
  
  
  And I just now realize that I'm stupid and mixed this up with milou. krunner 
is a framework so there is no stable branch...
  
  I'll update the diff to the [0,250] case.

REPOSITORY
  R308 KRunner

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

To: fvogt, #frameworks, broulik
Cc: bruns, kde-frameworks-devel, LeGast00n, michaelh, ngraham


D21607: Don't delay emission of matchesChanged indefinitely

2019-06-05 Thread Fabian Vogt
fvogt added a comment.


  In D21607#474771 , @fvogt wrote:
  
  > I'm thinking about doing it completely differently now though, with a 0 
latency case (untested):
  >
  >   if(lastMatchChangeSignalled.hasExpired(250)) {
  >   matchChangeTimer.stop();
  >   emit q->matchesChanged(context.matches());
  >   } else {
  >   matchChangeTimer.start(250 - lastMatchChangeSignalled.elapsed());
  >   }
  >
  
  
  Just tried this and it's not too bad, but a noticable change in behaviour. As 
results are shown basically immediately once they're available, it's now 
visible how the entries are filled.
  
  So for the stable branches I'd like to keep the current version of the diff 
with a latency of [100,599] while for master something like the above with a 
latency of [0,500] can be tried.

REPOSITORY
  R308 KRunner

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

To: fvogt, #frameworks, broulik
Cc: bruns, kde-frameworks-devel, LeGast00n, michaelh, ngraham


D21607: Don't delay emission of matchesChanged indefinitely

2019-06-05 Thread Fabian Vogt
fvogt added a comment.


  In D21607#474763 , @bruns wrote:
  
  > This would emit the signal more often, but wouldn't
  >
  >   if (!matchChangeTimer.isActive())
  > matchChangeTimer.start(100)
  >
  >
  > achieve essentially the same?
  
  
  That would do it even more often.
  
  I'm thinking about doing it completely differently now though, with a 0 
latency case (untested):
  
if(lastMatchChangeSignalled.hasExpired(100)) {
matchChangeTimer.stop();
emit q->matchesChanged(context.matches());
} else {
matchChangeTimer.start(100 - lastMatchChangeSignalled.expired());
}
  
  What do you think?

REPOSITORY
  R308 KRunner

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

To: fvogt, #frameworks, broulik
Cc: bruns, kde-frameworks-devel, LeGast00n, michaelh, ngraham


D21607: Don't delay emission of matchesChanged indefinitely

2019-06-05 Thread Fabian Vogt
fvogt created this revision.
fvogt added reviewers: Frameworks, broulik.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
fvogt requested review of this revision.

REVISION SUMMARY
  Currently the signal is only emitted if there was no change to the matches in
  the last 100ms. Especially during krunner startup and early result collection,
  this is unlikely to happen though, so make sure that the signal is emitted
  at least once every ~500ms.

TEST PLAN
  Sometimes results only appeared after a noticable delay, now this
  delay is much shorter.

REPOSITORY
  R308 KRunner

BRANCH
  master

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

AFFECTED FILES
  src/runnermanager.cpp

To: fvogt, #frameworks, broulik
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns


D21606: RFC: ThreadWeaver Job Decorators not used properly and have no effect

2019-06-05 Thread Fabian Vogt
fvogt created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
fvogt requested review of this revision.

REVISION SUMMARY
  ThreadWeaver Job Decorators don't actually hook into an existing job.
  Instead, they provide the same interface as the job and just pass through
  all calls.
  
  The way the QObjectDecorator is used by KRunner results in no signals actually
  getting emitted, making most of the code pointless.
  
  This patch is just a quick hack to show what would happen if the jobDone 
signal
  worked and is mostly intended to present the issue.

TEST PLAN
  jobDone is called now.

REPOSITORY
  R308 KRunner

BRANCH
  hack

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

AFFECTED FILES
  src/runnerjobs.cpp

To: fvogt
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns


D20659: Copy container in Component::cleanUp before interating

2019-04-18 Thread Fabian Vogt
fvogt added a comment.


  In D20659#452642 , @ngraham wrote:
  
  > Did this fix https://bugs.kde.org/show_bug.cgi?id=406642?
  
  
  No, that particular crash (bug 406426) is already fixed. I marked it as dup.

REPOSITORY
  R268 KGlobalAccel

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

To: fvogt, #frameworks, davidedmundson
Cc: ngraham, lbeltrame, kde-frameworks-devel, michaelh, bruns


D20659: Copy container in Component::cleanUp before interating

2019-04-18 Thread Fabian Vogt
This revision was automatically updated to reflect the committed changes.
Closed by commit R268:78a711361db3: Copy container in Component::cleanUp before 
interating (authored by fvogt).

REPOSITORY
  R268 KGlobalAccel

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20659?vs=56517=56524

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

AFFECTED FILES
  src/runtime/component.cpp

To: fvogt, #frameworks, davidedmundson
Cc: lbeltrame, kde-frameworks-devel, michaelh, ngraham, bruns


D20659: Copy container in Component::cleanUp before interating

2019-04-18 Thread Fabian Vogt
fvogt retitled this revision from "Detach container in Component::cleanUp 
before interating" to "Copy container in Component::cleanUp before interating".

REPOSITORY
  R268 KGlobalAccel

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

To: fvogt, #frameworks
Cc: lbeltrame, kde-frameworks-devel, michaelh, ngraham, bruns


D20659: Detach container in Component::cleanUp before interating

2019-04-18 Thread Fabian Vogt
fvogt edited the summary of this revision.
fvogt edited the test plan for this revision.

REPOSITORY
  R268 KGlobalAccel

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

To: fvogt, #frameworks
Cc: lbeltrame, kde-frameworks-devel, michaelh, ngraham, bruns


D20659: Detach container in Component::cleanUp before interating

2019-04-18 Thread Fabian Vogt
fvogt updated this revision to Diff 56517.
fvogt added a comment.


  Use auto (which might actually make it build)

REPOSITORY
  R268 KGlobalAccel

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20659?vs=56515=56517

BRANCH
  master

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

AFFECTED FILES
  src/runtime/component.cpp

To: fvogt
Cc: kde-frameworks-devel, michaelh, ngraham, bruns


D20659: Detach container in Component::cleanUp before interating

2019-04-18 Thread Fabian Vogt
fvogt created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
fvogt requested review of this revision.

REVISION SUMMARY
  Might fix crashes similiar to bug 406426

TEST PLAN
  Not tested, will ask someone to do that with valgrind

REPOSITORY
  R268 KGlobalAccel

BRANCH
  master

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

AFFECTED FILES
  src/runtime/component.cpp

To: fvogt
Cc: kde-frameworks-devel, michaelh, ngraham, bruns


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

2019-04-13 Thread Fabian Vogt
fvogt accepted this revision.
fvogt added a comment.


  Code still looks good to me - I can't comment on the cmake parts though.

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


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

2019-04-07 Thread Fabian Vogt
fvogt accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D20096

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

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


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

2019-04-06 Thread Fabian Vogt
fvogt added a comment.


  Looking good to me, @bruns: any addiitonal comments?

REPOSITORY
  R241 KIO

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

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


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

2019-04-06 Thread Fabian Vogt
fvogt requested changes to this revision.
fvogt added a comment.
This revision now requires changes to proceed.


  Looks good to me otherwise.

INLINE COMMENTS

> file.cpp:1033
> +/* And linux version using statx syscall */
> +if (buff.stx_btime.tv_nsec > 0) {
> +entry.fastInsert(KIO::UDSEntry::UDS_CREATION_TIME, 
> buff.stx_btime.tv_sec);

This check seems to be wrong with me - there can be files with legitimate zero 
`tv_nsec`.

Use `buff.stx_mask & STATX_BTIME` instead.

REPOSITORY
  R241 KIO

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

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


  1   2   3   4   >