from:"Frank Reininghaus"

Re: QString -> QStringLiteral conversions might make applications crash on exit

2016-02-29 Thread Frank Reininghaus

Hi,

2016-02-29 19:04 GMT+01:00 Milian Wolff:
> On Friday, February 26, 2016 1:37:57 AM CET Frank Reininghaus wrote:
>> Hi everyone,
>>
>> sorry if most of you know about this already, but since it seems that
>> QStringLiterals are being introduced everywhere right now, I think
>> that it is important to raise awareness about the fact that this might
>> be more dangerous that it seems at first sight.
>
> make sure you read up on that topic on the Qt devel mailing list - the issue
> you describe here has been spotted with Qt DBUS as well afaik.

Yes, I will, thanks!

>> This becomes a problem if the read-only data that the QString refers
>> to are not there any more, which can happen if the QString was stored
>> in a global static object from one library, and the QStringLiteral is
>> from another library, which might have been unloaded before the global
>> static object was destroyed.
>>
>> I believe that this is just what happens right now with a global
>> static KIconLoader from kicontnkhemes and a QStringLiteral from
>> frameworkintegration:
>>
>> https://bugs.kde.org/show_bug.cgi?id=359758
>>
>> If my analysis is wrong, please let me know!
>
> Considering that the above bug says it crashes when you do
>
> dolphin --icon 
>
> when do you put "" into a QStringLiteral? Considering that this is
> UTF8 data you get at runtime via argv, I doubt that this is a related issue.
> If at all, dolphin should crash always on exit, independent of an argv
> parameter.

Maybe it should always crash, but it doesn't ;-) The reporter of
https://bugs.kde.org/show_bug.cgi?id=359758 has found, and I have
confirmed, that this only happens if the "--icon " argument
is used, and only since the QStringLiteral change was made in
frameworkintergration.

The "" is never put into a QStringLiteral, of course. As I
wrote in the bug report last week, the QStringLiteral that causes the
crash is "dialog-close" in frameworkintegration
(src/kstyle/kstyle.cpp). This patch in frameworkintegration fixes the
problem for me:

