KDE CI: Frameworks kio kf5-qt5 SUSEQt5.9 - Build # 94 - Still Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks%20kio%20kf5-qt5%20SUSEQt5.9/94/
 Project:
Frameworks kio kf5-qt5 SUSEQt5.9
 Date of build:
Thu, 03 May 2018 03:39:46 +
 Build duration:
33 min and counting
   JUnit Tests
  Name: (root) Failed: 1 test(s), Passed: 57 test(s), Skipped: 0 test(s), Total: 58 test(s)Failed: TestSuite.kiofilewidgets-kfileplacesmodeltest
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report64%
(23/36)67%
(294/442)67%
(294/442)53%
(31674/59769)38%
(18608/48987)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsautotests100%
(73/73)100%
(73/73)94%
(8598/9151)48%
(5253/10861)autotests.http100%
(9/9)100%
(9/9)100%
(586/587)59%
(217/368)autotests.kcookiejar100%
(1/1)100%
(1/1)91%
(180/198)67%
(63/94)src100%
(1/1)100%
(1/1)86%
(6/7)67%
(4/6)src.core84%
(100/119)84%
(100/119)59%
(8430/14392)50%
(4916/9755)src.core.kssl100%
(1/1)100%
(1/1)40%
(35/88)50%
(3/6)src.filewidgets79%
(31/39)79%
(31/39)49%
(3884/7883)33%
(1632/4928)src.gui100%
(2/2)100%
(2/2)95%
(104/110)77%
(57/74)src.ioslaves.file100%
(5/5)100%
(5/5)52%
(522/1008)42%
(418/1004)src.ioslaves.file.kauth0%
(0/3)0%
(0/3)0%
(0/104)0%
(0/75)src.ioslaves.ftp0%
(0/2)0%
(0/2)0%
(0/1365)0%
(0/1515)src.ioslaves.help0%
(0/5)0%
(0/5)0%
(0/247)0%
(0/184)src.ioslaves.http89%
(8/9)89%
(8/9)41%
(1783/4338)35%
(1375/3979)src.ioslaves.http.kcookiejar33%
(2/6)33%
(2/6)47%
(630/1333)55%
(648/1174)src.ioslaves.remote100%
(2/2)100%
(2/2)28%
(72/258)8%
(19/242)src.ioslaves.remote.kdedmodule0%
(0/4)0%
(0/4)0%
(0/14)100%
(0/0)src.ioslaves.telnet0%
(0/1)0%
(0/1)0%
(0/43)0%
(0/30)src.ioslaves.trash64%
(7/11)64%

D12648: Update mount point after mount operations

[David Edmundson]

davidedmundson added a comment.


  > There is still no guarantee the method return is equivalent/isochronous 
with the property change.
  
  Sure there is. We know udisks has completely finished mounting a filesystem, 
therefore we know udisks property about whether the file system is mounted will 
have changed.
  It's equivalent to calling QLabel::setText and then after it returns knowing 
you can get the right result from QLabel::text
  
  That's not relying on an implementation detail, it's that anything else would 
be a definite bug.

REPOSITORY
  R245 Solid

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

To: davidedmundson, #plasma, broulik, mart
Cc: bruns, #frameworks, michaelh

D12674: Mark `Phonon4Qt5` dependency as optional in CMakeLists file

[Alexander Schlarb]

tundracomp created this revision.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.
tundracomp requested review of this revision.

REVISION SUMMARY
  Usage of Phonon is already optional in the source code; this commit updates 
the CMake file to reflect this. All tests pass with Phonon missing.

REPOSITORY
  R289 KNotifications

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

AFFECTED FILES
  CMakeLists.txt

To: tundracomp
Cc: #frameworks, michaelh, bruns

D12648: Update mount point after mount operations

[Stefan Brüns]

bruns added a comment.


  In D12648#257365 , @davidedmundson 
wrote:
  
  > Let me clarify
  >
  > At the time of the return from udisks the property on udisks is updated but 
the signal saying the property is updated is not yet sent, as solid caches 
properties we have a wrong value locally but udisks has the correct value 
remotely
  
  
  There is still no guarantee the method return is equivalent/isochronous with 
the property change. The property is changed when you receive the 
propertiesChanged signal, not when the method returns. The current behaviour is 
an implementation detail you should not rely on.

REPOSITORY
  R245 Solid

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

To: davidedmundson, #plasma, broulik, mart
Cc: bruns, #frameworks, michaelh

D12337: Give the file dialogs a "Sort by" menu button on the toolbar

[Nathaniel Graham]

ngraham added a comment.


  I see your point @rkflx, but I think it's a mistake to use an icon whose 
appearance suggests that it represents state when in fact it doesn't--that's 
the kind of thing that causes the misconception. Since ascending alphabetical 
order is one of the available states, using an icon that with "ascending" and 
"alphabetical" iconography is bound to confuse and frustrate a few people who 
mistakenly expect it to reflect the active sort order.
  
  Either way, since this contains a string change, it's headed for 5.47 anyway 
(thanks for being a stickler and reminding me to pay attention to this), so I 
think we have some time to get a better icon.

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D12337

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

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

D12659: two new UDS structures

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> udsentry_benchmark.cpp:483
> +inline Field(const uint index, long long value = 0) : m_long(value), 
> m_index(index) {}
> +// This operator is essential to gain some speed, because the 
> default == is slow
> +inline bool operator == (const Field ) const {

This comment is still wrong - you want to compare the key only, not the whole 
entry

Also, you no longer need it, as you use a lambda for the comparision now.

> udsentry_benchmark.cpp:488
> +
> +QString m_str = QStringLiteral();
> +long long m_long = LLONG_MIN;

non-POD types should not be initialized explicitly

> udsentry_benchmark.cpp:490
> +long long m_long = LLONG_MIN;
> +uint m_index = -1;
> +};

`unsigned int` and -1?

