Re: Review Request 126324: [MSWin/OS X] save and restore window geometry instead of only size (WIP/Suggestion)

[Jaime Torres Amate]

> On Dec. 17, 2015, 4:16 p.m., Martin Gräßlin wrote:
> >

Hello,

  This is just a warning to know if this patch has been tested in a two monitor 
environment in a laptop.
  In a pyqt application I save and restore the size and position of the window 
(without additional checks), using:
settings.setValue("size", self.ui.size())
settings.setValue("pos", self.ui.pos())   # save

self.ui.resize(settings.value("size", QSize(400, 400)))
self.ui.move(settings.value("pos", QPoint(200, 200)))   # restore
 
  It works perfectly fine except in this case:
   The window, when saved, was in a monitor that disappears in the next run. In 
the next run (without the monitor) the window is not shown, because it is still 
in the other monitor, but it is shown in the taskbar, I have to go to the 
taskbar or with an advanced task manager, tell the window to become maximized, 
then it is shown.
  If this patch shows the window when the monitor disappears, then, please, 
ignore this comment.


- Jaime Torres


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126324/#review89665
---


On Dec. 14, 2015, 4:04 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126324/
> ---
> 
> (Updated Dec. 14, 2015, 4:04 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X and KDE Frameworks.
> 
> 
> Repository: kconfig
> 
> 
> Description
> ---
> 
> In KDElibs4, the KMainWindow::saveWindowSize() and 
> KMainWindow::restoreWindowSize() function saved and restored not only the 
> size but also the position (i.e. the geometry) of windows, using 
> QWidget::saveGeometry and QWidget::restoreGeometry.
> 
> 2 main reasons for this (according to the comments):
> - Under X11 restoring the position is tricky
> - X11 has a window manager which might be considered responsible for that 
> functionality (and I suppose most modern WMs have the feature enabled by 
> default?)
> 
> Both arguments are moot on MS Windows and OS X, and on both platforms users 
> expect to see window positions restored as well as window size. On OS X there 
> is also little choice in the matter: most applications offer the geometry 
> restore without asking (IIRC it is the same on MS Windows).
> 
> I would thus like to propose to port the platform-specific code that existed 
> for MS Windows (and for OS X as a MacPorts patch that apparently was never 
> submitted upstreams). I realise that this violates the message conveyed by 
> the function names but I would like to think that this is a case where 
> function is more important.
> 
> You may also notice that the Mac version does not store resolution-specific 
> settings. This happens to work best on OS X, where multi-screen support has 
> been present since the early nineties, and where window geometry is restored 
> regardless of the screen resolution (i.e. connect a different external screen 
> with a different resolution, and windows will reopen as they were on that 
> screen, not with some default geometry).
> I required I can update the comments in the header to reflect this subtlety.
> 
> Note that for optimal functionality a companion patch to `KMainWindow::event` 
> is required:
> ```
> --- a/src/kmainwindow.cpp
> +++ b/src/kmainwindow.cpp
> @@ -772,7 +772,7 @@ bool KMainWindow::event(QEvent *ev)
>  {
>  K_D(KMainWindow);
>  switch (ev->type()) {
> -#ifdef Q_OS_WIN
> +#if defined(Q_OS_WIN) || defined(Q_OS_OSX)
>  case QEvent::Move:
>  #endif
>  case QEvent::Resize:
> ```
> 
> This ensures that the window geometry save is performed also after a move (to 
> update the position) without requiring a dummy resizing operation.
> Do I need to create a separate RR for this change or is it small enough that 
> I can push it if and when this RR is accepted?
> 
> 
> Diffs
> -
> 
>   src/gui/kwindowconfig.h 48a8f3c 
>   src/gui/kwindowconfig.cpp d2f355c 
> 
> Diff: https://git.reviewboard.kde.org/r/126324/diff/
> 
> 
> Testing
> ---
> 
> On OS X 10.6 through 10.9 with various KDElibs4 versions and now with Qt 
> 5.5.1 and frameworks 5.16.0 (and Kate as a test application).
> I presume that the MS Windows code has been tested sufficiently in KDELibs4; 
> I have only adapted it to Qt5 and tested if it builds.
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126324: [MSWin/OS X] save and restore window geometry instead of only size (WIP/Suggestion)

[Jaime Torres Amate]

> On Dec. 17, 2015, 4:16 p.m., Martin Gräßlin wrote:
> >
> 
> Jaime Torres Amate wrote:
> Hello,
> 
>   This is just a warning to know if this patch has been tested in a two 
> monitor environment in a laptop.
>   In a pyqt application I save and restore the size and position of the 
> window (without additional checks), using:
> settings.setValue("size", self.ui.size())
> settings.setValue("pos", self.ui.pos())   # save
> 
> self.ui.resize(settings.value("size", QSize(400, 400)))
> self.ui.move(settings.value("pos", QPoint(200, 200)))   # restore
>  
>   It works perfectly fine except in this case:
>The window, when saved, was in a monitor that disappears in the next 
> run. In the next run (without the monitor) the window is not shown, because 
> it is still in the other monitor, but it is shown in the taskbar, I have to 
> go to the taskbar or with an advanced task manager, tell the window to become 
> maximized, then it is shown.
>   If this patch shows the window when the monitor disappears, then, 
> please, ignore this comment.
> 
> René J.V. Bertin wrote:
> What OS/windowing environment is that?
> 
> To the best of my knowledge, OS X will rearrange windows when a monitor 
> is disconnected (regardless whether on a laptop or desktop host) and should 
> do the same when reopening a window in an offscreen position if that window 
> isn't meant to be offscreen. I haven't yet tested this because of the 
> rearranging thing, but good point. I'll see if it can be simulated by storing 
> an offscreen position (this is where the binary nature of the saved data is 
> annoying indeed!)

Oh!, I'm sorry, I forgot to say. It is a windows 7. It rearranges other 
windows, but as it's position is restored, it goes to the non connected 
monitor. I hope it does not happen with this patch (In linux it does not happen 
to that pyqt program).


- Jaime Torres


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126324/#review89665
---


On Dec. 14, 2015, 4:04 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126324/
> ---
> 
> (Updated Dec. 14, 2015, 4:04 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X and KDE Frameworks.
> 
> 
> Repository: kconfig
> 
> 
> Description
> ---
> 
> In KDElibs4, the KMainWindow::saveWindowSize() and 
> KMainWindow::restoreWindowSize() function saved and restored not only the 
> size but also the position (i.e. the geometry) of windows, using 
> QWidget::saveGeometry and QWidget::restoreGeometry.
> 
> 2 main reasons for this (according to the comments):
> - Under X11 restoring the position is tricky
> - X11 has a window manager which might be considered responsible for that 
> functionality (and I suppose most modern WMs have the feature enabled by 
> default?)
> 
> Both arguments are moot on MS Windows and OS X, and on both platforms users 
> expect to see window positions restored as well as window size. On OS X there 
> is also little choice in the matter: most applications offer the geometry 
> restore without asking (IIRC it is the same on MS Windows).
> 
> I would thus like to propose to port the platform-specific code that existed 
> for MS Windows (and for OS X as a MacPorts patch that apparently was never 
> submitted upstreams). I realise that this violates the message conveyed by 
> the function names but I would like to think that this is a case where 
> function is more important.
> 
> You may also notice that the Mac version does not store resolution-specific 
> settings. This happens to work best on OS X, where multi-screen support has 
> been present since the early nineties, and where window geometry is restored 
> regardless of the screen resolution (i.e. connect a different external screen 
> with a different resolution, and windows will reopen as they were on that 
> screen, not with some default geometry).
> I required I can update the comments in the header to reflect this subtlety.
> 
> Note that for optimal functionality a companion patch to `KMainWindow::event` 
> is required:
> ```
> --- a/src/kmainwindow.cpp
> +++ b/src/kmainwindow.cpp
> @@ -772,7 +772,7 @@ bool KMainWindow::event(QEvent *ev)
>  {
>  K_D(KMainWindow);
>  switch (ev->type()) {
&

Re: Review Request 126324: [MSWin/OS X] save and restore window geometry instead of only size (WIP/Suggestion)

[Jaime Torres Amate]

> On Dec. 17, 2015, 4:16 p.m., Martin Gräßlin wrote:
> >
> 
> Jaime Torres Amate wrote:
> Hello,
> 
>   This is just a warning to know if this patch has been tested in a two 
> monitor environment in a laptop.
>   In a pyqt application I save and restore the size and position of the 
> window (without additional checks), using:
> settings.setValue("size", self.ui.size())
> settings.setValue("pos", self.ui.pos())   # save
> 
> self.ui.resize(settings.value("size", QSize(400, 400)))
> self.ui.move(settings.value("pos", QPoint(200, 200)))   # restore
>  
>   It works perfectly fine except in this case:
>The window, when saved, was in a monitor that disappears in the next 
> run. In the next run (without the monitor) the window is not shown, because 
> it is still in the other monitor, but it is shown in the taskbar, I have to 
> go to the taskbar or with an advanced task manager, tell the window to become 
> maximized, then it is shown.
>   If this patch shows the window when the monitor disappears, then, 
> please, ignore this comment.
> 
> René J.V. Bertin wrote:
> What OS/windowing environment is that?
> 
> To the best of my knowledge, OS X will rearrange windows when a monitor 
> is disconnected (regardless whether on a laptop or desktop host) and should 
> do the same when reopening a window in an offscreen position if that window 
> isn't meant to be offscreen. I haven't yet tested this because of the 
> rearranging thing, but good point. I'll see if it can be simulated by storing 
> an offscreen position (this is where the binary nature of the saved data is 
> annoying indeed!)
> 
> Jaime Torres Amate wrote:
> Oh!, I'm sorry, I forgot to say. It is a windows 7. It rearranges other 
> windows, but as it's position is restored, it goes to the non connected 
> monitor. I hope it does not happen with this patch (In linux it does not 
> happen to that pyqt program).
> 
> René J.V. Bertin wrote:
> Even if it doesn't on OS X, someone will have to test on MS Windows.
> 
> I presume one can obtain the limits within which the position has to lie 
> and constrain the position so that at least part of the window is visible 
> (even if those limits reflect only the rectangle within which the available 
> screens lie).
> 
> René J.V. Bertin wrote:
> I just checked this, doing `frameGeo.adjust()` with a selection of huge 
> values in `GeometryData::applyToWindow()`. As I thought, OS X doesn't allow 
> you to open a regular window completely offscreen. The requested position is 
> altered such that there's always just enough of it that remains visible and 
> available for grabbing and moving it to a better position.
> 
> Jaime: do you know what `self.ui.pos()` resolves to, 
> `QWindow::position()` or `QWindow::framePosition()`? I would not really 
> surprise me if trying to set the former (through `setPosition()`) gives 
> different results than using the latter (through `setFramePosition()`) in 
> case of offscreen co-ordinates.

Hi, the documentation here is lacking the description, but I guess it is 
position(), as there are other bindings for frameGeometry().
Thanks for checking it. I have no more objections.
Best regards.


- Jaime Torres


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126324/#review89665
---


On Dec. 14, 2015, 4:04 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126324/
> ---
> 
> (Updated Dec. 14, 2015, 4:04 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X and KDE Frameworks.
> 
> 
> Repository: kconfig
> 
> 
> Description
> ---
> 
> In KDElibs4, the KMainWindow::saveWindowSize() and 
> KMainWindow::restoreWindowSize() function saved and restored not only the 
> size but also the position (i.e. the geometry) of windows, using 
> QWidget::saveGeometry and QWidget::restoreGeometry.
> 
> 2 main reasons for this (according to the comments):
> - Under X11 restoring the position is tricky
> - X11 has a window manager which might be considered responsible for that 
> functionality (and I suppose most modern WMs have the feature enabled by 
> default?)
> 
> Both arguments are moot on MS Windows and OS X, and on both platforms users 
> expec

Re: Review Request 128466: Rename Checksums tab to Integrity

[Jaime Torres Amate]

> On July 18, 2016, 12:05 p.m., Sebastian Kügler wrote:
> > Please don't ship it, yet.
> > 
> > 
> > I find the UI illogical. There's a groupbox grouping the checksum buttons, 
> > but then you can input the checksum above, so essentially, the groupbox is 
> > unnecessary and confusing.
> > 
> > Perhaps the whole thing could be simplified by naming the tab "Checksums" 
> > and removing the groupbox altogether, as it's not providing any semantic 
> > value.
> > 
> > A usability reviewer should have a look.
> 
> Kai Uwe Broulik wrote:
> This dialog has been created in Review 128283 and Usability has been 
> involved from the beginning...
> 
> Sebastian Kügler wrote:
> It has changed in a significant way, though, making it illogical.
> 
> (Not that I understand the "Share" title in the original review, but 
> that's another matter.)
> 
> This needs more work.
> 
> Elvis Angelaccio wrote:
> > Perhaps the whole thing could be simplified by naming the tab 
> "Checksums" and removing the groupbox altogether, as it's not providing any 
> semantic value.
> 
> Preview here: https://share.kde.org/index.php/s/RUs9gAhIQqpFIqF
> 
> Sebastian Kügler wrote:
> This looks logical to me, and it's simpler: Very good!
> 
> (Take that as "sebas withdraws his objection" :))
> 
> Thomas Pfeiffer wrote:
> Clear -1 to removing the group box.
> 
> Here is tha rationale:
> For most "regular" users, only the lineedit at the top is relevant. The 
> calculate stuff is just distraction and - worse - potential confusion. If we 
> remove any visual distinction between the two, we just make the situation 
> worse for them.
> 
> I agree that calling the tab "Checksums" is still better, though, because 
> "Integrity" is too vague.
> 
> For the "average user" I just re-tested this with, what would actually 
> help her is creating a second box around the verification part, calling the 
> top one "Verify checksum" and the bottom "Calculate checksums".
> That way if she was told e.g. by a website to verify a checksum, she'd 
> knew she could simply ignore the whole calculation part.
> 
> Overall simplicity should not be the top priority here: The priority 
> should be to make it clear to users who just want to check whether a download 
> went okay which part they should care about and which they can ignore, while 
> still providing the calculation part for advanced users who want to do that.
> 
> Of course another option would be to split it in two tabs, but that might 
> make the tab bar quite long especially in languages like German.
> 
> Sebastian Kügler wrote:
> The latter part seems redundant then. How can you verify a checksum 
> without calculating it?
> 
> Thomas Pfeiffer wrote:
> See _that_ is why the bottom box was originally called "Share". Of course 
> the verification part also does calculation, but it does so without you 
> telling it to. You paste in the checksum, and it tells you whether it matches 
> or not.
> The manual calculation at the bottom is for people who share a file with 
> others and want to accompany it with a checksum for _them_ to verify it.
> 
> I think we might get away with "Calculate" anyway because those who don#t 
> know much about checksums don't need to know that the verify part calculates 
> as well, and those who know it should still be able to use the thing 
> correctly.
> 
> Sebastian Kügler wrote:
> That 'Share' title completely puzzled me, and I think I'm the kind of 
> user this dialog should work for very well. (I need to verify checksums all 
> the time.)
> 
> To be quite honest, from getting it explained, I get the strong 
> impression that you're overthinking it.
> 
> To me, the most logical would be:
> 
> * Calculate checksums at the top
> * Under that, the input field so I can c/p or type my checksum in there 
> to have it compared automatically. 
> 
> That's both, the order of the workflow as well as the logical order of 
> operation. 'Calculate' underneath would raise exactly the same question as I 
> put above: "...but but but ... it could not verify it without calculating it, 
> yet I have to hit a button to calculate ... maybe I should try again and 
> maybe I should just use the commandline to be sure". 
> 
> Point in case: for this kind of stuff, simplicity trumps since it makes 
> it easier to TRUST the dialog. I can't trust anything I don't fully 
> understand or have doubts about, and that's what the groupbox design and the 
> share title cause: doubts.
> 
> Ivan Čukić wrote:
> > See that is why the bottom box was originally called "Share".
> 
> When I saw the UI, it did not even occur to me that it behaves like this 
> comment suggested. I'd say that is a wrong thing to happen with an UI.
> 
> > To me, the most logical would be:
> >
> >Calculate checksums at the top
> >Under th

Review Request 128627: avoid crash starting kate with QT_FATAL_WARNINGS=1

[Jaime Torres Amate]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128627/
---

Review request for KDE Frameworks.


Repository: kio


Description
---

avoid connecting KUrl from null


Diffs
-

  src/widgets/kurlrequester.cpp 77e7c1d 

Diff: https://git.reviewboard.kde.org/r/128627/diff/


Testing
---

before the patch, there was 4 connect(null, .), that crashed within:

#0  0x7441f975 in __GI_raise (sig=sig@entry=6) at 
../sysdeps/unix/sysv/linux/raise.c:54
#1  0x74420d8a in __GI_abort () at abort.c:78
#2  0x750ca6e5 in QMessageLogger::warning(char const*, ...) const 
(context=..., message=...)
at global/qlogging.cpp:1648
#3  0x750ca6e5 in QMessageLogger::warning(char const*, ...) const 
(this=this@entry=0x7fffc270, msg=msg@entry=0x75412a58 
"QObject::connect: invalid null parameter") at global/qlogging.cpp:557
#4  0x752ec011 in QObjectPrivate::connectImpl(QObject const*, int, 
QObject const*, void**, QtPrivate::QSlotObjectBase*, Qt::ConnectionType, int 
const*, QMetaObject const*) (sender=sender@entry=0x0, signal_index=, receiver=receiver@entry=0xb7c970, slot=slot@entry=0x7fffc420, 
slotObj=slotObj@entry=0xb58500, type=Qt::AutoConnection, types=0x0, 
senderMetaObject=0x76a81900 ) at 
kernel/qobject.cpp:4674
#5  0x752ec2d2 in QObject::connectImpl(QObject const*, void**, QObject 
const*, void**, QtPrivate::QSlotObjectBase*, Qt::ConnectionType, int const*, 
QMetaObject const*) (sender=sender@entry=0x0, 
signal=signal@entry=0x7fffc410, receiver=receiver@entry=0xb7c970, 
slot=slot@entry=0x7fffc420, slotObj=0xb58500, type=Qt::AutoConnection, 
types=0x0, senderMetaObject=0x76a81900 )
at kernel/qobject.cpp:4658
#6  0x77ef2465 in KUrlRequester::KUrlRequesterPrivate::init() 
(type=Qt::AutoConnection, slot=
(void (KUrlRequester::*)(KUrlRequester * const, const QString &)) 
0x77eee1c0 , receiver=0xb7c970, 
signal=
(void (QLineEdit::*)(QLineEdit * const, const QString &)) 0x764d34b0 
, sender=0x0) at 
/usr/include/qt5/QtCore/qobject.h:239

applying the patch, kate starts (there is no connect from null)


Thanks,

Jaime Torres Amate

Re: Review Request 128627: avoid crash starting kate with QT_FATAL_WARNINGS=1

[Jaime Torres Amate]

> On Aug. 8, 2016, 12:53 p.m., David Faure wrote:
> > I don't get it, how can there be no lineedit in a KUrlRequester? Is this 
> > simply called too early?

Oh, I'm sorry I missed to copy part of the backtrace:

#7  0x77ef2465 in KUrlRequester::KUrlRequesterPrivate::init() 
(receiver=0xb7c970, this=0xa7c920)
at /g/5kde/frameworks/kio/src/widgets/kurlrequester.cpp:132
#8  0x77ef2465 in KUrlRequester::KUrlRequesterPrivate::init() 
(this=0xa7c920)
at /g/5kde/frameworks/kio/src/widgets/kurlrequester.cpp:330
#9  0x77ef2894 in KUrlRequester::KUrlRequester(QWidget*, QWidget*) 
(this=this@entry=0xb7c970, editWidget=editWidget@entry=0xb7c9c0, 
parent=parent@entry=0xc287e0)
at /g/5kde/frameworks/kio/src/widgets/kurlrequester.cpp:279
#10 0x77ef28d1 in KUrlComboRequester::KUrlComboRequester(QWidget*) 
(this=0xb7c970, parent=0xc287e0) at 
/g/5kde/frameworks/kio/src/widgets/kurlrequester.cpp:632


- Jaime Torres


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128627/#review98199
---


On Aug. 8, 2016, 7:38 a.m., Jaime Torres Amate wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128627/
> ---
> 
> (Updated Aug. 8, 2016, 7:38 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> avoid connecting KUrl from null
> 
> 
> Diffs
> -
> 
>   src/widgets/kurlrequester.cpp 77e7c1d 
> 
> Diff: https://git.reviewboard.kde.org/r/128627/diff/
> 
> 
> Testing
> ---
> 
> before the patch, there was 4 connect(null, .), that crashed within:
> 
> #0  0x7441f975 in __GI_raise (sig=sig@entry=6) at 
> ../sysdeps/unix/sysv/linux/raise.c:54
> #1  0x74420d8a in __GI_abort () at abort.c:78
> #2  0x750ca6e5 in QMessageLogger::warning(char const*, ...) const 
> (context=..., message=...)
> at global/qlogging.cpp:1648
> #3  0x750ca6e5 in QMessageLogger::warning(char const*, ...) const 
> (this=this@entry=0x7fffc270, msg=msg@entry=0x75412a58 
> "QObject::connect: invalid null parameter") at global/qlogging.cpp:557
> #4  0x752ec011 in QObjectPrivate::connectImpl(QObject const*, int, 
> QObject const*, void**, QtPrivate::QSlotObjectBase*, Qt::ConnectionType, int 
> const*, QMetaObject const*) (sender=sender@entry=0x0, signal_index= out>, receiver=receiver@entry=0xb7c970, slot=slot@entry=0x7fffc420, 
> slotObj=slotObj@entry=0xb58500, type=Qt::AutoConnection, types=0x0, 
> senderMetaObject=0x76a81900 ) at 
> kernel/qobject.cpp:4674
> #5  0x752ec2d2 in QObject::connectImpl(QObject const*, void**, 
> QObject const*, void**, QtPrivate::QSlotObjectBase*, Qt::ConnectionType, int 
> const*, QMetaObject const*) (sender=sender@entry=0x0, 
> signal=signal@entry=0x7fffc410, receiver=receiver@entry=0xb7c970, 
> slot=slot@entry=0x7fffc420, slotObj=0xb58500, type=Qt::AutoConnection, 
> types=0x0, senderMetaObject=0x76a81900 )
> at kernel/qobject.cpp:4658
> #6  0x77ef2465 in KUrlRequester::KUrlRequesterPrivate::init() 
> (type=Qt::AutoConnection, slot=
> (void (KUrlRequester::*)(KUrlRequester * const, const QString &)) 
> 0x77eee1c0 , 
> receiver=0xb7c970, signal=
> (void (QLineEdit::*)(QLineEdit * const, const QString &)) 0x764d34b0 
> , sender=0x0) at 
> /usr/include/qt5/QtCore/qobject.h:239
> 
> applying the patch, kate starts (there is no connect from null)
> 
> 
> Thanks,
> 
> Jaime Torres Amate
> 
>

Re: Review Request 128627: avoid crash starting kate with QT_FATAL_WARNINGS=1

[Jaime Torres Amate]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128627/
---

(Updated Aug. 8, 2016, 7:34 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks.


Changes
---

Submitted with commit bd235d2c54a7965f36d850ba14903a0541906583 by Jaime Torres 
to branch master.


Repository: kio


Description
---

avoid connecting KUrl from null


Diffs
-

  src/widgets/kurlrequester.cpp 77e7c1d 

Diff: https://git.reviewboard.kde.org/r/128627/diff/


Testing
---

before the patch, there was 4 connect(null, .), that crashed within:

#0  0x7441f975 in __GI_raise (sig=sig@entry=6) at 
../sysdeps/unix/sysv/linux/raise.c:54
#1  0x74420d8a in __GI_abort () at abort.c:78
#2  0x750ca6e5 in QMessageLogger::warning(char const*, ...) const 
(context=..., message=...)
at global/qlogging.cpp:1648
#3  0x750ca6e5 in QMessageLogger::warning(char const*, ...) const 
(this=this@entry=0x7fffc270, msg=msg@entry=0x75412a58 
"QObject::connect: invalid null parameter") at global/qlogging.cpp:557
#4  0x752ec011 in QObjectPrivate::connectImpl(QObject const*, int, 
QObject const*, void**, QtPrivate::QSlotObjectBase*, Qt::ConnectionType, int 
const*, QMetaObject const*) (sender=sender@entry=0x0, signal_index=, receiver=receiver@entry=0xb7c970, slot=slot@entry=0x7fffc420, 
slotObj=slotObj@entry=0xb58500, type=Qt::AutoConnection, types=0x0, 
senderMetaObject=0x76a81900 ) at 
kernel/qobject.cpp:4674
#5  0x752ec2d2 in QObject::connectImpl(QObject const*, void**, QObject 
const*, void**, QtPrivate::QSlotObjectBase*, Qt::ConnectionType, int const*, 
QMetaObject const*) (sender=sender@entry=0x0, 
signal=signal@entry=0x7fffc410, receiver=receiver@entry=0xb7c970, 
slot=slot@entry=0x7fffc420, slotObj=0xb58500, type=Qt::AutoConnection, 
types=0x0, senderMetaObject=0x76a81900 )
at kernel/qobject.cpp:4658
#6  0x77ef2465 in KUrlRequester::KUrlRequesterPrivate::init() 
(type=Qt::AutoConnection, slot=
(void (KUrlRequester::*)(KUrlRequester * const, const QString &)) 
0x77eee1c0 , receiver=0xb7c970, 
signal=
(void (QLineEdit::*)(QLineEdit * const, const QString &)) 0x764d34b0 
, sender=0x0) at 
/usr/include/qt5/QtCore/qobject.h:239

applying the patch, kate starts (there is no connect from null)


Thanks,

Jaime Torres Amate

D10989: Check for nullptr in indexForNode

[Jaime Torres Amate]

jtamate abandoned this revision.
jtamate added a comment.




REPOSITORY
  R241 KIO

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

To: jtamate, #frameworks, dfaure
Cc: kde-frameworks-devel, mpyne, LeGast00n, cblack, michaelh, ngraham, bruns

D11487: optimization of TextLineData::attribute

[Jaime Torres Amate]

jtamate added a comment.


  Uploaded a similar file F5761614: fake.xml.gz 

  It is created using http://interglacial.com/~sburke/pub/rtf2xml.html from a 
rtf with two images and some text.

REPOSITORY
  R39 KTextEditor

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

To: jtamate, #frameworks, #kate
Cc: dhaumann, mwolff, cullmann, michaelh, kevinapavew, ngraham, demsking, sars

D11487: optimization of TextLineData::attribute

[Jaime Torres Amate]

jtamate added a comment.


  I'm sorry, the previous file lines are 10 times longer than expected.
  This one {F5761669 }can be managed by 
kate, not only by okteta.

REPOSITORY
  R39 KTextEditor

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

To: jtamate, #frameworks, #kate
Cc: dhaumann, mwolff, cullmann, michaelh, kevinapavew, ngraham, demsking, sars

D11487: optimization of TextLineData::attribute

[Jaime Torres Amate]

jtamate added a comment.


  In D11487#231110 , @mwolff wrote:
  
  > Considering spell checking is involved - can you show a screenshot for how 
the file looks like for you? There shouldn't be a lot of spell checking going 
on, or so I hope...
  
  
  F5762231: fake_first.png  opening the 
file and accepting to extend the line limit.
  F5762233: fake_last.png  after pressing 
ctrl+end

REPOSITORY
  R39 KTextEditor

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

To: jtamate, #frameworks, #kate
Cc: anthonyfieroni, dhaumann, mwolff, cullmann, michaelh, kevinapavew, ngraham, 
demsking, sars

D11487: optimization of TextLineData::attribute

[Jaime Torres Amate]

jtamate added a comment.


  In D11487#231522 , @mwolff wrote:
  
  > @jtamate looking at your screenshots, it represents closely what I see 
locally. Most notably, there are no red underlines in your screenshots which 
could arise due to spell checking. Thus I really wonder why you are seeing such 
a big hotspot there.
  >
  > Try perf, or try a poor mans profiler like GDB and regularly interrupt. Do 
you really end up in `TextLineData::attribute()`? Or, alternatively: Measure 
the time it takes for kate/kwrite to open the file and then go to the end. Then 
compare this before and after your patch. Do you see anything in the order of 
~75% reduction for the time then too? Note how callgrind only measure 
instructions, so a supposed reduction of 75% of instructions should certainly 
have an impact on time too - of course not 75% too... I simply cannot fathom 
why you are seeing such an impact but I cannot reproduce this at all!
  
  
  I've done some measurements, as the times are so big, with a stopwatch 2 
times each test.
  With "Enable autodetection of Language" and "Automatic spell checking enabled 
by default" enabled,
  the test as before: since pressing "Temporarily raise limit and reload file", 
press Ctrl+end and finish the scroll to the end of the document.
  
  without any version of the patch:
  1 min 25 seconds 
  With @mwolf solution, used only in spellCheckWrtHighlightingRanges.
  38 seconds
  With the binary search:
  34 seconds

REPOSITORY
  R39 KTextEditor

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

To: jtamate, #frameworks, #kate
Cc: anthonyfieroni, dhaumann, mwolff, cullmann, michaelh, kevinapavew, ngraham, 
demsking, sars

D11604: kdirlistertest doesn't fail at random

[Jaime Torres Amate]

jtamate created this revision.
jtamate added reviewers: Frameworks, dfaure.
Restricted Application added a project: Frameworks.
jtamate requested review of this revision.

REVISION SUMMARY
  - Instead of testing the creation and deletion of one file, do it with 1000 
files.
  - test the mime type with the on in the QMimeDatabase for an .html file.
  - increase some wait times.

TEST PLAN
  Repeating the test several times, it failed sometimes due to timing problems, 
doesn't fail any more for me.

REPOSITORY
  R241 KIO

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

AFFECTED FILES
  autotests/kdirlistertest.cpp
  autotests/kdirlistertest.h

To: jtamate, #frameworks, dfaure
Cc: michaelh, ngraham

D11487: optimization of TextLineData::attribute

[Jaime Torres Amate]

jtamate added a comment.


  > One question to that though: Why do you sort/lookup by `x.offset + x.length 
<= p`? Note how lower_bound returns the first iterator that is _not_ going to 
return true.
  
  Assuming there are neither overlaps nor unsorted entries.
  Lets call X the iterator returned by lower_bound, suppose X is not cend(), so 
X.offset + X.length > p. 
  If other iterator Y could satisfy Y.offset + Y.length > p and Y.offset <= 
X.offset, it means there are overlaps, contradiction.
  Therefore,  the rest of the iterators has the following properties:
  Y.offset + Y.length > p and Y.offset > X.offset
  or
  Y.offset + Y.length <= p and Y.offset < X.offset
  
  > To me, it looks like your code cannot actually work and will always return 
0?
  
  Nope. Otherwise the tests fail, more precisely kateindenttest.
  
  > Personally, I'd try to use `upper_bound` with `x.offset < p` in the 
comparison. The iterator should then point to the first item that has it's 
offset larger than `p`. So decrementing the iterator once (while checking 
against `begin()`) yields the iterator that could potentially match. Thus, 
check if `p` is contained in its range and if so return it's attribute, 
otherwise return 0.
  
  I've tried. The code gets uglier.
  
  > Besides this: I am still looking for an explanation why spell checking is 
so extremely slow for you. I have the same settings enabled, and spell checking 
is seemingly fast for me... Am I missing some dictionary or something other to 
reproduce this?
  
  Perhaps some Ignored words? I have Amarok, KAddressBook, KDevelop, KHTML, 
KIO, 
  
  > Also, what is "@mwolf solution"
  
  we can return the iterator and take it as an argument again. I tried locally 
the python style, returning a QPair.
  
  > Can you try that locally and see how it goes for you?
  
  I only get one second less, 33 seconds. The problem is that attribute() is 
called from more places. Is it worth to have two implementations?

REPOSITORY
  R39 KTextEditor

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

To: jtamate, #frameworks, #kate
Cc: anthonyfieroni, dhaumann, mwolff, cullmann, michaelh, kevinapavew, ngraham, 
demsking, sars

D11604: kdirlistertest doesn't fail at random

[Jaime Torres Amate]

jtamate updated this revision to Diff 30323.
jtamate added a comment.


  QStringLiteral instead of QString.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11604?vs=30285&id=30323

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

AFFECTED FILES
  autotests/kdirlistertest.cpp
  autotests/kdirlistertest.h

To: jtamate, #frameworks, dfaure
Cc: apol, michaelh, ngraham

D11487: optimization of TextLineData::attribute

[Jaime Torres Amate]

jtamate updated this revision to Diff 30331.
jtamate edited the summary of this revision.
jtamate added a comment.


  Using upper_bound with <
  Changed the name of the iterator
  Using -> instead of (*x)
  
  These are the calls I get since I open the document:
  doc= KTextEditor::DocumentPrivate(0x228fe10)  range= [ (0, 0)  ->  (0, 0) ]  
dictionary= "" singleLine= true returnSingleRange= false
  doc= KTextEditor::DocumentPrivate(0x228fe10)  range= [ (1, 0)  ->  (1, 4096) 
]  dictionary= "" singleLine= true returnSingleRange= false
  doc= KTextEditor::DocumentPrivate(0x228fe10)  range= [ (0, 0)  ->  (0, 4090) 
]  dictionary= "" singleLine= true returnSingleRange= false
  sonnet.core: Missing trigrams for languages: QSet("en_CA", "en_GB", "en_AU")
  doc= KTextEditor::DocumentPrivate(0x228fe10)  range= [ (0, 0)  ->  (0, 
358720) ]  dictionary= "" singleLine= true returnSingleRange= false
  doc= KTextEditor::DocumentPrivate(0x228fe10)  range= [ (5, 0)  ->  (5, 
162682) ]  dictionary= "" singleLine= true returnSingleRange= false

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11487?vs=30134&id=30331

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

AFFECTED FILES
  src/buffer/katetextline.cpp
  src/buffer/katetextline.h

To: jtamate, #frameworks, #kate
Cc: anthonyfieroni, dhaumann, mwolff, cullmann, michaelh, kevinapavew, ngraham, 
demsking, sars

D11487: optimization of TextLineData::attribute

[Jaime Torres Amate]

This revision was automatically updated to reflect the committed changes.
jtamate marked 2 inline comments as done.
Closed by commit R39:787318967fce: optimization of TextLineData::attribute 
(authored by jtamate).

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11487?vs=30331&id=30334

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

AFFECTED FILES
  src/buffer/katetextline.cpp
  src/buffer/katetextline.h

To: jtamate, #frameworks, #kate, cullmann
Cc: anthonyfieroni, dhaumann, mwolff, cullmann, michaelh, kevinapavew, ngraham, 
demsking, sars

D11487: optimization of TextLineData::attribute

[Jaime Torres Amate]

jtamate added a comment.


  In D11487#232497 , @mwolff wrote:
  
  > @jtamate I just checked, the function is called with the same parameters 
for me locally. What output do you get for this:
  >
  > For me this is the interesting bit:
  >
  >   15.097 debug: unknown[unknown:0]: [ (0, 0)  ->  (0, 358720) ] "" true 
false
  >   15.097 debug: unknown[unknown:0]: 0 1979 358720
  >   23.013 debug: unknown[unknown:0]: [ (5, 0)  ->  (5, 162682) ] "" true 
false
  >   23.013 debug: unknown[unknown:0]: 5 12 162682
  >   
  
  
  
  
  > So for line 0 I should actually get quite abysmal performance too, worst 
case 1979 * 358720, but it flies right through here... Now I wonder what kind 
of CPU you are using? I have a i7-5600U CPU @ 2.60GHz here in my laptop, which 
has:
  > 
  >   $ lstopo --of console
  >   Machine (11GB)
  > Package L#0 + L3 L#0 (4096KB)
  >   L2 L#0 (256KB) + L1d L#0 (32KB) + L1i L#0 (32KB) + Core L#0
  > PU L#0 (P#0)
  > PU L#1 (P#1)
  >   L2 L#1 (256KB) + L1d L#1 (32KB) + L1i L#1 (32KB) + Core L#1
  > PU L#2 (P#2)
  > PU L#3 (P#3)
  > 
  > 
  > The 1979 entries of attributes (each 12byte large) produce ~24KB of data, 
which fits nicely within my L1 cache. Maybe I'm just lucky and this is what 
hides this issue from me?
  
  Same debug output.
  
  AMD Phenom(tm) II X6 1100T Processor, 3.30 GHz
  Machine (16GB)
  
Package L#0 + L3 L#0 (6144KB)
  L2 L#0 (512KB) + L1d L#0 (64KB) + L1i L#0 (64KB) + Core L#0 + PU L#0 (P#0)
  L2 L#1 (512KB) + L1d L#1 (64KB) + L1i L#1 (64KB) + Core L#1 + PU L#1 (P#1)
  L2 L#2 (512KB) + L1d L#2 (64KB) + L1i L#2 (64KB) + Core L#2 + PU L#2 (P#2)
  L2 L#3 (512KB) + L1d L#3 (64KB) + L1i L#3 (64KB) + Core L#3 + PU L#3 (P#3)
  L2 L#4 (512KB) + L1d L#4 (64KB) + L1i L#4 (64KB) + Core L#4 + PU L#4 (P#4)
  L2 L#5 (512KB) + L1d L#5 (64KB) + L1i L#5 (64KB) + Core L#5 + PU L#5 (P#5)

REPOSITORY
  R39 KTextEditor

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

To: jtamate, #frameworks, #kate, cullmann
Cc: anthonyfieroni, dhaumann, mwolff, cullmann, michaelh, kevinapavew, ngraham, 
demsking, sars

D11811: avoid Asan runtime error: shift exponent -1 is negative

[Jaime Torres Amate]

jtamate created this revision.
jtamate added reviewers: Kate, Frameworks.
Restricted Application added projects: Kate, Frameworks.
jtamate requested review of this revision.

REVISION SUMMARY
  Avoid doing a 1<<-1.

TEST PLAN
  Before: 
frameworks/ktexteditor/src/completion/katecompletionconfig.cpp:195:33: runtime 
error: shift exponent -1 is negative
  after: silence

REPOSITORY
  R39 KTextEditor

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

AFFECTED FILES
  src/completion/katecompletionconfig.cpp

To: jtamate, #kate, #frameworks
Cc: michaelh, kevinapavew, ngraham, demsking, cullmann, sars, dhaumann

D11487: optimization of TextLineData::attribute

[Jaime Torres Amate]

jtamate added a comment.


  In D11487#232555 , @mwolff wrote:
  
  > WTF :D Your desktop CPU has clearly a better performance than my mobile 
CPU, no? Is AMD really so much worse here? How can that be - I don't get it :D
  >
  > Anyhow, I give up trying to understand this now - thanks a lot for your 
repeated input Jaime!
  
  
  I've found the problem for the poor performance. In .kdesrc-buildrc, I have 
also a like cxxflags # -fsanitize=address -fsanitize=signed-integer-overflow  
-fsanitize=bounds-strict -fsanitize=undefined -fsanitize-recover=address
  That line removes the -O2 from the compiler options. Added manually in the 
way of cxxflags -O2 -mtune=native # -fsanitize=address .
  I "only" get 10 seconds less, 22 seconds with the patch applied.

REPOSITORY
  R39 KTextEditor

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

To: jtamate, #frameworks, #kate, cullmann
Cc: anthonyfieroni, dhaumann, mwolff, cullmann, michaelh, kevinapavew, ngraham, 
demsking, sars

D11811: avoid Asan runtime error: shift exponent -1 is negative

[Jaime Torres Amate]

jtamate updated this revision to Diff 31271.
jtamate added a comment.


  Implemented mwolf solution.

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11811?vs=30952&id=31271

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

AFFECTED FILES
  src/completion/katecompletionconfig.cpp

To: jtamate, #kate, #frameworks
Cc: mwolff, brauch, michaelh, kevinapavew, ngraham, demsking, cullmann, sars, 
dhaumann

D11811: avoid Asan runtime error: shift exponent -1 is negative

[Jaime Torres Amate]

This revision was automatically updated to reflect the committed changes.
Closed by commit R39:3e8bbe0ad838: avoid Asan runtime error: shift exponent -1 
is negative (authored by jtamate).

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11811?vs=31271&id=31312

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

AFFECTED FILES
  src/completion/katecompletionconfig.cpp

To: jtamate, #kate, #frameworks, mwolff
Cc: mwolff, brauch, michaelh, kevinapavew, ngraham, demsking, cullmann, sars, 
dhaumann

D12014: [kcoreaddons] convert to new connect syntax

[Jaime Torres Amate]

jtamate created this revision.
jtamate added a reviewer: dfaure.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.
jtamate requested review of this revision.

REVISION SUMMARY
  Convert to the new connect syntax.
  As the job2 parameter was not used, remove it.

REPOSITORY
  R244 KCoreAddons

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

AFFECTED FILES
  src/lib/jobs/kjobuidelegate.cpp
  src/lib/jobs/kjobuidelegate.h

To: jtamate, dfaure
Cc: #frameworks, michaelh, ngraham, bruns

D12016: [ktexteditor] much faster positionFromCursor

[Jaime Torres Amate]

jtamate created this revision.
jtamate added a reviewer: Kate.
Restricted Application added projects: Kate, Frameworks.
Restricted Application added a subscriber: Frameworks.
jtamate requested review of this revision.

REVISION SUMMARY
  Use static variables to store information about last call.
  As most of the time the cursor is close to the previous position, avoid to 
calculate every time the size of the lines from the beginning of the document, 
just calculate it from the previous cursor position.

TEST PLAN
  Open a callgrind log file with 2.840.605 lines
  Go to the end of the file, and then play with the PageUp, PageDown and cursor 
keys.
  Before: it was unable to do continuous PageUp refreshes.
  After: the PageUp at any part of the file as as fast as in the beginning of 
the document.

REPOSITORY
  R39 KTextEditor

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

AFFECTED FILES
  src/view/kateviewaccessible.h

To: jtamate, #kate
Cc: #frameworks, michaelh, kevinapavew, ngraham, bruns, demsking, cullmann, 
sars, dhaumann

D12014: [kcoreaddons] convert to new connect syntax

[Jaime Torres Amate]

This revision was automatically updated to reflect the committed changes.
Closed by commit R244:ae47ecdb2839: [kcoreaddons] convert to new connect syntax 
(authored by jtamate).

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12014?vs=31570&id=31653

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

AFFECTED FILES
  src/lib/jobs/kjobuidelegate.cpp
  src/lib/jobs/kjobuidelegate.h

To: jtamate, dfaure
Cc: #frameworks, michaelh, ngraham, bruns

D12016: [ktexteditor] much faster positionFromCursor

[Jaime Torres Amate]

jtamate updated this revision to Diff 31733.
jtamate edited the summary of this revision.
jtamate edited the test plan for this revision.
jtamate added a reviewer: Frameworks.
jtamate added a comment.


  Cached the position in static variables of KateViewAccessible.
  The cache is invalidated when the signal Document::textChanged is received.
  Unfortunately, KateViewAccessible must inherit also QObject or connect will 
not work, because KateViewAccessible didn't inherit QObject.
  **Is this change Binary compatible?**
  
  At first I thought the changes will come in setText, but in my tests it has 
never been called.
  
  To check the correctness of the result, I've run the fast and slow paths in 
kate, with several windows over the same document, changing it in sereveral 
ways, and the results always match.

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12016?vs=31579&id=31733

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

AFFECTED FILES
  src/CMakeLists.txt
  src/view/kateviewaccessible.cpp
  src/view/kateviewaccessible.h

To: jtamate, #kate, cullmann, #frameworks
Cc: cullmann, #frameworks, michaelh, kevinapavew, ngraham, bruns, demsking, 
sars, dhaumann

D12016: [ktexteditor] much faster positionFromCursor

[Jaime Torres Amate]

jtamate added a comment.


  In D12016#243079 , @cullmann wrote:
  
  > Hi, must the variable be static or would an object member be good enough?
  
  
  It can't be a member object, because some methods that call 
positionFromCursor are const. I tried :-(

REPOSITORY
  R39 KTextEditor

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

To: jtamate, #kate, cullmann, #frameworks
Cc: cullmann, #frameworks, michaelh, kevinapavew, ngraham, bruns, demsking, 
sars, dhaumann

D12016: [ktexteditor] much faster positionFromCursor

[Jaime Torres Amate]

jtamate updated this revision to Diff 31737.
jtamate edited the summary of this revision.
jtamate added a comment.


  Implemented now as object method instead of static.
  Changed the calls from KateViewInternal from static to use the reference the 
method QAccessible::queryAccessibleInterface(this) returns.

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12016?vs=31733&id=31737

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

AFFECTED FILES
  src/view/kateviewaccessible.h
  src/view/kateviewinternal.cpp

To: jtamate, #kate, cullmann, #frameworks
Cc: cullmann, #frameworks, michaelh, kevinapavew, ngraham, bruns, demsking, 
sars, dhaumann

D12016: [ktexteditor] much faster positionFromCursor

[Jaime Torres Amate]

jtamate updated this revision to Diff 31760.
jtamate added a comment.


  No QObject inheritance, only a QMetaObject::Connection member to disconnect 
the connection.
  As this is a non exported class, use the same method name and make it 
non-static.
  KateViewInternal::cursorMoved checks if QAccessible::isActive().

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12016?vs=31737&id=31760

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

AFFECTED FILES
  src/view/kateviewaccessible.h
  src/view/kateviewinternal.cpp

To: jtamate, #kate, cullmann, #frameworks
Cc: brauch, cullmann, #frameworks, michaelh, kevinapavew, ngraham, bruns, 
demsking, sars, dhaumann

D10702: Always use a job to delete files to avoid freezing process waiting on IO

[Jaime Torres Amate]

jtamate added a comment.


  This freezing process happens for me in ktorrent the first time in a day I 
delete a file, but not the files after, even if they are iso images (>4GiB).
  Are we sure the problem is here and not in the notification step, for example 
loading the sound data and starting the sound processor?
  Does the freezing process happens if you delete a second big file?

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure, ngraham, #dolphin, jtamate
Cc: jtamate, markg, ngraham, #frameworks, michaelh, bruns

D12095: convert setDirLister to the new connect syntax

[Jaime Torres Amate]

jtamate created this revision.
jtamate added reviewers: dfaure, Frameworks.
Restricted Application added a project: Frameworks.
jtamate requested review of this revision.

REVISION SUMMARY
  The only tricky part is the use of QOverload<>::of to distinguish between 
signals with same name but different signatures.

TEST PLAN
  kdirmodeltest passes all tests.

REPOSITORY
  R241 KIO

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

AFFECTED FILES
  src/widgets/kdirmodel.cpp

To: jtamate, dfaure, #frameworks
Cc: michaelh, ngraham, bruns

D12095: convert setDirLister to the new connect syntax

[Jaime Torres Amate]

jtamate updated this revision to Diff 31851.
jtamate added a comment.


  Someday, in a not so far future, I'll remember to always look for private 
slot definitions.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12095?vs=31830&id=31851

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

AFFECTED FILES
  src/widgets/kdirmodel.cpp
  src/widgets/kdirmodel.h

To: jtamate, dfaure, #frameworks, apol
Cc: apol, michaelh, ngraham, bruns

D12016: [ktexteditor] much faster positionFromCursor

[Jaime Torres Amate]

jtamate updated this revision to Diff 31853.
jtamate marked 5 inline comments as done.
jtamate added a comment.


  Included more comments.
  Addressed style coding.
  
  In a callgrind log file with 1606189 lines, at the end of the file, pressing 
PageUp, using the code included at the end:
  
  old implementation elapsed =  122
  new implementation elapsed =  0
  old implementation elapsed =  119
  new implementation elapsed =  0
  
  After some insertions:
  old implementation elapsed =  137
  new implementation elapsed =  118
  old implementation elapsed =  123
  new implementation elapsed =  0
  
  - src/view/kateviewaccessible.h
  
  +++ src/view/kateviewaccessible.h
  @@ -29,6 +29,8 @@
   #include 
   #include 
   #include 
  +#include 
  +#include 
  
  /**
  
  - This class implements a QAccessible-interface for a KateViewInternal.
  
  @@ -194,6 +196,9 @@ public:
  
 */
int positionFromCursor(KateViewInternal *view, const KTextEditor::Cursor 
&cursor) const
{
  
  +QElapsedTimer t;
  +t.start();
  +
  
int pos = m_lastPosition;
const auto *doc = view->view()->document();
 
  
  @@ -229,6 +234,22 @@ public:
  
m_lastCursor = cursor;
m_lastPosition = pos;
 
  
  +qWarning() << "new implementation elapsed = " << t.restart();
  +// previous implementation
  +int _pos = 0;
  +for (int _line = 0; _line < cursor.line(); ++_line) {
  +// length of the line plus newline
  +_pos += view->view()->document()->line(_line).size() + 1;
  +}
  +_pos += cursor.column();
  +qWarning() << "old implementation elapsed = " << t.elapsed();
  +
  +return pos;
  +if (_pos != (pos + cursor.column())) {
  +qWarning() << "implementations differ, old=" << _pos << " new=" 
<<
  +pos + cursor.column();
  +}
  +
  
return pos + cursor.column();
}

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12016?vs=31760&id=31853

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

AFFECTED FILES
  src/view/kateviewaccessible.h
  src/view/kateviewinternal.cpp

To: jtamate, #kate, cullmann, #frameworks
Cc: mwolff, brauch, cullmann, #frameworks, michaelh, kevinapavew, ngraham, 
bruns, demsking, sars, dhaumann

D12016: [ktexteditor] much faster positionFromCursor

[Jaime Torres Amate]

jtamate marked an inline comment as done.
jtamate added a comment.




INLINE COMMENTS

> mwolff wrote in kateviewaccessible.h:191
> I know the old code used int already, but shouldn't this better be a quint64 
> as it's a file offset?

All the methods of QAccesibeTextInrface 
 expect an int.
What should be done if the document has more data than INT_MAX?
deactivate the class and show a warning? In any case, I think this is out of 
scope of this patch.

REPOSITORY
  R39 KTextEditor

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

To: jtamate, #kate, cullmann, #frameworks
Cc: mwolff, brauch, cullmann, #frameworks, michaelh, kevinapavew, ngraham, 
bruns, demsking, sars, dhaumann

D12095: convert setDirLister to the new connect syntax

[Jaime Torres Amate]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:940d2763e41b: convert setDirLister to the new connect 
syntax (authored by jtamate).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12095?vs=31851&id=31907

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

AFFECTED FILES
  src/widgets/kdirmodel.cpp
  src/widgets/kdirmodel.h

To: jtamate, dfaure, #frameworks, apol
Cc: apol, michaelh, ngraham, bruns

D11013: Detect incorrect paramenter in findProtocol

[Jaime Torres Amate]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:287093136efc: Detect incorrect paramenter in findProtocol 
(authored by jtamate).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D11013?vs=28679&id=32168#toc

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11013?vs=28679&id=32168

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

AFFECTED FILES
  src/core/kprotocolinfofactory.cpp

To: jtamate, #frameworks, dfaure
Cc: michaelh, ngraham, bruns

D12228: kdirlister new connect syntax

[Jaime Torres Amate]

jtamate created this revision.
jtamate added reviewers: dfaure, Frameworks.
Restricted Application added a project: Frameworks.
jtamate requested review of this revision.

REVISION SUMMARY
  (partial patch)
  There is a problem converting KCoreDirLister to the new connect syntax:
  
  In void KCoreDirLister::Private::connectJob(KIO::ListJob *job) (line 2724),
  In the old syntax it is using **private signals** from KJob without problems.
  But in the new syntax this is detected by the compiler and it does not 
compile.
  
  One solution is to put a big TODO for KF6.
  Another solution could be to make the signals public.
  Is there another solution?

TEST PLAN
  Try to compile, then pass the tests.

REPOSITORY
  R241 KIO

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

AFFECTED FILES
  src/core/kcoredirlister.cpp
  src/core/kcoredirlister_p.h

To: jtamate, dfaure, #frameworks
Cc: michaelh, ngraham, bruns

D12242: KJob public signals

[Jaime Torres Amate]

jtamate created this revision.
jtamate added reviewers: dfaure, Frameworks.
Restricted Application added a project: Frameworks.
jtamate requested review of this revision.

REVISION SUMMARY
  KIO::KCoreDirLister connects to those signals, they must be public so the new 
connect syntax compiles.

TEST PLAN
  kio compiles with new connect syntax.

REPOSITORY
  R244 KCoreAddons

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

AFFECTED FILES
  src/lib/jobs/kjob.h

To: jtamate, dfaure, #frameworks
Cc: michaelh, ngraham, bruns

D12242: KJob public signals

[Jaime Torres Amate]

jtamate updated this revision to Diff 32267.
jtamate added a comment.


  Changed from "can't" to "shouldn't".

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12242?vs=32258&id=32267

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

AFFECTED FILES
  src/lib/jobs/kjob.h

To: jtamate, dfaure, #frameworks
Cc: michaelh, ngraham, bruns

D10702: Always use a job to delete files to avoid freezing process waiting on IO

[Jaime Torres Amate]

jtamate added a comment.


  > The current state is that it freezes as long as the file deletion (unlink) 
lasts.
  
  Ok, let's go for it.
  
  Another use case when this could be useful is when the filesystem is slow, 
one possible way to know if this is the case is using something like
  
bool KFileItemPrivate::isSlow() const

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure, ngraham, #dolphin, jtamate
Cc: jtamate, markg, ngraham, #frameworks, michaelh, bruns

D12242: KJob public signals

[Jaime Torres Amate]

This revision was automatically updated to reflect the committed changes.
Closed by commit R244:c0b312a41f7f: KJob public signals (authored by jtamate).

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12242?vs=32267&id=32307

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

AFFECTED FILES
  src/lib/jobs/kjob.h

To: jtamate, dfaure, #frameworks
Cc: michaelh, ngraham, bruns

D12228: kdirlister new connect syntax

[Jaime Torres Amate]

jtamate updated this revision to Diff 32362.
jtamate edited the summary of this revision.
jtamate edited the test plan for this revision.
jtamate added a comment.


  I have renamed one private slot because I didn't find a better solution.
  QOverload seems to work only for signals, not for slots.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12228?vs=32195&id=32362

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

AFFECTED FILES
  src/core/kcoredirlister.cpp
  src/core/kcoredirlister_p.h

To: jtamate, dfaure, #frameworks
Cc: michaelh, ngraham, bruns

D12228: kdirlister new connect syntax

[Jaime Torres Amate]

jtamate edited the summary of this revision.

REPOSITORY
  R241 KIO

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

To: jtamate, dfaure, #frameworks
Cc: michaelh, ngraham, bruns

D12333: Put the open/save dialog's toolbar above all other widgets, like Dolphin does

[Jaime Torres Amate]

jtamate added a comment.


  Lately, the zoom widget is placed at the bottom in most programs, perhaps it 
can be moved at the left of the Save/Open Cancel buttons.
  If the mouse distance is a problem, the navigation buttons can be placed at 
the top right, just where the zoom widget is now.
  This is  just another idea.

REPOSITORY
  R241 KIO

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

To: ngraham, #frameworks, #dolphin, #vdg
Cc: jtamate, broulik, anemeth, rkflx, michaelh, bruns

D12233: Avoid manipulation of lists with quadratic complexity

[Jaime Torres Amate]

jtamate added inline comments.

INLINE COMMENTS

> pendingfilequeue.cpp:68
> +const auto startRemoving = std::partition(m_cache.begin(), end, 
> isDescendant);
> +for (auto it = startRemoving; it != end; it++) {
> +m_pendingFiles.remove(it->path());

According to std::partition 

what matches isDescendant should be from m_cache.begin() up to startRemoving, 
unless you want to remove what does not match isDescendant, isn't it?

REPOSITORY
  R293 Baloo

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

To: michaelh, #baloo
Cc: jtamate, bruns, #frameworks, ashaposhnikov, michaelh, astippich, spoorun

D12371: fix always reproducible crash

[Jaime Torres Amate]

jtamate created this revision.
jtamate added reviewers: dfaure, Frameworks.
Restricted Application added a project: Frameworks.
jtamate requested review of this revision.

REVISION SUMMARY
  Don't take ownership of KDirLister in KDirModel.
  When KDirModel is destroyed, it also deleted the dirlister, but 
KCoreDirListerCache didn't knew anything about this deletion, and when it 
consulted listersCurrentlyHolding in slotRedirection resulted always in a crash.
  
  As a side effect, it also fixes the crash I got in D10989 


TEST PLAN
  With samba started locally.
  Execute kwrite
  Press "save as" and go to network, then Samba shared resources, wait for the 
error. Then change to smb://127.0.0.1 and press enter, select a place to save 
the file in the local samba.
  Again press "save as" and go to network, then Samba shared resources. 
  Previously, always crash. 
  Now, again the error message (Next thing to investigate).
  
  In my [limited] tests this doens't introduce memory leaks.

REPOSITORY
  R241 KIO

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

AFFECTED FILES
  src/widgets/kdirmodel.cpp
  src/widgets/kdirmodel.h

To: jtamate, dfaure, #frameworks
Cc: michaelh, bruns

D12371: fix always reproducible crash

[Jaime Torres Amate]

jtamate updated this revision to Diff 32640.
jtamate edited the summary of this revision.
jtamate edited the test plan for this revision.
jtamate added a comment.


  Implemented as Aleix suggested, using QPointer.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12371?vs=32626&id=32640

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

AFFECTED FILES
  src/core/kcoredirlister.cpp
  src/core/kcoredirlister_p.h

To: jtamate, dfaure, #frameworks
Cc: apol, michaelh, bruns

D12371: fix always reproducible crash

[Jaime Torres Amate]

jtamate added a comment.


  In D12371#250366 , @apol wrote:
  
  > ^^' this won't work though. If the object gets deleted you will get a null 
pointer in the list.
  
  
  If the QPointer kdl detects that the guarded pointer is invalid it is removed 
from the list.
  
if (kdl.isNull()) {
curHolders.removeAll(kdl);
}
  
  I'm unable to create the crash.

REPOSITORY
  R241 KIO

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

To: jtamate, dfaure, #frameworks, apol
Cc: apol, michaelh, bruns

D12228: kdirlister new connect syntax

[Jaime Torres Amate]

jtamate updated this revision to Diff 32656.
jtamate edited the summary of this revision.
jtamate edited the test plan for this revision.
jtamate added a comment.


  Change the first connect.
  No need to change the name of the slot.
  Pass the plain error message.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12228?vs=32362&id=32656

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

AFFECTED FILES
  src/core/kcoredirlister.cpp

To: jtamate, dfaure, #frameworks
Cc: michaelh, bruns

D12228: kdirlister new connect syntax

[Jaime Torres Amate]

jtamate updated this revision to Diff 32658.
jtamate added a comment.


  Added space.
  Removed SLOT.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12228?vs=32656&id=32658

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

AFFECTED FILES
  src/core/kcoredirlister.cpp
  src/core/kcoredirlister_p.h

To: jtamate, dfaure, #frameworks
Cc: michaelh, bruns

D12228: kdirlister new connect syntax

[Jaime Torres Amate]

jtamate updated this revision to Diff 32661.
jtamate added a comment.


  Removed Q_PRIVATE_SLOT. Didn't saw them again. :-(

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12228?vs=32658&id=32661

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

AFFECTED FILES
  src/core/kcoredirlister.cpp
  src/core/kcoredirlister.h

To: jtamate, dfaure, #frameworks
Cc: michaelh, bruns

D12228: kdirlister new connect syntax

[Jaime Torres Amate]

jtamate updated this revision to Diff 32663.
jtamate added a comment.


  Removed comment.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12228?vs=32661&id=32663

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

AFFECTED FILES
  src/core/kcoredirlister.cpp
  src/core/kcoredirlister.h

To: jtamate, dfaure, #frameworks
Cc: michaelh, bruns

D12371: fix always reproducible crash

[Jaime Torres Amate]

jtamate added a comment.


  More investigation, in bold what causes the problem.
  
  moveListersWithoutCachedItemsJob append  KDirLister(0x3b510c70) to holders 
for url= QUrl("file:///virtual/kde5/5kde/build/frameworks/kio")
  forgetDirs removeAll in directoryData[ 
"file:///virtual/kde5/5kde/build/frameworks/kio" ] for KDirLister(0x3b510c70)
  moveListersWithoutCachedItemsJob append  KDirLister(0x3b510c70) to holders 
for url= QUrl("remote:/")
  forgetDirs removeAll in directoryData[ "remote:/" ] for KDirLister(0x3b510c70)
  **forgetDirs not found: "smb://" in directoryData for KDirLister(0x3b510c70) 
**
  moveListersWithoutCachedItemsJob append  KDirLister(0x3b510c70) to holders 
for url= QUrl("smb:///")
  **moveListersWithoutCachedItemsJob append  KDirLister(0x3b510c70) to holders 
for url= QUrl("smb://") **
  forgetDirs removeAll in directoryData[ "smb:///" ] for KDirLister(0x3b510c70)
  moveListersWithoutCachedItemsJob append  KDirLister(0x3b510c70) to holders 
for url= QUrl("smb://127.0.0.1/")
  forgetDirs removeAll in directoryData[ "smb://127.0.0.1/" ] for 
KDirLister(0x3b510c70)
  moveListersWithoutCachedItemsJob append  KDirLister(0x3b510c70) to holders 
for url= QUrl("smb://127.0.0.1/kk")
  forgetDirs removeAll in directoryData[ "smb://127.0.0.1/kk" ] for 
KCoreDirLister(0x3b510c70)
  forgetCachedItemsJob append  KDirLister(0x4e273400) to holders for url= 
QUrl("smb://127.0.0.1/kk")
  forgetDirs removeAll in directoryData[ "smb://127.0.0.1/kk" ] for 
KDirLister(0x4e273400)
  forgetCachedItemsJob append  KDirLister(0x4e273400) to holders for url= 
QUrl("file:///virtual/kde5/5kde/build/frameworks/kio")
  forgetDirs removeAll in directoryData[ 
"file:///virtual/kde5/5kde/build/frameworks/kio" ] for KDirLister(0x4e273400)
  moveListersWithoutCachedItemsJob append  KDirLister(0x4e273400) to holders 
for url= QUrl("smb://127.0.0.1/kk")
  forgetDirs removeAll in directoryData[ "smb://127.0.0.1/kk" ] for 
KDirLister(0x4e273400)
  forgetCachedItemsJob append  KDirLister(0x4e273400) to holders for url= 
QUrl("file:///virtual/kde5/5kde/build/frameworks/kio")
  forgetDirs removeAll in directoryData[ 
"file:///virtual/kde5/5kde/build/frameworks/kio" ] for KDirLister(0x4e273400)
  forgetCachedItemsJob append  KDirLister(0x4e273400) to holders for url= 
QUrl("file:///virtual/kde5/5kde/build/frameworks/kio")
  forgetDirs removeAll in directoryData[ 
"file:///virtual/kde5/5kde/build/frameworks/kio" ] for KDirLister(0x4e273400)
  forgetCachedItemsJob append  KDirLister(0x4e273400) to holders for url= 
QUrl("remote:/")
  forgetDirs removeAll in directoryData[ "remote:/" ] for KDirLister(0x4e273400)
  forgetDirs removeAll in directoryData[ "smb://" ] for KDirLister(0x4e273400)
  **number of holders for  "smb://" 1**
  
==23870== Invalid read of size 8
==23870==at 0x5F41167: KCoreDirListerCache::slotRedirection(KIO::Job*, 
QUrl const&) (kcoredirlister.cpp:1484)
==23870==by 0x5F4A3A9: call (qobjectdefs_impl.h:136)
==23870==by 0x5F4A3A9: call, 
void> (qobjectdefs_impl.h:169)
==23870==by 0x5F4A3A9: QtPrivate::QSlotObject, void>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, 
bool*) (qobjectdefs_impl.h:398)
==23870==by 0xA0F736B: call (qobjectdefs_impl.h:378)
==23870==by 0xA0F736B: QMetaObject::activate(QObject*, int, int, 
void**) (qobject.cpp:3749)
==23870==by 0x5EED068: KIO::ListJob::redirection(KIO::Job*, QUrl 
const&) (moc_listjob.cpp:246)
==23870==by 0x5EED481: KIO::ListJobPrivate::slotRedirection(QUrl 
const&) (listjob.cpp:222)
==23870==by 0x5EED4C1: operator() (listjob.cpp:294)
==23870==by 0x5EED4C1: call (qobjectdefs_impl.h:130)
==23870==by 0x5EED4C1: call, void> 
(qobjectdefs_impl.h:240)
==23870==by 0x5EED4C1: 
QtPrivate::QFunctorSlotObject, void>::impl(int, 
QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) (qobjectdefs_impl.h:423)
==23870==by 0xA0F736B: call (qobjectdefs_impl.h:378)
==23870==by 0xA0F736B: QMetaObject::activate(QObject*, int, int, 
void**) (qobject.cpp:3749)
==23870==by 0x5ED159A: KIO::SlaveInterface::redirection(QUrl const&) 
(moc_slaveinterface.cpp:518)
==23870==by 0x5ED3B39: KIO::SlaveInterface::dispatch(int, QByteArray 
const&) (slaveinterface.cpp:248)
==23870==by 0x5ED16F5: KIO::SlaveInterface::dispatch() 
(slaveinterface.cpp:89)
==23870==by 0x5ED64A6: KIO::Slave::gotInput() (slave.cpp:406)
==23870==by 0x5F660AB: KIO::Slave::qt_static_metacall(QObject*, 
QMetaObject::Call, int, void**) (moc_slave.cpp:89)
==23870==  Address 0x3b510c70 is 0 bytes inside a block of size 32 free'd
==23870==at 0x4C2F7BB: operator delete(void*) (in 
/usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==23870==by 0x5BD45B6: KDirLister::~KDirLister() (kdirlister.cpp:55)
==23870==by 0xA0F503A: QObjectPrivate::deleteChildren() 
(qobject.cpp:1992)
==23870==by 0xA0FE38A: QObject::~QObject() (qobject.cpp:1022

D12228: kdirlister new connect syntax

[Jaime Torres Amate]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:ee0a0f1ef323: kdirlister new connect syntax (authored by 
jtamate).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D12228?vs=32663&id=32686#toc

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12228?vs=32663&id=32686

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

AFFECTED FILES
  src/core/kcoredirlister.cpp
  src/core/kcoredirlister.h

To: jtamate, dfaure, #frameworks
Cc: michaelh, bruns

D12371: fix always reproducible crash

[Jaime Torres Amate]

jtamate added a comment.


  > One solution is to call printDebug(), which will output lots of information 
including the contents of directoryData().
  
  I have some questions:
  Is a KCoreDirLister able to handle more than one directory? The same 
KCoreDirLister pointer is holding smb:// and smb:/// at some point.
  Shouldn't something be done in the cache when the KCoreDirLister changes from 
smb:// to smb:/// ?
  
  The output I get, with a printDebug() call before Iterating over dirs;
  
kf5.kio.core.dirlister: +KCoreDirLister
kf5.kio.core.dirlister: 
kf5.kio.core.dirlister: +KCoreDirLister
kf5.kio.core.dirlister: ~KCoreDirLister KCoreDirLister(0x2b6add0)
kf5.kio.core.dirlister: lister: KCoreDirLister(0x2b6add0) silent= false
kf5.kio.core.dirlister: KCoreDirLister(0x2b6add0)
kf5.kio.core.dirlister: =Items in use:
kf5.kio.core.dirlister: Directory data:
kf5.kio.core.dirlister: Jobs:
kf5.kio.core.dirlister: Items in cache:
kf5.kio.core.dirlister: =
kf5.kio.core.dirlister: Iterating over dirs ()
kf5.kio.core.dirlister: KDirLister(0x2b69460) url= QUrl("file:///kk") keep= 
false reload= false
kf5.kio.core.dirlister: =Items in use:
kf5.kio.core.dirlister: Directory data:
kf5.kio.core.dirlister: Jobs:
kf5.kio.core.dirlister: Items in cache:
kf5.kio.core.dirlister: =
kf5.kio.core.dirlister: lister: KDirLister(0x2b69460) silent= true
kf5.kio.core.dirlister: KDirLister(0x2b69460)
kf5.kio.core.dirlister: =Items in use:
kf5.kio.core.dirlister: Directory data:
kf5.kio.core.dirlister: Jobs:
kf5.kio.core.dirlister: Items in cache:
kf5.kio.core.dirlister: =
kf5.kio.core.dirlister: Iterating over dirs ()
kf5.kio.core.dirlister: Listing directory: QUrl("file:///kk")
kf5.kio.core.dirlister: Entry now being listed by (KDirLister(0x2b69460))
kf5.kio.core.dirlister: new entries for  QUrl("file:///kk")
kf5.kio.core.dirlister: finished listing QUrl("file:///kk")
kf5.kio.core.dirlister: KDirLister(0x2b69460) numJobs: 0
kf5.kio.core.dirlister: =Items in use:
kf5.kio.core.dirlister: "file:///kk" URL: QUrl("file:///kk") rootItem: 
QUrl("file:///kk") autoUpdates refcount: 1 complete: true "with 0 items."
kf5.kio.core.dirlister: Directory data:
kf5.kio.core.dirlister:"file:///kk" 0 listers: ""
kf5.kio.core.dirlister:"file:///kk" 1 holders: " 0x2b69460"
kf5.kio.core.dirlister: Jobs:
kf5.kio.core.dirlister: Items in cache:
kf5.kio.core.dirlister: =
kf5.kio.core.dirlister: KDirLister(0x2b69460) url= QUrl("remote:/") keep= 
false reload= false
kf5.kio.core.dirlister: =Items in use:
kf5.kio.core.dirlister: "file:///kk" URL: QUrl("file:///kk") rootItem: 
QUrl("file:///kk") autoUpdates refcount: 1 complete: true "with 0 items."
kf5.kio.core.dirlister: Directory data:
kf5.kio.core.dirlister:"file:///kk" 0 listers: ""
kf5.kio.core.dirlister:"file:///kk" 1 holders: " 0x2b69460"
kf5.kio.core.dirlister: Jobs:
kf5.kio.core.dirlister: Items in cache:
kf5.kio.core.dirlister: =
kf5.kio.core.dirlister: lister: KDirLister(0x2b69460) silent= true
kf5.kio.core.dirlister: KDirLister(0x2b69460)  url= QUrl("file:///kk")
kf5.kio.core.dirlister: KDirLister(0x2b69460)
kf5.kio.core.dirlister: =Items in use:
kf5.kio.core.dirlister: "file:///kk" URL: QUrl("file:///kk") rootItem: 
QUrl("file:///kk") autoUpdates refcount: 1 complete: true "with 0 items."
kf5.kio.core.dirlister: Directory data:
kf5.kio.core.dirlister:"file:///kk" 0 listers: ""
kf5.kio.core.dirlister:"file:///kk" 1 holders: " 0x2b69460"
kf5.kio.core.dirlister: Jobs:
kf5.kio.core.dirlister: Items in cache:
kf5.kio.core.dirlister: =
kf5.kio.core.dirlister: Iterating over dirs (QUrl("file:///kk"))
kf5.kio.core.dirlister: KDirLister(0x2b69460)  _url:  QUrl("file:///kk")
kf5.kio.core.dirlister: KDirLister(0x2b69460) item moved into cache: 
QUrl("file:///kk")
kf5.kio.core.dirlister: Listing directory: QUrl("remote:/")
kf5.kio.core.dirlister: Entry now being listed by (KDirLister(0x2b69460))
kf5.kio.core.dirlister: new entries for  QUrl("remote:/")
kf5.kio.core.dirlister: Adding item:  
QUrl("remote:/x-wizard_service.desktop")
kf5.kio.core.dirlister: in QUrl("remote:/") item: 
QUrl("remote:/x-wizard_service.desktop")
kf5.kio.core.dirlister: Adding item:  QUrl("remote:/bluetooth-network")
kf5.kio.core.dirlister: in QUrl("remote:/") item: 
QUrl("remote:/bluetooth-network")
kf5.kio.core.dirlister: Adding item:  QUrl("remote:/mtp-network")
kf5.kio.core.dirlister: in QUrl("remote:/") item: 
QUrl("remote:/mtp-network")
kf5.kio.core.dirlister: Adding item:  QUrl("remote:/network")
kf5.kio.core.dirlister: in QUrl("remote:/") item: QUrl("remote:/network")
kf5.kio.core.dirlister: Adding item:

D12371: fix always reproducible crash

[Jaime Torres Amate]

jtamate updated this revision to Diff 33045.
jtamate edited the test plan for this revision.
jtamate added a comment.


  Just to know if I'm in the correct path, because I'm getting out of ideas. 
  Use in the hash table, and probably in other parts, always the url with at 
most one trailing /.
  
  Pros: The crash is gone.
  Cons: The error handling is gone (with this partial patch). Four asserts are 
hit.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12371?vs=32640&id=33045

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

AFFECTED FILES
  src/core/kcoredirlister.cpp

To: jtamate, dfaure, #frameworks, apol
Cc: apol, michaelh, bruns

D12511: optimization of KTextEditor::DocumentPrivate::views()

[Jaime Torres Amate]

jtamate created this revision.
jtamate added a reviewer: Kate.
Restricted Application added projects: Kate, Frameworks.
Restricted Application added a subscriber: Frameworks.
jtamate requested review of this revision.

REVISION SUMMARY
  Calculate the list of keys of a hash table is quite expensive.
  It is cheaper to keep that list and change it when a view is created or 
removed.

TEST PLAN
  Run kwrite and paste a relative long text several times and then undo until 
the text is empty, with only one view.
  Before: F5821740: kwrite_before.png  it 
used 65% of cpu in Kate::TextBuffer::notifyAboutRangeChange
  After: F5821746: kwrite_after.png  it 
uses 19% of cpu in in Kate::TextBuffer::notifyAboutRangeChange

REPOSITORY
  R39 KTextEditor

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

AFFECTED FILES
  src/document/katedocument.cpp
  src/document/katedocument.h

To: jtamate, #kate
Cc: #frameworks, michaelh, kevinapavew, ngraham, bruns, demsking, cullmann, 
sars, dhaumann

D12538: Select item without clicking the Open/Save button

[Jaime Torres Amate]

jtamate added a comment.


  I'm one of those that choose a file and add a "_2" to the filenames.  ;-)
  
  Please, please, ask the KDE Usability Project about this change.

REPOSITORY
  R241 KIO

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

To: anemeth, #frameworks, #vdg
Cc: jtamate, ngraham, #frameworks, michaelh, bruns

D12371: fix always reproducible crash

[Jaime Torres Amate]

jtamate updated this revision to Diff 33211.
jtamate edited the summary of this revision.
jtamate edited the test plan for this revision.
jtamate added a comment.


  The responsible for changing smb:// into smb:/// was: void 
KFileWidgetPrivate::_k_enterUrl(const QUrl &url)
  With this patch, the crashes are gone, as there is no redirection to smb:///
  But now there is no smb error message. F5824747: smb_error_spanish.png 
 (in spanish). unless smb:/// is 
introduced manually as the url.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12371?vs=33045&id=33211

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

AFFECTED FILES
  src/filewidgets/kfilewidget.cpp

To: jtamate, dfaure, #frameworks, apol
Cc: anthonyfieroni, apol, michaelh, bruns

D12371: fix always reproducible crash

[Jaime Torres Amate]

jtamate updated this revision to Diff 33242.
jtamate edited the summary of this revision.
jtamate added a comment.


  If I use if (!u.path().isEmpty()) or if (!u.path().isEmpty() && 
!u.path().endsWith('/')), 
  as soon as I enter the url fish://127.0.0.1 (without trailing /), I'm 
redirected to fish://127.0.0.1/home/jtorres (that didn't happen before), and 
after finishing reading the directory, assertion.
  
kf5.kio.core: Internal error: job is listing 
QUrl("fish://127.0.0.1/home/jtorres") but directoryData says no listers are 
currently listing  "fish://127.0.0.1/home/jtorres"
kf5.kio.core.dirlister: Items in use:
kf5.kio.core.dirlister: "fish://127.0.0.1/home/jtorres" URL: 
QUrl("fish://127.0.0.1/home/jtorres") rootItem: 
QUrl("fish://127.0.0.1/home/jtorres") autoUpdates refcount: 1 complete: true 
"with 1010 items."
kf5.kio.core.dirlister: Directory data:
kf5.kio.core.dirlister:"fish://127.0.0.1/home/jtorres" 0 listers: ""
kf5.kio.core.dirlister:"fish://127.0.0.1/home/jtorres" 1 holders: " 
0x3279dd0"
kf5.kio.core.dirlister: Jobs:
kf5.kio.core.dirlister: Items in cache:
kf5.kio.core.dirlister: 
"file:///virtual/kde5/5kde/build/frameworks/kio" rootItem: 
"file:///virtual/kde5/5kde/build/frameworks/kio" with 16 items.
ASSERT: "!dirData.listersCurrentlyListing.isEmpty()" in file 
/g/5kde/frameworks/kio/src/core/kcoredirlister.cpp, line 1208


#9  QMessageLogger::fatal (this=this@entry=0x7fff05486530, 
msg=msg@entry=0x7f36ba7ddeb0 "ASSERT: \"%s\" in file %s, line %d") at 
global/qlogging.cpp:816
#10 0x7f36ba548e86 in qt_assert 
(assertion=assertion@entry=0x7f36beb7b410 
"!dirData.listersCurrentlyListing.isEmpty()", file=file@entry=0x7f36beb7b0b0 
"/g/5kde/frameworks/kio/src/core/kcoredirlister.cpp", line=line@entry=1208) at 
global/qglobal.cpp:3123
#11 0x7f36beb40230 in KCoreDirListerCache::slotEntries 
(this=0x7f36bedaf420 <(anonymous 
namespace)::Q_QGS_kDirListerCache::innerFunction()::holder>, job=, entries=...) at /g/5kde/frameworks/kio/src/core/kcoredirlister.cpp:1208
...
#18 0x7f36beae500d in KIO::ListJob::entries (this=this@entry=0x324dd20, 
_t1=, _t1@entry=0x324dd20, _t2=...) at 
/virtual/kde5/5kde/build/frameworks/kio/src/core/KF5KIOCore_autogen/include/moc_listjob.cpp:232
#19 0x7f36beae6ce2 in KIO::ListJobPrivate::slotListEntries 
(this=0x37bd250, list=...) at /g/5kde/frameworks/kio/src/core/listjob.cpp:154
#20 0x7f36beae7052 in KIO::ListJobPrivateoperator() (list=..., __closure=) at 
/g/5kde/frameworks/kio/src/core/listjob.cpp:288
  
  Checking for the slash in the string representation of the url, not only in 
the path part, works for me.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12371?vs=33211&id=33242

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

AFFECTED FILES
  src/filewidgets/kfilewidget.cpp

To: jtamate, dfaure, #frameworks, apol
Cc: anthonyfieroni, apol, michaelh, bruns

D12659: two new UDS structures

[Jaime Torres Amate]

jtamate created this revision.
jtamate added reviewers: dfaure, Frameworks.
Restricted Application added a project: Frameworks.
jtamate requested review of this revision.

REVISION SUMMARY
  The two new UDS structures are similar to Frank's, but instead of using two 
vectors, use only one, with the index next to the data, and an overwritten == 
operator to test only the indexes (without it, the structures are slow).
  The first structure uses linear access and in the autotests is the fastest.
  The second structure uses binary access and scales better.
  
  I've modified the last 3 test to measure insertion time and read time as used 
in KFileItemPrivate::cmp.
  
  If you like one of the new structures, it can replace the one currently used 
in KIO::UDSEntryPrivate.

TEST PLAN
  run the test several times
  
  The interesing results in my pc:
  
INFO   : UdsEntryBenchmark::testTwoVectorsSlave() warmup stage result  
: 56
INFO   : UdsEntryBenchmark::testTwoVectorsSlave() accumulation stage 
result: 55
PASS   : UdsEntryBenchmark::testTwoVectorsSlave()
RESULT : UdsEntryBenchmark::testTwoVectorsSlave():
 0.0016 msecs per iteration (total: 55, iterations: 32768)
INFO   : UdsEntryBenchmark::testTwoVectorsApp() warmup stage result  : 
67
INFO   : UdsEntryBenchmark::testTwoVectorsApp() accumulation stage result: 
68
PASS   : UdsEntryBenchmark::testTwoVectorsApp()
RESULT : UdsEntryBenchmark::testTwoVectorsApp():
 0.00051 msecs per iteration (total: 68, iterations: 131072)
INFO   : UdsEntryBenchmark::testTwoVectorsSlaveAnother() warmup stage 
result  : 89
INFO   : UdsEntryBenchmark::testTwoVectorsSlaveAnother() accumulation stage 
result: 91
PASS   : UdsEntryBenchmark::testTwoVectorsSlaveAnother()
RESULT : UdsEntryBenchmark::testTwoVectorsSlaveAnother():
 0.0013 msecs per iteration (total: 91, iterations: 65536)
INFO   : UdsEntryBenchmark::testTwoVectorsAppAnother() warmup stage result  
: 64
INFO   : UdsEntryBenchmark::testTwoVectorsAppAnother() accumulation stage 
result: 64
PASS   : UdsEntryBenchmark::testTwoVectorsAppAnother()
RESULT : UdsEntryBenchmark::testTwoVectorsAppAnother():
 0.00048 msecs per iteration (total: 64, iterations: 131072)
INFO   : UdsEntryBenchmark::testTwoVectorsSlaveAnotherV2() warmup stage 
result  : 98
INFO   : UdsEntryBenchmark::testTwoVectorsSlaveAnotherV2() accumulation 
stage result: 101
PASS   : UdsEntryBenchmark::testTwoVectorsSlaveAnotherV2()
RESULT : UdsEntryBenchmark::testTwoVectorsSlaveAnotherV2():
 0.00154 msecs per iteration (total: 101, iterations: 65536)
INFO   : UdsEntryBenchmark::testTwoVectorsAppAnotherV2() warmup stage 
result  : 65
INFO   : UdsEntryBenchmark::testTwoVectorsAppAnotherV2() accumulation stage 
result: 68
PASS   : UdsEntryBenchmark::testTwoVectorsAppAnotherV2()
RESULT : UdsEntryBenchmark::testTwoVectorsAppAnotherV2():
 0.00051 msecs per iteration (total: 68, iterations: 131072)


PASS   : UdsEntryBenchmark::testTwoVectorsSlave()
RESULT : UdsEntryBenchmark::testTwoVectorsSlave():
 10,918 instruction reads per iteration (total: 10,918, iterations: 1)
PASS   : UdsEntryBenchmark::testTwoVectorsApp()
RESULT : UdsEntryBenchmark::testTwoVectorsApp():
 3,182 instruction reads per iteration (total: 3,182, iterations: 1)
PASS   : UdsEntryBenchmark::testTwoVectorsSlaveAnother()
RESULT : UdsEntryBenchmark::testTwoVectorsSlaveAnother():
 8,981 instruction reads per iteration (total: 8,981, iterations: 1)
PASS   : UdsEntryBenchmark::testTwoVectorsAppAnother()
RESULT : UdsEntryBenchmark::testTwoVectorsAppAnother():
 3,102 instruction reads per iteration (total: 3,102, iterations: 1)
PASS   : UdsEntryBenchmark::testTwoVectorsSlaveAnotherV2()
RESULT : UdsEntryBenchmark::testTwoVectorsSlaveAnotherV2():
 10,305 instruction reads per iteration (total: 10,305, iterations: 1)
PASS   : UdsEntryBenchmark::testTwoVectorsAppAnotherV2()
RESULT : UdsEntryBenchmark::testTwoVectorsAppAnotherV2():
 3,244 instruction reads per iteration (total: 3,244, iterations: 1)

PASS   : UdsEntryBenchmark::testTwoVectorsSlave()
RESULT : UdsEntryBenchmark::testTwoVectorsSlave():
 16,450 CPU ticks per iteration (total: 16,450, iterations: 1)
PASS   : UdsEntryBenchmark::testTwoVectorsApp()
RESULT : UdsEntryBenchmark::testTwoVectorsApp():
 4,736 CPU ticks per iteration (total: 4,736, iterations: 1)
PASS   : UdsEntryBenchmark::testTwoVectorsSlaveAnother()
RESULT : UdsEntryBenchmark::testTwoVectorsSlaveAnother():
 11,740 CPU ticks per iteration (total: 11,740, iterations: 1)
PASS   : UdsEntryBenchmark::testTwoVectorsAppAnother()
RESULT : UdsEntryBenchmark::testTwoVectorsAppAnother():
 4,297 CPU ticks per iteration (total: 4,297, iterations: 1)
PASS   : UdsEntryBenchmar

D12659: two new UDS structures

[Jaime Torres Amate]

jtamate marked 9 inline comments as done.
jtamate added a comment.


  > If anyone attempts this, please name the struct and its members, don't use 
QPair ;-)
  >  But no, that cannot possibly be faster. QVariant has lots of overhead 
itself.
  
  I've tried, just before reading your comment :-)
  Three tests: fill the structure, compare two structures and read 3 values.
  
  AnotherV2  (If someone finds a better name, it will be welcome).
  
0.00041 msecs per iteration (total: 55, iterations: 131072)
0.00022 msecs per iteration (total: 59, iterations: 262144)
0.00048 msecs per iteration (total: 64, iterations: 131072)
  
  QPair+QVariant:
  
0.00056 msecs per iteration (total: 74, iterations: 131072)
0.00020 msecs per iteration (total: 55, iterations: 262144)
0.00049 msecs per iteration (total: 65, iterations: 131072)

INLINE COMMENTS

> dfaure wrote in udsentry_benchmark.cpp:619
> This relies on insert being called in ascending "field" order, for 
> lower_bound to work.
> But that is not necessarily the case in kioslaves, so you'd have to insert at 
> "index" here, instead of appending.

Yes, it was badly done. Changed the fill order to detect this problems.

REPOSITORY
  R241 KIO

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

To: jtamate, dfaure, #frameworks
Cc: bruns, michaelh

D12659: two new UDS structures

[Jaime Torres Amate]

jtamate updated this revision to Diff 33505.
jtamate marked 3 inline comments as done.
jtamate edited the summary of this revision.
jtamate edited the test plan for this revision.
jtamate added a comment.


  Fixed the ordered insertion.
  Using std::vector and std::find_if.
  Initialize everything to try to detect a change of type in a "insert" over a 
different type.
  
  Using templates to reduce somehow the code size.
  The last 3 structures are tested 3 times:
  
  - Fill the structure
  - Compare two structures
  - Read 3 values
  
  > When you say "scales better", we're talking about the number of fields in 
the udsentry, not the number of items. But kioslaves don't fill in 1000 fields, 
so I have the feeling that scaling with the number of fields isn't a 
requirement.
  
  Yes, I was talking about the number of fields in the udsentry. I had to test 
it, just in case.
  
  > Are those benchmarks run in Release (or RelWithDebInfo) mode, rather than 
Debug (which is a big no no for benchmarks)? Qt should be compiled with 
optimizations enabled too.
  
  Yes, since the last comment of D11487  
everything is compiled with -O2 -mtune=native
  Qt is the one provided by OpenSuse.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12659?vs=33480&id=33505

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

AFFECTED FILES
  autotests/udsentry_benchmark.cpp

To: jtamate, dfaure, #frameworks
Cc: bruns, michaelh

D12016: [ktexteditor] much faster positionFromCursor

[Jaime Torres Amate]

jtamate added a comment.


  In D12016#257324 , @anthonyfieroni 
wrote:
  
  > Maybe i miss something, but i think if something should be extended it's 
view not accessible.
  
  
  Do you mean to extend KateViewInternal in such a way that positionFromCursor 
in KateViewAccessible becomes unnecessary?

REPOSITORY
  R39 KTextEditor

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

To: jtamate, #kate, cullmann, #frameworks
Cc: anthonyfieroni, mwolff, brauch, cullmann, #frameworks, michaelh, 
kevinapavew, ngraham, bruns, demsking, sars, dhaumann

D12659: two new UDS structures

[Jaime Torres Amate]

jtamate updated this revision to Diff 33507.
jtamate added a comment.


  Another way to improve insertion speed in release could be:
  Transform "insert" to really insert only, with ASSERTS if it used as 
"replaceOrInsert" and add new methods "replaceOrInsert".
  
  The measurements in the first structure has been done commenting the asserts.
  before: 0.00039 msecs per iteration (total: 52, iterations: 131072)
  after:0.00033 msecs per iteration (total: 88, iterations: 262144)

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12659?vs=33505&id=33507

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

AFFECTED FILES
  autotests/udsentry_benchmark.cpp

To: jtamate, dfaure, #frameworks
Cc: bruns, michaelh

D12659: two new UDS structures

[Jaime Torres Amate]

jtamate updated this revision to Diff 33543.
jtamate marked 13 inline comments as done.
jtamate edited the summary of this revision.
jtamate edited the test plan for this revision.
jtamate added a comment.


  I've included some methods that will be needed if AnotherUDSEntry replaces 
the current one.
  I hope to have addressed all your comments.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12659?vs=33507&id=33543

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

AFFECTED FILES
  autotests/udsentry_benchmark.cpp

To: jtamate, dfaure, #frameworks
Cc: bruns, michaelh

D12511: optimization of KTextEditor::DocumentPrivate::views()

[Jaime Torres Amate]

jtamate updated this revision to Diff 33574.
jtamate added a comment.


  You are right, there is no change to m_views there.

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12511?vs=33047&id=33574

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

AFFECTED FILES
  src/document/katedocument.cpp
  src/document/katedocument.h

To: jtamate, #kate, cullmann
Cc: cullmann, #frameworks, michaelh, kevinapavew, ngraham, bruns, demsking, 
sars, dhaumann

D12659: two new UDS structures

[Jaime Torres Amate]

jtamate updated this revision to Diff 33607.
jtamate added a comment.


  Removed some methods that are not used in the benchmarks.
  Added the asserts to check the type of the data at the beginning of every 
insert and replaceOrInsert.
  
  FYI: Checking the new implementation in live, so far only two asserts in 
listjob running kfind.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12659?vs=33543&id=33607

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

AFFECTED FILES
  autotests/udsentry_benchmark.cpp

To: jtamate, dfaure, #frameworks
Cc: bruns, michaelh

D12659: two new UDS structures

[Jaime Torres Amate]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:01b2e536aeb3: two new UDS structures (authored by 
jtamate).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12659?vs=33607&id=33608

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

AFFECTED FILES
  autotests/udsentry_benchmark.cpp

To: jtamate, dfaure, #frameworks
Cc: bruns, michaelh

D12016: [ktexteditor] much faster positionFromCursor

[Jaime Torres Amate]

jtamate added a comment.


  In D12016#257862 , @cullmann wrote:
  
  > Actually, given the functionality is only used there, if that code is in 
the view or in that helper class makes no real difference, or?
  
  
  I've tried to extend KateViewInternal in such a way that positionFromCursor 
in KateViewAccessible becomes unnecessary, but it became a mess.
  The problem is that all the "coordinates" used there are Cursors, that are in 
the form line+column. 
  But in KateViewAccessible it uses characters from the beginning, and caching 
only in one place (positionFromCursor) does not create a mess.

REPOSITORY
  R39 KTextEditor

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

To: jtamate, #kate, cullmann, #frameworks
Cc: anthonyfieroni, mwolff, brauch, cullmann, #frameworks, michaelh, 
kevinapavew, ngraham, bruns, demsking, sars, dhaumann

D12511: optimization of KTextEditor::DocumentPrivate::views()

[Jaime Torres Amate]

jtamate updated this revision to Diff 33613.
jtamate edited the test plan for this revision.
jtamate added a comment.


  Add and remove from the list.
  
  About the tools I use to profile:
  
  - I'm used to use slow machines, I have no problems running under valgrind.
  - When I can run a program and I'm able to repeat the workload that produces 
high cpu usage or slowness, I do like to use callgrind/kcachegrind, just 
because I get inclusive costs.
  - When a program gets slow after some time running or under some unexpected 
circumstances, yes I use perf, because I do not need to restart the program and 
I can do live measurements.
  - But I **do love** the kcachegrind view of the % of instructions for every 
method within the source code.
  
  In any case, the hotspot results pasting 10 times and undoing:
  Before: F5833105: kwrite_perf_before.png 

  After: F5833106: kwrite_perf_after.png 

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12511?vs=33574&id=33613

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

AFFECTED FILES
  src/document/katedocument.cpp
  src/document/katedocument.h

To: jtamate, #kate, cullmann, mwolff
Cc: mwolff, cullmann, #frameworks, michaelh, kevinapavew, ngraham, bruns, 
demsking, sars, dhaumann

D12016: [ktexteditor] much faster positionFromCursor

[Jaime Torres Amate]

This revision was automatically updated to reflect the committed changes.
Closed by commit R39:81996f55b1ab: [ktexteditor] much faster positionFromCursor 
(authored by jtamate).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D12016?vs=31853&id=33615#toc

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12016?vs=31853&id=33615

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

AFFECTED FILES
  src/view/kateviewaccessible.h
  src/view/kateviewinternal.cpp

To: jtamate, #kate, cullmann, #frameworks
Cc: anthonyfieroni, mwolff, brauch, cullmann, #frameworks, michaelh, 
kevinapavew, ngraham, bruns, demsking, sars, dhaumann

D12696: Use the new uds implementation

[Jaime Torres Amate]

jtamate created this revision.
jtamate added reviewers: dfaure, Frameworks.
Restricted Application added a project: Frameworks.
jtamate requested review of this revision.

REVISION SUMMARY
  This new implementation is the fastest in the autotests.
  The replaceOrInsert in KFileItem::setName is due to renaming a file.
  The replaceOrInsert in ListJobPrivate::slotListEntries is due to Kfind.

TEST PLAN
  It should be used extensively at least one release cycle before pushing it.
  
  Passes the autotests.
  Don't assert in any kde program that uses kio.

REPOSITORY
  R241 KIO

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

AFFECTED FILES
  src/core/kfileitem.cpp
  src/core/listjob.cpp
  src/core/udsentry.cpp
  src/core/udsentry.h

To: jtamate, dfaure, #frameworks
Cc: michaelh, bruns

D11604: kdirlistertest doesn't fail at random

[Jaime Torres Amate]

jtamate updated this revision to Diff 33623.
jtamate edited the summary of this revision.
jtamate added a comment.


  Add the new tests instead of replace.
  Use QTRY_COMPARE instead of custom loops.
  Reduce the number of files to 100.
  
  In my machine, the mime type of file.html is "application/x-extension-html"
  because of F5833260: file_associations.png 


REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11604?vs=30323&id=33623

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

AFFECTED FILES
  autotests/kdirlistertest.cpp
  autotests/kdirlistertest.h

To: jtamate, #frameworks, dfaure
Cc: apol, michaelh, bruns

D12511: optimization of KTextEditor::DocumentPrivate::views()

[Jaime Torres Amate]

jtamate added a comment.


  In D12511#258193 , @mwolff wrote:
  
  > Actually, no. Ignore what I said. The pictures you are showing are pretty 
meaningless. Did you run perf with `--call-graph dwarf`? Better look at the 
flamegraph and search for the function you are interested in 
(Kate::TextBuffer::notifyAboutRangeChange) or use the Caller/Callee view to get 
an aggregated view of your change.
  
  
  That's not the impression I got running kwrite in live and under a slow 
machine (valgrind), before I had to wait for the paste to finish, and after the 
patch not. But kwrite is still slow undoing the paste.
  
  I did run perf record -g, the same test with the patch applied after pasting 
the text 25 times and undoing with this result: F5833430: 
kwrite_perf_after_25.png .
  The same test (pasting 25 times and undoing) with --call-graph dwarf with 
this result: F5833432: kwrite_perf_after_25_dwarf.png 
, and F5833445: 
kwrite_perf_after_25_dwarf_caller.png .

REPOSITORY
  R241 KIO

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

To: jtamate, #kate, cullmann, mwolff
Cc: mwolff, cullmann, #frameworks, michaelh, kevinapavew, ngraham, bruns, 
demsking, sars, dhaumann

D12511: optimization of KTextEditor::DocumentPrivate::views()

[Jaime Torres Amate]

jtamate added a comment.


  In D12511#258212 , @mwolff wrote:
  
  > 
https://phabricator.kde.org/file/data/w4qogv4brtxlc5p5bnwr/PHID-FILE-q62giymcptudpl5m6bt3/kwrite_perf_after_25_dwarf_caller.png
 shows ~1.5% in notifyAboutRangeChange (inclusively). Is that before or after 
your patch here?
  
  
  After applying the patch.
  Before applying the patch: F5833542: kwrite_perf_before_25_dwarf_caller.png 


REPOSITORY
  R241 KIO

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

To: jtamate, #kate, cullmann, mwolff
Cc: mwolff, cullmann, #frameworks, michaelh, kevinapavew, ngraham, bruns, 
demsking, sars, dhaumann

D12511: optimization of KTextEditor::DocumentPrivate::views()

[Jaime Torres Amate]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R39:90157a222b5e: optimization of 
KTextEditor::DocumentPrivate::views() (authored by jtamate).

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12511?vs=33613&id=33644

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

AFFECTED FILES
  src/document/katedocument.cpp
  src/document/katedocument.h

To: jtamate, #kate, cullmann, mwolff
Cc: mwolff, cullmann, #frameworks, michaelh, kevinapavew, ngraham, bruns, 
demsking, sars, dhaumann

D12696: Use the new uds implementation

[Jaime Torres Amate]

jtamate updated this revision to Diff 33758.
jtamate marked 9 inline comments as done.
jtamate edited the summary of this revision.
jtamate added a comment.


  Hopefully, all the issues fixed.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12696?vs=33622&id=33758

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

AFFECTED FILES
  src/core/kfileitem.cpp
  src/core/listjob.cpp
  src/core/udsentry.cpp
  src/core/udsentry.h

To: jtamate, dfaure, #frameworks
Cc: michaelh, ngraham, bruns

D12696: Use the new uds implementation

[Jaime Torres Amate]

jtamate updated this revision to Diff 33809.
jtamate added a comment.


  Don't use java style.
  As UDSEntryPrivate is private, declare public methods save, load and 
debugUDSEntry, otherwise I couldn't access the private d pointer from the << 
and >> operators.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12696?vs=33758&id=33809

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

AFFECTED FILES
  src/core/kfileitem.cpp
  src/core/listjob.cpp
  src/core/udsentry.cpp
  src/core/udsentry.h

To: jtamate, dfaure, #frameworks
Cc: bruns, michaelh, ngraham

D12696: Use the new uds implementation

[Jaime Torres Amate]

jtamate updated this revision to Diff 33815.
jtamate added a comment.


  Make some friends functions.
  A bad type in copy&paste didn't allowed me the use of the module functions 
save/load/debugUDSEntry,

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12696?vs=33809&id=33815

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

AFFECTED FILES
  src/core/kfileitem.cpp
  src/core/listjob.cpp
  src/core/udsentry.cpp
  src/core/udsentry.h

To: jtamate, dfaure, #frameworks
Cc: bruns, michaelh, ngraham

D12696: Use the new uds implementation

[Jaime Torres Amate]

jtamate added inline comments.

INLINE COMMENTS

> dfaure wrote in udsentry.cpp:454
> Hmm why can't this be the friend function directly?
> 
> I don't like the added global functions in the public header...

> Hmm why can't this be the friend function directly?

Because the compiler (clang++ in this case) doesn't know which one to apply.
Unless both definitions are equivalent (in source and binary) and the later can 
be removed. That is something I don't known.

  src/core/slavebase.cpp:728:14: error: use of overloaded operator '<<' is 
ambiguous (with operand types 'QDataStream' and 'const KIO::UDSEntry')
  KIO_DATA << entry;
   ^  ~
  src/core/udsentry.h:315:40: note: candidate function
  friend QDataStream &operator<< (QDataStream &s, const KIO::UDSEntry &a);
  ^
  src/core/udsentry.h:361:29: note: candidate function
  KIOCORE_EXPORT QDataStream &operator<< (QDataStream &s, const KIO::UDSEntry 
&a);
  ^

> I don't like the added global functions in the public header...

Me neither, but otherwise clang++will not be able to find the function.

  src/core/udsentry.h:314:19: error: no function named 'save' with type 'void 
(QDataStream &, const KIO::UDSEntry &)' was found in the specified scope
  friend void ::save(QDataStream &, const KIO::UDSEntry &);
^

> dfaure wrote in udsentry.cpp:45
> 1ms is relative to a benchmark which isn't clear when reading this code in 
> other contexts. As someone said in another review, do we still need this 
> operator== anyway, given that you pass lambdas to find_if()?

ok, ok, bye bye ==

REPOSITORY
  R241 KIO

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

To: jtamate, dfaure, #frameworks
Cc: bruns, michaelh, ngraham

D12776: 4 more cases of empty protocol

[Jaime Torres Amate]

jtamate created this revision.
jtamate added a reviewer: Frameworks.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: kde-frameworks-devel.
jtamate requested review of this revision.

REVISION SUMMARY
  After browsing a compressed .xz file in dolphin, those methods were 
requesting information about an empty protocol.

TEST PLAN
  Browse a compressed file using dolphin without assertions.

REPOSITORY
  R241 KIO

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

AFFECTED FILES
  src/core/kprotocolinfo.cpp
  src/core/kurlauthorized.cpp

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

D12371: fix always reproducible crash

[Jaime Torres Amate]

jtamate updated this revision to Diff 33872.
jtamate marked an inline comment as done.
jtamate edited the summary of this revision.
jtamate added a comment.
Restricted Application added a subscriber: kde-frameworks-devel.


  The crash after the redirection in fish://127.0.0.1 was due to a remaining of 
an unfinished patch in other files. 
  After removing it and  testing the patch in different machines and different 
compilers, this solves this crash and I haven't noticed regressions.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12371?vs=33242&id=33872

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

AFFECTED FILES
  src/filewidgets/kfilewidget.cpp

To: jtamate, dfaure, #frameworks, apol
Cc: kde-frameworks-devel, anthonyfieroni, apol, michaelh, ngraham, bruns

D12696: Use the new uds implementation

[Jaime Torres Amate]

jtamate updated this revision to Diff 33889.
jtamate edited the summary of this revision.
jtamate added a comment.
Restricted Application added a subscriber: kde-frameworks-devel.


  Direct friendship with the operators.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12696?vs=33815&id=33889

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

AFFECTED FILES
  src/core/kfileitem.cpp
  src/core/listjob.cpp
  src/core/udsentry.cpp
  src/core/udsentry.h

To: jtamate, dfaure, #frameworks
Cc: kde-frameworks-devel, bruns, michaelh, ngraham

D12696: Use the new uds implementation

[Jaime Torres Amate]

jtamate updated this revision to Diff 33929.
jtamate marked 8 inline comments as done and 3 inline comments as done.
jtamate edited the summary of this revision.
jtamate added a comment.


  Added the documentation for insert.
  Removed the () from the QDataStream& operators, but must be kept for QDebug 
or clang++ will not compile.
  Adjusted the reserved amounts.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12696?vs=33889&id=33929

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

AFFECTED FILES
  src/core/kfileitem.cpp
  src/core/listjob.cpp
  src/core/udsentry.cpp
  src/core/udsentry.h

To: jtamate, dfaure, #frameworks
Cc: kde-frameworks-devel, bruns, michaelh, ngraham

D12371: Don't redirect smb:/ to smb:// and then to smb:///

[Jaime Torres Amate]

jtamate retitled this revision from "fix always reproducible crash" to "Don't 
redirect smb:/ to smb:// and then to smb:///".
jtamate edited the summary of this revision.

REPOSITORY
  R241 KIO

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

To: jtamate, dfaure, #frameworks, apol
Cc: kde-frameworks-devel, anthonyfieroni, apol, michaelh, ngraham, bruns

D12371: Don't redirect smb:/ to smb:// and then to smb:///

[Jaime Torres Amate]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R241:2a57054a718a: Don't redirect smb:/ to smb:// and 
then to smb:/// (authored by jtamate).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D12371?vs=33872&id=33956#toc

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12371?vs=33872&id=33956

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

AFFECTED FILES
  src/filewidgets/kfilewidget.cpp

To: jtamate, dfaure, #frameworks, apol
Cc: kde-frameworks-devel, anthonyfieroni, apol, michaelh, ngraham, bruns

D12696: Use the new uds implementation

[Jaime Torres Amate]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:e80c31163170: Use the new uds implementation (authored by 
jtamate).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12696?vs=33929&id=33957

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

AFFECTED FILES
  src/core/kfileitem.cpp
  src/core/listjob.cpp
  src/core/udsentry.cpp
  src/core/udsentry.h

To: jtamate, dfaure, #frameworks
Cc: kde-frameworks-devel, bruns, michaelh, ngraham

D12865: transferjob new connect syntax

[Jaime Torres Amate]

jtamate created this revision.
jtamate added reviewers: dfaure, Frameworks.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: kde-frameworks-devel.
jtamate requested review of this revision.

REVISION SUMMARY
  Use the new connect syntax.
  A new QMetaObject::Connection is introduced to disconnect the signal 
connected in the constructor.

TEST PLAN
  copy from compressed file

REPOSITORY
  R241 KIO

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

AFFECTED FILES
  src/core/job_p.h
  src/core/transferjob.cpp
  src/core/transferjob.h

To: jtamate, dfaure, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D12897: Reserve space for the cachedLineForRanges Qhash (optimization)

[Jaime Torres Amate]

jtamate created this revision.
jtamate added reviewers: Kate, Frameworks.
Restricted Application added projects: Kate, Frameworks.
Restricted Application added subscribers: kde-frameworks-devel, kwrite-devel.
jtamate requested review of this revision.

REVISION SUMMARY
  Most of the time was spent allocating space for the hashdata.
  The perf numbers speak by themself: 
  from 42.3% (cycles inc.) F5849664: kwrite_reserver_perf_dwarf_before.png 

  to 16.1% (cycles inc.) F5849665: kwrite_reserver_perf_dwarf_after.png 


TEST PLAN
  repeat {
  
Paste, go to beginning of file
  
  } 5 times
  Undo 5 times

REPOSITORY
  R39 KTextEditor

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

AFFECTED FILES
  src/buffer/katetextblock.cpp

To: jtamate, #kate, #frameworks
Cc: kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, 
demsking, cullmann, sars, dhaumann

D12945: kcoredirlister lstItems benchmark

[Jaime Torres Amate]

jtamate created this revision.
jtamate added reviewers: dfaure, Frameworks.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: kde-frameworks-devel.
jtamate requested review of this revision.

REVISION SUMMARY
  Decide which data structure is best for kcoredirlister lstItems.
  Defined as NonMovableFileItemList lstItems;  in kcoredirlister_p.h (484).
  
  The results in one machine:
  
  |  | QList | QListBinary | QListBinaryHash | QHash |
  | add  | 17| 35  | 20  | 18|
  | findByName   | 937   | 969 | 1.326   | 1.626 |
  | findByUrl| 1.953 | 66  | 7,6 | 7,2   |
  | findByUrlAll | 692   | 25  | 8,2 | 8,0   |
  |

REPOSITORY
  R241 KIO

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/kcoredirlister_benchmark.cpp

To: jtamate, dfaure, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D11282: less expensive findByUrl in KCoreDirListerCache

[Jaime Torres Amate]

jtamate added a dependency: D12945: kcoredirlister lstItems benchmark.

REPOSITORY
  R241 KIO

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

To: jtamate, #frameworks, dfaure
Cc: mwolff, michaelh, ngraham, bruns

D12945: kcoredirlister lstItems benchmark

[Jaime Torres Amate]

jtamate added a dependent revision: D11282: less expensive findByUrl in 
KCoreDirListerCache.

REPOSITORY
  R241 KIO

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

To: jtamate, dfaure, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

[Jaime Torres Amate]

jtamate updated this revision to Diff 34708.
jtamate edited the summary of this revision.
jtamate edited the test plan for this revision.
jtamate added a comment.
Restricted Application added a subscriber: kde-frameworks-devel.


  Based on the tests done in D12945  and 
D11282 , the best solution is to have the 
result of qHash(url) in KFileItem to compare items in the binary search.
  In two cases, the KFileItem in the list has to be moved to the right 
position, this is still faster than before.
  Introduce two methods to insert an item into the list and to move the item to 
the right position.
  
  The crash in kdirmodeltest was due to getting twice the signal of a file 
changed into a directory.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10742?vs=29572&id=34708

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

AFFECTED FILES
  src/core/kcoredirlister.cpp
  src/core/kcoredirlister_p.h
  src/core/kfileitem.cpp
  src/core/kfileitem.h
  src/widgets/kdirmodel.cpp

To: jtamate, #frameworks, dfaure
Cc: kde-frameworks-devel, mwolff, markg, michaelh, ngraham, bruns

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

[Jaime Torres Amate]

jtamate updated this revision to Diff 34775.
jtamate marked 5 inline comments as done.
jtamate edited the summary of this revision.
jtamate added a comment.


  Removed m_hash, after implementing the right checks it was slower than 
comparing QUrls.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10742?vs=34708&id=34775

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

AFFECTED FILES
  src/core/kcoredirlister.cpp
  src/core/kcoredirlister_p.h
  src/core/kfileitem.cpp
  src/core/kfileitem.h
  src/widgets/kdirmodel.cpp

To: jtamate, #frameworks, dfaure
Cc: bruns, kde-frameworks-devel, mwolff, markg, michaelh, ngraham

D11282: less expensive findByUrl in KCoreDirListerCache

[Jaime Torres Amate]

jtamate added a comment.


  About the KCoreDirLister::Private::addNewItems method, benchmarking the 
current and sorted list implementation with 5000 fileItems:
  
  It only takes 1.5 msecs per iteration without filters and 1.9 msecs with a 
nameFilter and a mimeFilter.
  For reference, doing a std::partition of a copy of the list with 
isItemVisible also takes 1.5 msecs per iteration without filters and 1.4 with 
filters.

INLINE COMMENTS

> mwolff wrote in kcoredirlister.cpp:852
> is it OK that you operate on a copy of the item here? the item in the hash 
> won't be modified, is that on purpose?

The item in the hash will be refreshed, but not the item returned.
This is not called when a rename is detected, therefore the key(url) is the 
same.

REPOSITORY
  R241 KIO

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

To: jtamate, #frameworks, dfaure
Cc: kde-frameworks-devel, bruns, mwolff, michaelh, ngraham

D13189: [kcoredirlister] Remove as many url.toString() as possible

[Jaime Torres Amate]

jtamate created this revision.
jtamate added reviewers: dfaure, Frameworks.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: kde-frameworks-devel.
jtamate requested review of this revision.

REVISION SUMMARY
  Change itemsInUse, itemsCached and DirectoryDataHash keys from QString to 
QUrl, avoiding as many QUrl toString() as possible.

TEST PLAN
  It passes the autotests using 20 msecs less.

REPOSITORY
  R241 KIO

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

AFFECTED FILES
  src/core/kcoredirlister.cpp
  src/core/kcoredirlister_p.h

To: jtamate, dfaure, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13189: [kcoredirlister] Remove as many url.toString() as possible

[Jaime Torres Amate]

jtamate updated this revision to Diff 35114.
jtamate marked an inline comment as done.
jtamate added a comment.


  const added.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13189?vs=35085&id=35114

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

AFFECTED FILES
  src/core/kcoredirlister.cpp
  src/core/kcoredirlister_p.h

To: jtamate, dfaure, #frameworks
Cc: apol, kde-frameworks-devel, michaelh, ngraham, bruns

D13211: Enable comparing KFileItems by url

[Jaime Torres Amate]

jtamate added dependent revisions: D10742: get rid of the raw KFileItem 
pointers in KCoreDirListerCache, D12945: kcoredirlister lstItems benchmark.

REPOSITORY
  R241 KIO

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

To: jtamate, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

[Jaime Torres Amate]

jtamate added a dependency: D13211: Enable comparing KFileItems by url.

REPOSITORY
  R241 KIO

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

To: jtamate, #frameworks, dfaure
Cc: bruns, kde-frameworks-devel, mwolff, markg, michaelh, ngraham