diff --git a/src/kstyle/kstyle.cpp b/src/kstyle/kstyle.cpp
index 6ba5d51..429ede9 100644
--- a/src/kstyle/kstyle.cpp
+++ b/src/kstyle/kstyle.cpp
@@ -362,7 +362,7 @@ QIcon KStyle::standardIcon(StandardPixmap
standardIcon, const QStyleOption */*op
 case QStyle::SP_DialogSaveButton:
 return QIcon::fromTheme(QStringLiteral("document-save"));
 case QStyle::SP_DialogCloseButton:
-return QIcon::fromTheme(QStringLiteral("dialog-close"));
+return QIcon::fromTheme(QString("dialog-close"));
 case QStyle::SP_DialogApplyButton:
 return QIcon::fromTheme(QStringLiteral("dialog-ok-apply"));
 case QStyle::SP_DialogResetButton:

I have no idea why "dialog-close" is the problem. I just found it with
a rather primitive debugging approach, see the bug report.

I also don't know why the --icon argument triggers the problem. Maybe
it affects how the kstyle plugin is loaded and unloaded.

>> If this crash is really caused by the QStringLiteral, then we should
>> think about how we want to treat QStringLiteral in the future. The
>> current approach seems to be "use it everywhere", but this might cause
>> more crashes in the future.
>
> To debug this issue, use valgrind (sadly Thiago's suggestion on an improved
> diagnostic for the issue hasn't been implemented yet), which should at least
> give a somewhat more explicit backtrace than GDB afaik. Furthermore, in GDB.
> do a "info shared" before shutting down GDB, and once when you hit the crash.
> Then look at where the data that is being deleted (i.e. the QString) is
> stored, and map it to one of the *.so's. If it was a QStringLiteral you will
> find out which lib it was that way.

I had already tried Valgrind last week (same backtrace as with gdb and
an "Access not within mapped region" error).

Thanks for the "info shared" hint! The address is close to the memory
region of libKF5Style.so.5, but not quite inside if I'm reading the
output correctly. This is the part of the "info shared" output before
the crash that looks close to the address which caused the crash
(0x7fffdd778a80):

0x7fffddc44b50  0x7fffddc4bddb  Yes
/home/kde-frameworks/qt5/qtbase/plugins/platforminputcontexts/libcomposeplatforminputcontextplugin.so
0x7fffdd9989a0  0x7fffdd9f93c5  Yes
/home/kde-frameworks/kf5/lib64/plugins/styles/breeze.so
0x7fffdd7730d0  0x7fffdd777d9f  Yes
/home/kde-frameworks/kf5/lib64/libKF5Style.so.5
0x7fffdd5686a0  0x7fffdd56b144  Yes
/home/kde-frameworks/kf5/lib64/plugins/kf5/FrameworkIntegrationPlugin.so



Program received signal SIGSEGV, Segmentation fault.
0x74b3bbb0 in load

Re: QString -> QStringLiteral conversions might make applications crash on exit

2016-02-28 Thread Frank Reininghaus

Hi Albert,

2016-02-28 11:54 GMT+01:00 Albert Astals Cid:
> El Friday 26 February 2016, a les 01:37:57, Frank Reininghaus va escriure:
>> Hi everyone,
>>
>> sorry if most of you know about this already, but since it seems that
>> QStringLiterals are being introduced everywhere right now, I think
>> that it is important to raise awareness about the fact that this might
>> be more dangerous that it seems at first sight.
>>
>> QStringLiteral has the nice property that it stores the string data in
>> read-only memory and avoids heap allocations when it is used to
>> construct a QString. The QString, and any copies that are made, just
>> contain a pointer to read-only data. There is a very nice overview by
>> Olivier at
>>
>> https://woboq.com/blog/qstringliteral.html
>>
>> However, QString is still a non-POD type, even if it has been
>> constructed with QStringLiteral (or copied from such a QString), so
>> its destructor is being run, which accesses the read-only data, then
>> finds out that it's been made with QStringLiteral, such that no
>> refcount updates and deallocations are needed.
>>
>> This becomes a problem if the read-only data that the QString refers
>> to are not there any more, which can happen if the QString was stored
>> in a global static object from one library, and the QStringLiteral is
>> from another library, which might have been unloaded before the global
>> static object was destroyed.
>>
>> I believe that this is just what happens right now with a global
>> static KIconLoader from kicontnkhemes and a QStringLiteral from
>> frameworkintegration:
>>
>> https://bugs.kde.org/show_bug.cgi?id=359758
>>
>> If my analysis is wrong, please let me know!
>
> If you know what commit causes it I'd say you have two options:
>  * Find exactly which of the changes in the patch is causing the problem, add
> a test case that crashes and then commit the smallest change that fixes the
> crash
>  * Revert the commit and CC the person that did the commit asking him to be
> more careful.

I could go for option 1 because I already found which QStringLiteral
caused the problem, but I'll try to find out in the next few days if
the workaround that Jan showed can also be used in KIconLoader. This
would prevent that the same problem happens again if QStringLiterals
are also used elsewhere to load icons.

But I'm not 100% sure if there aren't more places in KF5 and KDE
Applications where using QStringLiteral could cause similar problems.

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

Re: Review Request 127111: kurlnavigator: add new signal selectParentOfPreviousUrl

2016-02-22 Thread Frank Reininghaus

> On Feb. 20, 2016, 7:46 p.m., David Faure wrote:
> > src/filewidgets/kurlnavigator.h, line 428
> > <https://git.reviewboard.kde.org/r/127111/diff/1/?file=444588#file444588line428>
> >
> > Signal names usually end with "ed", they reflect a state change or an 
> > action change.
> > 
> > "select" here is what you want the app to do (now that I read the bug 
> > report; otherwise it was very unclear just from the API docs), but you have 
> > no guarantee that the app will do that.
> > 
> > void urlChangedToParent(const QUrl )
> > 
> > ?
> > 
> > An alternative is to let the app do the "finding the immediate child" 
> > logic by just emitting
> > 
> > void urlChangedToParent(const QUrl , const QUrl )
> > 
> > It seems to me that this is a better signal, because it's more generic. 
> > On the other hand, if you are afraid that multiple apps would then need to 
> > implement the "first child" logic, then this could be done by a helper 
> > method in this class. But at least the signal has a much more generic 
> > purpose than being geared towards this specific feature,
> > which increases the chances that it is useful for other things later.

I also thought first that something like urlChangedToParent(QUrl) would be a 
good signal name, but it might make people think that this signal will always 
be emitted when navigating from a URL to its (possibly indirect) parent.

However, this is not the case - the new signal will only be emitted if the URL 
change of the KUrlNavigator was triggered by a call of setLocationUrl(const 
QUrl&). If the URL change was caused by invoking the goBack() or goForward() 
slots, then the signal will not be emitted. The reason is that the user of 
KUrlNavigator will try to restore the view state (using the result from 
KUrlNavigator's locationState() function) then, and selecting any parents would 
interfere with that (see the discussion in 
https://git.reviewboard.kde.org/r/123253/ ).

So the idea is that the new signal is only emitted if no history action was 
responsible for the URL change. This makes it possible for Dolphin and other 
users of KUrlNavigator to behave like some other file managers, which also 
select the parent of the previous URL, unless the URL change was caused by 
back/forward. So anything with 'changed' is not really accurate, because a 
change is not sufficient to emit this signal.

Still, it might make sense to have an 'ed' in the name. 'requested' appears 
often in signals which are not so much about state changes, but have the 
purpose to make the receiver do something. How about

urlSelectionRequested(const QUrl &) ?

- Frank

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

On Feb. 18, 2016, 9:53 p.m., Gregor Mi wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127111/
> ---
> 
> (Updated Feb. 18, 2016, 9:53 p.m.)
> 
> 
> Review request for Dolphin, KDE Frameworks, Emmanuel Pescosta, and Frank 
> Reininghaus.
> 
> 
> Bugs: 335616
> https://bugs.kde.org/show_bug.cgi?id=335616
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> Moved logic from https://git.reviewboard.kde.org/r/123253 to here.
> 
> Provides a signal to implement bug 
> https://bugs.kde.org/show_bug.cgi?id=335616: "Dolphin: Navigate to parent 
> folder selects child folder".
> 
> 
> Diffs
> -
> 
>   src/filewidgets/kurlnavigator.h 4ffe56acf9ef7a80ba27ba3a08327e363f98fb0d 
>   src/filewidgets/kurlnavigator.cpp 64d2a6d1d5cf8ca2e0aaa61d00ac1ffb1a9866b3 
>   src/filewidgets/urlutil.h PRE-CREATION 
>   tests/CMakeLists.txt dc88ce9fd5c5ae6ad135b72785370c0094969b5c 
>   tests/urlutiltest.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/127111/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gregor Mi
> 
>

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

Re: Review Request 126894: Refactor the listjobtest to allow listing of multile paths.

2016-01-26 Thread Frank Reininghaus

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

Thanks Milian for looking at this test. I implemented it quite some time ago as 
a quick hack that allows to see how changes in the internal data structures of 
UDSEntry affect the performance and especially the memory usage of KIO::ListJob 
(which is what applications typically use to obtain UDSEntries). I did it in 
Qt4 times, when one could not use lambdas in connections. It's great to see the 
test simplified and extended now :-)

I have two remarks:

* Appending the listed entries to a list was intentional. If they are discarded 
immediately, then their memory is freed, and it is not possible to see how much 
memory the entries would consume in an application that lists one or more 
paths. I see that this original intention of the test is not obvious at all 
though. I should have added a comment - sorry about that!
* The `std::cin.ignore()` was also intentional. It allows to investigate the 
memory usage with htop, ps, or other tools while the process is still running. 
I know that one could also use Valgrind/Massif+massif-visualizer to get a 
detailed memory usage report (which also contains detailed information about 
where and when memory was allocated), but this has the disadvantage that the 
bookkeeping overhead of the memory allocator is not included (or at least it 
was not last time I checked). The massif method would tell you that an 
application that allocates 100 MiB in a single block uses more memory than an 
application that allocates 10 million blocks of 8 bytes each, but this is not 
true because some bookkeeping overhead is added to each allocation. Each 
allocation takes at least 32 bytes of memory when using GCC+glibc on a 64-bit 
system. But I guess that there must be a better way than the `cin.ignore()` 
hack to easily get the real memory usage.

- Frank Reininghaus

On Jan. 26, 2016, 3:34 nachm., Milian Wolff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126894/
> ---
> 
> (Updated Jan. 26, 2016, 3:34 nachm.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> With this change it is now possible to list multiple paths
> as defined via the command line.
> 
> While at it, I refactored the code to clean it up:
> 
> - rely on QEventLoopLocker to quit the application once all jobs
>   have finished
> - use a lambda to count the listed entries
> - don't append to a KIO::UDSEntryList to cound the listed entries
> 
> 
> Diffs
> -
> 
>   tests/listjobtest.cpp 702b09950734894a9f82746d58071225419b4e3f 
> 
> Diff: https://git.reviewboard.kde.org/r/126894/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Milian Wolff
> 
>

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

Re: Review Request 126750: Make KIconDialog and its sub-dialog Qt::WindowModal, rather than Qt::NonModal

2016-01-18 Thread Frank Reininghaus



> On Jan. 15, 2016, 7:20 vorm., Martin Gräßlin wrote:
> > > The KIconDialog itself works fine, but the "Browse..." sub dialog, which 
> > > is a grand child of the modal dialog, is opened in the background and 
> > > cannot be used
> > 
> > this sounds like a Qt bug or a KFileDialog bug. The sub dialog should set a 
> > transient hint and that's respected by the window manager with and without 
> > having the modal flag.
> > 
> > My suggestion is that we verify it's a bug, locate it and fix it and don't 
> > work around it. To verify that it is a bug: xprop and xwininfo of all three 
> > windows are needed.

Thanks Martin. I'm not familiar with these window management issues, so your 
feedback is greatly appreciated. I attached the xprop and xwinfo output for the 
three windows to the bug report: https://bugs.kde.org/show_bug.cgi?id=355310#c8

It would be great if you could take a look.


- Frank


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


On Jan. 14, 2016, 11:36 nachm., Frank Reininghaus wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126750/
> ---
> 
> (Updated Jan. 14, 2016, 11:36 nachm.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Bugs: 355310
> https://bugs.kde.org/show_bug.cgi?id=355310
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> ---
> 
> Currently, KIconDialog::showDialog() sets the modality to Qt::NonModal by 
> calling
> 
> setModal(false);
> 
> I found that this was done in 
> https://quickgit.kde.org/?p=kdelibs.git=commit=d0d2639c126f88a44c852b738550a9427c6260bb
>  in order to prevent that a modal dialog locks an entire application, such as 
> Plasma.
> 
> Unfortunately, this has an unwanted side effect in the "Places" of Dolphin 
> and the file dialog: the KIconDialog is the child of a modal "Add Places 
> Entry" dialog there. The KIconDialog itself works fine, but the "Browse..." 
> sub dialog, which is a grand child of the modal dialog, is opened in the 
> background and cannot be used (it could be that this worked for some reason 
> in Qt4 times - I guess we would have received bug reports about this issue 
> earlier otherwise).
> 
> This can be fixed by setting the modality to Qt::WindowModal, which ensures 
> that the dialogs block their respective parents (but not the entire 
> application - that would happen if the modality was set to 
> Qt::ApplicationModal, for example by calling setModal(true)).
> 
> Note that there are two setModal(true) calls in the KIconDialog constructors. 
> They have no effect if the dialog is opened with showDialog() (which is what 
> happens if the dialog is opened by clicking a KIconButton) because the 
> modality is overwritten there. I'm not sure though if there are any other 
> uses of KIconDialog which would break if the apparently superfluous calls 
> were removed. This might need further investigation.
> 
> 
> Diffs
> -
> 
>   src/kicondialog.cpp cca4ed3 
> 
> Diff: https://git.reviewboard.kde.org/r/126750/diff/
> 
> 
> Testing
> ---
> 
> The "Browse..." sub dialog of the icon dialog works fine again in Dolphin and 
> KWrite's file dialog when creating a new "Place".
> 
> I could not test if this affects Plasma somehow because I currently do not 
> have a full self-built Plasma session running. It could probably be checked 
> by opening the "Properties..." of a file in FolderView, clicking the icon, 
> and then opening the "Browse..." sub dialog of the KIconDialog. This should 
> hopefully not lock the entire Plasma session (because the dialogs are window 
> modal, and not application modal). If anyone finds problems with that or has 
> ideas for improvement, please let me know!
> 
> 
> Thanks,
> 
> Frank Reininghaus
> 
>

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

Review Request 126750: Make KIconDialog and its sub-dialog Qt::WindowModal, rather than Qt::NonModal

2016-01-14 Thread Frank Reininghaus


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

Review request for KDE Frameworks.


Bugs: 355310
https://bugs.kde.org/show_bug.cgi?id=355310


Repository: kiconthemes


Description
---

Currently, KIconDialog::showDialog() sets the modality to Qt::NonModal by 
calling

setModal(false);

I found that this was done in 
https://quickgit.kde.org/?p=kdelibs.git=commit=d0d2639c126f88a44c852b738550a9427c6260bb
 in order to prevent that a modal dialog locks an entire application, such as 
Plasma.

Unfortunately, this has an unwanted side effect in the "Places" of Dolphin and 
the file dialog: the KIconDialog is the child of a modal "Add Places Entry" 
dialog there. The KIconDialog itself works fine, but the "Browse..." sub 
dialog, which is a grand child of the modal dialog, is opened in the background 
and cannot be used (it could be that this worked for some reason in Qt4 times - 
I guess we would have received bug reports about this issue earlier otherwise).

This can be fixed by setting the modality to Qt::WindowModal, which ensures 
that the dialogs block their respective parents (but not the entire application 
- that would happen if the modality was set to Qt::ApplicationModal, for 
example by calling setModal(true)).

Note that there are two setModal(true) calls in the KIconDialog constructors. 
They have no effect if the dialog is opened with showDialog() (which is what 
happens if the dialog is opened by clicking a KIconButton) because the modality 
is overwritten there. I'm not sure though if there are any other uses of 
KIconDialog which would break if the apparently superfluous calls were removed. 
This might need further investigation.


Diffs
-

  src/kicondialog.cpp cca4ed3 

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


Testing
---

The "Browse..." sub dialog of the icon dialog works fine again in Dolphin and 
KWrite's file dialog when creating a new "Place".

I could not test if this affects Plasma somehow because I currently do not have 
a full self-built Plasma session running. It could probably be checked by 
opening the "Properties..." of a file in FolderView, clicking the icon, and 
then opening the "Browse..." sub dialog of the KIconDialog. This should 
hopefully not lock the entire Plasma session (because the dialogs are window 
modal, and not application modal). If anyone finds problems with that or has 
ideas for improvement, please let me know!


Thanks,

Frank Reininghaus

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

Re: Review Request 125515: Preserve relative link targets when copying symlinks.

2015-10-22 Thread Frank Reininghaus


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

Ship it!


4096 bytes looks reasonable. I think I would still find a fail-safe solution 
with a dynamically increasing buffer prettier, but it's so extremely unlikely 
that this will ever cause problems that it's not worth arguing about it :-)

- Frank Reininghaus


On Okt. 10, 2015, 3:29 nachm., David Faure wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125515/
> ---
> 
> (Updated Okt. 10, 2015, 3:29 nachm.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Bugs: 352927
> https://bugs.kde.org/show_bug.cgi?id=352927
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> BUG: 352927
> REVIEW: 125515
> Change-Id: I7d3c988da32cae9d14750c8adb9ca5af6d140572
> 
> 
> Diffs
> -
> 
>   autotests/jobtest.h ef8c3e111ec647cc59c5a9715ab3220ce1651c9e 
>   autotests/jobtest.cpp c24bcead70f78f2bec3b938819fb2fa842e937d5 
>   src/ioslaves/file/file.cpp a0a533c957628b00eff175a949685d4497c5f095 
> 
> Diff: https://git.reviewboard.kde.org/r/125515/diff/
> 
> 
> Testing
> ---
> 
> 2 unit tests added
> 
> 
> Thanks,
> 
> David Faure
> 
>

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

Re: Jenkins-kde-ci: kio master kf5-qt5 » Linux,gcc - Build # 119 - Still Unstable!

2015-10-07 Thread Frank Reininghaus

Am 07.10.2015 22:25 schrieb "Albert Astals Cid" <aa...@kde.org>:
>
> El Tuesday 06 October 2015, a les 19:20:00, Frank Reininghaus va escriure:
> > Hi,
> >
> > 2015-10-06 15:04 GMT+02:00  <no-re...@kde.org>:
> > > GENERAL INFO
> > >
> > > BUILD UNSTABLE
> > > Build URL:
> > >
https://build.kde.org/job/kio%20master%20kf5-qt5/PLATFORM=Linux,compiler=
> > > gcc/119/ Project: PLATFORM=Linux,compiler=gcc
> > > Date of build: Tue, 06 Oct 2015 12:34:13 +
> > > Build duration: 14 min
> > >
> > > CHANGE SET
> > > Revision 4f464d4ade465ad009758d12908f72189953c4f1 by scripty:
(SVN_SILENT
> > > made messages (.desktop file) - always resolve ours)>
> > >   change: edit src/new_file_templates/TextFile.desktop
> > >   change: edit src/widgets/konqpopupmenuplugin.desktop
> > >   change: edit src/new_file_templates/linkProgram.desktop
> > >   change: edit src/new_file_templates/linkPath.desktop
> > >   change: edit src/new_file_templates/linkURL.desktop
> > >   change: edit src/new_file_templates/Directory.desktop
> > >   change: edit src/new_file_templates/HTMLFile.desktop
> > >
> > > JUNIT RESULTS
> > >
> > > Name: (root) Failed: 1 test(s), Passed: 45 test(s), Skipped: 0
test(s),
> > > Total: 46 test(s)Failed: TestSuite.kiofilewidgets-knewfilemenutest
> > it looks like this test failure has something to do with my recent
> > commit that added the "new file" templates. I cannot reproduce it
> > locally though. The log at
> >
> >
https://build.kde.org/job/kio%20master%20kf5-qt5/PLATFORM=Linux,compiler=gcc
> > /119/testReport/%28root%29/TestSuite/kiofilewidgets_knewfilemenutest/
> >
> > indicates that the test tries to use klauncher, which is not running
> > on the server. klauncher is part of kinit, which is apparently not a
> > dependency of kio, maybe this is the problem?
> >
> > The last successful executions of the test simply skipped it, see, e.g.,
> >
> >
https://build.kde.org/job/kio%20master%20kf5-qt5/PLATFORM=Linux,compiler=gcc
> > /117/testReport/%28root%29/TestSuite/kiofilewidgets_knewfilemenutest/
> >
> > At first sight, it looks to me like the test might be broken because
> > it relies on stuff that kio does not officially depend on. It was just
> > not noticed before because KNewFileMenu did not work at all because of
> > the missing templates, such that the test was skipped.
> >
> > Does anyone see a good way to fix this without disabling the test?
>
> So what depends on klauncher the test or knewfilemenu?

I'm not quite sure (cannot check right now because I'm travelling without
my laptop and analyzing code on the phone screen is a bit tedious). Thanks
for bringing up the question - in frameworks,  we should really make sure
that not only build time dependencies, but also run time dependencies are
handled correctly.

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

Re: Review Request 125425: Add the desktop file that is required for adding services to the context menu for files and directories

2015-10-06 Thread Frank Reininghaus

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

(Updated Oct. 6, 2015, 8:30 a.m.)

Status
--

This change has been marked as submitted.

Review request for KDE Frameworks and David Faure.

Changes
---

Submitted with commit 4b24b70c93523c5bc56c90c04a5a666331e96a1b by Frank
Reininghaus to branch master.

Bugs: 350769
https://bugs.kde.org/show_bug.cgi?id=350769

Repository: kio

Description
---

This is a modified version of the file konqpopupmenuplugin.desktop in
kde-baseapps (see
https://quickgit.kde.org/?p=kde-baseapps.git=blob=94a680ac215b4638a0c7cdd2b20bc7830b9619f2=35e8bc2992f48ffaff9007cfbf8faf3c856b18a3=lib%2Fkonq%2Fkonqpopupmenuplugin.desktop
for the latest version).

I modified the name to kioservicemenuplugin.desktop because the file has not
been Konqueror-specific for quite some time. I also updated the 'Comment'
accordingly and removed the outdated translations.

I hope I did that right - any comments are welcome!

Note: Just like https://git.reviewboard.kde.org/r/124983/ this should probably
be pushed to master after the tagging for the next version because of the
translations. On the one hand, the translation of this 'Comment' might not be
that important because the it is not shown anywhere in the UI as far as I know
(it is shown in the 'Type' column in Dolphin though when viewing the directory
where this file is installed). But on the other hand, it might be better to
resolve both context menu issues in the same KIO release. What do others think?

Diffs
-

src/widgets/CMakeLists.txt 820cd34
src/widgets/konqpopupmenuplugin.desktop PRE-CREATION

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

Testing
---

Konsole service actions are shown in the context menu again.

Thanks,

Frank Reininghaus

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

Re: Review Request 124983: Move the desktop files and file templates for the "Create New..." menu from kde-baseapps to kio

2015-10-06 Thread Frank Reininghaus


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

(Updated Oct. 6, 2015, 8:30 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and David Faure.


Changes
---

Submitted with commit 1b98a313983b75f3340639e5cd9519c56fca7113 by Frank 
Reininghaus to branch master.


Bugs: 349654
https://bugs.kde.org/show_bug.cgi?id=349654


Repository: kio


Description
---

Currently, the standard entries of KNewFileMenu are in lib/konq in the 
kde-baseapps repository, which has no released KF5 version. Therefore, the menu 
is empty now for most users. Those who build their KDE software with 
kdesrc-build, whose standard KF5 config includes kde-baseapps, might now have 
noticed the issue yet.

This patch simply moves the relevant files to kio. Some questions that might be 
worth discussing:

* Is kio considered the correct place for this, or should the files be in 
kio-extras?
* Are all the menu entries still relevant, or should some be removed or 
modified?
* Is it OK to move the desktop files including translations, or will this 
confuse the i18n infrastructure that our brave translators use?


Diffs
-

  src/CMakeLists.txt a1d644d 
  src/new_file_templates/CMakeLists.txt PRE-CREATION 
  src/new_file_templates/Directory.desktop PRE-CREATION 
  src/new_file_templates/HTMLFile.desktop PRE-CREATION 
  src/new_file_templates/HTMLFile.html PRE-CREATION 
  src/new_file_templates/Program.desktop PRE-CREATION 
  src/new_file_templates/TextFile.desktop PRE-CREATION 
  src/new_file_templates/TextFile.txt PRE-CREATION 
  src/new_file_templates/URL.desktop PRE-CREATION 
  src/new_file_templates/linkPath.desktop PRE-CREATION 
  src/new_file_templates/linkProgram.desktop PRE-CREATION 
  src/new_file_templates/linkURL.desktop PRE-CREATION 

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


Testing
---

I disabled the build of kde-baseapps locally. The "Create New..." menu was 
empty then, but it is populated again with this patch.


Thanks,

Frank Reininghaus

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

Re: Jenkins-kde-ci: kio master kf5-qt5 » Linux,gcc - Build # 119 - Still Unstable!

2015-10-06 Thread Frank Reininghaus

Hi,

2015-10-06 15:04 GMT+02:00  :
>
> GENERAL INFO
>
> BUILD UNSTABLE
> Build URL: 
> https://build.kde.org/job/kio%20master%20kf5-qt5/PLATFORM=Linux,compiler=gcc/119/
> Project: PLATFORM=Linux,compiler=gcc
> Date of build: Tue, 06 Oct 2015 12:34:13 +
> Build duration: 14 min
>
> CHANGE SET
> Revision 4f464d4ade465ad009758d12908f72189953c4f1 by scripty: (SVN_SILENT 
> made messages (.desktop file) - always resolve ours)
>   change: edit src/new_file_templates/TextFile.desktop
>   change: edit src/widgets/konqpopupmenuplugin.desktop
>   change: edit src/new_file_templates/linkProgram.desktop
>   change: edit src/new_file_templates/linkPath.desktop
>   change: edit src/new_file_templates/linkURL.desktop
>   change: edit src/new_file_templates/Directory.desktop
>   change: edit src/new_file_templates/HTMLFile.desktop
>
>
> JUNIT RESULTS
>
> Name: (root) Failed: 1 test(s), Passed: 45 test(s), Skipped: 0 test(s), 
> Total: 46 test(s)Failed: TestSuite.kiofilewidgets-knewfilemenutest

it looks like this test failure has something to do with my recent
commit that added the "new file" templates. I cannot reproduce it
locally though. The log at

https://build.kde.org/job/kio%20master%20kf5-qt5/PLATFORM=Linux,compiler=gcc/119/testReport/%28root%29/TestSuite/kiofilewidgets_knewfilemenutest/

indicates that the test tries to use klauncher, which is not running
on the server. klauncher is part of kinit, which is apparently not a
dependency of kio, maybe this is the problem?

The last successful executions of the test simply skipped it, see, e.g.,

https://build.kde.org/job/kio%20master%20kf5-qt5/PLATFORM=Linux,compiler=gcc/117/testReport/%28root%29/TestSuite/kiofilewidgets_knewfilemenutest/

At first sight, it looks to me like the test might be broken because
it relies on stuff that kio does not officially depend on. It was just
not noticed before because KNewFileMenu did not work at all because of
the missing templates, such that the test was skipped.

Does anyone see a good way to fix this without disabling the test?

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

Re: Review Request 125515: Preserve relative link targets when copying symlinks.

2015-10-04 Thread Frank Reininghaus


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


Looks good! The only thing that I'm not sure about is whether 1000 bytes are 
guaranteed to be enough. Some quick Googling tells me that path lengths of 4096 
are possible. Maybe we could allocate a temporary large array on the heap if 
the readlink call fails with the 1000 byte buffer on the stack?

- Frank Reininghaus


On Okt. 4, 2015, 9:24 vorm., David Faure wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125515/
> ---
> 
> (Updated Okt. 4, 2015, 9:24 vorm.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Bugs: 352927
> https://bugs.kde.org/show_bug.cgi?id=352927
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> BUG: 352927
> Change-Id: I7d3c988da32cae9d14750c8adb9ca5af6d140572
> 
> 
> Diffs
> -
> 
>   autotests/jobtest.h ef8c3e111ec647cc59c5a9715ab3220ce1651c9e 
>   src/ioslaves/file/file.cpp a0a533c957628b00eff175a949685d4497c5f095 
>   autotests/jobtest.cpp c24bcead70f78f2bec3b938819fb2fa842e937d5 
> 
> Diff: https://git.reviewboard.kde.org/r/125515/diff/
> 
> 
> Testing
> ---
> 
> 2 unit tests added
> 
> 
> Thanks,
> 
> David Faure
> 
>

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

Re: Review Request 125425: Add the desktop file that is required for adding services to the context menu for files and directories

2015-10-04 Thread Frank Reininghaus



> On Okt. 3, 2015, 8:08 vorm., David Faure wrote:
> > I just realized another problem with this approach: since the latest 
> > libkonq release installs konqpopupmenuplugin.desktop already, this patch 
> > will make the next KIO release conflict with that last libkonq release 
> > (which by definition cannot have a version check). Moving stuff between 
> > products is hard!
> > 
> > So you were right, better install this under a different filename like in 
> > your v1 of the patch (just doublecheck that having two definitions of the 
> > same servicetype in two different files doesn't break things, but I don't 
> > think it does). And in the second step (later) we can reuse that filename 
> > to provide a different servicetypename and deprecate KonqPopupMenuPlugin.
> > Sorry for not realizing this sooner.
> > 
> > In any case, due to the issue with translations we wanted to commit this 
> > after today's release (but of course this delays the fix by one month).
> > Unless Luigi is available this weekend to do the merging, but it's getting 
> > tight.
> 
> Luigi Toscano wrote:
> I would say that it's better to postpone for the next release (merge 
> immediately after the tag), so there is a bit more time to catch issues and 
> it's not risky for this release (too tight now, exactly).
> 
> Hrvoje Senjan wrote:
> > since the latest libkonq release installs konqpopupmenuplugin.desktop 
> already
> 
> But that release is kdelibs4 based, so there shouldn't be any conflict 
> here...
> 
> David Faure wrote:
> Oh, is there really no release that installs konqpopupmenuplugin.desktop 
> into servicestypes5? I thought people said otherwise in bug reports, but I 
> didn't fully follow that (and looking again now I indeed don't see that 
> clearly said).
> 
> If this was never released it's easier indeed, this patch can go in 
> (after the release) then.

Yes, there is currently no KF5-based release of anything that installs this 
file. Service menus might work for some users though if distros package 
lib/konq (which is ported to KF5, but not released).

I'll push this and the commit for the "Create New" menu issue tomorrow if there 
are no objections. Maybe I'll also send a mail to the kde-packager list, such 
that the packagers can decide if they want to backport them. This would prevent 
that users will have to wait until November (or even longer, if distros do not 
update the KF5 libs regularly) until they get a functional context menu again.


- Frank


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


On Okt. 3, 2015, 7:50 vorm., Frank Reininghaus wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125425/
> ---
> 
> (Updated Okt. 3, 2015, 7:50 vorm.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Bugs: 350769
> https://bugs.kde.org/show_bug.cgi?id=350769
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> This is a modified version of the file konqpopupmenuplugin.desktop in 
> kde-baseapps (see 
> https://quickgit.kde.org/?p=kde-baseapps.git=blob=94a680ac215b4638a0c7cdd2b20bc7830b9619f2=35e8bc2992f48ffaff9007cfbf8faf3c856b18a3=lib%2Fkonq%2Fkonqpopupmenuplugin.desktop
>  for the latest version).
> 
> I modified the name to kioservicemenuplugin.desktop because the file has not 
> been Konqueror-specific for quite some time. I also updated the 'Comment' 
> accordingly and removed the outdated translations.
> 
> I hope I did that right - any comments are welcome!
> 
> Note: Just like https://git.reviewboard.kde.org/r/124983/ this should 
> probably be pushed to master after the tagging for the next version because 
> of the translations. On the one hand, the translation of this 'Comment' might 
> not be that important because the it is not shown anywhere in the UI as far 
> as I know (it is shown in the 'Type' column in Dolphin though when viewing 
> the directory where this file is installed). But on the other hand, it might 
> be better to resolve both context menu issues in the same KIO release. What 
> do others think?
> 
> 
> Diffs
> -
> 
>   src/widgets/CMakeLists.txt 820cd34 
>   src/widgets/konqpopupmenuplugin.desktop PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/125425/diff/
> 
> 
> Testing
> ---
> 
> Konsole service actions are shown in the context menu again.
> 
> 
> Thanks,
> 
> Frank Reininghaus
> 
>

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

Re: Review Request 125425: Add the desktop file that is required for adding services to the context menu for files and directories

2015-10-03 Thread Frank Reininghaus

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

(Updated Okt. 3, 2015, 7:50 vorm.)

Review request for KDE Frameworks and David Faure.

Changes
---

I propose to do this in two steps:

1. Move konqpopupmenu.plugin from kde-baseapps to kio (this is what the current
version of this review does).
2. Add a new KIOServiceMenuPlugin service type, make it work like
KonqPopupMenu/Plugin, and log a warning if a plugin only implements the latter.

I'm willing to also do step 2, but after I had a quick look at the code, I
think that KFileItemActions::addServiceActionsTo needs some refactoring first.
I currently don't have the time for that, but I think that the absence of
service menu plugins should be fixed rather sooner than later, so I think that
implementing step 1 first makes sense.

If this is approved, then I will push it together with
https://git.reviewboard.kde.org/r/124983/ to minimize the trouble for the
translators and see how the installation of the files can be suppressed in
lib/konq if the KIO version is high enough.

Bugs: 350769
https://bugs.kde.org/show_bug.cgi?id=350769

Repository: kio

Description
---

I hope I did that right - any comments are welcome!

Diffs (updated)
-

src/widgets/CMakeLists.txt 820cd34
src/widgets/konqpopupmenuplugin.desktop PRE-CREATION

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

Testing
---

Konsole service actions are shown in the context menu again.

Thanks,

Frank Reininghaus

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

Re: Review Request 125425: Add the desktop file that is required for adding services to the context menu for files and directories

2015-09-30 Thread Frank Reininghaus



> On Sept. 28, 2015, 7:30 vorm., David Faure wrote:
> > The filename not matching the servicetype name in it, is very confusing.
> > 
> > How about deprecating KonqPopupMenu/Plugin, introducing a 
> > KIOServiceMenuPlugin servicetype, installing desktop files for both, 
> > querying for both and skipping the installation of 
> > konqpopupmenuplugin.desktop if the KIO version is > 5.15?
> 
> Frank Reininghaus wrote:
> If we change the ServiceType entry in the file, then every service menu 
> will have to be changed, right? Not only those that are installed by 
> applications and libraries that are hosted on git.kde.org (which we could at 
> least find and fix), but also service menus that are released on 
> kde-apps.org, or unreleased menus that users have written for themselves. I 
> thought that it's unrealistic that this happens for every single service 
> menu, that's why I thought that the KonqPopupMenu/Plugin type should be kept. 
> Or am I overlooking something? If not, then I'm happy to change the file name 
> - probably having a konqpopupmenuplugin.desktop in KIO is really less 
> confusing than the ServiceType/file name mismatch.
> 
> David Faure wrote:
> Yes this is why I was suggesting to install desktop files for both names.
> Deprecating != removing.
> 
> Make both servicetype names work, but issue a warning if a plugin only 
> implements KonqPopupMenu/Plugin and not KIOServiceMenuPlugin (both is ok, 
> it's a way to make it work for older versions of KF5). Only 
> KIOServiceMenuPlugin is ok of course.
> 
> Anyway, that's more work, if you want to just rename the file and move on 
> I'm ok with that.
> In any case, make sure to avoid the conflict between kio and libkonq 
> installing the same file, with a version check.

Thanks David for the clarification! Yes, that makes a lot of sense. Sorry for 
the misunderstanding - I did not realize that you meant "skipping the 
installation of konqpopupmenuplugin.desktop *in kde-baseapps* if the KIO 
version is > 5.15". I'll update the patch soon.


- Frank


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


On Sept. 27, 2015, 6:18 nachm., Frank Reininghaus wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125425/
> ---
> 
> (Updated Sept. 27, 2015, 6:18 nachm.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Bugs: 350769
> https://bugs.kde.org/show_bug.cgi?id=350769
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> This is a modified version of the file konqpopupmenuplugin.desktop in 
> kde-baseapps (see 
> https://quickgit.kde.org/?p=kde-baseapps.git=blob=94a680ac215b4638a0c7cdd2b20bc7830b9619f2=35e8bc2992f48ffaff9007cfbf8faf3c856b18a3=lib%2Fkonq%2Fkonqpopupmenuplugin.desktop
>  for the latest version).
> 
> I modified the name to kioservicemenuplugin.desktop because the file has not 
> been Konqueror-specific for quite some time. I also updated the 'Comment' 
> accordingly and removed the outdated translations.
> 
> I hope I did that right - any comments are welcome!
> 
> Note: Just like https://git.reviewboard.kde.org/r/124983/ this should 
> probably be pushed to master after the tagging for the next version because 
> of the translations. On the one hand, the translation of this 'Comment' might 
> not be that important because the it is not shown anywhere in the UI as far 
> as I know (it is shown in the 'Type' column in Dolphin though when viewing 
> the directory where this file is installed). But on the other hand, it might 
> be better to resolve both context menu issues in the same KIO release. What 
> do others think?
> 
> 
> Diffs
> -
> 
>   src/widgets/CMakeLists.txt 820cd34 
>   src/widgets/kioservicemenuplugin.desktop PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/125425/diff/
> 
> 
> Testing
> ---
> 
> Konsole service actions are shown in the context menu again.
> 
> 
> Thanks,
> 
> Frank Reininghaus
> 
>

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

Re: Review Request 125425: Add the desktop file that is required for adding services to the context menu for files and directories

2015-09-29 Thread Frank Reininghaus



> On Sept. 28, 2015, 7:30 vorm., David Faure wrote:
> > The filename not matching the servicetype name in it, is very confusing.
> > 
> > How about deprecating KonqPopupMenu/Plugin, introducing a 
> > KIOServiceMenuPlugin servicetype, installing desktop files for both, 
> > querying for both and skipping the installation of 
> > konqpopupmenuplugin.desktop if the KIO version is > 5.15?

If we change the ServiceType entry in the file, then every service menu will 
have to be changed, right? Not only those that are installed by applications 
and libraries that are hosted on git.kde.org (which we could at least find and 
fix), but also service menus that are released on kde-apps.org, or unreleased 
menus that users have written for themselves. I thought that it's unrealistic 
that this happens for every single service menu, that's why I thought that the 
KonqPopupMenu/Plugin type should be kept. Or am I overlooking something? If 
not, then I'm happy to change the file name - probably having a 
konqpopupmenuplugin.desktop in KIO is really less confusing than the 
ServiceType/file name mismatch.


- Frank


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


On Sept. 27, 2015, 6:18 nachm., Frank Reininghaus wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125425/
> ---
> 
> (Updated Sept. 27, 2015, 6:18 nachm.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Bugs: 350769
> https://bugs.kde.org/show_bug.cgi?id=350769
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> This is a modified version of the file konqpopupmenuplugin.desktop in 
> kde-baseapps (see 
> https://quickgit.kde.org/?p=kde-baseapps.git=blob=94a680ac215b4638a0c7cdd2b20bc7830b9619f2=35e8bc2992f48ffaff9007cfbf8faf3c856b18a3=lib%2Fkonq%2Fkonqpopupmenuplugin.desktop
>  for the latest version).
> 
> I modified the name to kioservicemenuplugin.desktop because the file has not 
> been Konqueror-specific for quite some time. I also updated the 'Comment' 
> accordingly and removed the outdated translations.
> 
> I hope I did that right - any comments are welcome!
> 
> Note: Just like https://git.reviewboard.kde.org/r/124983/ this should 
> probably be pushed to master after the tagging for the next version because 
> of the translations. On the one hand, the translation of this 'Comment' might 
> not be that important because the it is not shown anywhere in the UI as far 
> as I know (it is shown in the 'Type' column in Dolphin though when viewing 
> the directory where this file is installed). But on the other hand, it might 
> be better to resolve both context menu issues in the same KIO release. What 
> do others think?
> 
> 
> Diffs
> -
> 
>   src/widgets/CMakeLists.txt 820cd34 
>   src/widgets/kioservicemenuplugin.desktop PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/125425/diff/
> 
> 
> Testing
> ---
> 
> Konsole service actions are shown in the context menu again.
> 
> 
> Thanks,
> 
> Frank Reininghaus
> 
>

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

Review Request 125425: Add the desktop file that is required for adding services to the context menu for files and directories

2015-09-27 Thread Frank Reininghaus

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

Review request for KDE Frameworks and David Faure.

Bugs: 350769
https://bugs.kde.org/show_bug.cgi?id=350769

Repository: kio

Description
---

I hope I did that right - any comments are welcome!

Diffs
-

src/widgets/CMakeLists.txt 820cd34
src/widgets/kioservicemenuplugin.desktop PRE-CREATION

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

Testing
---

Konsole service actions are shown in the context menu again.

Thanks,

Frank Reininghaus

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

Re: Review Request 124983: Move the desktop files and file templates for the "Create New..." menu from kde-baseapps to kio

2015-09-22 Thread Frank Reininghaus


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

(Updated Sept. 22, 2015, 4:37 nachm.)


Review request for KDE Frameworks and David Faure.


Changes
---

Thanks for the comments, David, Luigi and Albert!

I've removed all devices, as suggested by David (we can actually also remove 
the "Link to Device" menu from KNewFileMenu then, but I'll do that in a second 
patch once the present change is in master).

If there are any further suggestions for improvement, please let me know, such 
that they can be included when I push this to master after the next KF5 release.


Bugs: 349654
https://bugs.kde.org/show_bug.cgi?id=349654


Repository: kio


Description
---

Currently, the standard entries of KNewFileMenu are in lib/konq in the 
kde-baseapps repository, which has no released KF5 version. Therefore, the menu 
is empty now for most users. Those who build their KDE software with 
kdesrc-build, whose standard KF5 config includes kde-baseapps, might now have 
noticed the issue yet.

This patch simply moves the relevant files to kio. Some questions that might be 
worth discussing:

* Is kio considered the correct place for this, or should the files be in 
kio-extras?
* Are all the menu entries still relevant, or should some be removed or 
modified?
* Is it OK to move the desktop files including translations, or will this 
confuse the i18n infrastructure that our brave translators use?


Diffs (updated)
-

  src/CMakeLists.txt a1d644d 
  src/new_file_templates/CMakeLists.txt PRE-CREATION 
  src/new_file_templates/Directory.desktop PRE-CREATION 
  src/new_file_templates/HTMLFile.desktop PRE-CREATION 
  src/new_file_templates/HTMLFile.html PRE-CREATION 
  src/new_file_templates/Program.desktop PRE-CREATION 
  src/new_file_templates/TextFile.desktop PRE-CREATION 
  src/new_file_templates/TextFile.txt PRE-CREATION 
  src/new_file_templates/URL.desktop PRE-CREATION 
  src/new_file_templates/linkPath.desktop PRE-CREATION 
  src/new_file_templates/linkProgram.desktop PRE-CREATION 
  src/new_file_templates/linkURL.desktop PRE-CREATION 

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


Testing
---

I disabled the build of kde-baseapps locally. The "Create New..." menu was 
empty then, but it is populated again with this patch.


Thanks,

Frank Reininghaus

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

Review Request 125268: Fix signal-slot connections in KNewFileMenuPrivate::confirmCreatingHiddenDir(QString)

2015-09-16 Thread Frank Reininghaus


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

Review request for KDE Frameworks and David Faure.


Bugs: 352770
https://bugs.kde.org/show_bug.cgi?id=352770


Repository: kio


Description
---

Currently, the buttons in the dialog that is shown when trying to create a 
hidden directory (e.g., by pressing F10 in Dolphin and then entering a name 
that starts with a dot) do not work. This is because the slots are connected to 
the 'accepted' and 'rejected' signals of the QDialog. Actually, these signals 
are emitted by the QDialogButtonBox.


Diffs
-

  src/filewidgets/knewfilemenu.cpp 2e8f9c6 

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


Testing
---

Both buttons in the dialog work now: it is possible to create hidden 
directories, and it is also possible to choose a new name.


Thanks,

Frank Reininghaus

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

Re: Review Request 125268: Fix signal-slot connections in KNewFileMenuPrivate::confirmCreatingHiddenDir(QString)

2015-09-16 Thread Frank Reininghaus


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

(Updated Sept. 16, 2015, 9:01 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and David Faure.


Changes
---

Submitted with commit 689fd43ae226ad026dd2fb6e198a5fef40c24b13 by Frank 
Reininghaus to branch master.


Bugs: 352770
https://bugs.kde.org/show_bug.cgi?id=352770


Repository: kio


Description
---

Currently, the buttons in the dialog that is shown when trying to create a 
hidden directory (e.g., by pressing F10 in Dolphin and then entering a name 
that starts with a dot) do not work. This is because the slots are connected to 
the 'accepted' and 'rejected' signals of the QDialog. Actually, these signals 
are emitted by the QDialogButtonBox.


Diffs
-

  src/filewidgets/knewfilemenu.cpp 2e8f9c6 

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


Testing
---

Both buttons in the dialog work now: it is possible to create hidden 
directories, and it is also possible to choose a new name.


Thanks,

Frank Reininghaus

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

Re: Review Request 125158: add logic to use icons for default xdg user dirs

2015-09-12 Thread Frank Reininghaus

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

This looks quite inefficient to me. Every time KFileItem::iconName() is called, 
you iterate through the QMap (by the way, you could just as well use a vector 
of pairs if you don't do any map lookups), create 8 temporary QStringLists by 
calling QStandardPaths::standardLocations(it.key()), and then iterate through 
each of these lists and compare localDirectory to every list item.

I think it would be much better to create a static map that maps the location 
strings to the icon names directly. Then you could do everything with a single 
lookup in the map.

I'm still not sure if I understand why the process which creates these 
directories cannot just add a .directory file with the desired icon name. Is 
there a reason why the problem can't be solved this way?

- Frank Reininghaus

On Sept. 11, 2015, 10:50 vorm., Harald Sitter wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125158/
> ---
> 
> (Updated Sept. 11, 2015, 10:50 vorm.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Bugs: 352498
> https://bugs.kde.org/show_bug.cgi?id=352498
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> BUG: 352498
> 
> 
> Diffs
> -
> 
>   autotests/kfileitemtest.h 615324f2b45fdc90a7841bdd0c8aa7f47cdf57a2 
>   autotests/kfileitemtest.cpp 5f728a411401fe3009924b66970d9ae6f12c60f2 
>   src/core/kfileitem.cpp 966d8626708a8f2672f1777c873f4e27e13023d6 
> 
> Diff: https://git.reviewboard.kde.org/r/125158/diff/
> 
> 
> Testing
> ---
> 
> maked
> autotested
> installed
> dolphin and file open dialogs now show icons
> 
> 
> Thanks,
> 
> Harald Sitter
> 
>

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

Review Request 124983: Move the desktop files and file templates for the Create New... menu from kde-baseapps to kio

2015-08-29 Thread Frank Reininghaus


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

Review request for KDE Frameworks and David Faure.


Bugs: 349654
https://bugs.kde.org/show_bug.cgi?id=349654


Repository: kio


Description
---

Currently, the standard entries of KNewFileMenu are in lib/konq in the 
kde-baseapps repository, which has no released KF5 version. Therefore, the menu 
is empty now for most users. Those who build their KDE software with 
kdesrc-build, whose standard KF5 config includes kde-baseapps, might now have 
noticed the issue yet.

This patch simply moves the relevant files to kio. Some questions that might be 
worth discussing:

* Is kio considered the correct place for this, or should the files be in 
kio-extras?
* Are all the menu entries still relevant, or should some be removed or 
modified?
* Is it OK to move the desktop files including translations, or will this 
confuse the i18n infrastructure that our brave translators use?


Diffs
-

  src/CMakeLists.txt a1d644d 
  src/new_file_templates/CAMERA-Device.desktop PRE-CREATION 
  src/new_file_templates/CDROM-Device.desktop PRE-CREATION 
  src/new_file_templates/CDWRITER-Device.desktop PRE-CREATION 
  src/new_file_templates/CMakeLists.txt PRE-CREATION 
  src/new_file_templates/DVDROM-Device.desktop PRE-CREATION 
  src/new_file_templates/Directory.desktop PRE-CREATION 
  src/new_file_templates/Floppy.desktop PRE-CREATION 
  src/new_file_templates/HD.desktop PRE-CREATION 
  src/new_file_templates/HTMLFile.desktop PRE-CREATION 
  src/new_file_templates/HTMLFile.html PRE-CREATION 
  src/new_file_templates/MO-Device.desktop PRE-CREATION 
  src/new_file_templates/NFS.desktop PRE-CREATION 
  src/new_file_templates/Program.desktop PRE-CREATION 
  src/new_file_templates/TextFile.desktop PRE-CREATION 
  src/new_file_templates/TextFile.txt PRE-CREATION 
  src/new_file_templates/URL.desktop PRE-CREATION 
  src/new_file_templates/ZIP-Device.desktop PRE-CREATION 
  src/new_file_templates/linkCAMERA.desktop PRE-CREATION 
  src/new_file_templates/linkCDROM.desktop PRE-CREATION 
  src/new_file_templates/linkCDWRITER.desktop PRE-CREATION 
  src/new_file_templates/linkDVDROM.desktop PRE-CREATION 
  src/new_file_templates/linkFloppy.desktop PRE-CREATION 
  src/new_file_templates/linkHD.desktop PRE-CREATION 
  src/new_file_templates/linkMO.desktop PRE-CREATION 
  src/new_file_templates/linkNFS.desktop PRE-CREATION 
  src/new_file_templates/linkPath.desktop PRE-CREATION 
  src/new_file_templates/linkProgram.desktop PRE-CREATION 
  src/new_file_templates/linkURL.desktop PRE-CREATION 
  src/new_file_templates/linkZIP.desktop PRE-CREATION 

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


Testing
---

I disabled the build of kde-baseapps locally. The Create New... menu was 
empty then, but it is populated again with this patch.


Thanks,

Frank Reininghaus

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

Re: Review Request 123724: Use QTemporaryFile instead of hardcoding /tmp.

2015-05-12 Thread Frank Reininghaus



 On Mai 12, 2015, 3:49 nachm., Jan Kundrát wrote:
  Was the old code a part of some release? If yes, this should get a CVE 
  security announcement because it allows a local attacker to e.g. force you 
  to overwirte some of your user's files.
 
 Michael Palimaka wrote:
 It looks like it was introduced in 
 999e774b3ce117598df2029364bd10f4347be81c and released in 0.2.0 and later.

Could you elaborate on how such an attack would work? Even if we ignore that 
the code in question is part of an autotest which is probably never installed 
anywhere, such that systems of packagers, developers and users who build from 
source are the only possible targets, I really don't see how an attacker could 
use the code to cause any unintended damage. Anyone who runs the test regularly 
creates and deletes the file /tmp/kpeople_test_db already, so what other damage 
could a local attacker cause?


- Frank


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


On Mai 12, 2015, 12:49 nachm., Michael Palimaka wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123724/
 ---
 
 (Updated Mai 12, 2015, 12:49 nachm.)
 
 
 Review request for KDE Frameworks and KDEPIM.
 
 
 Repository: kpeople
 
 
 Description
 ---
 
 Hardcoding files like this seems like a bad idea.
 
 
 Diffs
 -
 
   autotests/persondatatests.h 30eeeb5cd647c713f1b438543a54516ced9f3ede 
   autotests/persondatatests.cpp 73098d3717509ad80761bbd02000b4ce5060bbb2 
   autotests/personsmodeltest.h 5b8879521f334459c4f73c2708b3368c543e40a3 
   autotests/personsmodeltest.cpp b19d1baf8a2c2e617d4b6128df29fbab3b8e61a7 
 
 Diff: https://git.reviewboard.kde.org/r/123724/diff/
 
 
 Testing
 ---
 
 Tests still pass.
 
 
 Thanks,
 
 Michael Palimaka
 


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

Re: OSX/CI: kio placed files erroneously due to missing required backslash in path

2014-12-28 Thread Frank Reininghaus

Hi,

2014-12-23 9:44 GMT+01:00 David Faure:
 On Tuesday 23 December 2014 00:59:56 Marko Käning wrote:
   Fixed in
 http://commits.kde.org/kio/c5522b6931908d3fd8ad97555a3edf2a3e859b50

 Ooops, should I have pushed this through Gerrit before committing?

 Nope, that's fine, trivial fix. Thanks!

Wouldn't it be better to use QDir::separator() though, in order to
make sure that it works on non-Unix operating systems?

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

Re: Review Request 118452: Reduce the memory usage of UDSEntry by using QVector, rather than QHash, for the internal data storage

2014-12-28 Thread Frank Reininghaus

On Dez. 20, 2014, 11:46 vorm., David Faure wrote:
src/core/udsentry.cpp, line 51
https://git.reviewboard.kde.org/r/118452/diff/3/?file=332749#file332749line51

Can you add an example, to ease the understanding of the data structure?

Do I understand it correctly that this is how it works?
udsIndexes=[UDS_NAME, UDS_FILE_SIZE, ...]
and fields=[Field(filename), Field(1234), ...]

and therefore the two vectors will always have the same size?

Yes indeed, thanks for bringing that issue up! I'll add the example and then
push the commit in a minute.

- Frank

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

On Dez. 11, 2014, 10:08 nachm., Frank Reininghaus wrote:

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

(Updated Dez. 11, 2014, 10:08 nachm.)

Review request for KDE Frameworks and David Faure.

Repository: kio

Description
---

I am continuing to split up https://git.reviewboard.kde.org/r/113355/ , which
attempts to make UDSEntry more efficient memory and CPU-wise, into
independent parts. This is the third step after
https://git.reviewboard.kde.org/r/113591/ and
https://git.reviewboard.kde.org/r/115739/ .

The present patch modifies the internal data storage of UDSEntry. UDSEntry
contains a mapping from unsigned int keys to Field values, where Field is a
struct that contains a QString and a long long (some keys correspond to
numeric values, like size, date, etc, whereas others, like user and group,
correspond to a QString).

Currently, UDSEntry stores all data in a QHashuint, Field internally. This
ensures that everything can be accessed in O(1) time, but is not very
efficient memory-wise because a separate memory allocation is done for each
hash node.

I propose to change this and store both the uint keys and the Field values in
a QVector each. This means that accessing a value becomes a O(n) operation,
since the entire QVector of keys may need to be scanned in order to find a
value, but because the number n of values in a UDSEntry is usually quite
small and can currently not exceed a number ~100, this should not matter in
practice.

Some parts of https://git.reviewboard.kde.org/r/113355/ are still missing:

(a) The QVectors which store the keys (which are usually the same for all
items in a directory) are not shared yet. Changing this would reduce the
memory usage further, but I decided to keep this change separate in order to
keep the current patch small and easy to understand. Moreover, this makes it
easier to benchmark other similar approaches (e.g., replace QVector by
std::vector, or store keys and values together in a
std::vectorstd::pairuint,Field).

(b) No space is reserved in the vectors when key/value pairs are inserted one
by one. Implementing this would make UDSEntry faster on the slave side (since
repeated re-allocations would not be necessary any more), but this can be
done in a later patch. Moreover, it might not be needed any more if UDSEntry
is not used directly any more on the slave side, as suggested on the
frameworks mailing list by Aaron (good idea BTW!).

Diffs
-

autotests/udsentry_benchmark.cpp b5fa8d6
src/core/udsentry.h 98a7035
src/core/udsentry.cpp b806e0e
src/ioslaves/file/file.cpp 1a2a767
tests/udsentrybenchmark.cpp 9bedb7b

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

Testing
---

Unit tests still pass.

The memory usage of listjobtest with a directory with 100,000 files is
reduced from 71344 K to 35392 K according to KSysGuard. I see similar savings
when opening the directory in Dolphin.

I still haven't set up a Qt5/KF5 build in release mode (shame on me!), so I
cannot present any benchmark results.

File Attachments

Benchmark results

https://git.reviewboard.kde.org/media/uploaded/files/2014/12/09/038e443c-78eb-443b-b33a-b451b28d10ea__UDSEntry-benchmarks.png

Thanks,

Frank Reininghaus

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

Re: Review Request 118452: Reduce the memory usage of UDSEntry by using QVector, rather than QHash, for the internal data storage

2014-12-28 Thread Frank Reininghaus

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

(Updated Dez. 28, 2014, 10:22 nachm.)

Review request for KDE Frameworks and David Faure.

Repository: kio

Description
---

I am continuing to split up https://git.reviewboard.kde.org/r/113355/ , which
attempts to make UDSEntry more efficient memory and CPU-wise, into independent
parts. This is the third step after
https://git.reviewboard.kde.org/r/113591/ and
https://git.reviewboard.kde.org/r/115739/ .

The present patch modifies the internal data storage of UDSEntry. UDSEntry
contains a mapping from unsigned int keys to Field values, where Field is a
struct that contains a QString and a long long (some keys correspond to numeric
values, like size, date, etc, whereas others, like user and group, correspond
to a QString).

Currently, UDSEntry stores all data in a QHashuint, Field internally. This
ensures that everything can be accessed in O(1) time, but is not very efficient
memory-wise because a separate memory allocation is done for each hash node.

I propose to change this and store both the uint keys and the Field values in a
QVector each. This means that accessing a value becomes a O(n) operation, since
the entire QVector of keys may need to be scanned in order to find a value, but
because the number n of values in a UDSEntry is usually quite small and can
currently not exceed a number ~100, this should not matter in practice.

Some parts of https://git.reviewboard.kde.org/r/113355/ are still missing:

(a) The QVectors which store the keys (which are usually the same for all items
in a directory) are not shared yet. Changing this would reduce the memory usage
further, but I decided to keep this change separate in order to keep the
current patch small and easy to understand. Moreover, this makes it easier to
benchmark other similar approaches (e.g., replace QVector by std::vector, or
store keys and values together in a std::vectorstd::pairuint,Field).

(b) No space is reserved in the vectors when key/value pairs are inserted one
by one. Implementing this would make UDSEntry faster on the slave side (since
repeated re-allocations would not be necessary any more), but this can be done
in a later patch. Moreover, it might not be needed any more if UDSEntry is not
used directly any more on the slave side, as suggested on the frameworks
mailing list by Aaron (good idea BTW!).

Diffs (updated)
-

tests/udsentrybenchmark.cpp 9bedb7b
autotests/udsentry_benchmark.cpp b5fa8d6
src/core/udsentry.h 98a7035
src/core/udsentry.cpp b806e0e
src/ioslaves/file/file.cpp 1a2a767

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

Testing
---

Unit tests still pass.

The memory usage of listjobtest with a directory with 100,000 files is reduced
from 71344 K to 35392 K according to KSysGuard. I see similar savings when
opening the directory in Dolphin.

I still haven't set up a Qt5/KF5 build in release mode (shame on me!), so I
cannot present any benchmark results.

File Attachments

Benchmark results

https://git.reviewboard.kde.org/media/uploaded/files/2014/12/09/038e443c-78eb-443b-b33a-b451b28d10ea__UDSEntry-benchmarks.png

Thanks,

Frank Reininghaus

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

Re: Review Request 118452: Reduce the memory usage of UDSEntry by using QVector, rather than QHash, for the internal data storage

2014-12-28 Thread Frank Reininghaus

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

(Updated Dec. 28, 2014, 10:22 p.m.)

Status
--

This change has been marked as submitted.

Review request for KDE Frameworks and David Faure.

Repository: kio

Description
---

I am continuing to split up https://git.reviewboard.kde.org/r/113355/ , which
attempts to make UDSEntry more efficient memory and CPU-wise, into independent
parts. This is the third step after
https://git.reviewboard.kde.org/r/113591/ and
https://git.reviewboard.kde.org/r/115739/ .

The present patch modifies the internal data storage of UDSEntry. UDSEntry
contains a mapping from unsigned int keys to Field values, where Field is a
struct that contains a QString and a long long (some keys correspond to numeric
values, like size, date, etc, whereas others, like user and group, correspond
to a QString).

Currently, UDSEntry stores all data in a QHashuint, Field internally. This
ensures that everything can be accessed in O(1) time, but is not very efficient
memory-wise because a separate memory allocation is done for each hash node.

I propose to change this and store both the uint keys and the Field values in a
QVector each. This means that accessing a value becomes a O(n) operation, since
the entire QVector of keys may need to be scanned in order to find a value, but
because the number n of values in a UDSEntry is usually quite small and can
currently not exceed a number ~100, this should not matter in practice.

Some parts of https://git.reviewboard.kde.org/r/113355/ are still missing:

(a) The QVectors which store the keys (which are usually the same for all items
in a directory) are not shared yet. Changing this would reduce the memory usage
further, but I decided to keep this change separate in order to keep the
current patch small and easy to understand. Moreover, this makes it easier to
benchmark other similar approaches (e.g., replace QVector by std::vector, or
store keys and values together in a std::vectorstd::pairuint,Field).

(b) No space is reserved in the vectors when key/value pairs are inserted one
by one. Implementing this would make UDSEntry faster on the slave side (since
repeated re-allocations would not be necessary any more), but this can be done
in a later patch. Moreover, it might not be needed any more if UDSEntry is not
used directly any more on the slave side, as suggested on the frameworks
mailing list by Aaron (good idea BTW!).

Diffs
-

tests/udsentrybenchmark.cpp 9bedb7b
autotests/udsentry_benchmark.cpp b5fa8d6
src/core/udsentry.h 98a7035
src/core/udsentry.cpp b806e0e
src/ioslaves/file/file.cpp 1a2a767

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

Testing
---

Unit tests still pass.

The memory usage of listjobtest with a directory with 100,000 files is reduced
from 71344 K to 35392 K according to KSysGuard. I see similar savings when
opening the directory in Dolphin.

I still haven't set up a Qt5/KF5 build in release mode (shame on me!), so I
cannot present any benchmark results.

File Attachments

Benchmark results

https://git.reviewboard.kde.org/media/uploaded/files/2014/12/09/038e443c-78eb-443b-b33a-b451b28d10ea__UDSEntry-benchmarks.png

Thanks,

Frank Reininghaus

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

Change in kio[master]: Apply the edited URL to the navigator when clicking the Che...

2014-12-14 Thread Frank Reininghaus (Code Review)

Frank Reininghaus has uploaded a new change for review.

  https://gerrit.vesnicky.cesnet.cz/r/233

Change subject: Apply the edited URL to the navigator when clicking the Check 
button
..

Apply the edited URL to the navigator when clicking the Check button

If the navigator is in editable mode, pressing Enter applies the
edited URL to the view. This commit also enables this behavior for the
case that the user clicks the button to the right of the line edit in
order to switch back to breadcrumb mode. Before this commit, the edited
URL was lost.

BUG: 257538
Change-Id: I553afb1f6af033a026e8d8cf08f57f682ac723c2
FIXED-IN: 5.6.0
---
M src/filewidgets/kurlnavigator.cpp
M src/filewidgets/kurlnavigator.h
2 files changed, 27 insertions(+), 5 deletions(-)


  git pull ssh://gerrit.vesnicky.cesnet.cz:29418/kio refs/changes/33/233/1

diff --git a/src/filewidgets/kurlnavigator.cpp 
b/src/filewidgets/kurlnavigator.cpp
index d0fd71a..ba19f28 100644
--- a/src/filewidgets/kurlnavigator.cpp
+++ b/src/filewidgets/kurlnavigator.cpp
@@ -69,6 +69,9 @@
 
 void initialize(const QUrl url);
 
+/** Applies the edited URL in m_pathBox to the URL navigator */
+void applyUncommittedUrl();
+
 void slotReturnPressed();
 void slotProtocolChanged(const QString );
 void openPathSelectorMenu();
@@ -81,9 +84,14 @@
 void appendWidget(QWidget *widget, int stretch = 0);
 
 /**
+ * This slot is connected to the clicked signal of the navigation bar 
button. It calls switchView().
+ * Moreover, if switching from editable mode to the breadcrumb view, it 
calls applyUncommittedUrl().
+ */
+void slotToggleEditableButtonPressed();
+
+/**
  * Switches the navigation bar between the breadcrumb view and the
- * traditional view (see setUrlEditable()) and is connected to the clicked 
signal
- * of the navigation bar button.
+ * traditional view (see setUrlEditable()).
  */
 void switchView();
 
@@ -254,7 +262,7 @@
 m_toggleEditableMode-installEventFilter(q);
 m_toggleEditableMode-setMinimumWidth(20);
 connect(m_toggleEditableMode, SIGNAL(clicked()),
-q, SLOT(switchView()));
+q, SLOT(slotToggleEditableButtonPressed()));
 
 if (m_placesSelector != 0) {
 m_layout-addWidget(m_placesSelector);
@@ -291,7 +299,7 @@
 m_layout-insertWidget(m_layout-count() - 1, widget, stretch);
 }
 