> udsentry_benchmark.cpp:498
> +}
> +void insert(uint field, const QString )
> +{

bad naming, you use field and class Field for different things

> udsentry_benchmark.cpp:506
> +{
> +auto index = std::find_if(storage.begin(), storage.end(),
> +  [field](const Field ) {return 
> index.m_index == field;});

bad naming again, index is not an index but an entry or field

REPOSITORY
  R241 KIO

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

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

D12320: {RFC] add ability to read embedded cover files

[Matthieu Gallien]

mgallien added a comment.


  Thanks for your work. Please find a few remarks inline.

INLINE COMMENTS

> embeddedimagedata.h:25
> +#include "kfilemetadata_export.h"
> +#include 
> +

You can do a forward declaration of QMimeDatabase because it is only referred 
through a reference.

> embeddedimagedata.h:40
> +class Private;
> +Private *d;
> +

You should be using a std::unique_ptr instead of a raw pointer. You also should 
take care of either forbidding copy (operator= and copy constructor) or 
implement them to perform a deep copy including duplicating the private object.
Currently, it will be implicitly shared between instances.
In many classes in KDE repository you do not need to do it because QObject 
inheritance gives you exactly that.

> embeddedimagedata.h:42
> +
> +QByteArray getFrontCover(const QString , const QString 
> ) const;
> +

The private method can go to the private class.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien, michaelh
Cc: bruns, #frameworks, ashaposhnikov, michaelh, astippich, spoorun

D12337: Give the file dialogs a "Sort by" menu button on the toolbar

[Henrik Fehlauer]

rkflx accepted this revision.
rkflx added a comment.
This revision is now accepted and ready to land.


  Thanks, LGTM now.

INLINE COMMENTS

> ngraham wrote in kdiroperator.cpp:1888
> The problem conceptually is that `view-sort-ascending` is semantically 
> inaccurate for anything but ascending order. We don't actually have an icon 
> yet that means "general sort options are here!" That's covered by 
> https://bugs.kde.org/show_bug.cgi?id=393318, and I've pinged Andreas again. 
> No matter what flawed we choose as a placeholder, I'm going to wait for the 
> better icon before landing this, so for now let's just leave it the way it is.

I think it is a misconception that toolbar icons represent state. I don't know 
of any toolbar in our software where this is the case, so why should users 
suddenly expect that what's on the icon represents exactly what is happening, 
e.g. A-Z ascending? It is merely an example of what type of actions they can 
expect when clicking on the button.

Icons are a symbolic representation of general concept, not a literal display 
of a specific state.

---

Anyway, if you want block everything on that, that's what we'll do.

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D12337

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

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

D12662: Add InlineNoteInterface

[Michal Srb]

michalsrb added a comment.


  In D12662#257342 , @brauch wrote:
  
  > Looks good from the implementation too so far. One thing I do not see is 
any changes to the cursorToX / xToCursor functions, is there really no change 
required there?
  
  
  No change was really needed. As the spaces are created when laying out the 
line, either by offsetting the start of the line or by formatting the text, it 
gets stored in the layout and the cursorToX / xToCursor work as expected. Even 
moving the cursor around with arrow keys works nicely (the cursor moves to the 
closest position above/below even if one of the lines is shifted).
  
  > Some things which come to my mind for testing would be:
  > 
  > - is selection rendered correctly if it includes notes, at the end, 
beginning, or middle of lines, also mult-line selections?
  
  I thought regular selection works fine, but actually I just found a glitch; 
if there is a note at the beginning of the line, it does not render the 
selection background under it:
  
  F5831112: Screenshot_20180502_205152.png 

  
  A selection in block mode behaves like if the notes are not there:
  
  F5831117: Screenshot_20180502_205212.png 

  
  I can't tell whether it is good or bad behavior. Do you think it should stay 
this way - selecting the block of the real text, or should it instead select a 
"visual" block?
  
  I was thinking it would be best to avoid this problem and hide the notes when 
doing block mode selection, but should that be done by ktexteditor directly, or 
by the user of the interface? I also imagine that anything that uses it would 
provide some quick on/off toggle for cases like this.
  
  > - what happens when clicking or dragging from or into the notes?
  
  Clicking into the note always places the text cursor on the right side of the 
note - in other words, the note acts as extension of the letter on its left. 
Dragging makes normal selection from that position.
  
  > - does it still work properly with dynamic word-wrap on?
  
  Hmm, looks like notes at the beginning of the text ruin the breaking of the 
line:
  
  F5831141: Screenshot_20180502_212929.png 

  
  And it also does not place the notes well if the wrapped line keeps keeps 
indentation, like in this case:
  
  F5831123: Screenshot_20180502_210236.png 

  
  > - does it work properly with code-folding? what happens if a note is at the 
border of a folding region?
  
  It works fine. When a line is hidden, so are the notes in it.
  
  One more thing that needs to be decided: At this moment if a note is placed 
past the end of the text too far to the right and word-wrap is on, it does not 
wrap, but renders partially or completely out of view. (This happens kinda by 
default as the layout of the line is not modified for notes out of the text.) 
My thinking was that it does not matter as one would either place notes in the 
text, or outside at a column that will be visible without scrolling/wrapping. 
But if you think it should wrap, I can try to find a way to make that happen.
  
  Thank you for the review, I'll fix these problems and make the changes you 
noted.

REPOSITORY
  R39 KTextEditor

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

To: michalsrb, #ktexteditor
Cc: brauch, #frameworks, michaelh, kevinapavew, ngraham, bruns, demsking, 
cullmann, sars, dhaumann

D12648: Update mount point after mount operations

[David Edmundson]

davidedmundson added a comment.


  Let me clarify
  
  At the time of the return from udisks the property on udisks is updated but 
the signal saying the property is updated is not yet sent, as solid caches 
properties we have a wrong value locally but udisks has the correct value 
remotely

REPOSITORY
  R245 Solid

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

To: davidedmundson, #plasma, broulik, mart
Cc: bruns, #frameworks, michaelh

D12392: Fix the "Default" color scheme to match Breeze again

[Nathaniel Graham]

ngraham added a comment.


  The user already has to create a named color scheme if he wants to actually 
//save and use// his modifications.

REPOSITORY
  R265 KConfigWidgets

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

To: ngraham, #breeze, #plasma, hein
Cc: cfeck, abetts, #frameworks, michaelh, bruns

D12662: Add InlineNoteInterface

[Sven Brauch]

brauch added a comment.


  Looks good from the implementation too so far. One thing I do not see is any 
changes to the cursorToX / xToCursor functions, is there really no change 
required there?
  
  Some things which come to my mind for testing would be:
  
  - is selection rendered correctly if it includes notes, at the end, 
beginning, or middle of lines, also mult-line selections?
  - what happens when clicking or dragging from or into the notes?
  - does it still work properly with dynamic word-wrap on?
  - does it work properly with code-folding? what happens if a note is at the 
border of a folding region?

INLINE COMMENTS

> katedocument.cpp:5295
> +
> +connect(provider, SIGNAL(reset()), this, SLOT(inlineNotesReset()));
> +connect(provider, SIGNAL(lineChanged(int)), this, 
> SLOT(inlineNotesLineChanged(int)));

Can you use new-style connect here, i.e. the function-pointer syntax
connect(provider, ::reset, this, 
::inlineNotesReset)?
This gives compile-time argument type checking.

> katerenderer.cpp:771
> +// Determine the position where to paint the note.
> + // We start by getting the x coordinate of cursor placed to the 
> column.
> +qreal x = 
> range->viewLine(viewLine).lineLayout().cursorToX(column) - xStart;

indent

> katerenderer.cpp:777
> +// note should be painted and the cursor gets placed at the 
> right side of it. So we have to
> + // subtract the width of the note to get to left side of the 
> hole.
> +x -= inlineNote->width(lineHeight(), currentFontMetrics());

indent

REPOSITORY
  R39 KTextEditor

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

To: michalsrb, #ktexteditor
Cc: brauch, #frameworks, michaelh, kevinapavew, ngraham, bruns, demsking, 
cullmann, sars, dhaumann

D12392: Fix the "Default" color scheme to match Breeze again

[Christoph Feck]

cfeck added a comment.


  "Current" could be named "Unnamed" or "Unsaved". It cannot be removed, unless 
you want to force the user to always create a named color scheme when he does 
modifications.

REPOSITORY
  R265 KConfigWidgets

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

To: ngraham, #breeze, #plasma, hein
Cc: cfeck, abetts, #frameworks, michaelh, bruns

D12662: Add InlineNoteInterface

[Michal Srb]

michalsrb added a comment.


  Here are screenshots from the sample plugin:
  
  F5831086: txt.png  F5831085: cpp.png 
 F5831084: qml.png 

  
  The plugin provides notes that are just hardcoded for those specific files. 
The style of the notes is completely up to the user of the interface.

REPOSITORY
  R39 KTextEditor

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

To: michalsrb, #ktexteditor
Cc: brauch, #frameworks, michaelh, kevinapavew, ngraham, bruns, demsking, 
cullmann, sars, dhaumann

D12392: Fix the "Default" color scheme to match Breeze again

[Nathaniel Graham]

ngraham added a comment.


  "Current" is whatever the current theme is and can be ignored (and hopefully 
gotten rid of in the future). The issue here is that "Default" is //supposed 
to// be identical to "Breeze", but it's currently not. This patch fixes that.

REPOSITORY
  R265 KConfigWidgets

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

To: ngraham, #breeze, #plasma, hein
Cc: abetts, #frameworks, michaelh, bruns

D12659: two new UDS structures

[Jaime Torres Amate]

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


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

REPOSITORY
  R241 KIO

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

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

AFFECTED FILES
  autotests/udsentry_benchmark.cpp

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

D12392: Fix the "Default" color scheme to match Breeze again

[Andres Betts]

abetts added a comment.


  Just trying to understand. What is happening here is that we have 3 color 
schemes that are all theoretically the same as Breeze. However, the "default" 
and "Breeze" scheme are currently not the same. Therefore, we have to make sure 
that they are. If that's the case
  
  +1

REPOSITORY
  R265 KConfigWidgets

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

To: ngraham, #breeze, #plasma, hein
Cc: abetts, #frameworks, michaelh, bruns

D12392: Fix the "Default" color scheme to match Breeze again

[Nathaniel Graham]

ngraham added a comment.


  Friendly ping!

REPOSITORY
  R265 KConfigWidgets

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

To: ngraham, #breeze, #plasma, hein
Cc: #frameworks, michaelh, bruns

D12016: [ktexteditor] much faster positionFromCursor

[Anthony Fieroni]

anthonyfieroni added a comment.


  In D12016#257325 , @jtamate wrote:
  
  > Do you mean to extend KateViewInternal in such a way that 
positionFromCursor in KateViewAccessible becomes unnecessary?
  
  
  Yes, mainly it should be a tiny helper.

REPOSITORY
  R39 KTextEditor

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

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

D12016: [ktexteditor] much faster positionFromCursor

[Jaime Torres Amate]

jtamate added a comment.


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

REPOSITORY
  R39 KTextEditor

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

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

D12016: [ktexteditor] much faster positionFromCursor

[Anthony Fieroni]

anthonyfieroni added a comment.


  Maybe i miss something, but i think if something should be extended it's view 
not accessible.

REPOSITORY
  R39 KTextEditor

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

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

D12388: Output device color curves correction

[Martin Flöser]

graesslin requested changes to this revision.
graesslin added a comment.
This revision now requires changes to proceed.


  Please increase the versions.

INLINE COMMENTS

> output-management.xml:82
>  
>  
>  

please increment version

> output-management.xml:142
>  
> +
> +

since is missing

> output-management.xml:142
>  
> +
> +

you cannot add requests in between, a new request must be the last.

> outputdevice.xml:32
>  
>  
>  

version needs incrementation

> outputdevice.xml:179
>  
> +
> +

you cannot add events in between, new one needs to be last.

REPOSITORY
  R127 KWayland

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

To: romangg, #frameworks, davidedmundson, graesslin
Cc: graesslin, davidedmundson, zzag, cfeck, michaelh, bruns

D12659: two new UDS structures

[Jaime Torres Amate]

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


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

REPOSITORY
  R241 KIO

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

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

AFFECTED FILES
  autotests/udsentry_benchmark.cpp

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

D12648: Update mount point after mount operations

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> udisksstorageaccess.cpp:170
> +m_device->invalidateCache();
>  m_device->broadcastActionDone("setup");
>  

This replaces one race condition with another (less likely) one. There is no 
guarantee the property is already updated when checkAccessibility is called.

The correct approach would be to defer until the propertiesChanged signal 
arrives and broadcast the "setup done" afterwards.

A different, but more intrusive approach would be to defer any check for the 
"acessible" property on the caller side, waiting for the accessibilityChanged 
signal.

REPOSITORY
  R245 Solid

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

To: davidedmundson, #plasma, broulik, mart
Cc: bruns, #frameworks, michaelh

D12659: two new UDS structures

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> udsentry_benchmark.cpp:467
> +inline Field(const uint index, long long value = 0) : m_long(value), 
> m_index(index) { }
> +// This operator is essential to gain some speed, because the 
> default == is slow
> +inline bool operator == (const Field ) {

The default `==` will not only compare the key, but also the value (thats the 
reason for being slow), so you have completely different behaviour. When 
comparing only the index, you will update the value for an existing key, 
otherwise you will append a new value to the same key.

REPOSITORY
  R241 KIO

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

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

D12659: two new UDS structures

[Stefan Brüns]

bruns added a comment.


  In D12659#257294 , @dfaure wrote:
  
  > In D12659#257292 , @bruns wrote:
  >
  > > In D12659#257098 , @dfaure 
wrote:
  > >
  > > > Thanks for that investigation. Interesting that linear search is faster 
than binary search in the same data structure... maybe the compiler optimizes 
it better? Did you profile V2 to find out where the time is spent, or do you 
have a better explanation?
  > >
  > >
  > > Binary search has a (small) overhead, you can hardly beat linear search 
when the number of entries is small. When you use binary search, you pay when 
inserting - find the right position, probably move other items. When you do 
inserts one item a time, you have O(n^2) behavior.
  >
  >
  > I know that but that's not what the code was doing, he is appending in 
(hopefully) sorted order, so the difference was only at lookup time, where I 
can't see any overhead due to binary search.
  
  
  Even when appending in sorted order, the correct position has to be checked, 
which is O(n), the appending itself is O(1), as no movement is involved. 
Appending unsorted is always O(1) (when capacity is sufficient).
  
  >>> When you say "scales better", we're talking about the number of fields in 
the udsentry, not the number of items. But kioslaves don't fill in 1000 fields, 
so I have the feeling that scaling with the number of fields isn't a 
requirement.
  >> 
  >> Using one list is better than two lists (one heap allocation instead of 
two), one size check when inserting ...
  > 
  > Sure, but I'm only comparing "Another" and "AnotherV2", which both use a 
single vector.
  
  The comment was addressing the difference between this approach and the 
"FrankUDSEntry".
  
  >> A good data structure here is one contigous vector storing both key and 
(small value or d-pointer). Inserting is O(1) when the space has been reserved, 
lookup is O(n). (For binary search, lookup is O(log n) - for 8 entries, this is 
== 3, linear search is 8/2 == 4, but no overhead).
  >>  I would suggest QVector>. Each element, taking 
alignment into account, is 24 bytes.
  > 
  > If anyone attempts this, please name the struct and its members, don't use 
QPair ;-)
  >  But no, that cannot possibly be faster. QVariant has lots of overhead 
itself.
  
  QVariant has hardly any overhead in this case - the struct here stores one 
uint and one QString (which is justs its d-ptr), while the QVariant uses one 
uint for the type info and one union for the data. For both the uint case and 
QString, the data is stored inline in the QVariant data union.
  
  Using QVariant has the benefit of avoiding a homegrown structure. It is not 
faster, but it is also not slower.

REPOSITORY
  R241 KIO

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

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

D12659: two new UDS structures

[Jaime Torres Amate]

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


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

INLINE COMMENTS

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

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

REPOSITORY
  R241 KIO

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

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

D12662: Add InlineNoteInterface

[Sven Brauch]

brauch added a comment.


  Awesome idea! Do you have a screenshot of how it looks?

REPOSITORY
  R39 KTextEditor

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

To: michalsrb, #ktexteditor
Cc: brauch, #frameworks, michaelh, kevinapavew, ngraham, bruns, demsking, 
cullmann, sars, dhaumann

D12659: two new UDS structures

[David Faure]

dfaure added a comment.


  In D12659#257292 , @bruns wrote:
  
  > In D12659#257098 , @dfaure wrote:
  >
  > > Thanks for that investigation. Interesting that linear search is faster 
than binary search in the same data structure... maybe the compiler optimizes 
it better? Did you profile V2 to find out where the time is spent, or do you 
have a better explanation?
  >
  >
  > Binary search has a (small) overhead, you can hardly beat linear search 
when the number of entries is small. When you use binary search, you pay when 
inserting - find the right position, probably move other items. When you do 
inserts one item a time, you have O(n^2) behavior.
  
  
  I know that but that's not what the code was doing, he is appending in 
(hopefully) sorted order, so the difference was only at lookup time, where I 
can't see any overhead due to binary search.
  
  >> When you say "scales better", we're talking about the number of fields in 
the udsentry, not the number of items. But kioslaves don't fill in 1000 fields, 
so I have the feeling that scaling with the number of fields isn't a 
requirement.
  > 
  > Using one list is better than two lists (one heap allocation instead of 
two), one size check when inserting ...
  
  Sure, but I'm only comparing "Another" and "AnotherV2", which both use a 
single vector.
  
  > A good data structure here is one contigous vector storing both key and 
(small value or d-pointer). Inserting is O(1) when the space has been reserved, 
lookup is O(n). (For binary search, lookup is O(log n) - for 8 entries, this is 
== 3, linear search is 8/2 == 4, but no overhead).
  >  I would suggest QVector>. Each element, taking 
alignment into account, is 24 bytes.
  
  If anyone attempts this, please name the struct and its members, don't use 
QPair ;-)
  But no, that cannot possibly be faster. QVariant has lots of overhead itself.

REPOSITORY
  R241 KIO

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

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

D12659: two new UDS structures

[Stefan Brüns]

bruns added a comment.


  In D12659#257098 , @dfaure wrote:
  
  > Thanks for that investigation. Interesting that linear search is faster 
than binary search in the same data structure... maybe the compiler optimizes 
it better? Did you profile V2 to find out where the time is spent, or do you 
have a better explanation?
  
  
  Binary search has a (small) overhead, you can hardly beat linear search when 
the number of entries is small. When you use binary search, you pay when 
inserting - find the right position, probably move other items. When you do 
inserts one item a time, you have O(n^2) behavior.
  
  > But even if both were equal performance-wise I'd favor linear search 
because sorted insertion is easy to get wrong - as this patch shows ;)
  > 
  > When you say "scales better", we're talking about the number of fields in 
the udsentry, not the number of items. But kioslaves don't fill in 1000 fields, 
so I have the feeling that scaling with the number of fields isn't a 
requirement.
  
  Using one list is better than two lists (one heap allocation instead of two), 
one size check when inserting ...
  
  A good data structure here is one contigous vector storing both key and 
(small value or d-pointer). Inserting is O(1) when the space has been reserved, 
lookup is O(n). (For binary search, lookup is O(log n) - for 8 entries, this is 
== 3, linear search is 8/2 == 4, but no overhead).
  
  I would suggest QVector>. Each element, taking 
alignment into account, is 24 bytes.
  
  QMap and QHash are clearly overkill here. The involved pointer chasing ruins 
any performance benefit from the asymtotically faster lookup.

REPOSITORY
  R241 KIO

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

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

KDE CI: Frameworks plasma-framework kf5-qt5 SUSEQt5.10 - Build # 150 - Still Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks%20plasma-framework%20kf5-qt5%20SUSEQt5.10/150/
 Project:
Frameworks plasma-framework kf5-qt5 SUSEQt5.10
 Date of build:
Wed, 02 May 2018 15:34:29 +
 Build duration:
14 min and counting
   JUnit Tests
  Name: (root) Failed: 7 test(s), Passed: 8 test(s), Skipped: 0 test(s), Total: 15 test(s)Failed: TestSuite.dialognativetestFailed: TestSuite.plasma-configmodeltestFailed: TestSuite.plasma-dialogqmltestFailed: TestSuite.plasma-fallbackpackagetestFailed: TestSuite.plasma-iconitemtestFailed: TestSuite.plasma-packagestructuretestFailed: TestSuite.plasma-storagetest
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report33%
(6/18)35%
(55/159)35%
(55/159)26%
(3556/13533)18%
(1977/10715)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsautotests85%
(22/26)85%
(22/26)53%
(607/1140)28%
(421/1492)src.declarativeimports.calendar0%
(0/11)0%
(0/11)0%
(0/470)0%
(0/263)src.declarativeimports.core22%
(4/18)22%
(4/18)11%
(253/2316)7%
(102/1554)src.declarativeimports.plasmacomponents0%
(0/9)0%
(0/9)0%
(0/525)0%
(0/214)src.declarativeimports.plasmaextracomponents0%
(0/5)0%
(0/5)0%
(0/44)0%
(0/27)src.declarativeimports.platformcomponents0%
(0/4)0%
(0/4)0%
(0/60)0%
(0/14)src.declarativeimports.platformcomponents.utils0%
(0/2)0%
(0/2)0%
(0/15)0%
(0/4)src.plasma55%
(12/22)55%
(12/22)41%
(1442/3532)28%
(827/2965)src.plasma.packagestructure0%
(0/7)0%
(0/7)0%
(0/141)0%
(0/14)src.plasma.private46%
(11/24)46%
(11/24)41%
(671/1621)28%
(317/1121)src.plasma.scripting0%
(0/3)0%
(0/3)0%
(0/162)0%
(0/132)src.plasmapkg0%
(0/1)0%
(0/1)0%
(0/45)0%
(0/40)src.plasmaquick42%
(5/12)42%
(5/12)27%
(552/2015)17%
(305/1779)src.plasmaquick.private33%
(1/3)33%
(1/3)28%
(31/110)36%
(5/14)src.scriptengines.qml.plasmoid0%
(0/6)0%
(0/6)0%
(0/1179)0%
(0/1058)tests.dpi0%
(0/2)0%
(0/2)0%
(0/22)0%
(0/2)tests.kplugins0%
   