-void KUrlNavigator::Private::slotReturnPressed()
+void KUrlNavigator::Private::applyUncommittedUrl()
 {
 // Parts of the following code have been taken
 // from the class KateFileSelector located in
@@ -311,6 +319,11 @@
 // synchronize the result in the path box.
 const QUrl currentUrl = q-locationUrl();
 m_pathBox-setUrl(currentUrl);
+}
+
+void KUrlNavigator::Private::slotReturnPressed()
+{
+applyUncommittedUrl();
 
 emit q-returnPressed();
 
@@ -383,6 +396,15 @@
 }
 }
 
+void KUrlNavigator::Private::slotToggleEditableButtonPressed()
+{
+if (m_editable) {
+applyUncommittedUrl();
+}
+
+switchView();
+}
+
 void KUrlNavigator::Private::switchView()
 {
 m_toggleEditableMode-setFocus();
diff --git a/src/filewidgets/kurlnavigator.h b/src/filewidgets/kurlnavigator.h
index 5ffa577..0c12bd5 100644
--- a/src/filewidgets/kurlnavigator.h
+++ b/src/filewidgets/kurlnavigator.h
@@ -450,7 +450,7 @@
 private:
 Q_PRIVATE_SLOT(d, void slotReturnPressed())
 Q_PRIVATE_SLOT(d, void slotProtocolChanged(const QString protocol))
-Q_PRIVATE_SLOT(d, void switchView())
+Q_PRIVATE_SLOT(d, void slotToggleEditableButtonPressed())
 Q_PRIVATE_SLOT(d, void dropUrls(const QUrl destination, QDropEvent *))
 Q_PRIVATE_SLOT(d, void slotNavigatorButtonClicked(const QUrl url, 
Qt::MouseButton button))
 Q_PRIVATE_SLOT(d, void openContextMenu())

-- 
To view, visit https://gerrit.vesnicky.cesnet.cz/r/233
To unsubscribe, visit https://gerrit.vesnicky.cesnet.cz/r/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I553afb1f6af033a026e8d8cf08f57f682ac723c2
Gerrit-PatchSet: 1
Gerrit-Project: kio
Gerrit-Branch: master
Gerrit-Owner: Frank Reininghaus frank7...@googlemail.com
Gerrit-Reviewer: Sysadmin Testing Account n...@kde.org
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118452: Reduce the memory usage of UDSEntry by using QVector, rather than QHash, for the internal data storage

2014-12-11 Thread Frank Reininghaus

On Dez. 11, 2014, 12:43 vorm., Milian Wolff wrote:
src/core/udsentry.cpp, line 72
https://git.reviewboard.kde.org/r/118452/diff/2/?file=332446#file332446line72

you are missing a reserve call here

Yes, good catch! Note: I do realise that we could reserve space for only 7 on
Windows here, but I'm not sure if that saving is worth cluttering the code with
another #ifdef.

- Frank

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

On Dez. 9, 2014, 10:44 nachm., Frank Reininghaus wrote:

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

(Updated Dez. 9, 2014, 10:44 nachm.)

Review request for KDE Frameworks and David Faure.

Repository: kio

Description
---

Some parts of https://git.reviewboard.kde.org/r/113355/ are still missing:

Diffs
-

autotests/udsentry_benchmark.cpp b5fa8d6
src/core/udsentry.h 98a7035
src/core/udsentry.cpp b806e0e
src/ioslaves/file/file.cpp 1a2a767
tests/udsentrybenchmark.cpp d9a118c

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

Testing
---

Unit tests still pass.

The memory usage of listjobtest with a directory with 100,000 files is
reduced from 71344 K to 35392 K according to KSysGuard. I see similar savings
when opening the directory in Dolphin.

I still haven't set up a Qt5/KF5 build in release mode (shame on me!), so I
cannot present any benchmark results.

File Attachments

Benchmark results

https://git.reviewboard.kde.org/media/uploaded/files/2014/12/09/038e443c-78eb-443b-b33a-b451b28d10ea__UDSEntry-benchmarks.png

Thanks,

Frank Reininghaus

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

Re: Review Request 118452: Reduce the memory usage of UDSEntry by using QVector, rather than QHash, for the internal data storage

2014-12-11 Thread Frank Reininghaus



 On Dez. 10, 2014, 11:03 nachm., Milian Wolff wrote:
  src/core/udsentry.cpp, line 155
  https://git.reviewboard.kde.org/r/118452/diff/2/?file=332446#file332446line155
 
  is this called often? if so, prefer to use a QList for the udsIndexes, 
  to prevent the costly conversion here.
  
  QList and QVector of uint are nearly the same. QVector just has a 
  much nicer API, and stuff like resize, which QList is lacking. For this 
  use-case, I think it should be OK though.
  
  Maybe add a TODO kf6 to change this to QVector?

If QList and QVector of uint are nearly the same depends on the definition of 
nearly, I guess ;-)

AFAIK, since QList treats all data types as pointers internally, it will add 
padding to the 4-byte uints on a 64 bit system, which will waste some memory, 
and probably also affect the performance since more data needs to be loaded 
into the CPU caches.

I think the listFields() function is not used much. KIO itself uses it only in 
unit tests, and most other libraries and applications probably don't use 
UDSEntry directly, but only indirectly via KDirLister and KFileItem.

Therefore, I'm not sure if changing the return type in KF6 is desirable. It 
would improve the performance in some cases, but these cases are probably 
rather exotic (?), and it seems that the API of both Qt and Frameworks consider 
QList as the default container for everything (even if this is debatable from a 
memory usage and performance point of view).


- Frank


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


On Dez. 9, 2014, 10:44 nachm., Frank Reininghaus wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/118452/
 ---
 
 (Updated Dez. 9, 2014, 10:44 nachm.)
 
 
 Review request for KDE Frameworks and David Faure.
 
 
 Repository: kio
 
 
 Description
 ---
 
 I am continuing to split up https://git.reviewboard.kde.org/r/113355/ , which 
 attempts to make UDSEntry more efficient memory and CPU-wise, into 
 independent parts. This is the third step after 
 https://git.reviewboard.kde.org/r/113591/ and 
 https://git.reviewboard.kde.org/r/115739/ .
 
 The present patch modifies the internal data storage of UDSEntry. UDSEntry 
 contains a mapping from unsigned int keys to Field values, where Field is a 
 struct that contains a QString and a long long (some keys correspond to 
 numeric values, like size, date, etc, whereas others, like user and group, 
 correspond to a QString).
 
 Currently, UDSEntry stores all data in a QHashuint, Field internally. This 
 ensures that everything can be accessed in O(1) time, but is not very 
 efficient memory-wise because a separate memory allocation is done for each 
 hash node.
 
 I propose to change this and store both the uint keys and the Field values in 
 a QVector each. This means that accessing a value becomes a O(n) operation, 
 since the entire QVector of keys may need to be scanned in order to find a 
 value, but because the number n of values in a UDSEntry is usually quite 
 small and can currently not exceed a number ~100, this should not matter in 
 practice.
 
 Some parts of https://git.reviewboard.kde.org/r/113355/ are still missing:
 
 (a) The QVectors which store the keys (which are usually the same for all 
 items in a directory) are not shared yet. Changing this would reduce the 
 memory usage further, but I decided to keep this change separate in order to 
 keep the current patch small and easy to understand. Moreover, this makes it 
 easier to benchmark other similar approaches (e.g., replace QVector by 
 std::vector, or store keys and values together in a 
 std::vectorstd::pairuint,Field).
 
 (b) No space is reserved in the vectors when key/value pairs are inserted one 
 by one. Implementing this would make UDSEntry faster on the slave side (since 
 repeated re-allocations would not be necessary any more), but this can be 
 done in a later patch. Moreover, it might not be needed any more if UDSEntry 
 is not used directly any more on the slave side, as suggested on the 
 frameworks mailing list by Aaron (good idea BTW!). 
 
 
 Diffs
 -
 
   autotests/udsentry_benchmark.cpp b5fa8d6 
   src/core/udsentry.h 98a7035 
   src/core/udsentry.cpp b806e0e 
   src/ioslaves/file/file.cpp 1a2a767 
   tests/udsentrybenchmark.cpp d9a118c 
 
 Diff: https://git.reviewboard.kde.org/r/118452/diff/
 
 
 Testing
 ---
 
 Unit tests still pass.
 
 The memory usage of listjobtest with a directory with 100,000 files is 
 reduced from 71344 K to 35392 K according to KSysGuard. I see similar savings 
 when opening the directory in Dolphin.
 
 I still haven't set up a Qt5/KF5 build in release mode

Re: Review Request 118452: Reduce the memory usage of UDSEntry by using QVector, rather than QHash, for the internal data storage

2014-12-11 Thread Frank Reininghaus

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

(Updated Dez. 11, 2014, 10:08 nachm.)

Review request for KDE Frameworks and David Faure.

Changes
---

Thanks for your comments, Milian and Mark!

Thanks also for fixing the benchmark, Milian. The point of the QCOMPARE calls
was, as you have probably guessed, to prevent that the compiler optimizes some
or all of the code in QBENCHMARK {...} away. But I admit that this approach was
a bit, erm, suboptimal. Your solution is much better indeed :-)

The idea to factor out the common code from the insert(...) functions was quite
good, thanks! It even made the code faster, maybe because this is more
cache-friendly and makes branch prediction easier.

For the createSmallEntries() benchmark, I get, with tests/udsentrybenchmark
-perf -perfcounter instr:k, the following number of instructions:

master:322801129
rev. 2 of this RR: 331020709 (+3%)
rev. 3 of this RR: 198900694 (-38% compared to master)

The QVarLengthArray idea is also quite good! I think this should be done in a
separate patch though. I was going to do some further experiments anyway (like
checking how much we can gain if we let the UDSEntries implicitly share the
'udsIndexes' QVectors, or if using std::vector instead of QVector makes any
difference - comparing that to QVarLengthArray would be interesting indeed).

Repository: kio

Description
---

I am continuing to split up https://git.reviewboard.kde.org/r/113355/ , which
attempts to make UDSEntry more efficient memory and CPU-wise, into independent
parts. This is the third step after
https://git.reviewboard.kde.org/r/113591/ and
https://git.reviewboard.kde.org/r/115739/ .

The present patch modifies the internal data storage of UDSEntry. UDSEntry
contains a mapping from unsigned int keys to Field values, where Field is a
struct that contains a QString and a long long (some keys correspond to numeric
values, like size, date, etc, whereas others, like user and group, correspond
to a QString).

Currently, UDSEntry stores all data in a QHashuint, Field internally. This
ensures that everything can be accessed in O(1) time, but is not very efficient
memory-wise because a separate memory allocation is done for each hash node.

I propose to change this and store both the uint keys and the Field values in a
QVector each. This means that accessing a value becomes a O(n) operation, since
the entire QVector of keys may need to be scanned in order to find a value, but
because the number n of values in a UDSEntry is usually quite small and can
currently not exceed a number ~100, this should not matter in practice.

Some parts of https://git.reviewboard.kde.org/r/113355/ are still missing:

(a) The QVectors which store the keys (which are usually the same for all items
in a directory) are not shared yet. Changing this would reduce the memory usage
further, but I decided to keep this change separate in order to keep the
current patch small and easy to understand. Moreover, this makes it easier to
benchmark other similar approaches (e.g., replace QVector by std::vector, or
store keys and values together in a std::vectorstd::pairuint,Field).

(b) No space is reserved in the vectors when key/value pairs are inserted one
by one. Implementing this would make UDSEntry faster on the slave side (since
repeated re-allocations would not be necessary any more), but this can be done
in a later patch. Moreover, it might not be needed any more if UDSEntry is not
used directly any more on the slave side, as suggested on the frameworks
mailing list by Aaron (good idea BTW!).

Diffs (updated)
-

autotests/udsentry_benchmark.cpp b5fa8d6
src/core/udsentry.h 98a7035
src/core/udsentry.cpp b806e0e
src/ioslaves/file/file.cpp 1a2a767
tests/udsentrybenchmark.cpp 9bedb7b

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

Testing
---

Unit tests still pass.

The memory usage of listjobtest with a directory with 100,000 files is reduced
from 71344 K to 35392 K according to KSysGuard. I see similar savings when
opening the directory in Dolphin.

I still haven't set up a Qt5/KF5 build in release mode (shame on me!), so I
cannot present any benchmark results.

File Attachments

Benchmark results

https://git.reviewboard.kde.org/media/uploaded/files/2014/12/09/038e443c-78eb-443b-b33a-b451b28d10ea__UDSEntry-benchmarks.png

Thanks,

Frank Reininghaus

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

Re: Review Request 118452: Reduce the memory usage of UDSEntry by using QVector, rather than QHash, for the internal data storage

2014-12-09 Thread Frank Reininghaus

 according to KSysGuard. I see similar savings when 
opening the directory in Dolphin.

I still haven't set up a Qt5/KF5 build in release mode (shame on me!), so I 
cannot present any benchmark results.


File Attachments (updated)


Benchmark results
  
https://git.reviewboard.kde.org/media/uploaded/files/2014/12/09/038e443c-78eb-443b-b33a-b451b28d10ea__UDSEntry-benchmarks.png


Thanks,

Frank Reininghaus

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

Re: Review Request 119607: Support for .hidden files

2014-12-04 Thread Frank Reininghaus



 On Sept. 14, 2014, 3:27 nachm., Frank Reininghaus wrote:
  src/core/kfileitem.h, line 262
  https://git.reviewboard.kde.org/r/119607/diff/2/?file=301216#file301216line262
 
  Since we probably do not want to make it possible that all users of 
  this class can make items hidden, I'm not sure if this should be part of 
  KFileItem's public API.
 
 David Faure wrote:
 I don't have an issue with that. Gives more possibilities to the app (or 
 file dialog) etc.
 
 Bruno Nova wrote:
 So, should this be left public or not?
 
 Bruno Nova wrote:
 I still need an answer here. :-)
 
 Besides this issue, there is another thing that was suggested that was 
 not implemented: unit tests.
 I'll try to do this tomorrow, but I'll probably not be capable of doing 
 it.

To be honest, I just don't see a valid use case for that - why would an app or 
the file dialog want to make the dir lister consider a particular file item 
hidden? If the app/file dialog wants to hide items from the user, there are 
other (and easier) ways to do that, since they already use a 
QSortFilterProxyModel or some other kind of filtering mechanism, right?

If anyone sees a good use case, then fair enough, but unless that is the case, 
I don't think that one should make it public API (which is extremely hard to 
change or remove in future versions!).


- Frank


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


On Dez. 4, 2014, 9:55 nachm., Bruno Nova wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/119607/
 ---
 
 (Updated Dez. 4, 2014, 9:55 nachm.)
 
 
 Review request for KDE Frameworks and David Faure.
 
 
 Bugs: 64740 and 246260
 https://bugs.kde.org/show_bug.cgi?id=64740
 https://bugs.kde.org/show_bug.cgi?id=246260
 
 
 Repository: kio
 
 
 Description
 ---
 
 This adds support for *.hidden* files to KDE.
 
 When listing a directory, the files/folders listed in the *.hidden* file will 
 be hidden, unless the user has chosen to show hidden files.
 
 This patch was initially based on the patch provided by Mark in Bug #246260.
 
 
 Diffs
 -
 
   src/core/kcoredirlister.cpp a31d629 
   src/core/kcoredirlister_p.h dce7dbc 
   src/core/kfileitem.h bebc241 
   src/core/kfileitem.cpp 74dc069 
 
 Diff: https://git.reviewboard.kde.org/r/119607/diff/
 
 
 Testing
 ---
 
 Built and tested the patch in Project Neon.
 Dolphin was still using KDE4/Qt4 version of the library, so I only tested 
 using the desktop folder widget, and folder desktop.
 It worked correctly when I pointed it to ~ and ~/Desktop (I added 
 .hidden there).
 However, it didn't work when I pointed it to the Desktop folder (the 
 default option, not the custom location ~/Desktop).
 More testing is required.
 
 The version for KDE4/Qt4 submitted to Bug #246260 was tested in Kubuntu 
 14.04, and it worked everywhere I tested (Dolphin, open/save dialogs, folder 
 widget and detailed/tree view in Dolphin).
 It wasn't an intensive test, though.
 
 
 Thanks,
 
 Bruno Nova
 


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

Re: How to port KTabBar::mouseMiddleClick?

2014-11-09 Thread Frank Reininghaus

Hi,

2014-11-06 2:59 GMT+01:00 Milian Wolff:
 Hey all,

 what do I do to get middle-click-closes-tab in Qt 5 without KTabBar?
 Previously, we used KTabBar and its mouseMiddleClick signal.

AFAIK, currently the only solution is to subclass QTabBar, override
the mousePressEvent method and emit a signal from there. Dolphin uses
this approach. There were many other reasons why Emmanuel created a
custom QTabBar subclass for Dolphin though .

If many apps need the middle click to close bevavior, then
reanimating KTabBar or getting this functionality into QTabBar might
be better than making every app create its own tab bar class.

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

Re: Review Request 120666: Get user's permission before executing scripts or desktop files

2014-10-23 Thread Frank Reininghaus



 On Okt. 23, 2014, 11:14 vorm., Emmanuel Pescosta wrote:
  src/widgets/krun.cpp, line 1013
  https://git.reviewboard.kde.org/r/120666/diff/2/?file=321616#file321616line1013
 
  Any reason why using show instead of exec? Exec would avoid the rather 
  complex and error-prone code path.
  
  IMHO we should prefer a blocking dialog in this case, because it asks 
  the user for permission.
 
 Arjun AK wrote:
 IMHO we should prefer a blocking dialog in this case, because it asks 
 the user for permission.
 
 faure: Are you okay with this?

Note that you have to be *extremely* careful when calling exec(), which runs a 
nested event loop. Anything can happen inside such a loop, including quitting 
the application. See https://git.reviewboard.kde.org/r/118858/ and the links in 
the comments there fore more information.

If you replace show() by exec() in your patch, you might get a crash in the line

m_dialogNotYetShown = false;

if the application quits inside the nested event loop.


- Frank


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


On Okt. 22, 2014, 4:10 nachm., Arjun AK wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/120666/
 ---
 
 (Updated Okt. 22, 2014, 4:10 nachm.)
 
 
 Review request for KDE Frameworks.
 
 
 Repository: kio
 
 
 Description
 ---
 
 This patch makes KIO show a dialog box asking the user what to do (either 
 open it using a text editor or execute it) when he clicks on a script or a 
 desktop file.
 
 See also: https://git.reviewboard.kde.org/r/120171/
 
 
 Diffs
 -
 
   src/widgets/CMakeLists.txt 4060cdf 
   src/widgets/executablefileopendialog.h PRE-CREATION 
   src/widgets/executablefileopendialog.cpp PRE-CREATION 
   src/widgets/krun.cpp 6ac42da 
   src/widgets/krun_p.h 69e2e98 
 
 Diff: https://git.reviewboard.kde.org/r/120666/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Arjun AK
 


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

Re: KConfig build fails

2014-07-14 Thread Frank Reininghaus

Hi,

2014-07-14 23:21 GMT+02:00 David Gil Oliva:
 Hi!

 KConfig build fails with this messages, all of them related to
 QBasicAtomicInt. Are they KF5 bugs?

probably this is the same issue that has been discussed in
https://git.reviewboard.kde.org/r/119257/ ? According to David F.,
this might depend on the Qt version that you are using.

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

Re: Review Request 118775: Make KFileItem a Q_MOVABLE type

2014-06-21 Thread Frank Reininghaus


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