KDE CI: Frameworks plasma-framework kf5-qt5 FreeBSDQt5.10 - Build # 2 - Still Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks%20plasma-framework%20kf5-qt5%20FreeBSDQt5.10/2/
 Project:
Frameworks plasma-framework kf5-qt5 FreeBSDQt5.10
 Date of build:
Wed, 02 May 2018 15:34:29 +
 Build duration:
4 min 9 sec and counting
   JUnit Tests
  Name: (root) Failed: 1 test(s), Passed: 13 test(s), Skipped: 0 test(s), Total: 14 test(s)Failed: TestSuite.plasma-packagestructuretest

D12438: Fix PlasmaCalendar widget to not mark days with minor events

[Friedrich W . H . Kossebau]

This revision was automatically updated to reflect the committed changes.
kossebau marked an inline comment as done.
Closed by commit R242:67f5ad29e8e6: Fix PlasmaCalendar widget to not mark days 
with minor events (authored by kossebau).

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12438?vs=32782=33501

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

AFFECTED FILES
  src/declarativeimports/calendar/daysmodel.cpp
  src/declarativeimports/calendar/daysmodel.h
  src/declarativeimports/calendar/qml/DayDelegate.qml

To: kossebau, #plasma, broulik, davidedmundson
Cc: davidedmundson, broulik, #frameworks, michaelh, bruns