(Updated June 21, 2014, 7:39 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and David Faure.


Repository: kio


Description
---

This is my second attempt to make KFileItem a Q_MOVABLE_TYPE, after

https://git.reviewboard.kde.org/r/111789/

which I had to revert because it broke KDirLister, see

http://mail.kde.org/pipermail/kde-frameworks-devel/2013-August/004135.html

The motivation for this change is that this reduces the memory usage of a 
QListKFileItem a.k.a. KFileItemList, which is used extensively in 
KDirLister's API and in applications, by 32 bytes per item on a 64-bit system, 
and that it may also improve the performance in some situations because many 
memory allocations are saved (for details on why making a type movable saves 
memory allocations when putting objects of that type into a QList, see the 
discussion in the related request https://git.reviewboard.kde.org/r/115739/ for 
UDSEntry).

The problem with the first attempt was that KDirListerCache actually relies on 
the fact that KFileItem is NOT movable in memory - it keeps pointers to 
KFileItems in a QList and expects that these pointers remain valid even if the 
list is resized, and the location of its contiguous data storage with size 
~number of items in the list in memory changes. This is the case right now 
because QList only keeps pointers to the KFileItems, and moving the pointers 
when the list is resized does not change the location of the actual KFileItems. 
For movable types, QList stores the objects directly, such that resizing the 
list may move the actual KFileItems. This conflicts with KDirListerCache's 
expectation that the KFileItems do not move.

David suggested to change the internal data storage of KDirListerCache to, 
e.g., a QLinkedList to circumvent this problem, see

http://mail.kde.org/pipermail/kde-frameworks-devel/2013-September/004845.html

I have a less intrusive proposal now: Make KFileItem movable, but replace all 
places where KDirListerCache expects a non-movable KFileItem with 
NonMovableFileItem, which is a class that inherits KFileItem, but does not 
have the movable property.

That way, the data storage inside KDirListerCache remains exactly the same, and 
everything outside that class benefits from the movability of KFileItems. Most 
changes in this patch are straightforward subsitutions.

The only place where performance might suffer is 
KCoreDirLister::itemsForDir(const QUrl dir, WhichItems which) in the which == 
AllItems case. The current code simply returns a shallow copy of the internal 
KFileItemList, but with this patch, the list has to be copied item by item 
(this happens in NonMovableFileItemList::operator KFileItemList()). However, 
the QLinkedList idea or any other approach which makes KFileItem movable, but 
keeps the KFileItems in KDirListerCache at fixed memory locations would suffer 
from the same problem.

I'm not sure if that function is used much in the AllItems case though. I put a 
Q_ASSERT(0);  into NonMovableFileItemList::operator KFileItemList() and was 
unable to trigger that assert with Dolphin.

Ideally, one would do some benchmarking and memory profiling of this patch and 
alternatives, such as the QLinkedList idea. However, I'm running out of time 
because the release schedule is progressing fast, and even though this change 
is quite straightforward, it is binary incompatible. This is why I am creating 
this review request right now.


Diffs
-

  src/core/kcoredirlister.cpp fef28db 
  src/core/kcoredirlister_p.h 2660e99 
  src/core/kfileitem.h bc2f90c 

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


Testing
---

Unit tests still pass. I verified that the memory usage of a KFileItemList with 
many items decreases as expected.


Thanks,

Frank Reininghaus

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

Re: Review Request 118775: Make KFileItem a Q_MOVABLE type

2014-06-19 Thread Frank Reininghaus


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

(Updated June 19, 2014, 2:47 p.m.)


Review request for KDE Frameworks and David Faure.


Changes
---

Implemented David's and Emmanuel's suggestions (which make a lot of sense, 
thanks!).


Repository: kio


Description
---

This is my second attempt to make KFileItem a Q_MOVABLE_TYPE, after

https://git.reviewboard.kde.org/r/111789/

which I had to revert because it broke KDirLister, see

http://mail.kde.org/pipermail/kde-frameworks-devel/2013-August/004135.html

The motivation for this change is that this reduces the memory usage of a 
QListKFileItem a.k.a. KFileItemList, which is used extensively in 
KDirLister's API and in applications, by 32 bytes per item on a 64-bit system, 
and that it may also improve the performance in some situations because many 
memory allocations are saved (for details on why making a type movable saves 
memory allocations when putting objects of that type into a QList, see the 
discussion in the related request https://git.reviewboard.kde.org/r/115739/ for 
UDSEntry).

The problem with the first attempt was that KDirListerCache actually relies on 
the fact that KFileItem is NOT movable in memory - it keeps pointers to 
KFileItems in a QList and expects that these pointers remain valid even if the 
list is resized, and the location of its contiguous data storage with size 
~number of items in the list in memory changes. This is the case right now 
because QList only keeps pointers to the KFileItems, and moving the pointers 
when the list is resized does not change the location of the actual KFileItems. 
For movable types, QList stores the objects directly, such that resizing the 
list may move the actual KFileItems. This conflicts with KDirListerCache's 
expectation that the KFileItems do not move.

David suggested to change the internal data storage of KDirListerCache to, 
e.g., a QLinkedList to circumvent this problem, see

http://mail.kde.org/pipermail/kde-frameworks-devel/2013-September/004845.html

I have a less intrusive proposal now: Make KFileItem movable, but replace all 
places where KDirListerCache expects a non-movable KFileItem with 
NonMovableFileItem, which is a class that inherits KFileItem, but does not 
have the movable property.

That way, the data storage inside KDirListerCache remains exactly the same, and 
everything outside that class benefits from the movability of KFileItems. Most 
changes in this patch are straightforward subsitutions.

The only place where performance might suffer is 
KCoreDirLister::itemsForDir(const QUrl dir, WhichItems which) in the which == 
AllItems case. The current code simply returns a shallow copy of the internal 
KFileItemList, but with this patch, the list has to be copied item by item 
(this happens in NonMovableFileItemList::operator KFileItemList()). However, 
the QLinkedList idea or any other approach which makes KFileItem movable, but 
keeps the KFileItems in KDirListerCache at fixed memory locations would suffer 
from the same problem.

I'm not sure if that function is used much in the AllItems case though. I put a 
Q_ASSERT(0);  into NonMovableFileItemList::operator KFileItemList() and was 
unable to trigger that assert with Dolphin.

Ideally, one would do some benchmarking and memory profiling of this patch and 
alternatives, such as the QLinkedList idea. However, I'm running out of time 
because the release schedule is progressing fast, and even though this change 
is quite straightforward, it is binary incompatible. This is why I am creating 
this review request right now.


Diffs (updated)
-

  src/core/kcoredirlister.cpp fef28db 
  src/core/kcoredirlister_p.h 2660e99 
  src/core/kfileitem.h bc2f90c 

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


Testing
---

Unit tests still pass. I verified that the memory usage of a KFileItemList with 
many items decreases as expected.


Thanks,

Frank Reininghaus

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

KCoreAddons does not install most of its headers on my system

2014-06-16 Thread Frank Reininghaus

Hello everyone,

I tried to set up a separate Qt5+KF5 build in release mode.
Unfortunately, I did not succeed :-(

The first package that fails to build (even if I repeat the
kdesrc-build process many times) is kservice. The comiler reports

[ 30%] Building CXX object
src/CMakeFiles/KF5Service.dir/services/kserviceaction.cpp.o
In file included from
/home/kde-frameworks-release/kde/src/frameworks/kservice/src/services/kmimetypetrader.h:23:0,
 from
/home/kde-frameworks-release/kde/src/frameworks/kservice/src/services/kmimetypetrader.cpp:20:
/home/kde-frameworks-release/kde/src/frameworks/kservice/src/services/kservice.h:27:28:
fatal error: kpluginfactory.h: No such file or directory
 #include kpluginfactory.h
^

I then found that kpluginfactory should actually be installed by
kcoreaddons, but that does not happen here. The install.log of
kcoreaddons looks like this:

# kdesrc-build running: 'gmake' 'install/fast'
# from directory: /home/kde-frameworks-release/kde/build/frameworks/kcoreaddons
Install the project...
-- Install configuration: release
-- Up-to-date: 
/home/kde-frameworks-release/kf5/lib64/cmake/KF5CoreAddons/KF5CoreAddonsConfig.cmake
-- Up-to-date: 
/home/kde-frameworks-release/kf5/lib64/cmake/KF5CoreAddons/KF5CoreAddonsConfigVersion.cmake
-- Up-to-date: 
/home/kde-frameworks-release/kf5/lib64/cmake/KF5CoreAddons/KF5CoreAddonsTargets.cmake
-- Up-to-date: 
/home/kde-frameworks-release/kf5/lib64/cmake/KF5CoreAddons/KF5CoreAddonsTargets-release.cmake
-- Up-to-date: 
/home/kde-frameworks-release/kf5/include/KF5/kcoreaddons_version.h
-- Up-to-date: 
/home/kde-frameworks-release/kf5/lib64/libKF5CoreAddons.so.4.100.0
-- Up-to-date: /home/kde-frameworks-release/kf5/lib64/libKF5CoreAddons.so.5
-- Up-to-date: /home/kde-frameworks-release/kf5/lib64/libKF5CoreAddons.so
-- Up-to-date: 
/home/kde-frameworks-release/kf5/include/KF5/KCoreAddons/KAboutData
-- Up-to-date: 
/home/kde-frameworks-release/kf5/include/KF5/KCoreAddons/kaboutdata.h
-- Up-to-date: 
/home/kde-frameworks-release/kf5/include/KF5/KCoreAddons/kcoreaddons_export.h
-- Up-to-date: 
/home/kde-frameworks-release/kf5/mkspecs/modules/qt_KCoreAddons.pri
-- Up-to-date: /home/kde-frameworks-release/kf5/share/mime/packages/kde5.xml
-- Updating MIME database at /home/kde-frameworks-release/kf5/share/mime


It looks like it installs nothing that is in subdirectories of
kcoreaddons/src/lib/.

It still works nicely with my kde-frameworks user. The only change
between both builds (except that one is based on a more recent Qt5
checkout) that I am aware of is that I replaced debug by release
as the build type.

I then reverted the debug-release change, cleared the kcoreaddons
build directory and re-ran kdesrc-build, but it does not help.

The CMake log does not look suspicious to me: http://paste.kde.org/p0ccahzhh

I tried to understand the CMakeLists.txt, but I'm not really familiar
with CMake and the ECM stuff, so I haven't found out what the problem
is yet.

Does anyone have an idea what I could do to investigate and possibly
fix the problem?

Thanks and best regards,
Frank
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 118775: Make KFileItem a Q_MOVABLE type

2014-06-16 Thread Frank Reininghaus


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

Review request for KDE Frameworks and David Faure.


Repository: kio


Description
---

This is my second attempt to make KFileItem a Q_MOVABLE_TYPE, after

https://git.reviewboard.kde.org/r/111789/

which I had to revert because it broke KDirLister, see

http://mail.kde.org/pipermail/kde-frameworks-devel/2013-August/004135.html

The motivation for this change is that this reduces the memory usage of a 
QListKFileItem a.k.a. KFileItemList, which is used extensively in 
KDirLister's API and in applications, by 32 bytes per item on a 64-bit system, 
and that it may also improve the performance in some situations because many 
memory allocations are saved (for details on why making a type movable saves 
memory allocations when putting objects of that type into a QList, see the 
discussion in the related request https://git.reviewboard.kde.org/r/115739/ for 
UDSEntry).

The problem with the first attempt was that KDirListerCache actually relies on 
the fact that KFileItem is NOT movable in memory - it keeps pointers to 
KFileItems in a QList and expects that these pointers remain valid even if the 
list is resized, and the location of its contiguous data storage with size 
~number of items in the list in memory changes. This is the case right now 
because QList only keeps pointers to the KFileItems, and moving the pointers 
when the list is resized does not change the location of the actual KFileItems. 
For movable types, QList stores the objects directly, such that resizing the 
list may move the actual KFileItems. This conflicts with KDirListerCache's 
expectation that the KFileItems do not move.

David suggested to change the internal data storage of KDirListerCache to, 
e.g., a QLinkedList to circumvent this problem, see

http://mail.kde.org/pipermail/kde-frameworks-devel/2013-September/004845.html

I have a less intrusive proposal now: Make KFileItem movable, but replace all 
places where KDirListerCache expects a non-movable KFileItem with 
NonMovableFileItem, which is a class that inherits KFileItem, but does not 
have the movable property.

That way, the data storage inside KDirListerCache remains exactly the same, and 
everything outside that class benefits from the movability of KFileItems. Most 
changes in this patch are straightforward subsitutions.

The only place where performance might suffer is 
KCoreDirLister::itemsForDir(const QUrl dir, WhichItems which) in the which == 
AllItems case. The current code simply returns a shallow copy of the internal 
KFileItemList, but with this patch, the list has to be copied item by item 
(this happens in NonMovableFileItemList::operator KFileItemList()). However, 
the QLinkedList idea or any other approach which makes KFileItem movable, but 
keeps the KFileItems in KDirListerCache at fixed memory locations would suffer 
from the same problem.

I'm not sure if that function is used much in the AllItems case though. I put a 
Q_ASSERT(0);  into NonMovableFileItemList::operator KFileItemList() and was 
unable to trigger that assert with Dolphin.

Ideally, one would do some benchmarking and memory profiling of this patch and 
alternatives, such as the QLinkedList idea. However, I'm running out of time 
because the release schedule is progressing fast, and even though this change 
is quite straightforward, it is binary incompatible. This is why I am creating 
this review request right now.


Diffs
-

  src/core/kcoredirlister.cpp fef28db 
  src/core/kcoredirlister_p.h 2660e99 
  src/core/kfileitem.h bc2f90c 

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


Testing
---

Unit tests still pass. I verified that the memory usage of a KFileItemList with 
many items decreases as expected.


Thanks,

Frank Reininghaus

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

Re: Review Request 118775: Make KFileItem a Q_MOVABLE type

2014-06-16 Thread Frank Reininghaus



 On June 16, 2014, 10:09 a.m., Emmanuel Pescosta wrote:
  src/core/kcoredirlister_p.h, lines 40-42
  https://git.reviewboard.kde.org/r/118775/diff/1/?file=281658#file281658line40
 
  using KFileItem::KFileItem; maybe?

Maybe - I'm always a bit unsure which C++11 features are allowed.


- Frank


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


On June 16, 2014, 7:58 a.m., Frank Reininghaus wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/118775/
 ---
 
 (Updated June 16, 2014, 7:58 a.m.)
 
 
 Review request for KDE Frameworks and David Faure.
 
 
 Repository: kio
 
 
 Description
 ---
 
 This is my second attempt to make KFileItem a Q_MOVABLE_TYPE, after
 
 https://git.reviewboard.kde.org/r/111789/
 
 which I had to revert because it broke KDirLister, see
 
 http://mail.kde.org/pipermail/kde-frameworks-devel/2013-August/004135.html
 
 The motivation for this change is that this reduces the memory usage of a 
 QListKFileItem a.k.a. KFileItemList, which is used extensively in 
 KDirLister's API and in applications, by 32 bytes per item on a 64-bit 
 system, and that it may also improve the performance in some situations 
 because many memory allocations are saved (for details on why making a type 
 movable saves memory allocations when putting objects of that type into a 
 QList, see the discussion in the related request 
 https://git.reviewboard.kde.org/r/115739/ for UDSEntry).
 
 The problem with the first attempt was that KDirListerCache actually relies 
 on the fact that KFileItem is NOT movable in memory - it keeps pointers to 
 KFileItems in a QList and expects that these pointers remain valid even if 
 the list is resized, and the location of its contiguous data storage with 
 size ~number of items in the list in memory changes. This is the case right 
 now because QList only keeps pointers to the KFileItems, and moving the 
 pointers when the list is resized does not change the location of the actual 
 KFileItems. For movable types, QList stores the objects directly, such that 
 resizing the list may move the actual KFileItems. This conflicts with 
 KDirListerCache's expectation that the KFileItems do not move.
 
 David suggested to change the internal data storage of KDirListerCache to, 
 e.g., a QLinkedList to circumvent this problem, see
 
 http://mail.kde.org/pipermail/kde-frameworks-devel/2013-September/004845.html
 
 I have a less intrusive proposal now: Make KFileItem movable, but replace all 
 places where KDirListerCache expects a non-movable KFileItem with 
 NonMovableFileItem, which is a class that inherits KFileItem, but does not 
 have the movable property.
 
 That way, the data storage inside KDirListerCache remains exactly the same, 
 and everything outside that class benefits from the movability of KFileItems. 
 Most changes in this patch are straightforward subsitutions.
 
 The only place where performance might suffer is 
 KCoreDirLister::itemsForDir(const QUrl dir, WhichItems which) in the which 
 == AllItems case. The current code simply returns a shallow copy of the 
 internal KFileItemList, but with this patch, the list has to be copied item 
 by item (this happens in NonMovableFileItemList::operator KFileItemList()). 
 However, the QLinkedList idea or any other approach which makes KFileItem 
 movable, but keeps the KFileItems in KDirListerCache at fixed memory 
 locations would suffer from the same problem.
 
 I'm not sure if that function is used much in the AllItems case though. I put 
 a Q_ASSERT(0);  into NonMovableFileItemList::operator KFileItemList() and 
 was unable to trigger that assert with Dolphin.
 
 Ideally, one would do some benchmarking and memory profiling of this patch 
 and alternatives, such as the QLinkedList idea. However, I'm running out of 
 time because the release schedule is progressing fast, and even though this 
 change is quite straightforward, it is binary incompatible. This is why I am 
 creating this review request right now.
 
 
 Diffs
 -
 
   src/core/kcoredirlister.cpp fef28db 
   src/core/kcoredirlister_p.h 2660e99 
   src/core/kfileitem.h bc2f90c 
 
 Diff: https://git.reviewboard.kde.org/r/118775/diff/
 
 
 Testing
 ---
 
 Unit tests still pass. I verified that the memory usage of a KFileItemList 
 with many items decreases as expected.
 
 
 Thanks,
 
 Frank Reininghaus
 


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

Re: Review Request 118452: Reduce the memory usage of UDSEntry by using QVector, rather than QHash, for the internal data storage

2014-06-12 Thread Frank Reininghaus

 if the memory consumption is more or less with this 
 approach (i expect slightly more). I haven't tested for that.


Thanks everyone for your efforts! Sorry for not replying earlier - I was away 
for a couple of days and still haven't quite caught up with all incoming KDE 
mails yet.

Proper benchmarking in release mode is very important, of course! However, note 
that I already did add a UDSEntry benchmark, which I hoped would model some app 
and slave workloads in the kio_file and the worst case with maximum number of 
fields somewhat realistically, in https://git.reviewboard.kde.org/r/115739/ - 
I guess nobody noticed because it is in tests/, rather than autotests/.

My reasoning for that was that benchmarks use a considerable amount of time by 
design, and 99.8% of the time, a developer or a CI system running the autotests 
will only be interested in test failures, and not in benchmark results. Right 
now, the new test takes 2.57 seconds with my un-optimized build, which is IMHO 
far too much for a test that is always run by everyone, but yields results that 
the vast majority will not look at. Adding autotests that take a noticeable 
amount of time might annoy people, waste energy and, perhaps most importantly, 
give developers an excellent excuse for not running them ;-) Therefore, I would 
suggest to move the new test to tests/. I haven't quite looked at it in detail, 
but my old autotest might be obsolete then.

In any case, I do know that this RR is currently sub-optimal on the slave side 
(see remark (b) in my RR description). I will try really hard to setup a 
realease build in the next few days (no time right now, have to go to work) and 
then post some benchmark results (including an improved version for the slave 
side).


- Frank


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


On June 1, 2014, 1:50 p.m., Frank Reininghaus wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/118452/
 ---
 
 (Updated June 1, 2014, 1:50 p.m.)
 
 
 Review request for KDE Frameworks and David Faure.
 
 
 Repository: kio
 
 
 Description
 ---
 
 I am continuing to split up https://git.reviewboard.kde.org/r/113355/ , which 
 attempts to make UDSEntry more efficient memory and CPU-wise, into 
 independent parts. This is the third step after 
 https://git.reviewboard.kde.org/r/113591/ and 
 https://git.reviewboard.kde.org/r/115739/ .
 
 The present patch modifies the internal data storage of UDSEntry. UDSEntry 
 contains a mapping from unsigned int keys to Field values, where Field is a 
 struct that contains a QString and a long long (some keys correspond to 
 numeric values, like size, date, etc, whereas others, like user and group, 
 correspond to a QString).
 
 Currently, UDSEntry stores all data in a QHashuint, Field internally. This 
 ensures that everything can be accessed in O(1) time, but is not very 
 efficient memory-wise because a separate memory allocation is done for each 
 hash node.
 
 I propose to change this and store both the uint keys and the Field values in 
 a QVector each. This means that accessing a value becomes a O(n) operation, 
 since the entire QVector of keys may need to be scanned in order to find a 
 value, but because the number n of values in a UDSEntry is usually quite 
 small and can currently not exceed a number ~100, this should not matter in 
 practice.
 
 Some parts of https://git.reviewboard.kde.org/r/113355/ are still missing:
 
 (a) The QVectors which store the keys (which are usually the same for all 
 items in a directory) are not shared yet. Changing this would reduce the 
 memory usage further, but I decided to keep this change separate in order to 
 keep the current patch small and easy to understand. Moreover, this makes it 
 easier to benchmark other similar approaches (e.g., replace QVector by 
 std::vector, or store keys and values together in a 
 std::vectorstd::pairuint,Field).
 
 (b) No space is reserved in the vectors when key/value pairs are inserted one 
 by one. Implementing this would make UDSEntry faster on the slave side (since 
 repeated re-allocations would not be necessary any more), but this can be 
 done in a later patch. Moreover, it might not be needed any more if UDSEntry 
 is not used directly any more on the slave side, as suggested on the 
 frameworks mailing list by Aaron (good idea BTW!). 
 
 
 Diffs
 -
 
   src/core/udsentry.cpp c6ac21a 
 
 Diff: https://git.reviewboard.kde.org/r/118452/diff/
 
 
 Testing
 ---
 
 Unit tests still pass.
 
 The memory usage of listjobtest with a directory with 100,000 files is 
 reduced from 71344 K to 35392 K according to KSysGuard. I see similar savings

Re: Review Request 118269: Fix crash when showing the confirmation dialog for trash/delete operations

2014-06-04 Thread Frank Reininghaus


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

(Updated June 4, 2014, 8:58 a.m.)


Status
--

This change has been discarded.


Review request for KDE Frameworks and David Faure.


Bugs: 334648
https://bugs.kde.org/show_bug.cgi?id=334648


Repository: kjobwidgets


Description
---

Currently Dolphin (and probably anything else that can delete files using KIO) 
crashes when it's supposed to show the confirmation dialog.

The problem is that KonqOperations::askDeleteConfirmation() sets up a 
KIO::JobUiDelegate object which has no associated job, and calls its 
setWindow(QWidget*) method before calling the actual askDeleteConfirmation() 
function.

KIO::JobUiDelegate::setWindow(QWidget*) then calls the setWindow function of 
the base class, KDialogJobUiDelegate::setWindow(QWidget*), which then crashes 
because it contains the line

Q_ASSERT(job())

and there is no job.

The problem does not exist in KDE SC 4.x - the code went through a big 
refactoring in

https://git.reviewboard.kde.org/r/111081/

which added the assert.

Just removing the assert won't help because the function then calls
KJobWidgets::setWindow(KJob *job, QWidget *widget)
which dereferences the job, i.e., we get a segfault instead.

This patch removes the assert and wraps the function in an if (job()) block 
instead. I'm not entirely sure if that is the correct solution though - any 
feedback is welcome. Alternatively, one could move the if-check to the child 
class, i.e., to KIO::JobUiDelegate::setWindow(QWidget*), if this is the only 
valid use of a KDialogJobUiDelegate without a job.

Or maybe it does not make much sense at all to have the askDeleteConfirmation 
function, which is probably always called before any job is set up, in a 
KDialogJobUiDelegate subclass? Changing that would probably require more 
intrusive changes though.


Diffs
-

  src/kdialogjobuidelegate.cpp fb4c99a 

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


Testing
---

Fixes the crash for me, and the confirmation dialog works as expected (i.e., 
the user can choose if the file should really be deleted/trashed or not).


Thanks,

Frank Reininghaus

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

Re: Review Request 118269: Fix crash when showing the confirmation dialog for trash/delete operations

2014-06-04 Thread Frank Reininghaus



 On June 1, 2014, 3:01 p.m., David Faure wrote:
  That makes the setWindow call (as called by konq_operations.cpp) useless, 
  though.
  
  This is related to the line in jobuidelegate.cpp which says
 QWidget *widget = job() ? window() : NULL; // ### job is NULL here, most 
  of the time, right?
  
  Clearly we need a better way to pass the QWidget* parent to the 
  askDeleteConfirmation() method, it's not working on either end (because no 
  job).
  
  But the API (JobUiDelegateExtension) is in KIOCore, so no QWindow nor 
  QWidget there.
  
  I think we need to use a member variable in KDialogJobUiDelegate in 
  addition to setting it in KJobWidgets.
 
 David Faure wrote:
 http://www.davidfaure.fr/2014/kjobwidgets.diff
 http://www.davidfaure.fr/2014/kio_jobuidelegate.diff
 
 Seems to work, I get a widget pointer when confirmation dialog pops up.

Thanks David!


- Frank


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


On May 22, 2014, 9:39 p.m., Frank Reininghaus wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/118269/
 ---
 
 (Updated May 22, 2014, 9:39 p.m.)
 
 
 Review request for KDE Frameworks and David Faure.
 
 
 Bugs: 334648
 https://bugs.kde.org/show_bug.cgi?id=334648
 
 
 Repository: kjobwidgets
 
 
 Description
 ---
 
 Currently Dolphin (and probably anything else that can delete files using 
 KIO) crashes when it's supposed to show the confirmation dialog.
 
 The problem is that KonqOperations::askDeleteConfirmation() sets up a 
 KIO::JobUiDelegate object which has no associated job, and calls its 
 setWindow(QWidget*) method before calling the actual askDeleteConfirmation() 
 function.
 
 KIO::JobUiDelegate::setWindow(QWidget*) then calls the setWindow function of 
 the base class, KDialogJobUiDelegate::setWindow(QWidget*), which then crashes 
 because it contains the line
 
 Q_ASSERT(job())
 
 and there is no job.
 
 The problem does not exist in KDE SC 4.x - the code went through a big 
 refactoring in
 
 https://git.reviewboard.kde.org/r/111081/
 
 which added the assert.
 
 Just removing the assert won't help because the function then calls
 KJobWidgets::setWindow(KJob *job, QWidget *widget)
 which dereferences the job, i.e., we get a segfault instead.
 
 This patch removes the assert and wraps the function in an if (job()) block 
 instead. I'm not entirely sure if that is the correct solution though - any 
 feedback is welcome. Alternatively, one could move the if-check to the child 
 class, i.e., to KIO::JobUiDelegate::setWindow(QWidget*), if this is the only 
 valid use of a KDialogJobUiDelegate without a job.
 
 Or maybe it does not make much sense at all to have the askDeleteConfirmation 
 function, which is probably always called before any job is set up, in a 
 KDialogJobUiDelegate subclass? Changing that would probably require more 
 intrusive changes though.
 
 
 Diffs
 -
 
   src/kdialogjobuidelegate.cpp fb4c99a 
 
 Diff: https://git.reviewboard.kde.org/r/118269/diff/
 
 
 Testing
 ---
 
 Fixes the crash for me, and the confirmation dialog works as expected (i.e., 
 the user can choose if the file should really be deleted/trashed or not).
 
 
 Thanks,
 
 Frank Reininghaus
 


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

Re: KIO directory listing - CPU slows down SSD

2014-06-02 Thread Frank Reininghaus

Hi,

2014-06-02 18:42 GMT+02:00 Aaron J. Seigo:
 On Thursday, May 29, 2014 16:32:28 Mark Gaiser wrote:
 dolphin on a massive folder. In fact, those that use kio::listdir for
 listing folders only have interfaces that become usable when all
 entries are fetched.

 assuming these UIs receive maintenance over the next years, this pattern is
 likely to fade away. instant feedback and incremental listing is a current
 reality.

 you know what would be very nice (but rather complex to achieve...)? for the
 client side of a KIO listing to say i only want the first N items anyways,
 i'll let you know when i can handle more... and for sorting to be optionally
 done on the slave side.

funny, some time ago I also thought that (at least partially) sorting
the items on the slave side would make it possible to show the items
in the view much faster - if there was a way to tell the slave items
are sorted naturally by name, and we only want the first 100 for the
time being, then the slave could go through the entire dirctory very
fast, and only send us the first 100 files (possibly putting the
others into a TODO list which would be sent later, after the first
files have been shown on the screen).

Right now, the fact that we get the items in unsorted batches prevents
us from showing the first batch right after it has been sent. If we
did, items would jump around repeatedly whenever a new batch arrives.

 it is pretty ironic that a UI to show, say, videos from youtube will populate
 at least as smoothly, and on crappy enough hardware even better than, listing
 local directory contents because of this kind of incremental fetch and server-
 side pre-processing.

 KIO listing is all-or-nothing batch oriented; a stream-based approach that
 supports seeking through listings that are pre-sorted/grouped in the slave
 process would be moderately gorgeous. it would prevent more IPC than necessary
 and allow the slave to use anyall service-specific features for 
 pre-sort/group
 of entries.

That would be awesome indeed!

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

Re: KIO directory listing - CPU slows down SSD

2014-06-02 Thread Frank Reininghaus

Hi,

2014-06-02 20:54 GMT+02:00 Mark Gaiser:
 On Mon, Jun 2, 2014 at 6:42 PM, Aaron J. Seigo wrote:
 On Thursday, May 29, 2014 16:32:28 Mark Gaiser wrote:
 dolphin on a massive folder. In fact, those that use kio::listdir for
 listing folders only have interfaces that become usable when all
 entries are fetched.

 assuming these UIs receive maintenance over the next years, this pattern is
 likely to fade away. instant feedback and incremental listing is a current
 reality.

 you know what would be very nice (but rather complex to achieve...)? for the
 client side of a KIO listing to say i only want the first N items anyways,
 i'll let you know when i can handle more... and for sorting to be optionally
 done on the slave side.

 That would be very nice since you would cut IPC communication
 massively (depending on the folder) to just those that you see which
 would result in even insanely large folders to be presented to the
 user near instantly.

 But don't you just move logic to the slave that way? And lose
 flexibility in the apps using the slave (like dolphin? Oh and
 complicating kio a bit to pass a sorting and/or grouping style.
 Right now it's not really difficult to add a completely new sort order
 in dolphin, but that becomes very difficult when you want to let the
 slave do the sorting.

Yes, such an approach would indeed require additional complexity in
KIO and the slaves and would sacrifice flexibility.

Therefore, it might be questionable if this is really the way to go -
I didn't really mean that this should be implemented when I said that
I had also thought about this approach in my reply to Aaron.
Sometimes, I just dream and think about things that we could do if we
had infinite resources ;-)

I've seen users complain that loading huge folders is much faster in,
e.g., Windows Explorer than in Dolphin (that was some time ago though,
we might have caught up a bit in the mean time). Compared to us,
Microsoft's and Apple's resources are, to a good approximation,
infinite, so I always imagined that they would use every conceivable
trick to make their file managers faster.

 Now imagine you actually have this feature implemented and dolphin (or
 Accretion) adds a new sorting way that isn't in the slave Then you're
 back to square one and perhaps even slower then you are right now.

 Also, for a slave to give you the n items in a sorted way requires the
 slave to fetch _all_ items to do the sorting.

Yes, but the slave will only have to fetch the file names for all
items (provided that we sort by name), nothing else.

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

Re: KIO directory listing - CPU slows down SSD

2014-06-02 Thread Frank Reininghaus

Hi,

2014-06-02 23:39 GMT+02:00 Frank Reininghaus:
 Hi,

 2014-06-02 20:54 GMT+02:00 Mark Gaiser:
 On Mon, Jun 2, 2014 at 6:42 PM, Aaron J. Seigo wrote:
 On Thursday, May 29, 2014 16:32:28 Mark Gaiser wrote:
 dolphin on a massive folder. In fact, those that use kio::listdir for
 listing folders only have interfaces that become usable when all
 entries are fetched.

 assuming these UIs receive maintenance over the next years, this pattern is
 likely to fade away. instant feedback and incremental listing is a current
 reality.

 you know what would be very nice (but rather complex to achieve...)? for the
 client side of a KIO listing to say i only want the first N items anyways,
 i'll let you know when i can handle more... and for sorting to be 
 optionally
 done on the slave side.

 That would be very nice since you would cut IPC communication
 massively (depending on the folder) to just those that you see which
 would result in even insanely large folders to be presented to the
 user near instantly.

 But don't you just move logic to the slave that way? And lose
 flexibility in the apps using the slave (like dolphin? Oh and
 complicating kio a bit to pass a sorting and/or grouping style.
 Right now it's not really difficult to add a completely new sort order
 in dolphin, but that becomes very difficult when you want to let the
 slave do the sorting.

 Yes, such an approach would indeed require additional complexity in
 KIO and the slaves and would sacrifice flexibility.

 Therefore, it might be questionable if this is really the way to go -
 I didn't really mean that this should be implemented when I said that
 I had also thought about this approach in my reply to Aaron.
 Sometimes, I just dream and think about things that we could do if we
 had infinite resources ;-)

As a side note, if we had infinite resources, making Dolphin QML-based
would also have a high priority for me, but since my resources are
quite limited, I will probably not be able to take part in any such
effort in the near future.

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

Review Request 118452: Reduce the memory usage of UDSEntry by using QVector, rather than QHash, for the internal data storage

2014-06-01 Thread Frank Reininghaus


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

Review request for KDE Frameworks and David Faure.


Repository: kio


Description
---

I am continuing to split up https://git.reviewboard.kde.org/r/113355/ , which 
attempts to make UDSEntry more efficient memory and CPU-wise, into independent 
parts. This is the third step after 
https://git.reviewboard.kde.org/r/113591/ and 
https://git.reviewboard.kde.org/r/115739/ .

The present patch modifies the internal data storage of UDSEntry. UDSEntry 
contains a mapping from unsigned int keys to Field values, where Field is a 
struct that contains a QString and a long long (some keys correspond to numeric 
values, like size, date, etc, whereas others, like user and group, correspond 
to a QString).

Currently, UDSEntry stores all data in a QHashuint, Field internally. This 
ensures that everything can be accessed in O(1) time, but is not very efficient 
memory-wise because a separate memory allocation is done for each hash node.

I propose to change this and store both the uint keys and the Field values in a 
QVector each. This means that accessing a value becomes a O(n) operation, since 
the entire QVector of keys may need to be scanned in order to find a value, but 
because the number n of values in a UDSEntry is usually quite small and can 
currently not exceed a number ~100, this should not matter in practice.

Some parts of https://git.reviewboard.kde.org/r/113355/ are still missing:

(a) The QVectors which store the keys (which are usually the same for all items 
in a directory) are not shared yet. Changing this would reduce the memory usage 
further, but I decided to keep this change separate in order to keep the 
current patch small and easy to understand. Moreover, this makes it easier to 
benchmark other similar approaches (e.g., replace QVector by std::vector, or 
store keys and values together in a std::vectorstd::pairuint,Field).

(b) No space is reserved in the vectors when key/value pairs are inserted one 
by one. Implementing this would make UDSEntry faster on the slave side (since 
repeated re-allocations would not be necessary any more), but this can be done 
in a later patch. Moreover, it might not be needed any more if UDSEntry is not 
used directly any more on the slave side, as suggested on the frameworks 
mailing list by Aaron (good idea BTW!). 


Diffs
-

  src/core/udsentry.cpp c6ac21a 

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


Testing
---

Unit tests still pass.

The memory usage of listjobtest with a directory with 100,000 files is reduced 
from 71344 K to 35392 K according to KSysGuard. I see similar savings when 
opening the directory in Dolphin.

I still haven't set up a Qt5/KF5 build in release mode (shame on me!), so I 
cannot present any benchmark results.


Thanks,

Frank Reininghaus

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

Review Request 118269: Fix crash when showing the confirmation dialog for trash/delete operations

2014-05-22 Thread Frank Reininghaus


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

Review request for KDE Frameworks and David Faure.


Bugs: 334648
https://bugs.kde.org/show_bug.cgi?id=334648


Repository: kjobwidgets


Description
---

Currently Dolphin (and probably anything else that can delete files using KIO) 
crashes when it's supposed to show the confirmation dialog.

The problem is that KonqOperations::askDeleteConfirmation() sets up a 
KIO::JobUiDelegate object which has no associated job, and calls its 
setWindow(QWidget*) method before calling the actual askDeleteConfirmation() 
function.

KIO::JobUiDelegate::setWindow(QWidget*) then calls the setWindow function of 
the base class, KDialogJobUiDelegate::setWindow(QWidget*), which then crashes 
because it contains the line

Q_ASSERT(job())

and there is no job.

The problem does not exist in KDE SC 4.x - the code went through a big 
refactoring in

https://git.reviewboard.kde.org/r/111081/

which added the assert.

Just removing the assert won't help because the function then calls
KJobWidgets::setWindow(KJob *job, QWidget *widget)
which dereferences the job, i.e., we get a segfault instead.

This patch removes the assert and wraps the function in an if (job()) block 
instead. I'm not entirely sure if that is the correct solution though - any 
feedback is welcome. Alternatively, one could move the if-check to the child 
class, i.e., to KIO::JobUiDelegate::setWindow(QWidget*), if this is the only 
valid use of a KDialogJobUiDelegate without a job.

Or maybe it does not make much sense at all to have the askDeleteConfirmation 
function, which is probably always called before any job is set up, in a 
KDialogJobUiDelegate subclass? Changing that would probably require more 
intrusive changes though.


Diffs
-

  src/kdialogjobuidelegate.cpp fb4c99a 

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


Testing
---

Fixes the crash for me, and the confirmation dialog works as expected (i.e., 
the user can choose if the file should really be deleted/trashed or not).


Thanks,

Frank Reininghaus

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

Re: Review Request 118128: Use actual bytes formatter for sizes in KDirModel

2014-05-14 Thread Frank Reininghaus



 On May 14, 2014, 2:24 p.m., David Faure wrote:
  It is correct that this is about a string representation of the filesize, 
  to displaying in a column of the model.
  For machine processing one can use KFileItem::size() after getting the 
  KFileItem out of the KDirModel.
  
  However, do we really want that the detailed listview in dolphin? I can 
  more easily recognize the biggest number by being the one with most digits, 
  than having to go through a list of kB, MB, and GB values.
  
  It might even break the sorting by size.
  
  Let's ask Frank Reininghaus, but I'm not sure this is a good idea.

I might not have fully understood what exactly this patch will change in the 
GUI - both the file dialog (which uses KDirModel) and Dolphin already do show 
B/KiB/MiB/etc. values in the detailed view here (on a system with the rather 
old KDE SC 4.10.5 - I currently don't have anything else to test here), but 
maybe something changed about that in frameworks?

If QSortFilterProxyModel uses the value returned by the function that is 
modified by this patch (I think it does?), then this might really break sorting 
by size in the dialog. Have you checked this, Martin?


- Frank


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


On May 14, 2014, 2:01 p.m., Martin Klapetek wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/118128/
 ---
 
 (Updated May 14, 2014, 2:01 p.m.)
 
 
 Review request for KDE Frameworks and David Faure.
 
 
 Repository: kio
 
 
 Description
 ---
 
 Now I'm not sure if returning the unit together with the size won't break 
 some things, but as it returns string already, I thought returning it in the 
 human readable form would be better than always returning bytes.
 
 
 Diffs
 -
 
   src/widgets/kdirmodel.cpp 70d5ee4 
 
 Diff: https://git.reviewboard.kde.org/r/118128/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Martin Klapetek
 


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

Re: KIO directory listing - CPU slows down SSD

2014-05-11 Thread Frank Reininghaus

Hi Mark,

2014-05-11 21:57 GMT+02:00 Mark Gaiser:
 Hi,

 I've been playing with KIO speed improvements for quite a while now
 and found some very interesting issues with KIO in combination with my
 SSD drive: Samsung SSD 840 PRO Series.

 My testcase is one directory filled with 500.000 files to test
 directory listing speed. Please don't comment on this big number. I'm
 well aware that it's insane! However, it shows bottlenecks that are
 there but don't become visible with small sized folders like 1000
 entries.

 Some numbers. Listing a directory using just C++ and Qt (so QT_STATBUF,
 QT_READDIR and QT_LSTAT -- those are just platform defines. Nothing
 custom is done there)

 500.000 files: ~700ms

 Executing the same test using KIO::listDir:

 500.000 files: ~4.500ms

just to be sure, did you test this with a release build of Qt5 and KF5
(i.e., with optimizations enabled)?

Your mail reminds me that finishing the UDSEntry improvements that I
have been working on some time ago [1] to kio.git is still on my TODO
list - the main thing that I wanted to do before submitting a new
review request is to set up a separate build of all of Qt 5 and KF5 in
release mode because this is the only way to get reliable benchmark
results. I'll try to get this done during the next week. This should
speed up creating, writing and reading UDSEntries a lot because it
saves much of the overhead that is required for the internal QHash in
UDSEntry, in particular the memory allocations for the individual hash
nodes. Then we might have a better base for estimating what else could
be done to speed up directory listings.

Cheers,
Frank

[1] https://git.reviewboard.kde.org/r/113355/
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 117799: Clean up private slots in KCompletion

2014-04-30 Thread Frank Reininghaus


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

Ship it!


The change looks reasonable to me. It's a very straightforward transformation, 
and  I haven't spotted anything that looks like it could break things. 
Considering that the tests still work for you, I think that there is no reason 
not to push it.

- Frank Reininghaus


On April 26, 2014, 11:23 p.m., David Gil Oliva wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/117799/
 ---
 
 (Updated April 26, 2014, 11:23 p.m.)
 
 
 Review request for KDE Frameworks.
 
 
 Repository: kcompletion
 
 
 Description
 ---
 
 Clean up private slots
 
 Some private slots didn't have the _k_* form and some methods with the _k_* 
 form weren't even used as slots.
 
 
 Diffs
 -
 
   src/kcombobox.h 752db2c 
   src/kcombobox.cpp da31394 
   src/kcompletionbox.h 3295c24 
   src/kcompletionbox.cpp 7d03d64 
   src/khistorycombobox.h ea56358 
   src/khistorycombobox.cpp 9e2f0be 
   src/klineedit.h 705147d 
   src/klineedit.cpp 9d02c12 
   src/klineedit_p.h e544224 
 
 Diff: https://git.reviewboard.kde.org/r/117799/diff/
 
 
 Testing
 ---
 
 It builds. Autotests pass.
 
 
 Thanks,
 
 David Gil Oliva
 


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

Re: Review Request 116886: Refactor private variables of KCompletion

2014-03-21 Thread Frank Reininghaus


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

Ship it!


Looks reasonable to me, thanks!

- Frank Reininghaus


On March 19, 2014, 11:01 p.m., David Gil Oliva wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/116886/
 ---
 
 (Updated March 19, 2014, 11:01 p.m.)
 
 
 Review request for KDE Frameworks.
 
 
 Repository: kcompletion
 
 
 Description
 ---
 
 Refactor private variables of KCompletion
 
 Also: reorder variables declaration to avoid padding
 
 
 Diffs
 -
 
   src/kcompletion.cpp 7396029 
   src/kcompletion_p.h e3fad26 
 
 Diff: https://git.reviewboard.kde.org/r/116886/diff/
 
 
 Testing
 ---
 
 It builds. Autotests pass.
 
 
 Thanks,
 
 David Gil Oliva
 


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

Re: Review Request 116886: Refactor private variables of KCompletion

2014-03-19 Thread Frank Reininghaus


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



src/kcompletion_p.h
https://git.reviewboard.kde.org/r/116886/#comment37578

This is not strictly related to your changes, but it looks a bit unusual to 
have one plain bool and two bool bitfields next to each other. Making all bools 
a bitfield won't make much difference now though because the compiler will 
always use more memory for this class in order to preserve the 4-byte or 8-byte 
alignment.

Another alignment-related issue is caused by your patch though: on a 64-bit 
system, moving the int member away from the bools will most likely increase 
sizeof(KCompletionPrivate) by 8 bytes because the compiler will add some 
padding to both in order to preserve the alignment of the neigbouring pointers.

It might not make a big difference because it's probably unusual to create 
thousands of KCompletionPrivate instances, but still, it seems unnecessary.

If one really wanted to make use of bitfields to save memory here, one 
could make 'order' a bitfield and move it next to the bools. 


- Frank Reininghaus


On March 18, 2014, 11:01 p.m., David Gil Oliva wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/116886/
 ---
 
 (Updated March 18, 2014, 11:01 p.m.)
 
 
 Review request for KDE Frameworks.
 
 
 Repository: kcompletion
 
 
 Description
 ---
 
 Refactor private variables of KCompletion
 
 Also: reorder variables declaration to avoid padding
 
 
 Diffs
 -
 
   src/kcompletion_p.h e3fad26 
   src/kcompletion.cpp 7396029 
 
 Diff: https://git.reviewboard.kde.org/r/116886/diff/
 
 
 Testing
 ---
 
 It builds. Autotests pass.
 
 
 Thanks,
 
 David Gil Oliva
 


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

Re: Review Request 116542: Fix compilation with clang 3.4.

2014-03-04 Thread Frank Reininghaus


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

Ship it!


I wrote that code - sorry for the trouble and thanks for taking care of it. I 
wasn't aware at all that there might be a problem if the operator== is 
declared outside the namespace, but if I had been, then I would have put it 
inside he namespace, of course. So Ship it! from me too.

- Frank Reininghaus


On March 2, 2014, 8:20 p.m., Milian Wolff wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/116542/
 ---
 
 (Updated March 2, 2014, 8:20 p.m.)
 
 
 Review request for KDE Frameworks.
 
 
 Repository: kio
 
 
 Description
 ---
 
 Fix compilation with clang 3.4.
 
 Note that I'm not too sure why this compiled with GCC
 and why clang rejects the global operator== definition and
 wants to have it in the KIO namespace. Someone with more C++
 ADL knowledge should chime in whether this is the right fix.
 
 
 In file included from kio/tests/udsentrybenchmark.cpp:22:
 In file included from /usr/include/qt/QtTest/QTest:1:
 /usr/include/qt/QtTest/qtest.h:203:24:
  error: call to function 'operator==' that is neither visible
  in the template definition nor found by argument-dependent lookup
 if (!(t1.at(i) == t2.at(i))) {
^
 kio/tests/udsentrybenchmark.cpp:286:22: note: in instantiation of
 function template specialization 'QTest::qCompareKIO::UDSEntry'
  requested here
 
 do { if (!QTest::qCompare(entries, m_smallEntries, entries,
  m_smallEntries, kio/tests/udsentrybenchmark.cpp, 286)) return;}
  while (0);
 
 kio/tests/udsentrybenchmark.cpp:246:6: note: 'operator==' should be
 declared prior to the call site or in namespace 'KIO'
 
 bool operator==(const KIO::UDSEntry a, const KIO::UDSEntry b)
  ^
 1 error generated.
 udsentrybenchmark.dir/build.make:54: recipe for target
  'tests/CMakeFiles/udsentrybenchmark.dir/udsentrybenchmark.cpp.o'
  failed
 
 
 
 Diffs
 -
 
   tests/udsentrybenchmark.cpp 75fc758e583f7586c7b9a576d984b40912fa3ace 
 
 Diff: https://git.reviewboard.kde.org/r/116542/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Milian Wolff
 


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

Re: Review Request 115739: Make UDSEntry a Q_MOVABLE type, and add some benchmarks and tests

2014-02-26 Thread Frank Reininghaus


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

(Updated Feb. 27, 2014, 7:52 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and David Faure.


Repository: kio


Description
---

I'm continuing my efforts to make UDSEntry more efficient, which were started 
in https://git.reviewboard.kde.org/r/113591/. This is the second step, and I'll 
probably do more in the future, for which I will split 
https://git.reviewboard.kde.org/r/113355/ into independent parts.

This patch contains the following changes (which are separate commits):

1. Make UDSEntry a Q_MOVABLE type. This has the effect that a QListUDSEntry 
a.k.a. UDSEntryList will store the actual entries in a single allocated block 
of memory, and not pointers to UDSEntries which are allocated individually on 
the heap (this means that this change is binary incompatible). This reduces the 
memory usage by 32 bytes per UDSEntry in a QList because each memory allocation 
uses at least 32 bytes on a 64-bit system.

This idea is similar to what I proposed for KFileItem in 
https://git.reviewboard.kde.org/r/111789/. That one had to be reverted later 
though because it turned out that KDirLister does rely on QListKFileItem 
storing only pointers to the KFileItems. I'm confident that no such trouble 
will happen for UDSEntry - all KIO unit tests still pass.

One could argue that we could simply use a UDSEntryVector instead of a 
UDSEntyList to achieve the same memory savings, but considering that the list 
is used quite frequently ( http://lxr.kde.org/ident?i=UDSEntryList ), this 
might require a lot of porting work and cause other unexpected problems.

Note that I could not make the Q_DECLARE_TYPEINFO declaration work if it was 
inside the KIO namespace, but I still preferred to do it immediately after the 
class declaration, so I had to interrupt the namespace briefly.

2. Add some benchmarks to measure how long typical UDSEntry operations take: 
add fields to an entry, read fields from an entry, store a UDSEntryList in a 
QByteArray, and read it back from the QByteArray.

All measurements are done both for UDSEntries with 8 fields (this is a rather 
typical use case because kio_file usually creates 8 fields), and for entries 
with the maximum number of fields.

3. Add a simple manual test ('listjobtest') that runs a KIO::ListJob for a 
given URL. This can be used to easily measure the memory usage of the 
UDSEntryList that contains an entry for each file at that URL.


Diffs
-

  src/core/udsentry.h 9550a7e 
  tests/CMakeLists.txt dbca6a5 
  tests/listjobtest.cpp PRE-CREATION 
  tests/udsentrybenchmark.cpp PRE-CREATION 

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


Testing
---

1. KIO unit tests still pass.

2. I've confirmed with the new 'listjobtest' and KSysGuard that the memory 
usage is really lowered by 32 bytes per item in a UDSEntryList.

3. The benchmark results do not change significantly. I only tested it with a 
debug build (i.e., with optimizations disabled) though, and I'm afraid I might 
be lacking the resources to set up an additional build of Qt5 and all of KF5 in 
release mode. However, since UDSEntry essentially only depends on qtbase, I 
might be able to just do a release build of qtbase and build a stand-alone 
version of UDSEntry+benchmarks on top of that. I'll look into this option for 
getting reliable benchmark results when I work on further improvements in the 
future.


Thanks,

Frank Reininghaus

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

Re: Review Request 115710: Hide private methods and slots behind the d-pointer in KHistoryComboBox

2014-02-24 Thread Frank Reininghaus



 On Feb. 15, 2014, 7:38 p.m., David Faure wrote:
  src/khistorycombobox.cpp, line 508
  https://git.reviewboard.kde.org/r/115710/diff/1/?file=243810#file243810line508
 
  infinite recursion!
  
  Sounds like a unittest for reset() should be added.
 
 David Gil Oliva wrote:
 There are two reset() methods, the private slot and the public method, 
 which only calls the private slot. My error was not calling the d-pointer 
 method:
 
 void KHistoryComboBox::reset()
 {
 Q_D(KHistoryComboBox);
 d-reset();
 }
 
 Maybe the private slot could be substituted by the public method (making 
 it a public slot)?
 
 By the way, I would need a hint about the unittest that you think that 
 should be added.

Probably it's sufficient to simply call KHistoryComboBox::reset() in a unit 
test. This test would crash immediately with version 1 because of the infinite 
recursion.


- Frank


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


On Feb. 23, 2014, 11:23 p.m., David Gil Oliva wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/115710/
 ---
 
 (Updated Feb. 23, 2014, 11:23 p.m.)
 
 
 Review request for KDE Frameworks.
 
 
 Repository: kcompletion
 
 
 Description
 ---
 
 Hide private methods and slots behind the d-pointer in KHistoryComboBox
 
 Also:
 -Remove header not used
 
 
 Diffs
 -
 
   src/khistorycombobox.h 3667eb4 
   src/khistorycombobox.cpp 6f81dda 
 
 Diff: https://git.reviewboard.kde.org/r/115710/diff/
 
 
 Testing
 ---
 
 It builds. Tests pass.
 
 
 Thanks,
 
 David Gil Oliva
 


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

Re: Review Request 115739: Make UDSEntry a Q_MOVABLE type, and add some benchmarks and tests

2014-02-16 Thread Frank Reininghaus



 On Feb. 15, 2014, 8:28 p.m., David Faure wrote:
  Yeah the description is wrong. Making something Q_MOVABLE means that 
  inserting into the list, or the copying that happens when reallocating for 
  more space, will be able to memmove() instead of copy-constructing items. 
  This doesn't change the actual memory layout (which only depends on the 
  size of the items).
  
  Basically, as long as UDSEntry doesn't have a q pointer in its private 
  class (which it doesn't), it's movable. So marking it as movable is 
  correct, but not with this commit message.
  
  As to a difference in performance, I would be surprised if it was 
  measurable. The copy ctor for UDSEntry plays with a refcount.

Yeah the description is wrong. Making something Q_MOVABLE means that inserting 
into the list, or the copying that happens when reallocating for more space, 
will be able to memmove() instead of copy-constructing items. This doesn't 
change the actual memory layout (which only depends on the size of the items)

I am afraid this is wrong. A QListT is essentially a small wrapper around a 
QListvoid*, which *always* uses memmove() to move around its items. If 
sizeof(T) = sizeof(void*), and it's known that using memmove() is safe for T 
(e.g. because it's POD or Qt itself declares it as Q_MOVABLE), then QList just 
reinterprets each void* as an item of type T.

If that is not the case, then QListT will effectively become a QListT*, and 
allocate memory for each item individually on the heap.

So yes, making something Q_MOVABLE definitely changes the actual memory layout.

Anyone who does not believe me is encouraged to have a quick look at these 
excellent posts by Marc Mutz:

http://marcmutz.wordpress.com/2010/07/29/sneak-preview-qlist-considered-harmful/

http://marcmutz.wordpress.com/effective-qt/containers/

or look at the source,

http://code.woboq.org/qt5/qtbase/src/corelib/tools/qlist.h.html

Note that void QListT::append(const T t) calls the function

void QListT::node_construct(Node *n, const T t)

which does

n-v = new T(t)

if QTypeInfoT::isLarge (which means that T is larger than a void*) or 
QTypeInfoT::isStatic (which means that T has not been declared as Q_MOVABLE). 
This is all explained in Marc's second post.


As to a difference in performance, I would be surprised if it was measurable. 
The copy ctor for UDSEntry plays with a refcount.

My point is *not* that this change saves us the refcount updates when items are 
moved around.

What is saves is that a small block (including the overhead that the memory 
allocator adds to each allocatin, usually 32 bytes) of memory is 
allocated/deallocated every time an item is added to/removed from the list.

This might still not affect the performance noticeably in many cases, but 
allocating memory is a lot more expensive than just increasing a refcount.


- Frank


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


On Feb. 13, 2014, 8:23 p.m., Frank Reininghaus wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/115739/
 ---
 
 (Updated Feb. 13, 2014, 8:23 p.m.)
 
 
 Review request for KDE Frameworks and David Faure.
 
 
 Repository: kio
 
 
 Description
 ---
 
 I'm continuing my efforts to make UDSEntry more efficient, which were started 
 in https://git.reviewboard.kde.org/r/113591/. This is the second step, and 
 I'll probably do more in the future, for which I will split 
 https://git.reviewboard.kde.org/r/113355/ into independent parts.
 
 This patch contains the following changes (which are separate commits):
 
 1. Make UDSEntry a Q_MOVABLE type. This has the effect that a QListUDSEntry 
 a.k.a. UDSEntryList will store the actual entries in a single allocated block 
 of memory, and not pointers to UDSEntries which are allocated individually on 
 the heap (this means that this change is binary incompatible). This reduces 
 the memory usage by 32 bytes per UDSEntry in a QList because each memory 
 allocation uses at least 32 bytes on a 64-bit system.
 
 This idea is similar to what I proposed for KFileItem in 
 https://git.reviewboard.kde.org/r/111789/. That one had to be reverted later 
 though because it turned out that KDirLister does rely on QListKFileItem 
 storing only pointers to the KFileItems. I'm confident that no such trouble 
 will happen for UDSEntry - all KIO unit tests still pass.
 
 One could argue that we could simply use a UDSEntryVector instead of a 
 UDSEntyList to achieve the same memory savings, but considering that the list 
 is used quite frequently ( http://lxr.kde.org/ident?i=UDSEntryList ), this 
 might require a lot of porting work and cause other unexpected problems

Review Request 115739: Make UDSEntry a Q_MOVABLE type, and add some benchmarks and tests

2014-02-13 Thread Frank Reininghaus


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

Review request for KDE Frameworks and David Faure.


Repository: kio


Description
---

I'm continuing my efforts to make UDSEntry more efficient, which were started 
in https://git.reviewboard.kde.org/r/113591/. This is the second step, and I'll 
probably do more in the future, for which I will split 
https://git.reviewboard.kde.org/r/113355/ into independent parts.

This patch contains the following changes (which are separate commits):

1. Make UDSEntry a Q_MOVABLE type. This has the effect that a QListUDSEntry 
a.k.a. UDSEntryList will store the actual entries in a single allocated block 
of memory, and not pointers to UDSEntries which are allocated individually on 
the heap (this means that this change is binary incompatible). This reduces the 
memory usage by 32 bytes per UDSEntry in a QList because each memory allocation 
uses at least 32 bytes on a 64-bit system.

This idea is similar to what I proposed for KFileItem in 
https://git.reviewboard.kde.org/r/111789/. That one had to be reverted later 
though because it turned out that KDirLister does rely on QListKFileItem 
storing only pointers to the KFileItems. I'm confident that no such trouble 
will happen for UDSEntry - all KIO unit tests still pass.

One could argue that we could simply use a UDSEntryVector instead of a 
UDSEntyList to achieve the same memory savings, but considering that the list 
is used quite frequently ( http://lxr.kde.org/ident?i=UDSEntryList ), this 
might require a lot of porting work and cause other unexpected problems.

Note that I could not make the Q_DECLARE_TYPEINFO declaration work if it was 
inside the KIO namespace, but I still preferred to do it immediately after the 
class declaration, so I had to interrupt the namespace briefly.

2. Add some benchmarks to measure how long typical UDSEntry operations take: 
add fields to an entry, read fields from an entry, store a UDSEntryList in a 
QByteArray, and read it back from the QByteArray.

All measurements are done both for UDSEntries with 8 fields (this is a rather 
typical use case because kio_file usually creates 8 fields), and for entries 
with the maximum number of fields.

3. Add a simple manual test ('listjobtest') that runs a KIO::ListJob for a 
given URL. This can be used to easily measure the memory usage of the 
UDSEntryList that contains an entry for each file at that URL.


Diffs
-

  src/core/udsentry.h 9550a7e 
  tests/CMakeLists.txt dbca6a5 
  tests/listjobtest.cpp PRE-CREATION 
  tests/udsentrybenchmark.cpp PRE-CREATION 

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


Testing
---

1. KIO unit tests still pass.

2. I've confirmed with the new 'listjobtest' and KSysGuard that the memory 
usage is really lowered by 32 bytes per item in a UDSEntryList.

3. The benchmark results do not change significantly. I only tested it with a 
debug build (i.e., with optimizations disabled) though, and I'm afraid I might 
be lacking the resources to set up an additional build of Qt5 and all of KF5 in 
release mode. However, since UDSEntry essentially only depends on qtbase, I 
might be able to just do a release build of qtbase and build a stand-alone 
version of UDSEntry+benchmarks on top of that. I'll look into this option for 
getting reliable benchmark results when I work on further improvements in the 
future.


Thanks,

Frank Reininghaus

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

Re: Review Request 115739: Make UDSEntry a Q_MOVABLE type, and add some benchmarks and tests

2014-02-13 Thread Frank Reininghaus



 On Feb. 13, 2014, 9:31 p.m., Christoph Feck wrote:
  Making the type movable does not make QList store it directly, how did you 
  check this?
  
  http://qt-project.org/doc/qt-5/qlist.html says:
  
  Internally, QListT is represented as an array of pointers to items of 
  type T. If T is itself a pointer type or a basic type that is no larger 
  than a pointer, or if T is one of Qt's shared classes, then QListT stores 
  the items directly in the pointer array.
 
 Christoph Feck wrote:
 Wait, UDSEntry is just a pointer, right?

Yes, it is a pointer to a UDSEntryPrivate. Thanks for bringing this issue up!


- Frank


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


On Feb. 13, 2014, 8:23 p.m., Frank Reininghaus wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/115739/
 ---
 
 (Updated Feb. 13, 2014, 8:23 p.m.)
 
 
 Review request for KDE Frameworks and David Faure.
 
 
 Repository: kio
 
 
 Description
 ---
 
 I'm continuing my efforts to make UDSEntry more efficient, which were started 
 in https://git.reviewboard.kde.org/r/113591/. This is the second step, and 
 I'll probably do more in the future, for which I will split 
 https://git.reviewboard.kde.org/r/113355/ into independent parts.
 
 This patch contains the following changes (which are separate commits):
 
 1. Make UDSEntry a Q_MOVABLE type. This has the effect that a QListUDSEntry 
 a.k.a. UDSEntryList will store the actual entries in a single allocated block 
 of memory, and not pointers to UDSEntries which are allocated individually on 
 the heap (this means that this change is binary incompatible). This reduces 
 the memory usage by 32 bytes per UDSEntry in a QList because each memory 
 allocation uses at least 32 bytes on a 64-bit system.
 
 This idea is similar to what I proposed for KFileItem in 
 https://git.reviewboard.kde.org/r/111789/. That one had to be reverted later 
 though because it turned out that KDirLister does rely on QListKFileItem 
 storing only pointers to the KFileItems. I'm confident that no such trouble 
 will happen for UDSEntry - all KIO unit tests still pass.
 
 One could argue that we could simply use a UDSEntryVector instead of a 
 UDSEntyList to achieve the same memory savings, but considering that the list 
 is used quite frequently ( http://lxr.kde.org/ident?i=UDSEntryList ), this 
 might require a lot of porting work and cause other unexpected problems.
 
 Note that I could not make the Q_DECLARE_TYPEINFO declaration work if it was 
 inside the KIO namespace, but I still preferred to do it immediately after 
 the class declaration, so I had to interrupt the namespace briefly.
 
 2. Add some benchmarks to measure how long typical UDSEntry operations take: 
 add fields to an entry, read fields from an entry, store a UDSEntryList in a 
 QByteArray, and read it back from the QByteArray.
 
 All measurements are done both for UDSEntries with 8 fields (this is a rather 
 typical use case because kio_file usually creates 8 fields), and for entries 
 with the maximum number of fields.
 
 3. Add a simple manual test ('listjobtest') that runs a KIO::ListJob for a 
 given URL. This can be used to easily measure the memory usage of the 
 UDSEntryList that contains an entry for each file at that URL.
 
 
 Diffs
 -
 
   src/core/udsentry.h 9550a7e 
   tests/CMakeLists.txt dbca6a5 
   tests/listjobtest.cpp PRE-CREATION 
   tests/udsentrybenchmark.cpp PRE-CREATION 
 
 Diff: https://git.reviewboard.kde.org/r/115739/diff/
 
 
 Testing
 ---
 
 1. KIO unit tests still pass.
 
 2. I've confirmed with the new 'listjobtest' and KSysGuard that the memory 
 usage is really lowered by 32 bytes per item in a UDSEntryList.
 
 3. The benchmark results do not change significantly. I only tested it with a 
 debug build (i.e., with optimizations disabled) though, and I'm afraid I 
 might be lacking the resources to set up an additional build of Qt5 and all 
 of KF5 in release mode. However, since UDSEntry essentially only depends on 
 qtbase, I might be able to just do a release build of qtbase and build a 
 stand-alone version of UDSEntry+benchmarks on top of that. I'll look into 
 this option for getting reliable benchmark results when I work on further 
 improvements in the future.
 
 
 Thanks,
 
 Frank Reininghaus
 


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

Review Request 114921: Make KFileItemActions the parent of the actions it creates

2014-01-09 Thread Frank Reininghaus


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

Review request for KDE Frameworks and David Faure.


Repository: kio


Description
---

This patch is a result of the discussion in 
http://lists.kde.org/?t=13868700941r=1w=2

Currently, KFileItemActions makes the widget that is set with 
setParentWidget(QWidget*) the parent not only of any dialogs that are shown (as 
advertised by the API docs), but also of the created actions. Nonetheless, 
KFileItemActions remembers pointers to all created actions and deletes them in 
the destructor. This can cause problems if the widget is deleted before the 
KFileItemActions instance - the destructor will then try to delete dangling 
pointers and cause a crash.

This problem can be fixed by making KFileItemActions the parent of the actions. 
This also makes the code a bit simpler because the m_ownActions member is not 
needed any more.

In fact, this issue is the cause of crashes in Dolphin (see 
https://bugs.kde.org/show_bug.cgi?id=259089). I still think that we don't 
really have to change it in kdelibs 4.x because the problem can be worked 
around (https://git.reviewboard.kde.org/r/114440/ , which isn't pushed yet 
because it turns out that there is still another source of crashes in the 
problematic Dolphin use case).


Diffs
-

  autotests/CMakeLists.txt 2868327 
  autotests/kfileitemactionstest.cpp PRE-CREATION 
  src/widgets/kfileitemactions.cpp eee2ebe 
  src/widgets/kfileitemactions_p.h 9f9a701 

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


Testing
---

New unit test crashes with master, and passes if the patch is applied.

Existing kio unit tests pass on my system (except for kiocore-kacltest, but I 
believe that the failure is unrelated to this patch).


Thanks,

Frank Reininghaus

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

Re: Review Request 114921: Make KFileItemActions the parent of the actions it creates

2014-01-09 Thread Frank Reininghaus


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

(Updated Jan. 9, 2014, 9:26 p.m.)


Review request for KDE Frameworks and David Faure.


Changes
---

Thanks everyone for the comments! I've updated the diff, which I will push soon:

1. Use QUrl::fromLocalFile() as suggested by David.
2. Updated copyright year.
3. Added a few 'const' where it was possible.

About Alexander's idea to make the QMenu the parent of the actions: usually, 
one does not make the menu the parent of the actions (see, e.g., David's 
explanation in http://lists.kde.org/?l=kfm-develm=138688704130331w=2 ). 
Nonetheless, we are not going to waste memory. At least Konqueror's and 
Dolphin's context menu implementations destroy the KFileItemActions instance 
(including, after this patch, the actions) when the menu itself is deleted.


Repository: kio


Description
---

This patch is a result of the discussion in 
http://lists.kde.org/?t=13868700941r=1w=2

Currently, KFileItemActions makes the widget that is set with 
setParentWidget(QWidget*) the parent not only of any dialogs that are shown (as 
advertised by the API docs), but also of the created actions. Nonetheless, 
KFileItemActions remembers pointers to all created actions and deletes them in 
the destructor. This can cause problems if the widget is deleted before the 
KFileItemActions instance - the destructor will then try to delete dangling 
pointers and cause a crash.

This problem can be fixed by making KFileItemActions the parent of the actions. 
This also makes the code a bit simpler because the m_ownActions member is not 
needed any more.

In fact, this issue is the cause of crashes in Dolphin (see 
https://bugs.kde.org/show_bug.cgi?id=259089). I still think that we don't 
really have to change it in kdelibs 4.x because the problem can be worked 
around (https://git.reviewboard.kde.org/r/114440/ , which isn't pushed yet 
because it turns out that there is still another source of crashes in the 
problematic Dolphin use case).


Diffs (updated)
-

  autotests/CMakeLists.txt 2868327 
  autotests/kfileitemactionstest.cpp PRE-CREATION 
  src/widgets/kfileitemactions.cpp eee2ebe 
  src/widgets/kfileitemactions_p.h 9f9a701 

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


Testing
---

New unit test crashes with master, and passes if the patch is applied.

Existing kio unit tests pass on my system (except for kiocore-kacltest, but I 
believe that the failure is unrelated to this patch).


Thanks,

Frank Reininghaus

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

Re: Review Request 114921: Make KFileItemActions the parent of the actions it creates

2014-01-09 Thread Frank Reininghaus


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

(Updated Jan. 9, 2014, 9:28 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and David Faure.


Repository: kio


Description
---

This patch is a result of the discussion in 
http://lists.kde.org/?t=13868700941r=1w=2

Currently, KFileItemActions makes the widget that is set with 
setParentWidget(QWidget*) the parent not only of any dialogs that are shown (as 
advertised by the API docs), but also of the created actions. Nonetheless, 
KFileItemActions remembers pointers to all created actions and deletes them in 
the destructor. This can cause problems if the widget is deleted before the 
KFileItemActions instance - the destructor will then try to delete dangling 
pointers and cause a crash.

This problem can be fixed by making KFileItemActions the parent of the actions. 
This also makes the code a bit simpler because the m_ownActions member is not 
needed any more.

In fact, this issue is the cause of crashes in Dolphin (see 
https://bugs.kde.org/show_bug.cgi?id=259089). I still think that we don't 
really have to change it in kdelibs 4.x because the problem can be worked 
around (https://git.reviewboard.kde.org/r/114440/ , which isn't pushed yet 
because it turns out that there is still another source of crashes in the 
problematic Dolphin use case).


Diffs
-

  autotests/CMakeLists.txt 2868327 
  autotests/kfileitemactionstest.cpp PRE-CREATION 
  src/widgets/kfileitemactions.cpp eee2ebe 
  src/widgets/kfileitemactions_p.h 9f9a701 

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


Testing
---

New unit test crashes with master, and passes if the patch is applied.

Existing kio unit tests pass on my system (except for kiocore-kacltest, but I 
believe that the failure is unrelated to this patch).


Thanks,

Frank Reininghaus

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

Re: Tracking bugs in Frameworks

2013-12-14 Thread Frank Reininghaus

Hi,

2013/12/14 David Edmundson:
 As we gear Frameworks up for release we need a way to track bugs that
 exist in the new Frameworks.

 We have two options; either we copy all the bugs in kdelibs, triaging,
 testing and moving to the right new component or we start fresh.

 There are approximate 1400 open bugs marked against in kdelibs; many
 of these I think are invalid duplicates. It's not an impossible amount
 to triage, but it would still be a large amount of work to test and
 then either move or resolve them.

 Given we still plan to support bugs in kdelibs officially for a while
 yet and I personally think it would be easiest for everyone to make a
 new bugzilla product called Frameworks and add newly found Frameworks
 bugs there.

an alternative would be to let every Framework have its own product.
Some parts of kdelibs have had their own products for quite some time
(e.g., kio and kfile), whereas others are tracked at the generic
product kdelibs, or at yet other products (like konqeuror/khtml)

IMHO, it would be much more transparent for bug triagers, developers,
and users if there was a nice 1:1 correspondence between the split
repositories/frameworks and the bugzilla products. One could argue
that each repository could also be a component of the product
Frameworks, but this would remove the option to define more
fine-grained components for a particular framework (e.g., the
current product kfile has some different components for different
parts of that library).

Maybe one could create a bugzilla product for each Framework (unless
the product exists already, like kio). One could set up a version
frameworks in each of them as long as the frameworks don't have
proper versions yet.

About the existing kdelibs bugs: I think they should just remain
assigned to kdelibs as long as 4.x is supported. If any of these
bugs is fixed, one just has to remember to forward-port the fix to the
split frameworks repositories.

Finally, I think it's crucial to consider the opinion of the people
who do the most work at bugs.kde.org. I've CC'ed some of them because
I'm not sure if they follow this mailing list.

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

Re: Review Request 113877: Use QMutableMapIterator for removing KStartupInfoIds

2013-11-25 Thread Frank Reininghaus



 On Nov. 25, 2013, 1:48 p.m., Kevin Ottens wrote:
  Any chance for a review? We really need to tie up loose ends now.

I think that this is a very nice piece of work, and the new data-driven test 
also looks good to me. The only thing I'm concerned about is the run time of 
the test - I like unit testing very much, but tests with a very long run time 
might make people avoid make test, and thus bear the risk that we see more 
regressions in the future. Maybe this review request is not the best place to 
discuss this issue though, in particular if it should go in soon in order to 
not delay the splitting (and also to fix the crash, of course). Maybe one could 
discuss requirements for unit tests at some later point. (One possible solution 
would be to make this a manual test. Yes, it would mean that people will run 
this test less often, but if it prevents that people stop running make test 
altogether, maybe it's still worth considering).

I don't really feel qualified to approve changes in kwindowsystem though ;-)


- Frank


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113877/#review44397
---


On Nov. 18, 2013, 6:04 a.m., Martin Gräßlin wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/113877/
 ---
 
 (Updated Nov. 18, 2013, 6:04 a.m.)
 
 
 Review request for KDE Frameworks.
 
 
 Repository: kdelibs
 
 
 Description
 ---
 
 If there were multiple KStartupInfoIds which were removed at the same time, 
 it was possible that the map got changed while being iterated, thus the data 
 corrupted and we hit a crash later on. This changes to a QMutableMapIterator 
 for checking the cleanup and allowing to safely remove ids during the 
 iteration.
 
 
 Diffs
 -
 
   tier1/kwindowsystem/src/kstartupinfo.cpp 402cc97 
   tier1/kwindowsystem/autotests/kstartupinfo_unittest.cpp 17890ff 
 
 Diff: http://git.reviewboard.kde.org/r/113877/diff/
 
 
 Testing
 ---
 
 When running the unit tests on the old code base it isn't guaranteed that it 
 crashes as it's a race and also depends on X. Just run a few times and it 
 will certainly hit the abort condition.
 
 For reference the backtrace of the crash before fix:
 #0  0x750131e5 in __GI_raise (sig=sig@entry=6) at 
 ../nptl/sysdeps/unix/sysv/linux/raise.c:56
 #1  0x75016398 in __GI_abort () at abort.c:90
 #2  0x75c3d402 in qt_message_fatal (context=..., message=...) at 
 global/qlogging.cpp:979
 #3  0x75c3afce in QMessageLogger::fatal (this=0x7fffbbc0, 
 msg=0x75f8d9f8 ASSERT: \%s\ in file %s, line %d) at 
 global/qlogging.cpp:384
 #4  0x75c369e6 in qt_assert (assertion=0x75f8b7f0 size == 0 || 
 offset  0 || size_t(offset) = sizeof(QArrayData), file=0x75f8b7b0 
 ../../include/QtCore/../../src/corelib/tools/qarraydata.h, line=62)
 at global/qglobal.cpp:2088
 #5  0x75c29bf8 in QArrayData::data (this=0x6cb830) at 
 ../../include/QtCore/../../src/corelib/tools/qarraydata.h:61
 #6  0x75c3015e in QTypedArrayDatachar::data (this=0x6cb830) at 
 ../../include/QtCore/../../src/corelib/tools/qarraydata.h:207
 #7  0x75c2fde7 in QByteArray::constData (this=0x6f4760) at 
 ../../include/QtCore/../../src/corelib/tools/qbytearray.h:433
 #8  0x75c5 in qstrcmp (str1=..., str2=...) at 
 tools/qbytearray.cpp:347
 #9  0x77ba4625 in operator (a1=..., a2=...) at 
 /opt/qt5/include/QtCore/qbytearray.h:550
 #10 0x77ba0eb9 in KStartupInfoId::operator (this=0x693858, id_P=...) 
 at 
 /home/martin/src/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/kstartupinfo.cpp:1175
 #11 0x77ba67b5 in qMapLessThanKeyKStartupInfoId (key1=..., 
 key2=...) at /opt/qt5/include/QtCore/qmap.h:75
 #12 0x77ba6ef3 in QMapNodeKStartupInfoId, 
 KStartupInfo::Data::lowerBound (this=0x693840, akey=...) at 
 /opt/qt5/include/QtCore/qmap.h:145
 #13 0x77ba66b4 in QMapDataKStartupInfoId, 
 KStartupInfo::Data::findNode (this=0x700870, akey=...) at 
 /opt/qt5/include/QtCore/qmap.h:292
 #14 0x77ba5a73 in QMapKStartupInfoId, KStartupInfo::Data::remove 
 (this=0x65acc0, akey=...) at /opt/qt5/include/QtCore/qmap.h:897
 #15 0x77b9dd42 in KStartupInfo::Private::remove_startup_info_internal 
 (this=0x65acb0, id_P=...) at 
 /home/martin/src/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/kstartupinfo.cpp:399
 #16 0x77ba039b in KStartupInfo::Private::startups_cleanup_internal 
 (this=0x65acb0, age_P=true) at 
 /home/martin/src/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/kstartupinfo.cpp:984
 #17 0x77ba02bf in KStartupInfo::Private::startups_cleanup 
 (this=0x65acb0) at

Re: Review Request 113877: Use QMutableMapIterator for removing KStartupInfoIds

2013-11-15 Thread Frank Reininghaus


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113877/#review43717
---


I think the root cause of the corruption is that there is a ++it in line 981, 
and then remove_startup_info_internal( key ) in the following line, which 
calls QMap::remove(key). The latter invalidates all existing iterators for the 
corresponding map, so any later use of it is unsafe.

An alternative would be to use it = startups.erase(it) instead. 
QMap::erase(it) always returns a valid iterator pointing to the next item 
(which may be end()). That approach could also be combined with your refactored 
function (which looks much nicer than the threefold repetition of mostly the 
same code, nice!), but I guess it's a matter of taste if you prefer the 
Java-style iterators, or you use the STL-style ones in the correct and safe 
way. I prefer the STL ones though, because you can still use *it and 
it-foo() to dereference them.

Most (or all?) Qt and STL containers support it = container.erase(it) for the 
removal of an item that an iterator points to, and having it point to the 
next item (or end()) afterwards.

- Frank Reininghaus


On Nov. 15, 2013, 9:48 a.m., Martin Gräßlin wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/113877/
 ---
 
 (Updated Nov. 15, 2013, 9:48 a.m.)
 
 
 Review request for KDE Frameworks.
 
 
 Repository: kdelibs
 
 
 Description
 ---
 
 If there were multiple KStartupInfoIds which were removed at the same time, 
 it was possible that the map got changed while being iterated, thus the data 
 corrupted and we hit a crash later on. This changes to a QMutableMapIterator 
 for checking the cleanup and allowing to safely remove ids during the 
 iteration.
 
 
 Diffs
 -
 
   tier1/kwindowsystem/autotests/kstartupinfo_unittest.cpp 17890ff 
   tier1/kwindowsystem/src/kstartupinfo.cpp 402cc97 
 
 Diff: http://git.reviewboard.kde.org/r/113877/diff/
 
 
 Testing
 ---
 
 When running the unit tests on the old code base it isn't guaranteed that it 
 crashes as it's a race and also depends on X. Just run a few times and it 
 will certainly hit the abort condition.
 
 For reference the backtrace of the crash before fix:
 #0  0x750131e5 in __GI_raise (sig=sig@entry=6) at 
 ../nptl/sysdeps/unix/sysv/linux/raise.c:56
 #1  0x75016398 in __GI_abort () at abort.c:90
 #2  0x75c3d402 in qt_message_fatal (context=..., message=...) at 
 global/qlogging.cpp:979
 #3  0x75c3afce in QMessageLogger::fatal (this=0x7fffbbc0, 
 msg=0x75f8d9f8 ASSERT: \%s\ in file %s, line %d) at 
 global/qlogging.cpp:384
 #4  0x75c369e6 in qt_assert (assertion=0x75f8b7f0 size == 0 || 
 offset  0 || size_t(offset) = sizeof(QArrayData), file=0x75f8b7b0 
 ../../include/QtCore/../../src/corelib/tools/qarraydata.h, line=62)
 at global/qglobal.cpp:2088
 #5  0x75c29bf8 in QArrayData::data (this=0x6cb830) at 
 ../../include/QtCore/../../src/corelib/tools/qarraydata.h:61
 #6  0x75c3015e in QTypedArrayDatachar::data (this=0x6cb830) at 
 ../../include/QtCore/../../src/corelib/tools/qarraydata.h:207
 #7  0x75c2fde7 in QByteArray::constData (this=0x6f4760) at 
 ../../include/QtCore/../../src/corelib/tools/qbytearray.h:433
 #8  0x75c5 in qstrcmp (str1=..., str2=...) at 
 tools/qbytearray.cpp:347
 #9  0x77ba4625 in operator (a1=..., a2=...) at 
 /opt/qt5/include/QtCore/qbytearray.h:550
 #10 0x77ba0eb9 in KStartupInfoId::operator (this=0x693858, id_P=...) 
 at 
 /home/martin/src/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/kstartupinfo.cpp:1175
 #11 0x77ba67b5 in qMapLessThanKeyKStartupInfoId (key1=..., 
 key2=...) at /opt/qt5/include/QtCore/qmap.h:75
 #12 0x77ba6ef3 in QMapNodeKStartupInfoId, 
 KStartupInfo::Data::lowerBound (this=0x693840, akey=...) at 
 /opt/qt5/include/QtCore/qmap.h:145
 #13 0x77ba66b4 in QMapDataKStartupInfoId, 
 KStartupInfo::Data::findNode (this=0x700870, akey=...) at 
 /opt/qt5/include/QtCore/qmap.h:292
 #14 0x77ba5a73 in QMapKStartupInfoId, KStartupInfo::Data::remove 
 (this=0x65acc0, akey=...) at /opt/qt5/include/QtCore/qmap.h:897
 #15 0x77b9dd42 in KStartupInfo::Private::remove_startup_info_internal 
 (this=0x65acb0, id_P=...) at 
 /home/martin/src/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/kstartupinfo.cpp:399
 #16 0x77ba039b in KStartupInfo::Private::startups_cleanup_internal 
 (this=0x65acb0, age_P=true) at 
 /home/martin/src/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/kstartupinfo.cpp:984
 #17 0x77ba02bf in KStartupInfo::Private::startups_cleanup 
 (this=0x65acb0) at 
 /home/martin/src/kf5/kdelibs-frameworks/tier1/kwindowsystem

Re: Review Request 113877: Use QMutableMapIterator for removing KStartupInfoIds

2013-11-15 Thread Frank Reininghaus


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113877/#review43727
---


I'm not a KWindowSystem expert, but anyway, here are some more things that I 
noticed, and that might or might not be worth considering:


1. I think that you can simplify the code in 
KStartupInfo::Private::startups_cleanup_internal(bool) slightly by using

it-age++;
if (it-silent() == Data::Yes)

etc. Might be a matter of taste though ;-) I think that this is slightly easier 
to read, but the compiler will most likely generate the same code for both 
variants.


2. Since your three test functions share the vast majority of their code, one 
could use data-driven testing:

http://qt-project.org/doc/qt-5.1/qttestlib/tutorial2.html

That could make future maintenance of the test functions easier, and it also 
makes it easier to see what the difference between the three tests is.


3. If I read the test code correctly, the unit test will have a runtime of at 
least 27 seconds because of the QTest::qWait(int) calls, right? I think that 
tests should run as fast as possible - if they don't, it gives people an 
excellent excuse for not running them, which greatly increases the risk of 
regressions. 

For the test that checks if the QSignalSpy finally has a certain count(), one 
could use a loop over a qWait(int) with a smaller timeout that terminates if 
the count() has reached the expected value. (For the case that the expected 
count() was 1, we had kWaitForSignal, but this is probably not an option for 
tier 1. Internally, that function uses a QEventLoop which has its quit() slot 
connected to the expected signal, I think).

I'm not sure about the test that waits for 21 seconds and passes if the count() 
is zero though. I guess that the 21 seconds are required for safety, such that 
the test has a predictable outcome even if runs on a slow system with heavy 
load? Some knowledge of the code to be tested might be required to come up with 
a less time-consuming solution. If there was a signal that was emitted if 
everything went OK, one could simply wait for it.


- Frank Reininghaus


On Nov. 15, 2013, 10:42 a.m., Martin Gräßlin wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/113877/
 ---
 
 (Updated Nov. 15, 2013, 10:42 a.m.)
 
 
 Review request for KDE Frameworks.
 
 
 Repository: kdelibs
 
 
 Description
 ---
 
 If there were multiple KStartupInfoIds which were removed at the same time, 
 it was possible that the map got changed while being iterated, thus the data 
 corrupted and we hit a crash later on. This changes to a QMutableMapIterator 
 for checking the cleanup and allowing to safely remove ids during the 
 iteration.
 
 
 Diffs
 -
 
   tier1/kwindowsystem/autotests/kstartupinfo_unittest.cpp 17890ff 
   tier1/kwindowsystem/src/kstartupinfo.cpp 402cc97 
 
 Diff: http://git.reviewboard.kde.org/r/113877/diff/
 
 
 Testing
 ---
 
 When running the unit tests on the old code base it isn't guaranteed that it 
 crashes as it's a race and also depends on X. Just run a few times and it 
 will certainly hit the abort condition.
 
 For reference the backtrace of the crash before fix:
 #0  0x750131e5 in __GI_raise (sig=sig@entry=6) at 
 ../nptl/sysdeps/unix/sysv/linux/raise.c:56
 #1  0x75016398 in __GI_abort () at abort.c:90
 #2  0x75c3d402 in qt_message_fatal (context=..., message=...) at 
 global/qlogging.cpp:979
 #3  0x75c3afce in QMessageLogger::fatal (this=0x7fffbbc0, 
 msg=0x75f8d9f8 ASSERT: \%s\ in file %s, line %d) at 
 global/qlogging.cpp:384
 #4  0x75c369e6 in qt_assert (assertion=0x75f8b7f0 size == 0 || 
 offset  0 || size_t(offset) = sizeof(QArrayData), file=0x75f8b7b0 
 ../../include/QtCore/../../src/corelib/tools/qarraydata.h, line=62)
 at global/qglobal.cpp:2088
 #5  0x75c29bf8 in QArrayData::data (this=0x6cb830) at 
 ../../include/QtCore/../../src/corelib/tools/qarraydata.h:61
 #6  0x75c3015e in QTypedArrayDatachar::data (this=0x6cb830) at 
 ../../include/QtCore/../../src/corelib/tools/qarraydata.h:207
 #7  0x75c2fde7 in QByteArray::constData (this=0x6f4760) at 
 ../../include/QtCore/../../src/corelib/tools/qbytearray.h:433
 #8  0x75c5 in qstrcmp (str1=..., str2=...) at 
 tools/qbytearray.cpp:347
 #9  0x77ba4625 in operator (a1=..., a2=...) at 
 /opt/qt5/include/QtCore/qbytearray.h:550
 #10 0x77ba0eb9 in KStartupInfoId::operator (this=0x693858, id_P=...) 
 at 
 /home/martin/src/kf5/kdelibs-frameworks/tier1/kwindowsystem/src/kstartupinfo.cpp:1175
 #11 0x77ba67b5 in qMapLessThanKeyKStartupInfoId (key1=..., 
 key2=...) at /opt/qt5/include/QtCore/qmap.h:75
 #12

Re: Review Request 112717: Start adopting QCollator

2013-09-17 Thread Frank Reininghaus



 On Sept. 15, 2013, 10:29 a.m., Frank Reininghaus wrote:
  Thanks for your cool work on QCollator! It will be interesting to see how 
  much we can gain by using QCollatorSortKey for sorting large sets of 
  QStrings :-)
  
  I'm not really familiar with most of the affected code, and I couldn't test 
  it yet (frameworks currently does not build for me, but it's most likely an 
  issue with my system which can fixed by doing a clean build), but here are 
  some things that I noticed:
  
  (a) Is there a reason why you use a helper class to wrap the QCollator in 
  kfile/kurlnavigatorbutton.cpp, but pass the QCollator directly to the sort 
  function in other places?
  
  (b) I'm wondering how cheap it is to initialize a QCollator. Could existing 
  code outside of kdelibs that uses KStringHandler::naturalCompare() for 
  sorting become slow if that happens N*log(N) times?
  
  (c) Something that was not clear to me at all when I first heard about 
  QCollator is that its behavior will depend on whether ICU headers were 
  installed when Qt was built or not, and that things like 
  setNumericMode(true) will simply be ignored (with a warning printed to the 
  terminal) if ICU was not available then. Even worse: 
  QCollator::numericMode() returns true in that case, but it does not use 
  numeric mode for sorting at all!
  
  I just found out about that when I tried to write a simple test program 
  that sorts strings using QCollator. It did not work, and after some 
  research I found out that this is because I only have the ICU libs, but not 
  the headers installed on my system.
  
  Now the Qt 5 packages that end up on our users' systems will probably be 
  compiled with ICU support, but still, I have a very uneasy feeling about 
  using a class that may or may not do what you expect, and that provides no 
  good way to find out if it will do what you expect (as I said, 
  QCollator::numericMode() from qcollator_posix.cpp always returns true). I'm 
  already seeing the bug reports coming from people who built Qt from source 
  and forgot (like me) to install the ICU headers before.
  
  I don't see a good solution for that problem. Even if we made the ICU 
  headers a hard dependency for frameworks, it could still be that Qt was 
  built without ICU support.
  
  Probably the best solution would be to try to get something like our 
  KStringHandler::naturalCompare() function into qcollator_posix.cpp, to make 
  sure that the fallback that is used if ICU isn't available actually uses 
  numeric mode if it's told to?
 
 
 Aleix Pol Gonzalez wrote:
 a) Well, I tried to minimize the number initializations, but I also tried 
 to reduce the code changes.
 b) I don't have such data. It's a possibility, that it's slower. Either 
 way, the less we do, the faster it will work.
 c) When you configure Qt, if ICU is found it will be used. I think it's 
 ok to assume that Dolphin on linux users will all have ICU available.
 
 I wouldn't hack around the posix backends, personally.
 
 Frank Reininghaus wrote:
 I think it's ok to assume that Dolphin on linux users will all have ICU 
 available.
 
 If you build Qt from source, you have to install ICU headers manually in 
 order to have ICU available (at least if the ICU-devel package is not 
 installed by the distro by default). This means that it's very easy to end up 
 with a QCollator that does not support numeric mode. Considering that many 
 people who contribute to KDE do build Qt from source, it will most likely go 
 wrong for some of them, so I tend to strongly disagree with the it's ok to 
 assume... statement. These people will notice that things don't work as 
 expected and either waste time analyzing the problem or file a bug report.
 
 I experienced this myself yesterday: I noticed that QCollator did not 
 work for me, and I was surprised about that because, according to the API 
 docs, setNumericMode(true) causes the sorting to be natural, and it does 
 not mention any conditions that have to be fulfilled. At least I saw the 
 warning message in Konsole, but if a user/developer doesn't even see that 
 (e.g., because it gets lost in .xsession-errors), how is he/she supposed to 
 know what the cause of the problem is?
 
 This is all my personal opinion, and I don't actually maintain any of the 
 affected code, but I tend to think that using a class that may or may not do 
 what it actually pretends to do, depending on things that are out of our 
 control, might not be a good idea.
 
 John Layt wrote:
 The plan for Qt is in 5.3 to have ICU is a hard-ish dependency on Linux.  
 QLocale will use ICU for all localization on Linux, and only provide a simple 
 POSIX fallback for embedded platforms that don't want ICU.  Distro's will be 
 told that they should always build with ICU support enabled.  (We were going 
 to make ICU a hard dependency on all platforms in 5.2, but Windows devs 
 objected too much

Re: Review Request 112717: Start adopting QCollator

2013-09-16 Thread Frank Reininghaus



 On Sept. 15, 2013, 10:29 a.m., Frank Reininghaus wrote:
  Thanks for your cool work on QCollator! It will be interesting to see how 
  much we can gain by using QCollatorSortKey for sorting large sets of 
  QStrings :-)
  
  I'm not really familiar with most of the affected code, and I couldn't test 
  it yet (frameworks currently does not build for me, but it's most likely an 
  issue with my system which can fixed by doing a clean build), but here are 
  some things that I noticed:
  
  (a) Is there a reason why you use a helper class to wrap the QCollator in 
  kfile/kurlnavigatorbutton.cpp, but pass the QCollator directly to the sort 
  function in other places?
  
  (b) I'm wondering how cheap it is to initialize a QCollator. Could existing 
  code outside of kdelibs that uses KStringHandler::naturalCompare() for 
  sorting become slow if that happens N*log(N) times?
  
  (c) Something that was not clear to me at all when I first heard about 
  QCollator is that its behavior will depend on whether ICU headers were 
  installed when Qt was built or not, and that things like 
  setNumericMode(true) will simply be ignored (with a warning printed to the 
  terminal) if ICU was not available then. Even worse: 
  QCollator::numericMode() returns true in that case, but it does not use 
  numeric mode for sorting at all!
  
  I just found out about that when I tried to write a simple test program 
  that sorts strings using QCollator. It did not work, and after some 
  research I found out that this is because I only have the ICU libs, but not 
  the headers installed on my system.
  
  Now the Qt 5 packages that end up on our users' systems will probably be 
  compiled with ICU support, but still, I have a very uneasy feeling about 
  using a class that may or may not do what you expect, and that provides no 
  good way to find out if it will do what you expect (as I said, 
  QCollator::numericMode() from qcollator_posix.cpp always returns true). I'm 
  already seeing the bug reports coming from people who built Qt from source 
  and forgot (like me) to install the ICU headers before.
  
  I don't see a good solution for that problem. Even if we made the ICU 
  headers a hard dependency for frameworks, it could still be that Qt was 
  built without ICU support.
  
  Probably the best solution would be to try to get something like our 
  KStringHandler::naturalCompare() function into qcollator_posix.cpp, to make 
  sure that the fallback that is used if ICU isn't available actually uses 
  numeric mode if it's told to?
 
 
 Aleix Pol Gonzalez wrote:
 a) Well, I tried to minimize the number initializations, but I also tried 
 to reduce the code changes.
 b) I don't have such data. It's a possibility, that it's slower. Either 
 way, the less we do, the faster it will work.
 c) When you configure Qt, if ICU is found it will be used. I think it's 
 ok to assume that Dolphin on linux users will all have ICU available.
 
 I wouldn't hack around the posix backends, personally.

I think it's ok to assume that Dolphin on linux users will all have ICU 
available.

If you build Qt from source, you have to install ICU headers manually in order 
to have ICU available (at least if the ICU-devel package is not installed by 
the distro by default). This means that it's very easy to end up with a 
QCollator that does not support numeric mode. Considering that many people 
who contribute to KDE do build Qt from source, it will most likely go wrong for 
some of them, so I tend to strongly disagree with the it's ok to assume... 
statement. These people will notice that things don't work as expected and 
either waste time analyzing the problem or file a bug report.

I experienced this myself yesterday: I noticed that QCollator did not work for 
me, and I was surprised about that because, according to the API docs, 
setNumericMode(true) causes the sorting to be natural, and it does not 
mention any conditions that have to be fulfilled. At least I saw the warning 
message in Konsole, but if a user/developer doesn't even see that (e.g., 
because it gets lost in .xsession-errors), how is he/she supposed to know what 
the cause of the problem is?

This is all my personal opinion, and I don't actually maintain any of the 
affected code, but I tend to think that using a class that may or may not do 
what it actually pretends to do, depending on things that are out of our 
control, might not be a good idea.


- Frank


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112717/#review40054
---


On Sept. 13, 2013, 5:55 p.m., Aleix Pol Gonzalez wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/112717