D12438: Fix PlasmaCalendar widget to not mark days with minor events

[Friedrich W . H . Kossebau]

kossebau marked an inline comment as done.
kossebau added inline comments.

INLINE COMMENTS

> broulik wrote in daysmodel.cpp:223
> `std::find_if`

Tried, but seems there is no simple way to use std::find_if or std::any_of 
algorithms here for the usecase of checking all values for a given key in a 
QMultiHash. The algorithms all only take fixed first and last iterators, so one 
first has to estimate the last iterator by something like std::equal_range, 
which as result means iterating 2x over the entries and does not make the code 
more comprehensible to the reader.
So as discussed on irc staying with current code for now.

> davidedmundson wrote in DayDelegate.qml:105
> surely this should be model !== undefined && model.contains
> 
> otherwise we're doing a filter twice.

Haven't got the complete picture on this code, the purpose of 
18af8703039390de8b803a7a9b3c5cfe48f70b7a 
 
leaves me slightly puzzled, so better to be done separately, cannot yet oversee 
the potential side effects.

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  fixdisplayofminorevents

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

To: kossebau, #plasma, broulik, davidedmundson
Cc: davidedmundson, broulik, #frameworks, michaelh, bruns

D12337: Give the file dialogs a "Sort by" menu button on the toolbar

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> kdiroperator.cpp:1887
>  KActionMenu *sortMenu = new KActionMenu(i18n("Sorting"), this);
> +sortMenu->setIcon(QIcon::fromTheme(QStringLiteral("itemize")));
> +sortMenu->setDelayed(false);