Re: Review Request 112717: Start adopting QCollator

2013-09-15 Thread Frank Reininghaus


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112717/#review40054
---


Thanks for your cool work on QCollator! It will be interesting to see how much 
we can gain by using QCollatorSortKey for sorting large sets of QStrings :-)

I'm not really familiar with most of the affected code, and I couldn't test it 
yet (frameworks currently does not build for me, but it's most likely an issue 
with my system which can fixed by doing a clean build), but here are some 
things that I noticed:

(a) Is there a reason why you use a helper class to wrap the QCollator in 
kfile/kurlnavigatorbutton.cpp, but pass the QCollator directly to the sort 
function in other places?

(b) I'm wondering how cheap it is to initialize a QCollator. Could existing 
code outside of kdelibs that uses KStringHandler::naturalCompare() for sorting 
become slow if that happens N*log(N) times?

(c) Something that was not clear to me at all when I first heard about 
QCollator is that its behavior will depend on whether ICU headers were 
installed when Qt was built or not, and that things like setNumericMode(true) 
will simply be ignored (with a warning printed to the terminal) if ICU was not 
available then. Even worse: QCollator::numericMode() returns true in that case, 
but it does not use numeric mode for sorting at all!

I just found out about that when I tried to write a simple test program that 
sorts strings using QCollator. It did not work, and after some research I found 
out that this is because I only have the ICU libs, but not the headers 
installed on my system.

Now the Qt 5 packages that end up on our users' systems will probably be 
compiled with ICU support, but still, I have a very uneasy feeling about using 
a class that may or may not do what you expect, and that provides no good way 
to find out if it will do what you expect (as I said, QCollator::numericMode() 
from qcollator_posix.cpp always returns true). I'm already seeing the bug 
reports coming from people who built Qt from source and forgot (like me) to 
install the ICU headers before.

I don't see a good solution for that problem. Even if we made the ICU headers a 
hard dependency for frameworks, it could still be that Qt was built without ICU 
support.

Probably the best solution would be to try to get something like our 
KStringHandler::naturalCompare() function into qcollator_posix.cpp, to make 
sure that the fallback that is used if ICU isn't available actually uses 
numeric mode if it's told to?


- Frank Reininghaus


On Sept. 13, 2013, 5:55 p.m., Aleix Pol Gonzalez wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/112717/
 ---
 
 (Updated Sept. 13, 2013, 5:55 p.m.)
 
 
 Review request for KDE Frameworks, Frank Reininghaus and Aurélien Gâteau.
 
 
 Description
 ---
 
 Now that QCollator is public API, I wanted to give it a try, so I decided to 
 port all uses KStringHandler::naturalCompare() to QCollator.
 
 All the porting was quite straightforward I'd say, the only weird part is 
 that I removed some tests of the KStringHandler tests. There are 2 kind of 
 tests disabled:
 - The ones that say that A == a in case of Qt::CaseInsensitive. ICU is 
 deterministic and it will decide itself which one goes in, so the test 
 doesn't make sense anymore.
 - There's a mention to the 237788 bug where in some cases our former 
 algorithm wouldn't be deterministic. This doesn't apply anymore, but also now 
 ICU takes care about it now, so there's little point of keeping unit testing 
 it.
 I decided to leave the unit test because it might be useful eventually 
 (although note that it was not being compiled so far). In any case we 
 probably want it out.
 
 In any case, the rest seems straightforward enough. I didn't concentrate on 
 performance though, in some cases we'll want to use the QCollatorSortKey.
 
 
 Diffs
 -
 
   KDE5PORTING.html 1287de7 
   kfile/kdirsortfilterproxymodel.cpp 7c7aa5f 
   kfile/kurlnavigatorbutton.cpp d2c27fd 
   staging/itemviews/src/CMakeLists.txt 353a413 
   staging/itemviews/src/kcategorizedsortfilterproxymodel.h a21e7ca 
   staging/itemviews/src/kcategorizedsortfilterproxymodel.cpp c8b652d 
   staging/itemviews/src/kcategorizedsortfilterproxymodel_p.h eb1a67b 
   staging/kcompletion/src/kcompletion.cpp 5f60a6c 
   staging/xmlgui/src/kshortcutsdialog_p.h ab102bc 
   staging/xmlgui/src/kshortcutseditoritem.cpp e89a8aa 
   tier1/kcoreaddons/autotests/CMakeLists.txt 19227dc 
   tier1/kcoreaddons/autotests/kstringhandlertest.cpp d12f086 
   tier1/kcoreaddons/src/lib/text/kstringhandler.h 1b08f6f 
   tier1/kcoreaddons/src/lib/text/kstringhandler.cpp 2f192aa 
 
 Diff: http://git.reviewboard.kde.org/r/112717/diff

Re: KFileItem (Re: Jenkins build became unstable: kdelibs_frameworks_qt5 #982)

2013-08-23 Thread Frank Reininghaus

Hi,

2013/8/22 David Faure:
 On Thursday 08 August 2013 13:17:18 Frank Reininghaus wrote:
 Hi David,

 2013/8/7 David Faure:
  On Tuesday 06 August 2013 20:53:05 Frank Reininghaus wrote:
  OK, I see now that it uses pointers to be able to modify the actual
  KFileItems in KDirListerCache (if it would just keep KFileItems and
  modify these, I guess that they would just detach from the ones inside
  the cache, leaving those unchanged).
 
  Yes.
 
 
  I suppose a solution would be to use a QLinkedList in KDirListerCache.
  This
  would basically cancel the Q_MOVABLE_TYPE for that particular list (since
  it would need to allocate nodes), but every other KFileItemList out there
  would still benefit.

 But it would also slow down things when KDirListerCache has to convert
 its internal data to KFileItemLists when emitting signals. Hm, I'll
 try to find some time to think about it and see if there is a good
 solution. I was going to have a close look at KDirListerCache anyway
 because I have the impression that the remaining sources of O(N^2)
 behavior in Dolphin when adding/removing many items to a directory in
 weird ways are in this class.

 Yeah, conversions would not be great. But I don't see where you think
 conversions would happen. The KFileItemList emitted by itemsAdded is created
 item by item anyway, in addNewItem(). It's not like we can take the list that
 is used as storage in KDirLister and emit that, since we only want to emit the
 *new* items, not all of them (everything is async and incremental).

OK, you're probably right. I had thought that copying the items from
one KFileItemList to another KFileItemList is cheaper than iterating
through a linked list, but that's probably wrong if the KFileItemList
only keeps pointers to the actual items. I'll try to find some time to
check that at some point.

  It looks like a solution for this problem is more complicated than I
  thought, so maybe I'll just revert the commit to make Jenkins happy
  again. However, I still think that making KFileItem a Q_MOVABLE_TYPE
  might be a desirable long-term goal because it saves quite a bit of
  memory (32 bytes per KFileItem on my system).
 
  32 *bytes* ? Are you sure? I think you meant 32 bits?

 Yes, I am sure, see below.

  In fact I'm surprised. sizeof(KFileItem) = sizeof(void*), right? So QList
  should already lay out the kfileitems next to each other in memory, the
  only issue is that insertion makes copies, but I don't see where a memory
  saving happens. I thought QList only wasted memory for types bigger than
  a pointer (in which case it had to allocate nodes) ?

 AFAIK, QList only puts items of type T next to each other in memory if
 sizeof(T) = sizeof(void*) *and* T is a Q_MOVABLE_TYPE.

 Seems right.

 If that is not
 the case, QListT only stores T* pointers next to each other in one
 block of memory.

 Yes, which means taking the size of one pointer more, i.e. 8 bytes per item.
 Not 32.

But the memory allocator needs a considerable amount of memory for
internal use, see below.

 I guess the reason for this design decision is that
 this permits QListT to share the same core code (which uses memcpy
 to move around items in memory), no matter what T is.

 It's mainly an optimization: as you saw, when items are small enough,
 replacing pointers with the item itself is a saving, not only in memory usage,
 but also in speed since the data is much closer together (=less memory pages
 to load). But if the data is bigger than a pointer, then it doesn't fit in
 what was originally sized to contain a pointer.

 For putting large items in contiguous memory, there's QVector.

 Also my experimental findings in
 https://git.reviewboard.kde.org/r/111789/ support that. According to
 massif/massif-visualizer, a simple program that creates a
 KFileItemList with 1 million Q_MOVABLE null KFileItems needs about 8
 MB (is to be expected because that's what you need for 1 million
 pointers on my 64-bit system). However, without Q_MOVABLE_TYPE, i.e.,
 with the current master or frameworks branch, it needs 16 MB because
 the list only stores KFileItem* pointers in the 8 MB block, and the
 memory for the items themselves is allocated with 1 million calls of
 new KFileItem - 8 MB more.

 However, this is only the net memory usage. In reality, the memory
 allocator also needs some memory for its own bookkeeping, and it thus
 adds a bit of overhead to any memory allocation with new or malloc.
 With KSysGuard, I found that the difference in memory consumption for
 my test program with/without Q_MOVABLE_TYPE is a bit more than 30.5
 MiB, and if you take into account that 1 MiB = 1024*1024 bytes, and my
 test used 1 million = 10^6 KFileItems, this looks pretty much like
 every new KFileItem occupies 32 bytes. And this is a bit too much
 for my taste ;-)

 It's not that I don't trust you or these tools, but I would like to see an
 explanation of the 32 bytes from the code rather than from high-level
 measurements. I can

Re: Review Request 111897: Move KFileMetaData (and friends) to kde4support