Before the patch lands, this icon will be changed to whatever 
https://bugs.kde.org/show_bug.cgi?id=393318 yields.

REPOSITORY
  R241 KIO

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

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

D12337: Give the file dialogs a "Sort by" menu button on the toolbar

[Nathaniel Graham]

ngraham edited the summary of this revision.

REPOSITORY
  R241 KIO

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

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

D12337: Give the file dialogs a "Sort by" menu button on the toolbar

[Nathaniel Graham]

ngraham edited the test plan for this revision.

REPOSITORY
  R241 KIO

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

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

D12337: Give the file dialogs a "Sort by" menu button on the toolbar

[Nathaniel Graham]

ngraham marked 8 inline comments as done.

REPOSITORY
  R241 KIO

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

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

D12337: Give the file dialogs a "Sort by" menu button on the toolbar

[Nathaniel Graham]

ngraham marked 6 inline comments as done.
ngraham added inline comments.

INLINE COMMENTS

> rkflx wrote in kdiroperator.cpp:1888
> While we agreed upon wanting a better icon, until that's done I'd prefer 
> `view-sort-ascending` instead. For me, `itemize` has no connection to sorting 
> at all, sorry.
> 
> I'm aware my alternative shows a specific mode, but TBH I don't think users 
> will be put off too much by this detail, in particular because it is the only 
> sorting-related icon in the dialog.
> 
> Anyway, that's just my preference. Let me know if you think `itemize` is 
> vastly better.

The problem conceptually is that `view-sort-ascending` is semantically 
inaccurate for anything but ascending order. We don't actually have an icon yet 
that means "general sort options are here!" That's covered by 
https://bugs.kde.org/show_bug.cgi?id=393318, and I've pinged Andreas again. No 
matter what flawed we choose as a placeholder, I'm going to wait for the better 
icon before landing this, so for now let's just leave it the way it is.

> rkflx wrote in kfilewidget.cpp:365
> ?

Oops, somehow `arc` merged this patch with D12333 
 when I downloaded it. Not sure how I 
managed to do that. Cleaned up now.

> rkflx wrote in kfilewidget.cpp:365-369
> ?

Same, sorry.

> rkflx wrote in kfilewidget.cpp:554-559
> This also worked for me, and would avoid the `foreach`:
> 
>   KActionMenu *x = new 
> KActionMenu(QIcon::fromTheme(QStringLiteral("configure")), i18n("Options"), 
> this);
>   x->setMenu(coll->action(QStringLiteral("sorting menu"))->menu());
>   x->setDelayed(false);
>   d->toolbar->addAction(x);

Found an even simpler way. :)

REPOSITORY
  R241 KIO

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

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

D12337: Give the file dialogs a "Sort by" menu button on the toolbar

[Nathaniel Graham]

ngraham updated this revision to Diff 33498.
ngraham added a comment.


  Revert accidental merge, and simplify the single-click menu activation

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12337?vs=33464=33498

BRANCH
  arcpatch-D12337

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

AFFECTED FILES
  src/filewidgets/kdiroperator.cpp
  src/filewidgets/kfilewidget.cpp

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

D12662: Add InlineNoteInterface

[Michal Srb]

michalsrb added a comment.


  This is my first attempt to propose such API, it may not be ideal.
  
  If the inline note is placed into text, the space for it is created using 
QTextCharFormat's absolute spacing between the two letters. I admit it is quite 
a misuse, but it was the least intrusive way to create the space. Everything 
else seems to work with it out of the box without further modifications.
  
  A kate plugin that demonstrates how to use it is here:
  https://github.com/michalsrb/sampleinlinenotesplugin
  
  You can find screenshots in the "samples" directory.

REPOSITORY
  R39 KTextEditor

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

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

D12662: Add InlineNoteInterface

[Michal Srb]

michalsrb added a reviewer: KTextEditor.

REPOSITORY
  R39 KTextEditor

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

To: michalsrb, #ktexteditor
Cc: #frameworks, michaelh, kevinapavew, ngraham, bruns, demsking, cullmann, 
sars, dhaumann

D12662: Add InlineNoteInterface

[Michal Srb]

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

REVISION SUMMARY
  The inline note interface provides a way to render arbitrary things in
  the text. The layout of the line is adapted to create space for the note.
  
  Possible applications include showing a name of a function parameter on call
  side or rendering square with color preview next to CSS color property.

REPOSITORY
  R39 KTextEditor

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

AFFECTED FILES
  src/document/katedocument.cpp
  src/document/katedocument.h
  src/include/CMakeLists.txt
  src/include/ktexteditor/inlinenoteinterface.h
  src/render/katerenderer.cpp
  src/utils/ktexteditor.cpp
  src/view/kateview.h

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

D12648: Update mount point after mount operations

[David Edmundson]

This revision was automatically updated to reflect the committed changes.
Closed by commit R245:d735708ff11c: Update mount point after mount operations 
(authored by davidedmundson).

REPOSITORY
  R245 Solid

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12648?vs=33460=33488

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

AFFECTED FILES
  src/solid/devices/backends/udisks2/udisksdevice.cpp
  src/solid/devices/backends/udisks2/udisksdevice.h
  src/solid/devices/backends/udisks2/udisksstorageaccess.cpp

To: davidedmundson, #plasma, broulik, mart
Cc: #frameworks, michaelh, bruns

D12648: Update mount point after mount operations

[Kai Uwe Broulik]

broulik accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R245 Solid

BRANCH
  master

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

To: davidedmundson, #plasma, broulik
Cc: #frameworks, michaelh, bruns