2013-08-06 Thread Frank Reininghaus



 On Aug. 6, 2013, 9 a.m., Vishesh Handa wrote:
  I was just working on the same thing.
  
  I'm not sure if we want to move this to kde4support. Can we just throw it 
  away? Or would that be terribly wrong? We have a replacement in 
  nepomuk-widgets.
  
  Strigi doesn't need to be ported to Qt5 since it is does not use Qt. 
  Soprano will have to be, but I don't think this code uses Soprano.
 
 Aleix Pol Gonzalez wrote:
 You were working on it? -.- it didn't have your name on it...
 
 I think that the classes called plugin should be removed, there's not 
 much else to remove otherwise.
 
 Vishesh Handa wrote:
 I'd just started today morning, then I decided to try and compile 
 everything. It has been 5 hours since then. I'm still compiling.
 
 There is just one user visible class - KFileMetadataWidget. The rest of 
 the classes are helper code. A large part of the helper code uses Nepomuk1. 
 If we move this to kde4support, then those Nepomuk1 dependencies have to be 
 removed. Removing them would make this into a wrapper over Strigi. The 
 question is - do we want that? Or do we just want to discard this class 
 completely?
 
 Based on [1] there seem to be 3 clients. Dolphin which uses it when 
 Nepomuk compilation is disabled. Conquirere, which is a Nepomuk based app and 
 should just use the one in nepomuk-widgets, and Konversation - I'm not sure 
 what to do about them. If we throw away this class then we will just be 
 breaking Konversation.
 
 I'm obviously in favor of discarding the class. Opinions?
 
 This also raises the larger question if we want classes in kde4support to 
 depend on unmaintained code? (Strigi)
 
 [1] http://lxr.kde.org/ident?i=KFileMetaDataWidget

 Dolphin which uses it when Nepomuk compilation is disabled.

Yes. However, I think we might want to drop the option to compile Dolphin 
without Nepomuk 2 when porting to Frameworks. Maintaining all the HAVE_NEPOMUK 
#ifdefs is not much fun, in particular not if the only benefit for the users 
who compile from source is that they can make Dolphin use unmaintained code.

 I'm obviously in favor of discarding the class. Opinions?

I think that this is a good idea. Maybe one could make it a typedef for (or a 
very thin wrapper around) Nepomuk2::MetaDataWidget? Then removing all this 
unmaintained code would even be a source compatible change. Maybe this is not 
possible right now because kdelibs cannot depend on nepomuk-widgets, but in the 
long term, it makes sense IMHO to have kde4support depend on all KDE libs that 
are required to make the porting as easy as possible.


- Frank


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/111897/#review37193
---


On Aug. 5, 2013, 6:06 p.m., Aleix Pol Gonzalez wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/111897/
 ---
 
 (Updated Aug. 5, 2013, 6:06 p.m.)
 
 
 Review request for KDE Frameworks and Vishesh Handa.
 
 
 Description
 ---
 
 As far as I've understood, we should have an alternative by Nepomuk for file 
 previewing for KF5, this patch moves the KFileMetaInfo and the files that 
 depend on it to KDE4Support.
 
 It's worth noting that there are 2 plugins (they're actually not plugins) 
 of the KPropertiesDialog that have been disabled because they had to be moved 
 with KFileMetaInfo. That is the kmetaprops.cpp and kpreviewprops.cpp
 
 
 Diffs
 -
 
   kio/CMakeLists.txt 035cf70 
   kio/kfile/kfilemetadataconfigurationwidget.h 6be2a0d 
   kio/kfile/kfilemetadataconfigurationwidget.cpp  
   kio/kfile/kfilemetadataprovider.cpp  
   kio/kfile/kfilemetadataprovider_p.h 8009bf4 
   kio/kfile/kfilemetadatareader.cpp  
   kio/kfile/kfilemetadatareader_p.h  
   kio/kfile/kfilemetadatareaderprocess.cpp  
   kio/kfile/kfilemetadatawidget.h 2dc4677 
   kio/kfile/kfilemetadatawidget.cpp  
   kio/kfile/kmetaprops.h a08c380 
   kio/kfile/kmetaprops.cpp  
   kio/kfile/knfotranslator.cpp  
   kio/kfile/knfotranslator_p.h  
   kio/kfile/kpreviewprops.h 8a974da 
   kio/kfile/kpreviewprops.cpp  
   kio/kfile/kpropertiesdialog.cpp 687e4bf 
   staging/kde4support/src/CMakeLists.txt 1d6369f 
   staging/kde4support/src/config-kde4support.h.cmake 03d3bf4 
 
 Diff: http://git.reviewboard.kde.org/r/111897/diff/
 
 
 Testing
 ---
 
 builds... actually i'm not sure if there's Qt5 soprano/strigi. what's hte 
 status?
 
 
 Thanks,
 
 Aleix Pol Gonzalez
 


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

Re: Revising changing configurations with KConfig

2013-08-01 Thread Frank Reininghaus

Hi,

2013/8/1 Kevin Ottens:
 On Thursday 01 August 2013 00:57:05 Aleix Pol wrote:
 Well, that setting is used in KDirSortFilterProxyModel as well... Should we
 just always sort naturally by default there then?

 Would make sense to add an accessor pair to KDirSortFilterProxyModel, and have
 the default to be true for that property. Then it's up to the client code to
 disable it if it wants to.

 Yes it makes some formerly automatic behavior manual, but at the same time
 both that property and KDirSortFilterProxyModel aren't used that much.

Well, KDirSortFilterProxyModel is being used indirectly by every
application which has a File Open/Save dialog, and by apps like Kate
which use a KDirOperator to display the contents of a directory
somewhere else.

I would not mind if every application had its own setting for natural
sorting, but I'm not sure if the users who prefer non-natural
sorting (yes, they do exist) would like it if they have to change the
setting in every application. However, I admit that the current KDE 4
solution (change the setting in Dolphin, and then every application
will use it) is not perfect either.

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

Review Request 111789: Improve KFileItemList memory usage and performance

2013-07-29 Thread Frank Reininghaus


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

Review request for KDE Frameworks and David Faure.


Description
---

The other day, I noticed that KFileItem is not declared as a Q_MOVABLE_TYPE. 
Therefore, QList does not store KFileItems directly, but only pointers to them, 
and allocates memory for every single KFileItem separately. This wastes quite a 
bit of memory.

It looks like now might be a good moment to fix this because we can break 
binary compatibility with KDE 4.x.


Diffs
-

  staging/kio/src/core/kfileitem.h 2c33f3c 

Diff: http://git.reviewboard.kde.org/r/111789/diff/


Testing
---

My poor man's aproach to test the memory usage and performace is here: 
http://paste.kde.org/p46abc91f/ (the reason for the 10 second delay is that I 
needed some time to take a KSysGuard screen shot).

It creates a KFileItemList with 1 million empty KFileItems. The memory usage 
change is shown in the pictures. I used both massif/massif-visualizer and 
KSysGuard to measure how much memory it uses. The KSysGuard measurement shows a 
far bigger difference - this is because massif only measures the net memory 
consumption and fails to consider the overhead which is caused by the memory 
allocator itself. The latter is quite considerable when many small memory 
allocations are made.

Moreover, I also measured the runtime of the test (without massif).

5 measurements without patch required between 171 ms and 189 ms.

5 measurements with patch required between 98 ms and 106 ms.


File Attachments


Memory usage WITHOUT patch
  http://git.reviewboard.kde.org/media/uploaded/files/2013/07/29/before.png
Memory usage WITH patch
  http://git.reviewboard.kde.org/media/uploaded/files/2013/07/29/after.png


Thanks,

Frank Reininghaus

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

Re: naturalCompare Qt5 task

2013-07-19 Thread Frank Reininghaus

Hi,

2013/7/19 Aleix Pol:
 Hi,
 I was looking at that task in the Qt5 epics list and I didn't understand it
 fully.

 contribute natural-comparison to Qt5 (see KStringHandler). In Qt there is
 naturalCompare function but private and not as good as from KStringHandler.
 Thiago says: add the feature to QCollator.

Could you explain why KStringHandler::naturalCompare() is better than
what QCollator offers?

QCollator has the very nice feature

QByteArray QCollator::sortKey(const QString string)

which allows to calculate the 'sortKey' for each string once, and then
use these for the sorting. This means that we could sort a large list
of strings with O(N*log(N)) cheap comparisons of QByteArrays, rather
than the same amount of very expensive
KStringHandler::naturalCompare() calls.

At the moment, KStringHandler::naturalCompare() is the major
bottleneck in Dolphin when you open a very large directory. If we
could use QCollator, that problem could be solved quite easily.

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

Re: KF5 Update Meeting Minutes 2013-w24

2013-06-12 Thread Frank Reininghaus

Hi,

thanks for the explanation!

2013/6/12 Kevin Ottens:
 Hello,

 On Tuesday 11 June 2013 21:08:45 Frank Reininghaus wrote:
 2013/6/11 Kevin Ottens:
 [...]

   * ItemViews will be tier 2 until we contribute the natural compare to Qt;

 I thought that Qt 5.1 will have the QCollator class, which can perform
 natural comparison of strings if you use numeric mode?

 Indeed, it's still private API though. Also the conclusion was our
 implementation is better, but looking at QCollator I'm not sure I agree with
 that.

The disadvantage of KStringHandler::
naturalCompare(QString, QString)
is that it's very slow. When you open a directory with really many
files in Dolphin, this function is the main bottleneck.

The big advantage of QCollator is that it has a function

QByteArray QCollator::sortKey(const QString string)

which lets you do the expensive stuff just N times for N items, and
then do O(N*log N) cheap comparisons of QByteArrays when you sort
them.

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

Re: KF5 Update Meeting Minutes 2013-w24

2013-06-11 Thread Frank Reininghaus

Hi,

2013/6/11 Kevin Ottens:
[...]
  * ItemViews will be tier 2 until we contribute the natural compare to Qt;

I thought that Qt 5.1 will have the QCollator class, which can perform
natural comparison of strings if you use numeric mode?

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

Re: please make it easier to hack on frameworks

2013-04-30 Thread Frank Reininghaus

Hi,

2013/4/30 Aaron J. Seigo:
 On Tuesday, April 30, 2013 11:20:47 Stephen Kelly wrote:

 I am clueless to understand why building cmake from git and installing it

 into your kf5 prefix is a showstopper. Can you tell me?

 Time is limited.

 Every repository that I have to build (e.g. cmake) that is not the
 repository I am trying to work on (e.g. plasma-frameworks) is time lost
 getting my tasks done.

 Moreover, it is one more thing to learn: where is cmake's git, how to build
 it, etc. For me that is not a big issue (I've build cmake from source many
 times in the past) but for people who might want to work on frameworks this
 kind of thing becomes a show stopper. Eventually they run into so many
 things they have to locate, build and keep up to date that they have no time
 / energy / desire to keep on.

 If we want to ensure that it is as difficult as possible to contribute to
 frameworks, congratulations, we're doing a great job of that. If frameworks
 is meant to be a project for you, Kevin, David and Alexander to work on then
 by all means don't worry about this. If the idea is, however, to make it
 attractive to others to work on, then some things need to change.

 The idea that it is ok to rely on unreleased software as part of the
 toolchain is one of those things.

but then using a released CMake rather than a recent git snapshot
would not be enough to make building more convenient. We would have to
ensure that the required CMake version is shipped by all major
distros, because I think that downloading released CMake tarballs
every now and then and building them is probably even more
inconvienient than cloning the git repository just once and keeping it
up to date thereafter.

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

Re: Review Request 109538: port KFileMetaDataReader to QProcess

2013-03-18 Thread Frank Reininghaus



 On March 17, 2013, 2:05 p.m., Vishesh Handa wrote:
  But why? KFileMetadataReader and the other KFileMetadataStuff should just 
  be marked as deprecated. Why are we porting them? We already have better 
  alternatives in the nepomuk-widgets repository.
 
 Martin Tobias Holmedahl Sandsmark wrote:
 Because it was a simple user of KProcess.
 
 But if we can just deprecate the whole class (and move it into 
 kde4support, I guess?) that's better. :-)
 
 Vishesh Handa wrote:
 As far as I'm concerned it can be deprecated. We can definitely deprecate 
 KFileMetadataWidget which is the only user of this KFileMetadataReader. 
 Dolphin now uses Nepomuk2::FileMetadataWidget. The only slight problem might 
 be that Dolphin still likes being Nepomuk Optional at compile time. So they 
 still use it. Maybe we should talk to Frank about this?
 
 The only other class is KFileMetaInfo, which uses Strigi directly. I 
 still have to write a replacement for that in Nepomuk.
 
 @David: I think we talked about this in Berlin. Should we deprecate 
 KFileMetadataWidget?
 
 Kevin Ottens wrote:
 Yep, that's what we discussed in Berlin, this one could move to 
 kde4support indeed.

I think we'll keep the Nepomuk optional for building Dolphin thing in KDE 
4.x. Accoding to feedback I got from users, some people like to build Dolphin 
with as few dependencies as possible. But for Frameworks, I don't really see a 
point in it. We'll depend on many different frameworks anyway, so replacing the 
parts of kdelibs that are needed for KFileMetaDataWidget by nepomuk-core and 
nepomuk-widgets has hard, non-optional dependencies looks reasonable to me.

But then I wonder why we should even bother to keep KFileMetaDataWidget at all? 
Shipping code that is unmaintained, even if it is just in kde4support, does 
not look like a good idea to me. Couldn't it just be removed? Porting to 
Nepomuk's widget is simple enough, and if people feel that this makes porting 
apps to Frameworks too hard, one could still put a simple header file into 
kde4support that makes KFileMetaDataWidget a typedef for Nepomuk's widget.


- Frank


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/109538/#review29377
---


On March 17, 2013, 1:26 p.m., Martin Tobias Holmedahl Sandsmark wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/109538/
 ---
 
 (Updated March 17, 2013, 1:26 p.m.)
 
 
 Review request for KDE Frameworks, kdelibs, David Faure, and Vishesh Handa.
 
 
 Description
 ---
 
 KFileMetaDataReader currently uses KProcess, this ports it to use QProcess 
 instead.
 
 
 Diffs
 -
 
   kio/kfile/kfilemetadatareader.cpp 88cadaa 
 
 Diff: http://git.reviewboard.kde.org/r/109538/diff/
 
 
 Testing
 ---
 
 it builds.
 
 
 Thanks,
 
 Martin Tobias Holmedahl Sandsmark
 


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

Re: Separating everything ?

2013-02-07 Thread Frank Reininghaus

Hi,

2013/2/7 Kevin Ottens:
 On Thursday 7 February 2013 16:09:51 Frank Reininghaus wrote:
 Is there anything obvious that I'm overlooking? One could argue that
 separate repositories make it easier for non-KDE people to contribute
 to one particular framework.

 It's not only contributing but also using. It's almost more important IMO
 (especially since more users mean more contributors).

But I would guess that most users would just use the packages provided
by their distro (and even though I've never done any tarball-releasing
and packaging myself, I doubt that creating separate packages out of
one repository with a clear directory structure is much  harder than
packaging separate repositories).

If, however, a user decides to clone the repository and build from
source or, even better, start contributing, I seriously doubt that
cloning one frameworks repository and then cd'ing to the interesting
framework would be perceived as an obstacle. Quite the contrary, I
think that in the not-so-unlikely event that someone uses two or more
frameworks, having to clone just one repository might make even the
user's life easier.

 But if each framework inside the kdelibs repository can easily be built
 separately, this point looks moot to me, and it cannot justify making the
 build and debug process more painful for the (probably rather common) case
 that a person wants to build and use all of KDE frameworks.

 Well, as you pointed out the build part is not more painful with proper tools
 (and kdesrc-build is getting quite good there). As for the debugging... well
 you have already quite some repositories today, I don't think it's making it
 worse

I do think that it is making it worse. Right now, kdelibs still
contains most of the code that is required to build any other KDE
package.

 (I doubt we want everything in a single repository just to be able to
 git bisect anyway :-)).

Right, we would not want the git bisect issue to stop the repository
splitting if there was a good reason to do it, but I haven't seen any
convincing argument yet why multiple repositories are better.

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

Re: Separating everything ?

2013-02-07 Thread Frank Reininghaus

Hi Patrick,

2013/2/8 Patrick Spendrin:
 Am 07.02.2013 23:32, schrieb Frank Reininghaus:
 ...

 Since I am reading this thread by chance, I might as well reply.

 One of the reasons of splitting kdelibs into separate repositories is to
 simplify the usage of single modules.
 From the perspective of a full *KDE* desktop, there is no problem in
 building  using all of kdelibs, since each library will be used several
 times from several applications.

 If you do not have a full *KDE* desktop (running a single KDE
 application on a gnome desktop or maybe the wish to use KDE technology
 in your Qt application), this will not be the case, and you will
 generate overhead. Of course the overhead can be cut down by (1)
 splitting kdelibs either at buildtime (by switching libraries on or off
 at cmake time) or (2)after building it (currently done by some distros
 to some extend). For KDE on Windows e.g. (1) will bring the overhead of
 having a complete kdelibs package for rather tiny libraries and (2) is
 simply forbidden by missing manpower.

thanks, but I'm afraid you misunderstood me. I never said that one
should be forced to build the entire thing (or circumvent that by
switching off libraries).

My idea was to have all frameworks in subfolders of the kdelibs or
kde-frameworks repository which can be built, used, installed and
released separately. The only change for people who want the latest
code from git would be that they clone not multiple repositories, but
just one, and then cd to the framework(s) they need, build and install
them. The only overhead would be the unused source code of the other
frameworks on the hard drive.

I see that there is a fear that a big repository will keep people from
contributing to a single framework. I can't quite imagine that this is
the case, but considering that I'm apparently the only one, I guess we
should better stop wasting time with this discussion and get back to
work.

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

Re: KDirModelV2, KDirListerV2 and UDSEntryV2 suggestions

2013-02-04 Thread Frank Reininghaus

Hi Mark,

2013/2/4 Mark:
 I really really really dislike KDirModel and friends (KDirLister,
 KFileItem).

before you get even more emotional in your next reply to this thread,
please consider reading

http://www.kde.org/code-of-conduct/

I sort of got used to reading disrespectful messages with little
useful content from a small minority of our user base, but I feel at
least mildly offended when I read such statements on a developer-only
mailing list. Not only because the classes you mention have been
serving us well for quite some time and David does an awesome job
maintaining them (he fixes a bug in no time at all as soon as I throw
a half-ready unit test at him), but also because they contain some
contributions from myself. Yes, it's mostly unit tests and one-line
patches, but in some cases, I needed a few hours of backtrace and code
reading before I could figure out what's going wrong [1].

Thanks for your understanding and best regards,
Frank

[1] https://bugs.kde.org/show_bug.cgi?id=196695
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: KDirModelV2, KDirListerV2 and UDSEntryV2 suggestions

2013-01-21 Thread Frank Reininghaus

Hi,

2013/1/21 Aaron J. Seigo:
 On Tuesday, January 15, 2013 10:40:58 Frank Reininghaus wrote:
 Right, but considering that Dolphin doesn't even use KDirModel any
 more, going the QAbstractItemView way would feel like throwing away
 most of what happened in Dolphin in the last 2 years.

 dolphin still uses QAbstractItemModels, no? that's all that matters; whether
 it uses KDirModel or KFileItemModle or SomeOtherModel is a detail.

No, it doesn't, see

http://ppenz.blogspot.de/2011/08/introducing-dolphin-20.html

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

Re: KDirModelV2, KDirListerV2 and UDSEntryV2 suggestions

2013-01-21 Thread Frank Reininghaus

Hi,

2013/1/21 Aaron J. Seigo ase...@kde.org:
 On Monday, January 21, 2013 17:53:48 Frank Reininghaus wrote:
 http://ppenz.blogspot.de/2011/08/introducing-dolphin-20.html

 Ah, that's right .. it uses ItemViewsNG. of which the last commits seem to be
 in 2010, and QGraphicsView is not receiving much attention now either :/

It doesn't use the 2010 version of ItemViews-NG. To quote from Peter's blog:

So the new view-engine for Dolphin 2.0 is build on a (very) modified
subset of Itemviews-NG.

As long as we work on it, it will not bit rot. (The situation might be
different for QGraphics* though).

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

Re: KDirModelV2, KDirListerV2 and UDSEntryV2 suggestions

2013-01-10 Thread Frank Reininghaus

Hi,

2013/1/9 Mark:
 On Wed, Jan 9, 2013 at 2:51 PM, David Faure wrote:
 On Wednesday 09 January 2013 11:15:20 Mark wrote:
 A little more in depth questions for KDirLister and KFileItem. In my
 profiling KFileItem ends up high due to various reasons, but
 KDirLister is also a bit of a heavy resource hog due to it's default
 behavior of fetching all file information (thus at least 1 stat call
 per file) which severely slows down the dir listing process for large
 folders.

 This stat call happens in kio_file though, not in the GUI process where
 KDirLister lives, right?
 So I'm surprised that you see that when profiling...
 or is there a nasty stat() in KDirLister somewhere?

 ehm, well i'm not really monitoring stat calls. I'm monitoring the
 time it takes for a directory listing on 1 million files to be
 available in my application.

I agree that we should try to behave well even if there are many
files. However, if it is ensured if all algorithms involved in loading
item information and view setup are O(N) or at most O(N*log(N)) (like
sorting), then the remaining slowness for many files is something that
the user might just have to live with IMHO.

I'm not saying that everything is good as it is. If there are some
operations with quadratic or worse complexity involved, then this
should be looked into, of course. And I also do realise that Dolphin
could benefit from making use of your listJob batching improvements
(https://git.reviewboard.kde.org/r/107520/, very nice work BTW!).

But making extremely intrusive changes to well-tested code which are
only motivated by the (IMHO rather exotic) 1 million files use case
and which would require large changes everywhere in KDE might not be
such a good idea. Just my opnion, of course.

Best regards,
Frank

P.S.: I agree that the lazy folder icon loading issue mentioned
elsewhere in this thread can be disturbing sometimes and could be
improved.
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: KDirModelV2, KDirListerV2 and UDSEntryV2 suggestions

2013-01-10 Thread Frank Reininghaus

Hi,

2013/1/10 Aaron J. Seigo:
 in the case of dolphin, i can imagine a goal that could lead to over-
 calculating: wanting even spacing between all icons, which in turn means
 knowing how much space each and every icon will require (thumbnail? how much
 text? etc.)

The number of text lines required for each file name is indeed
calculated in advance in order to get the 'maximum scroll offset' for
the view, i.e., the total height of the view containing all items.
This is used to set up the 'scroll bar value' - 'view offset'
mapping. One could possibly avoid that by assuming that all
non-visible items only use one line for the name, and only update that
value for items which are inside (or close to) the visible area. But
before considering such a thing, one would need to check if that is
really a significant bottleneck. Quite a bit of code would need to be
changed, and I can imagine that it's not completely trivial to get
this right.

 if that is the case currently, i would suggest dropping this goal
 and only change the spacing dimension when new icons are shown that require
 it. (again, easy enough with QML.)

Well, the hard part is probably to port Dolphin's views to QML in the
first place ;-) I must admit that my QML knowledge is rather limited,
but AFAIK, it does not provide any kind of tree view out of the box.
Moreover, it seems to me that QML views work best with
QAbstractItemModel subclasses. However, one of the main goals Peter
had when rewriting the view engine was to get rid of QModelIndex,
SortFilterProxyModels and things like these. And I really have the
feeling that using plain ints to index items and not needing things
like mapToSource() in every other function really does make things
easier.

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

Re: Introduction and PhononQt5 porting

2012-10-15 Thread Frank Reininghaus

Hi Jon,

2012/10/7 Jon Severinsson:
 Hi again

 With the previosly mentioned updated phonon [1], and these three fixes
 to kdelibs [2], it all compiles for me. The unit test does however show
 several failure, but I'll have to investigate that another day, as it is
 past midnight here...

 Best reggards
 Jon «Jonno» Severinsson

 [1] 
 https://mail.kde.org/pipermail/kde-frameworks-devel/2012-October/000979.html
 [2] https://github.com/jonseverinsson/kdelibs

first of all, thanks for your help!

Not every KDE developer reads this list, so it might make sense to
contact the Phonon people on their own list about your patches (the
same applies to other KDE subprojects you have posted patches for).

Moreover, we usually use a tool called ReviewBoard [1] for patch
review. It makes commenting on particular parts of a patch much more
convenient, especially for large patches. Just choose the right
'group' and set the branch to 'frameworks' when creating a review
request.

Best regards,
Frank

[1] https://git.reviewboard.kde.org/dashboard/
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Porting KWidgets to Qt5

2012-07-10 Thread Frank Reininghaus

Hi,

2012/7/9 Иван Комиссаров:
 I answered to them. They disagree with them in that case. For me is biggest
 priority is to port as much kde as code as possible.

getting code upstream into Qt is good, of course, but you can't expect
that the Qt people will accept KDE code without modifications. If they
have good suggestions how to make the API better and more general, you
should take this into account, e.g., Jeremy's idea to just have a
'tabClicked()' signal with a 'mouse button' argument. One could even
go further and add a 'keyboard modifiers' argument. Things like this
make the API more versatile and future-proof.

Some evidence to support that: QAction's triggered() signal yields no
info about how the action was triggered [1], which makes it impossible
to react to middle-clicks or Control-clicks on, e.g., toolbar buttons,
in a special way. Therefore, KAction has another triggered() signal
that has information about mouse buttons and keyboard modifiers [2].
This wouldn't be necessary if QAction had been written in a more
general way.

Best regards,
Frank

[1] http://doc-snapshot.qt-project.org/5.0/qaction.html#triggered
[2] 
http://api.kde.org/4.x-api/kdelibs-apidocs/kdeui/html/classKAction.html#a47c884a5a7b9b2284553fd5552a8
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Porting KWidgets to Qt5

2012-07-09 Thread Frank Reininghaus

Hi,

2012/7/9 Иван Комиссаров:
 Hm, it seems that Qt guys are not very happy seeing KDE features in qt.
 I would like someone from KDE to participate in discussions about changes.
 For example, they insist that KTabBar functionality is not needed as it can
 be easily implemented in subclass:)
 https://codereview.qt-project.org/#change,29650

 If someone from KDE is really interested, they can send me gerrit account,
 so i can add them as a reviwers

IMHO, the constructive criticism that your patch set 4 got from Jeremy
Katz makes sense. To me, it looks like the problem is not that Qt
guys are not very happy seeing KDE features in qt, but that you did
not take his arguments into account.

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

Re: Monitoring autotest coverage in frameworks

2012-03-08 Thread Frank Reininghaus

Hi,

Am 1. März 2012 00:44 schrieb Dario Freddi:
 I was wondering if we already had a way to generate reports on
 autotests coverage using lcov/gcov in frameworks. Looking at our cmake
 infrastructure, I spotted a build mode Profile which should
 apparently set the correct compiler flags (but actually, at least for
 me, it didn't work), I didn't go as far as seeing if it also forces
 every library to build static. Instead, there seems to be no mention
 of a lcov target.

 Question time: is anybody already working on this? I think it's quite
 important for us to have such a thing, and I'd be happy to provide
 patches for this to happen (would be cool if Alex or any other
 buildsystem gatekeeper gave a word about how this should be done/where
 should it go).

it seems that measuring test coverage does work in kdelibs 4.x, at
least there are results submitted daily to CDash:

http://my.cdash.org/index.php?project=kdelibsdate=2012-03-01

AFAIK, the results which CTest gathers during the tests using gcov are
written to an XML file (which is then submitted to CDash if you ran
the tests using, e.g., 'make Experimental').

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

90 matches

Mail list logo