D12648: Update mount point after mount operations

[Marco Martin]

mart accepted this revision.

REPOSITORY
  R245 Solid

BRANCH
  master

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

To: davidedmundson, #plasma, broulik, mart
Cc: #frameworks, michaelh, bruns

D12439: Add EventDataDecorator::hasEndDateTime()

[Friedrich W . H . Kossebau]

kossebau abandoned this revision.
kossebau added a comment.


  Seems code out there is already relying on `!isNaN(datetime)` so let's do the 
same for now. Besides, there can be events with also only end-date, so the same 
check would be needed for the start date, thus this API would be incomplete.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: kossebau, #plasma
Cc: broulik, #frameworks, michaelh, bruns

D12439: Add EventDataDecorator::hasEndDateTime()

[Kai Uwe Broulik]

broulik added a comment.


  `!isNaN(endDateTime.getTime())`?

REPOSITORY
  R242 Plasma Framework (Library)

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

To: kossebau, #plasma
Cc: broulik, #frameworks, michaelh, bruns

D12438: Fix PlasmaCalendar widget to not mark days with minor events

[David Edmundson]

davidedmundson accepted this revision.
davidedmundson added inline comments.

INLINE COMMENTS

> DayDelegate.qml:105
>  Loader {
> -active: model.containsEventItems !== undefined && 
> model.containsEventItems
> +active: model.containsMajorEventItems !== undefined && 
> model.containsMajorEventItems
>  anchors.bottom: parent.bottom

surely this should be model !== undefined && model.contains

otherwise we're doing a filter twice.

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  fixdisplayofminorevents

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

To: kossebau, #plasma, broulik, davidedmundson
Cc: davidedmundson, broulik, #frameworks, michaelh, bruns

D12438: Fix PlasmaCalendar widget to not mark days with minor events

[Kai Uwe Broulik]

broulik accepted this revision.
broulik added a comment.
This revision is now accepted and ready to land.


  address comment then ship it

INLINE COMMENTS

> daysmodel.cpp:223
> +auto it = m_eventsData.find(date);
> +while (it != m_eventsData.end() && it.key() == date) {
> +if (!it.value().isMinor()) {

`std::find_if`

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  fixdisplayofminorevents

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

To: kossebau, #plasma, broulik
Cc: broulik, #frameworks, michaelh, bruns

D12659: two new UDS structures

[David Faure]

dfaure added a comment.


  Thanks for that investigation. Interesting that linear search is faster than 
binary search in the same data structure... maybe the compiler optimizes it 
better? Did you profile V2 to find out where the time is spent, or do you have 
a better explanation?
  But even if both were equal performance-wise I'd favor linear search because 
sorted insertion is easy to get wrong - as this patch shows ;)
  
  When you say "scales better", we're talking about the number of fields in the 
udsentry, not the number of items. But kioslaves don't fill in 1000 fields, so 
I have the feeling that scaling with the number of fields isn't a requirement.
  
  Are those benchmarks run in Release (or RelWithDebInfo) mode, rather than 
Debug (which is a big no no for benchmarks)? Qt should be compiled with 
optimizations enabled too.
  
  PS: for all alternatives where the API is the same, we could use a template 
function (templated on the type of UDSEntry implementation) to perform the 
tests; this would ensure that all tests are actually testing the same thing ;-)

INLINE COMMENTS

> udsentry_benchmark.cpp:464
> +{
> +inline Field() : m_index(-1) {}
> +inline Field(const uint index, const QString ) : m_str(value), 
> m_long(0), m_index(index) {}

m_long should be initialized too (in-place initialized in the member 
declaration would help not forgetting that)

> udsentry_benchmark.cpp:468
> +// This operator is essential to gain some speed, because the 
> default == is slow
> +inline bool operator == (const Field ) {
> +return m_index == other.m_index;

const

> udsentry_benchmark.cpp:487
> +{
> +*index = Field(field, value);
> +return;

wouldn't it be faster to assign to index->m_str here?

  index->m_str = value;

> udsentry_benchmark.cpp:491
> +}
> +storage.append(Field(field, value));
> +}

std::vector with emplace_back would be interesting here, performance-wise.

If you keep using QVector, though, then Q_DECLARE_MOVABLE(Field) would help, 
especially when reserving too small.

> udsentry_benchmark.cpp:498
> +{
> +*index = Field(field, value);
> +return;

wouldn't it be faster to assign to index->m_long here?

  index->m_long = value;

> udsentry_benchmark.cpp:510
> +{
> +for (auto index = storage.constBegin(), end = storage.constEnd(); 
> index != end; ++index) {
> +if (index->m_index == field)

alternative: std::find_if.

> udsentry_benchmark.cpp:560
> +entry.stringValue(KIO::UDSEntry::UDS_GROUP) == 
> entry2.stringValue(KIO::UDSEntry::UDS_GROUP);
> +QCOMPARE(equal, true);
> +}

QVERIFY(equal)

> udsentry_benchmark.cpp:593
> +// This operator is essential to gain some speed, because the 
> default == is slow
> +inline bool operator == (const Field ) {
> +return m_index == other.m_index;

const

> udsentry_benchmark.cpp:619
> +}
> +storage.append(Field(field, value));
> +}

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

> udsentry_benchmark.cpp:634
> +}
> +QString stringValue(uint field)
> +{

const

> udsentry_benchmark.cpp:642
> +}
> +long long numberValue(uint field, long long defaultValue = -1)
> +{

const

REPOSITORY
  R241 KIO

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

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

k18n 5.45 update

[Jonathan Riddell]

Could we get a .1 update to k18n to include this fix?

https://cgit.kde.org/ki18n.git/commit/?id=9c32bdab29b345976eee3b9c1c4bebd76cdcdde2
https://phabricator.kde.org/D12216

It is causing issue with some packager of plasma-workspace 5.12.5

Jonathan

D12659: two new UDS structures

[Jaime Torres Amate]

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

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

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


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

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

D12337: Give the file dialogs a "Sort by" menu button on the toolbar

[Henrik Fehlauer]

rkflx added inline comments.

INLINE COMMENTS

> kdiroperator.cpp:1888
>  d->actionCollection->addAction(QStringLiteral("sorting menu"),  
> sortMenu);
> +sortMenu->setIcon(QIcon::fromTheme(QStringLiteral("itemize")));
>  

While we agreed upon wanting a better icon, until that's done I'd prefer 
`view-sort-ascending` instead. For me, `itemize` has no connection to sorting 
at all, sorry.

I'm aware my alternative shows a specific mode, but TBH I don't think users 
will be put off too much by this detail, in particular because it is the only 
sorting-related icon in the dialog.

Anyway, that's just my preference. Let me know if you think `itemize` is vastly 
better.

> kfilewidget.cpp:365
>  opsWidgetLayout->setSpacing(0);
> -//d->toolbar = new KToolBar(this, true);
> -d->toolbar = new KToolBar(d->opsWidget, true);
> +d->toolbar = new KToolBar(this, true);
>  d->toolbar->setObjectName(QStringLiteral("KFileWidget::toolbar"));

?

> kfilewidget.cpp:365-369
> -//d->toolbar = new KToolBar(this, true);
> -d->toolbar = new KToolBar(d->opsWidget, true);
> +d->toolbar = new KToolBar(this, true);
>  d->toolbar->setObjectName(QStringLiteral("KFileWidget::toolbar"));
>  d->toolbar->setMovable(false);
> -opsWidgetLayout->addWidget(d->toolbar);

?

> kfilewidget.cpp:554-559
> +// Tweak the look and feel of the sort menu button
> +foreach(QToolButton* button, d->toolbar->findChildren()) {
> +if (button->defaultAction() == coll->action(QStringLiteral("sorting 
> menu"))) {
> +button->setPopupMode(QToolButton::InstantPopup);
> +}
> +}

This also worked for me, and would avoid the `foreach`:

  KActionMenu *x = new 
KActionMenu(QIcon::fromTheme(QStringLiteral("configure")), i18n("Options"), 
this);
  x->setMenu(coll->action(QStringLiteral("sorting menu"))->menu());
  x->setDelayed(false);
  d->toolbar->addAction(x);

> kfilewidget.cpp:561
> +
> +
>  KUrlCompletion *pathCompletionObj = new 
> KUrlCompletion(KUrlCompletion::DirCompletion);

Unintentional newline?

> kfilewidget.cpp:1410
>  boxLayout->setMargin(0); // no additional margin to the already existing
> +boxLayout->addWidget(toolbar);
>  

?

REPOSITORY
  R241 KIO

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

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

D12647: Move the inline preview button into the menu

[Henrik Fehlauer]

rkflx added a comment.


  In T8552#140332 , @rkflx wrote:
  
  > > moving the Show Preview button to the menu
  >
  > Makes sense to me, but might be controversial with others.
  
  
  FWIW, if people do not like this (e.g. because they often toggle previews, or 
because it should look consistent with Dolphin), I'd also be fine with keeping 
the button if in the end it turns out we have the space for it.
  
  Keeping the button would also allow to show the tooltip text, explaining why 
the action sometimes is disabled. Having no such explanation for the menu item 
feels a bit awkward.
  
  Patch itself LGTM, but needs more +1's on the idea.

REPOSITORY
  R241 KIO

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

To: ngraham, #frameworks, rkflx
Cc: anemeth, michaelh, bruns

KDE CI: Frameworks breeze-icons kf5-qt5 SUSEQt5.9 - Build # 25 - Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks%20breeze-icons%20kf5-qt5%20SUSEQt5.9/25/
 Project:
Frameworks breeze-icons kf5-qt5 SUSEQt5.9
 Date of build:
Wed, 02 May 2018 07:30:37 +
 Build duration:
9 min 10 sec and counting
   JUnit Tests
  Name: (root) Failed: 1 test(s), Passed: 4 test(s), Skipped: 0 test(s), Total: 5 test(s)Failed: TestSuite.scalable
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report100%
(2/2)100%
(6/6)100%
(6/6)78%
(239/306)57%
(143/252)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsdefault100%
(1/1)100%
(1/1)74%
(42/57)54%
(15/28)autotests100%
(5/5)100%
(5/5)79%
(197/249)57%
(128/224)

KDE CI: Frameworks breeze-icons kf5-qt5 WindowsMSVCQt5.10 - Build # 26 - Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks%20breeze-icons%20kf5-qt5%20WindowsMSVCQt5.10/26/
 Project:
Frameworks breeze-icons kf5-qt5 WindowsMSVCQt5.10
 Date of build:
Wed, 02 May 2018 07:30:37 +
 Build duration:
3 min 57 sec and counting
   JUnit Tests
  Name: (root) Failed: 1 test(s), Passed: 3 test(s), Skipped: 0 test(s), Total: 4 test(s)Failed: TestSuite.scalable

KDE CI: Frameworks breeze-icons kf5-qt5 FreeBSDQt5.10 - Build # 2 - Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks%20breeze-icons%20kf5-qt5%20FreeBSDQt5.10/2/
 Project:
Frameworks breeze-icons kf5-qt5 FreeBSDQt5.10
 Date of build:
Wed, 02 May 2018 07:30:37 +
 Build duration:
2 min 0 sec and counting
   JUnit Tests
  Name: (root) Failed: 1 test(s), Passed: 3 test(s), Skipped: 0 test(s), Total: 4 test(s)Failed: TestSuite.scalable

KDE CI: Frameworks breeze-icons kf5-qt5 SUSEQt5.10 - Build # 81 - Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks%20breeze-icons%20kf5-qt5%20SUSEQt5.10/81/
 Project:
Frameworks breeze-icons kf5-qt5 SUSEQt5.10
 Date of build:
Wed, 02 May 2018 07:30:37 +
 Build duration:
1 min 45 sec and counting
   JUnit Tests
  Name: (root) Failed: 1 test(s), Passed: 4 test(s), Skipped: 0 test(s), Total: 5 test(s)Failed: TestSuite.scalable
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report100%
(2/2)100%
(6/6)100%
(6/6)78%
(239/306)57%
(143/252)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsdefault100%
(1/1)100%
(1/1)74%
(42/57)54%
(15/28)autotests100%
(5/5)100%
(5/5)79%
(197/249)57%
(128/224)