D18563: Don't create directory tree when a new folder has a '/' in the name

[Mark Gaiser]

markg added a comment.


  In D18563#402216 , @ngraham wrote:
  
  > It's rather ironic that in D18571 , I 
endorse a hidden productivity booster feature, but here, I reject one. :)
  >
  > I think I'm reconsidering. It's true that people get confused when they try 
to create folders with slashes in the name, but removing the feature entirely 
is probably not the best way to handle it.
  >
  > Ideas?
  
  
  Well, one idea does com to mind. But it would be a **lot** of work!
  The concept in Plasma is iirc still "strong by default, powerful when 
needed". Or something along those lines.
  
  With that in mind, it could be an option to disable this by default and 
introduce a new tab in the dolphin settings page where you list the power 
features and allow the user to enable them all (or one by one).
  It is a lot more code logic and does fall into the micro settings category (a 
place you normally don't want to be) but would be a possible solution in this 
case.
  
  Not saying this is the right direction. Merely one idea.
  
  Another idea is to warn the user but with a checkbox: "Don't show this 
warning again".

REPOSITORY
  R241 KIO

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

To: shubham, ngraham, #frameworks, #dolphin, dfaure, elvisangelaccio, pino
Cc: cfeck, acrouthamel, markg, ndavis, dfaure, elvisangelaccio, pino, 
kde-frameworks-devel, michaelh, ngraham, bruns

D18563: Don't create directory tree when a new folder has a '/' in the name

[Mark Gaiser]

markg added a comment.


  In D18563#401979 , @shubham wrote:
  
  > @ndavis UNIX does not allow / in any valid identifier name.
  >  Also looking at the bug report, it feels like people wanted to create a 
folder named "a/b/c" which UNEXPECTEDLY created directory tree. So in this, I 
have fix both the problems, first one which is not valid and the latter one 
which is unexpected behaviour.
  
  
  Define unexpected.
  
  People that don't know much about technology but merely happen to use KDE (so 
that would be the ones that also call for help if a console opens) the making 
of a folder names "a/b/c" causing the tree to me made is indeed unexpected.
  The more technical inclined people, the power users, will know that "/" is 
the directory separator and that typing "a/b/c" probably makes the whole tree. 
For those (and i bet those are still the majority of Plasma users) you would 
introduce a regression with this patch.
  
  I'm not saying it should go or stay. I only ever used it a couple of times 
and one of them being when debugging something years ago.
  I can imagine it to be useful for people who got used to it and now rely on 
it for their workflow.
  I can also imagine it not feeling intuitive.
  
  Fyi, mkdir "a/b/c" also doesn't work. mkdir -p "a/b/c" does!

REPOSITORY
  R241 KIO

BRANCH
  dir

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

To: shubham, ngraham, #frameworks, #dolphin, dfaure, elvisangelaccio, pino
Cc: markg, ndavis, dfaure, elvisangelaccio, pino, kde-frameworks-devel, 
michaelh, ngraham, bruns

D18380: KIO: make file dialog columns resizable again (and movable)

[Mark Gaiser]

markg added a comment.


  In D18380#400910 , @rjvbb wrote:
  
  > > But still, isn't there another way? Now the header and view are locked 
together. One doesn't work without the other.
  >
  > What's the problem with that? The custom header class isn't public. I did 
indeed use it for stuff that were not part of a class, or of the 
KDirOperatorDetailView class in earlier iterations. There's no hard reason for 
that, I just find it tidier (and it saves me from having to compile all other 
modules that import the kdiroperatordetail view headerfile when I change a 
detail that doesn't concern them).
  >
  > >   The thing that triggered me to comment wasn't any of that though. It's 
the "narrow mode". I cannot see how that wouldn't be seen as a bug by a user. 
With that little trick you're also overruling any font spacing settings the 
user might have had (in fontconfig) which is quite likely going to cause 
unexpected behavior and therefore bug reports.
  >
  > I'd say make a test case and convince us. You may have missed the fact that 
I'm no longer doing anything to the used font (the same in all columns). I'm 
just using the event flow to let Qt determine column widths using whatever 
information it wants, and then I "fixate" those widths in order to restore 
interactive mode. This is an admittedly complex way to do something that Qt 
doesn't allow us to do: 1) set interactive mode, 2) ask QHeaderView for the 
width that would be used in one of the automatic modes, 3) set those modes 
(with a minimum imposed on the name column).
  
  
  I know. I'm not attacking you here :) I've been in the same annoying 
situation countless of times when wanting to have this very feature. It's a 
pain Qt doesn't give it. But there are ways to solve it and yours is on the 
complicated side.
  
  > I can imagine that someone might file a bug about the name column getting a 
bit larger when resizing. In practice I'm not convinced many users will even 
notice this because it will happen only once, making the column more readable 
(and how much time does one spend resizing side-bars anyway). I think this is a 
bridge to take if and when we get there. A possible workaround would be to 
calculate our own minimum width in such a way that we arrive at the width Qt 
later decides to pick for some reason (for the default font at least).
  
  This is a typical "last mile" thingy. Yes, it works. But not as perfect as it 
could be which eventually is going to add up in hundreds of other "last mile" 
things. Each on their own not worth the time to fix. All together giving the 
user a "yep, they still need to work uit some bugs" feeling.
  
  > Something we (= a number of us) need to look into is what happens when you 
resize a view *after* a manual column adjustment, and what should happen in 
that case. Knowing that anything to make the manual adjustment permanent will 
only increase the code complexity.
  
  I'd say the last column will be the victim there. Just enable 
setStretchLastSection(true) (works in my example too) and let a resize be 
handled that way. This results in the view leaving all columns exactly as is 
when resizing except for the last column. The user still has to resize the name 
column to make it bigger/smaller and that very action should be stored and 
restored next time. It saves the hassle of trying to figure out what the user 
intended.
  
  > 
  > 
  >> The size calculation is not perfect
  > 
  > My initial implementation wasn't perfect either (Nate complained how it 
worked in narrow mode) ... and addressing that was what introduced most of the 
complexity.
  > 
  > At this point I have invested enough time and energy in what is essentially 
a behavioural detail. I like doing that kind of thing but I'm not motivated 
enough to start from scratch (I'll consider making little changes but not much 
more).
  
  Your work is well appreciated! It triggered me to look into it as well. 
Together we can probably figure out a perfect solution that does't leave any 
hiccups that the user will consider a bug, however short they might be. And as 
an added bonus might be used as a generic better QHeaderView version.
  
  p.s. This custom header view thing should definitely be a candidate for 
probably KItemViews. Not my version as-is, but the concept of having more 
flexibility in the header view.

REPOSITORY
  R241 KIO

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

To: rjvbb, ngraham, #frameworks, #dolphin, apol, dfaure, ahartmetz, markg
Cc: markg, cfeck, dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, 
ngraham, bruns

D18380: KIO: make file dialog columns resizable again (and movable)

[Mark Gaiser]

markg added a comment.


  In D18380#400903 , @ngraham wrote:
  
  > In D18380#400896 , @markg wrote:
  >
  > > In QHeaderView code, what we want is "QHeaderView::Interactive" [1] 
followed by QHeaderView::resizeSection [2]. Exactly as [1] described! 
QHeaderView merely lacks a convenience function for this, that is what we have 
to implement!
  >
  >
  > Using that alone results in a visual and functional regression of the 
original problem we were trying to fix (that file dialogs don't have a sensible 
default view):
  >
  > F6570515: Regression.png 
  >
  > If we go with this route, I would want to make sure we preserve the current 
behavior of other columns being right-aligned, and also make sure that the Name 
column has a good default width. If we can ensure that, I'm all for it!
  
  
  Lol :)
  It wasn't meant as a direct drop-in replacement for Rene's patch. KIO 
probably sets more on the headerview that isn't applied now (i assume). And if 
it doesn't then some things do need to be set to make it look nice.  It's 
merely meant to demonstrate that you can change the width of a column to 
whatever you desire while keeping the resizable feature and without the need to 
catch events and do magic there.
  
  Anyhow, you basically want a "stretchFirstColumn" while keeping. I named it 
"stretchColumn" but as it is now it only works if one column uses it. You need 
more complex layouting to get that to work if you want to stretch over multiple 
columns. But that's not the case here.
  Here it is. [1: header], [2: source], and a example on how to use it [3]. 
This obviously is far from done, but again only to demonstrate. If you like 
this approach i can put some time in it to make it less sensitive to errors.
  **be aware** to call "stretchColumn" after you show the dialog! Otherwise the 
sizes haven't been calculated yet. If you do exec, you can get around it by 
doing a QTimer::singleShot.
  
  [1] https://p.sc2.nl/H1S-2DomE
  [2] https://p.sc2.nl/ryQ2hwoX4
  [3] https://p.sc2.nl/Bybanvj7V

REPOSITORY
  R241 KIO

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

To: rjvbb, ngraham, #frameworks, #dolphin, apol, dfaure, ahartmetz, markg
Cc: markg, cfeck, dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, 
ngraham, bruns

D18380: KIO: make file dialog columns resizable again (and movable)

[Mark Gaiser]

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


  I too would quite like the to have resizeable columns back.
  But the approach you've chosen looks a little too complicated. Also, the fact 
that KDirOperatorDetailView::event needs to know about your custom QHeaderView 
is a code smell. Sometimes you do indeed need logic like that to make things 
work. But still, isn't there another way? Now the header and view are locked 
together. One doesn't work without the other.
  
  The thing that triggered me to comment wasn't any of that though. It's the 
"narrow mode". I cannot see how that wouldn't be seen as a bug by a user. With 
that little trick you're also overruling any font spacing settings the user 
might have had (in fontconfig) which is quite likely going to cause unexpected 
behavior and therefore bug reports.
  
  Now let's take a step back. What do we really want? We want:
  
  - Have the first column resizable and calculate the optimal size when the 
user had not resized the column yet.
  - Restore that setting if the user had manually resized it.
  
  In QHeaderView code, what we want is "QHeaderView::Interactive" [1] followed 
by QHeaderView::resizeSection [2]. Exactly as [1] described! QHeaderView merely 
lacks a convenience function for this, that is what we have to implement!
  I quickly threw this in a test project (minus the state saving) and i ended 
up with this fairly minimal QHeaderView subclass [3: header], [4: source] and 
this sample to see how you use it [5]. Note that there is one quirk in there in 
the source. The size calculation is not perfect and will fail on longer 
strings! That likely needs to be extended to take the style and margins into 
account. Anyhow, the goal of this example is to show a different approach with 
a far smaller code footprint and be far easier to debug. Use it as you please :)
  
  [1] http://doc.qt.io/qt-5/qheaderview.html#ResizeMode-enum
  [2] http://doc.qt.io/qt-5/qheaderview.html#resizeSection
  [3] https://p.sc2.nl/r1RJBLsQ4
  [4] https://p.sc2.nl/BJlMSLimN
  [5] https://p.sc2.nl/SJIYI8jQV

REPOSITORY
  R241 KIO

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

To: rjvbb, ngraham, #frameworks, #dolphin, apol, dfaure, ahartmetz, markg
Cc: markg, cfeck, dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, 
ngraham, bruns

D15071: Don't draw frames and shadows around images with transparency

[Mark Gaiser]

markg added a comment.


  (repost from D15069 )
  
  Hi Nate,
  
  I'm afraid this will give inconsistent frames, at least that will be the 
perception of the user.
  The hasAlpha function on a QPixmap (probably) boils down to executing this 
function:
  
bool QX11PlatformPixmap::hasAlphaChannel() const


   
{   


   
if (picture && d == 32) 


   
return true;


   



   
if (x11_mask && d == 1) 


   
return true;


   



   
return false;   


   
} 
  
  But there are quite some places in Qt where the alpha channel checks are done 
differently.The above would be for X11, it's likely different on wayland, etc...
  
  So where is this going to be inconsistent? Well, with images that "have" an 
alpha channel but don't use it.
  This happens when saving an image. It's often a setting to keep transparency 
or not (it is in photoshop and gimp).
  Therefore you can - and will - have people that have images with an alpha 
channel but don't use it so they don't get a frame. While the very same image 
when saved differently (and in png as well) will have a frame. That's going to 
be a bug report from that user i guess ;)
  
  Having said that, i'm much more in favor of an all-or-nothing approach. Not 
some logic somewhere that dictates when an image has a frame and when it 
doesn't.
  We've had frames for years so perhaps it's time to just flip the switch and 
see how users like it. So just flip that switch by default to no frames.
  
  - end of previous comment.
  
  As @ngraham figured out, Qt apparently does some more in depth transparency 
checking. I wonder if it does a pixel-by-pixel test, that would be expensive.
  Anyhow, i'd just say - to be consistent - to flip the switch. No frames at 
all anywhere. It looks just weird to me to have folders where some images have 
frames and some don't.
  For users it will be weird as well, they might even consider it a bug and 
report it.
  
  It would be a whole different story if the frames were part of a larger area 
(so including the filename), kinda like this 
. But it isn't. It's 
merely directly around the image.
  It probably (assumption) was a way to imitate Finder (of macOS) and the old 
"Windows Live Photo Gallery" specially the later one has a nearly 1-on-1 
matching with the frame Dolphin draws.

REPOSITORY
  R241 KIO

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

To: ngraham, #frameworks, #dolphin, #vdg, broulik, cfeck
Cc: markg, abetts, bruns, kde-frameworks-devel, michaelh, ngraham

D14893: [recentdocuments:/] Filter out files that can't be browsed with a file manager

[Mark Gaiser]

markg added a comment.


  It "looks" oke to me, but i don't know this code one bit.

INLINE COMMENTS

> recentdocuments.cpp:82-83
> +if (urlInside.scheme() == "recentdocuments"
> +// Filter out things that can't be viewed in a file 
> manager because they don't
> +// meet the user definition of a file for the purpose of 
> "recently accessed files"
> +|| !KProtocolManager::supportsListing(urlInside)

Comments inside a multi-line if statement! Unheard of!
Please move that to a more appropriate place.

REPOSITORY
  R320 KIO Extras

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

To: ngraham, broulik, #dolphin, #frameworks
Cc: markg

D13814: Speedup sort

[Mark Gaiser]

markg added a comment.


  In D13814#290512 , @bruns wrote:
  
  > The code looks fine now, but the summary is incorrect.
  >
  > The savings is not from using a lambda, but caused by initializing it once. 
If the old code had used `m_collator(other.m_collator)` in the copy 
constructor, construction would have been just a ref count increment and each 
of the following `m_collator.setFoo(...)` would have been noops (QCollator 
checks if the new value is different to its current value).
  >
  > Of course this would have triggered the QCollator bug as well.
  
  
  Is there even a need to have n QCollator objects?
  I'm talking about a QCollator _within_ a given view. Each view could 
obviously have it's own collator due to different view settings.

REPOSITORY
  R318 Dolphin

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

To: jtamate, #dolphin, #frameworks, markg, elvisangelaccio, bruns
Cc: elvisangelaccio, apol, bruns, markg, kfm-devel, spoorun, navarromorales, 
firef, andrebarros, emmanuelp

D13814: Speedup sort

[Mark Gaiser]

markg added a comment.


  Somehow i'm inclined to think that m_collator is wrong.
  But inspecting the code shows no issue as far as i can tell. It's a normal 
class member that lives as long as the KFileItemModel instance lives.
  
  Now here's a tricky part. KFileItemModel is new'ed for each view (and the 
folders panel) and as the sorting is (for strings) done with multiple threads.
  Perhaps there is a remote possibility of closing a view while sorting which 
will cause the lessThan lambda to have a dangling reference to m_collator and 
crash.
  
  That's just in theory though, i haven't been able to reproduce that. 
  But if that is true then you do have ways to create that dangling reference 
and thus crash while sorting.

INLINE COMMENTS

> kfileitemmodel.cpp:112
> +// warm up the collator so it does not has thread safety problems later
> +m_collator.compare(QStringLiteral(""), 
> QStringLiteral("bb"));
>  }

I don't know what @elvisangelaccio thinks about this, but i'm against it. This 
feels like a hacky workaround to me that should not be needed.

REPOSITORY
  R318 Dolphin

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

To: jtamate, #dolphin, #frameworks, markg, elvisangelaccio
Cc: elvisangelaccio, apol, bruns, markg, kfm-devel, spoorun, navarromorales, 
firef, andrebarros, emmanuelp

D13814: Speedup sort

[Mark Gaiser]

markg added a comment.


  In D13814#287959 , @jtamate wrote:
  
  > I really, really hate these things. 
  >  I've been testing this patch more than one month without any problem.
  >  And now, after pushing it, I got crashes everytime I start dolphin without 
parameters (user home).
  >  But not if I start it in / and keep changing directories, including user 
home.
  >
  > Please, test dolphin with latest sources.
  >
  > #6  0x7fc0ba4e5904 in 
icu_61_1::RuleBasedCollator::getAttribute(UColAttribute, UErrorCode&) const () 
from /usr/lib64/libicui18n.so.61.1
  >  #7  0x7fc0ba4e6dc6 in 
icu_61_1::RuleBasedCollator::setAttribute(UColAttribute, UColAttributeValue, 
UErrorCode&) () from /usr/lib64/libicui18n.so.61.1
  >  #8  0x7fc0c0dfec25 in QCollatorPrivate::init() () at 
tools/qcollator_icu.cpp:82
  >  #9  0x7fc0c0dfedab in QCollator::compare (this=0x1928070, 
s1=0x1bd6b98, len1=19, s2=0x1d70af8, len2=17) at tools/qcollator_icu.cpp:109
  >  #10 0x7fc0c880ac30 in KFileItemModel::sortRoleCompare 
(this=this@entry=0x1928040, a=a@entry=0x1c0e1f0, b=b@entry=0x1cfbdf0, 
collator=...) at 
/g/5kde/kde/applications/dolphin/src/kitemviews/kfileitemmodel.cpp:1836
  >  #11 0x7fc0c880b2a2 in KFileItemModel::lessThan 
(this=this@entry=0x1928040, a=0x1c0e1f0, b=0x1cfbdf0, collator=...) at 
/g/5kde/kde/applications/dolphin/src/kitemviews/kfileitemmodel.cpp:1707
  >  #12 0x7fc0c8811715 in KFileItemModeloperator() 
(__closure=0x7ffe8d4307a8, b=, a=) at 
/g/5kde/kde/applications/dolphin/src/kitemviews/kfileitemmodel.cpp:1717
  
  
  I just cloned dolphin with your changes.
  It compiled just fine.
  Starting it with and without arguments also worked just fine.
  
  Perhaps it's a local issue on your side? Have you tried cleaning your dolphin 
build (and perhaps KIO as well) and re-run it to see if it still happens?

REPOSITORY
  R318 Dolphin

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

To: jtamate, #dolphin, #frameworks, markg, elvisangelaccio
Cc: elvisangelaccio, apol, bruns, markg, kfm-devel, spoorun, navarromorales, 
firef, andrebarros, emmanuelp

D13124: Add Share action to Dolphin context menu

[Mark Gaiser]

markg added a comment.


  In D13124#286263 , 
@elvisangelaccio wrote:
  
  > In D13124#286115 , @broulik 
wrote:
  >
  > > Check out `KFileItemActions::addServiceActionsTo`
  >
  >
  > Actually, `KFileItemActions::addPluginActionsTo` ;)
  
  
  Thank you both :)
  After looking at that code (KFileItemActions::addPluginActionsTo that is), 
it's clear to me where and how it happens. That's an interesting approach 
Dolphin does there with calls to KFileItemActions. It fooled me as it's a local 
variable.
  
  What i still don't get is how "Share" appears below "Move To" as the call to 
add the "Share" menu is done before the "Move To" menu is added. Which also 
fooled me as "Move To" is added using the this pointer.
  The only reasoning i can think of is some sorting of menu items being done 
afterwards (which i don't see...) as the menu appears to be sorted 
alphabetically.

REPOSITORY
  R495 Purpose Library

BRANCH
  fiap

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

To: nicolasfella, apol, #dolphin, elvisangelaccio
Cc: markg, broulik, kde-frameworks-devel, elvisangelaccio, ngraham, apol, 
kfm-devel, #dolphin, michaelh, spoorun, navarromorales, isidorov, firef, 
andrebarros, bruns, emmanuelp

D13124: Add Share action to Dolphin context menu

[Mark Gaiser]

markg added a comment.


  This looks great! 
  But i don't get how it is magically included in Dolphin.. Perhaps someone 
could explain?
  I get that it's a fileitemaction plugin and that it's being loaded by dolphin 
(somehow), i take that for granted.
  But even so, if i look in the dolphin code that builds up the context menu 
for an item: 
https://cgit.kde.org/dolphin.git/tree/src/dolphincontextmenu.cpp#n307 i see 
absolutely nothing that can be loading this plugin.
  And as this plugin isn't touching that code at all, how does it do that?
  
  The line linked above adds the "Properties" entry. Right above that is "Move 
To" and "Copy To" submenus. There is nothing in between that loads plugins.. 
Yup, i'm really confused as i just don't see how this works.
  
  Someone? :)

REPOSITORY
  R495 Purpose Library

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

To: nicolasfella, apol
Cc: markg, broulik, kde-frameworks-devel, elvisangelaccio, ngraham, apol, 
kfm-devel, #dolphin, michaelh, spoorun, navarromorales, isidorov, firef, 
andrebarros, bruns, emmanuelp

D13814: Speedup sort

[Mark Gaiser]

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


  2x +1 = +2
  Ship it :)

REPOSITORY
  R318 Dolphin

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

To: jtamate, #dolphin, #frameworks, markg
Cc: apol, bruns, markg, kfm-devel, spoorun, navarromorales, isidorov, firef, 
andrebarros, emmanuelp

D13814: Speedup sort

[Mark Gaiser]

markg added a comment.


  In D13814#285686 , @bruns wrote:
  
  > In D13814#285660 , @markg wrote:
  >
  > > > I don't know if that's still an issue or if your patch re-introduces 
whatever the problem was (race conditions?). You could look back in the commit 
log when that was added to figure out more about it.
  > >
  > > To answer that myself, it was done in this commit: 
https://cgit.kde.org/dolphin.git/commit/src/kitemviews/kfileitemmodel.cpp?id=d9680ead8099df9a2b06bfed61a62923778996f2
  > >  And doesn't explain anything :)
  >
  >
  > If you follow the link in the comment, i.e. 
https://bugs.kde.org/show_bug.cgi?id=312679, it mentions date sorting and 
KDateTime being non-reentrant.
  
  
  That would be really interesting as KDateTime isn't used anywhere in Dolphin 
anymore (according to my grep on the source). It's all QDateTime now and that 
is all reentrant according to the Qt docs!
  In other words, the fix for that can probably be removed now.
  
  Just an assumption though. I haven't tried anything.

REPOSITORY
  R318 Dolphin

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

To: jtamate, #dolphin, #frameworks
Cc: bruns, markg, kfm-devel, spoorun, navarromorales, isidorov, firef, 
andrebarros, emmanuelp

D13814: Speedup sort

[Mark Gaiser]

markg added a comment.


  > I don't know if that's still an issue or if your patch re-introduces 
whatever the problem was (race conditions?). You could look back in the commit 
log when that was added to figure out more about it.
  
  To answer that myself, it was done in this commit: 
https://cgit.kde.org/dolphin.git/commit/src/kitemviews/kfileitemmodel.cpp?id=d9680ead8099df9a2b06bfed61a62923778996f2
  And doesn't explain anything :)

REPOSITORY
  R318 Dolphin

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

To: jtamate, #dolphin, #frameworks
Cc: markg, kfm-devel, spoorun, navarromorales, isidorov, firef, andrebarros, 
emmanuelp

D13814: Speedup sort

[Mark Gaiser]

markg added a comment.


  +1
  
  Great! :) Back to the good old fast performance it once had!
  Please do get rid of the underscore before the name. Nothing (afaict) does 
that in Dolphin, lets not introduce it. Just lessThan is fine.
  
  Also, note that this was done for a reason. I think it was the one in the 
comment: "non-reentrant comparison functions" which is why the roles other then 
NameRole are using single-threaded sort.
  I don't know if that's still an issue or if your patch re-introduces whatever 
the problem was (race conditions?). You could look back in the commit log when 
that was added to figure out more about it.

REPOSITORY
  R318 Dolphin

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

To: jtamate, #dolphin, #frameworks
Cc: markg, kfm-devel, spoorun, navarromorales, isidorov, firef, andrebarros, 
emmanuelp

D13211: Enable comparing KFileItems by url

[Mark Gaiser]

markg added a comment.


  While this works, there is a newer en better way for it.
  It's new in C++14 and called "transparent compare".
  In "this" case it won't change the resulting code of how you compare, but it 
might be worth checking that out.
  
  Read this: https://www.fluentcpp.com/2017/06/09/search-set-another-type-key/
  And watch this: https://www.youtube.com/watch?v=BBUacofxOP8
  
  those will explain how to use it.

REPOSITORY
  R241 KIO

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

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

D13358: Add new class that is a model of numbers between two values

[Mark Gaiser]

markg added a comment.


  This basically is the equivalent of the C++ range iterator 
(https://github.com/ryanhaining/cppitertools#range, but more libraries have a 
"range" iterator like that).
  
  In all the documentation blocks you miss the argument and return value 
documentation.
  
  I'm curious though, in which situation did you need a model like this?
  A situation i can think of is where you would always want to show x number of 
items (say for instance a top 10 downloads) and just leve the entries blank 
(but have the index numbers) if there isn't enough to fill the top 10.

INLINE COMMENTS

> knumbermodel.cpp:24
> +#include 
> +#include 
> +

QDebug is unused.

> knumbermodel.cpp:34
> +qreal step = 1.0;
> +QLocale::NumberOptions  formattingOptions = 
> QLocale::DefaultNumberOptions;
> +};

Drop one space..

> knumbermodel.cpp:114
> +{
> +return d->min + d->step * index.row();
> +}

This is only guaranteed to be a valid index if the callers call it with a valid 
index.
Better be safe then sorry and check for isValid().

> knumbermodel.cpp:121
> +}
> +return std::max(0.0, std::floor((d->max - d->min) / d->step)) + 1;
> +}

Won't this give compile warnings? It's double and int foo mixed.
Also, the +1 should probably be "+ d->step"

> knumbermodel.cpp:140-143
> +QHash roleNames;
> +roleNames[Display] = QByteArrayLiteral("display");
> +roleNames[Value] = QByteArrayLiteral("value");
> +return roleNames;

This can be compile-time generated.

REPOSITORY
  R275 KItemModels

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

To: davidedmundson
Cc: markg, kde-frameworks-devel, michaelh, ngraham, bruns

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

[Mark Gaiser]

markg added a comment.


  In D10702#273040 , @meven wrote:
  
  > Great suggestion Mark !
  >
  > I am a C++ beginner, I did not consider this neat C++ 14 feature.
  >
  > This will necessitate a c++ compiler dependency change though.
  >  Like Kwin did last July 
https://github.com/KDE/kwin/commit/ea5d611de1bc33869c13c27d75a7827201a5139d
  
  
  Well, it's nearly all C++11 :)
  Only the "300ms" (specifically the "ms") in there is new in C++14 (chrono 
literals: https://en.cppreference.com/w/cpp/chrono/duration). In this case 
removing the ms would make it C++11 and probably work (i haven't tested that).
  You could make it all in Qt (QThread, QFuture, ...). I'm just not that used 
to those classes en went for the STL ones instead.
  
  > 
  > 
  >>   That in it's own is slightly different to what the code currently does. 
Currently it calls slotReport after every 300 files. With this it would call 
slotReport after every 300ms. I don't think that's much of a problem.
  > 
  > I think a time based update would make more sense to the user.
  > 
  > I think deleteFiles and deleteDirs should both be wrapped in the async 
function.
  >  Otherwise, at best we would end up with multiple parallel file deletion 
which is not preferable (given current filesystems and hardware, we should 
favor sequential deletion) and at worst the same as today blocking the main 
thread.
  >  Or we might need some mutex/buffer to synchronize the unlink syscalls 
through Qt::remove() between different async deletion functions.
  > 
  > So this plus the added necessary synchronizing code, this might end up a 
big code change.
  
  Take care with modifying those class members though. You'd probably want to 
protect them with a mutex. Even though this code probably won't cause a race 
condition, better be safe then sorry :)
  
  > I will give a spin.

REPOSITORY
  R241 KIO

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

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

D13124: [RFC] Add Share action to Dolphin context menu

[Mark Gaiser]

markg added a comment.


  In D13124#272698 , @nicolasfella 
wrote:
  
  > Friendly ping?
  
  
  ?

REPOSITORY
  R495 Purpose Library

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

To: nicolasfella, #dolphin, apol, elvisangelaccio
Cc: markg, broulik, kde-frameworks-devel, elvisangelaccio, ngraham, apol, 
kfm-devel, #dolphin, michaelh, spoorun, navarromorales, isidorov, firef, 
andrebarros, bruns, emmanuelp

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

[Mark Gaiser]

markg added a comment.


  In D10702#272901 , @meven wrote:
  
  > Here is the script I have been using : 
https://gist.github.com/meven/f0b2a36c61240e1d6e19753afd1d3d68
  >
  > My benchmark logic is :
  >  Create a folder with x files of k sizes.
  >  Copy this folder.
  >  Delete this folder using kfmclient, measuring the elapsed time
  >
  > I have used 10 files of 1 byte, 10 of 1kb, 1000 of 3mb, 3 of 1Gb.
  >
  > I ran this on a 2TB hard disc drive.
  >
  > Here are the very limited results:
  >
  >   In seconds
  > avg
  >   before
  >   1b1,586   1,738   1,684   1,669333
  >   1k1,719   1,777   1,649   1,715
  >   3mb   9,206   9,164   8,419   8,929667
  >   1gb   30,736  20,559  29,981  27,092
  >  
  >   after
  >   1b4,637   1,721   1,599   2,652333
  >   1k32,186  1,726   1,685   11,86567
  >   3mb   2,491   7,287   7,896   5,891333
  >   1gb   1,464   13,344  17,271  10,693
  >  
  >
  >
  > It appears my benchmark methodology is mostly of no use due to huge 
outliners values.
  >  I am using kfmclient, but more than time I would need to measure the 
memory overhead also of using the kioslave instead of the fast-path but this 
could be tricky with cross-process execution.
  >
  > Also I think that if my patch was to get through we nay need to treat as a 
signature change: the behavior of the KIO::DeleteJob would change quite a bit 
from being most of the time synchronous to being asynchronous.
  >  Some App may have built on the assumption (knowingly or not) that the 
function only returns after the file(s) have been removed, which would not the 
case after this patch.
  >  An opt-in boolean option could be needed to trigger the new behavior while 
keeping the old one for applications that have been updated/reviewed yet and 
perhaps mark as deprecated the old behavior.
  >
  > Also I would like to take the time to add some tests, although I need to 
learn about how to write some and a .
  >
  > I would much apprieciate feedback.
  >  As the solution is not obvious and still in debatable to me, we could set 
up some IRC meeting. I will be hanging on #kde-fm.
  
  
  That downside is not a solution.
  Specifically with large folders, you'd really like to see some form of 
progression as it's being deleted. I would.
  
  Please take a good close look at my updated example of how to make this async:
  https://p.sc2.nl/ByvSb_-lQ
  I've added comments everywhere in there to explain how the code works and 
what needs to be done to use it in here.
  Just run it (you need C++14 and Qt) to see how it looks. (my qmake file 
incase you want it: https://p.sc2.nl/Hy25fO-xm)
  
  It keeps it on the client process (the performance optimization of David 
Faure) but offloads it to a different thread.
  I've extended the example to show the number of files that have been deleted 
every 300ms.
  That in it's own is slightly different to what the code currently does. 
Currently it calls slotReport after every 300 files. With this it would call 
slotReport after every 300ms. I don't think that's much of a problem.
  
  I can try and make a KIO patch for this if you want. As i now have the logic 
in that example code. (i just don't have a working frameworks dev environment 
at the moment)

REPOSITORY
  R241 KIO

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

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

D12218: Remove Reload button from the file dialogs' toolbar

[Mark Gaiser]

markg added a comment.


  In D12218#271375 , @rkflx wrote:
  
  > In D12218#271290 , 
@davidedmundson wrote:
  >
  > > > It can be useful for network views, but those are a minority of use 
cases
  > >
  > > Every large scale KDE deployment (from my POV by far our most important 
userbase) has a network share.
  > >
  > > It might not be needed often, but when it's needed it's needed.
  >
  >
  > Would you be okay with some of the compromises mentioned above, or is this 
again an all-or-nothing thing?
  >
  > - Show toolbar button only for network folders.
  > - Move Reload to `KUrlNavigator`.
  
  
  The problem would then be the detection of a network share.
  I can mount a cifs share locally, you'd not know the difference.
  Same with any filesystem. They can all become local. And - frankly - with SMB 
it has some performance benefits to mount it locally with cifs.
  
  You could go for the "isSlow" check (smb is included in there iirc), but it 
would be a quite weak check.
  I'd just say no to this. Reload needs to stay imho.

REPOSITORY
  R241 KIO

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

To: ngraham, #frameworks, davidedmundson
Cc: davidedmundson, kde-frameworks-devel, abetts, markg, broulik, rkflx, 
#dolphin, michaelh, ngraham, bruns

D12765: Use a more intuitive, user-friendly name for Samba share access

[Mark Gaiser]

markg added a comment.


  In D12765#260054 , @ngraham wrote:
  
  > Which ones, and where do they live?
  
  
  I would have expected it for NFS (same repository), but it apparently doesn't 
have a .desktop file at all.
  FTP (lives in the mina kio repo in the ioslaves folder) could have been a 
possibility but also doesn't have a .desktop file.
  
  So no others i suppose.

REPOSITORY
  R320 KIO Extras

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

To: ngraham, #frameworks, #dolphin, #vdg, apol, markg
Cc: markg, ltoscano, elvisangelaccio

D12765: Use a more intuitive, user-friendly name for Samba share access

[Mark Gaiser]

markg accepted this revision.
markg added a comment.


  Please do note that you have more protocols with shared folders that likely 
deserve a name change in the same fashion.

REPOSITORY
  R320 KIO Extras

BRANCH
  friendlier-samba-share-name (branched from master)

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

To: ngraham, #frameworks, #dolphin, #vdg, apol, markg
Cc: markg, ltoscano, elvisangelaccio

D12321: Hide file preview when icon is too small

[Mark Gaiser]

markg resigned from this revision.
markg added a comment.
This revision is now accepted and ready to land.


  I guess it's time for me to resign from this one.
  Good luck folks :)

REPOSITORY
  R241 KIO

BRANCH
  conditional_preview (branched from master)

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

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin
Cc: elvisangelaccio, markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, 
michaelh, bruns

D12321: Hide file preview when icon is too small

[Mark Gaiser]

markg added a comment.


  In D12321#253839 , @rkflx wrote:
  
  > @markg I just read the whole thing again. As far as I can see, your main 
concerns were:
  >
  > - Not being able to show previews for icon sets of small PNG files.
  
  
  I initially said nothing about PNG. You assumed SVG so i explicitly said PNG, 
but it's any non-raster format. Don't make this format specific.
  
  > - Confused users when an option is not available in some situations.
  > 
  >   For the first point, we were able to show that small PNG files can be 
shown as before. For the second point, we brought an example where this is 
already the case, with no confused users hunting us on Bugzilla. Various other 
questions also turned out to be non-issues (e.g. HiDPI support, remembering the 
user-set value etc.).
  
  And i have to admit that i was testing the wrong thing.
  If i take a grid view (in dolphin) and do that on a folder with lots of small 
icons i see them being resized.
  Apparently the behavior in the file open dialog is different in this regard.
  So i guess i'm 50% wrong there ;)
  
  > Furthermore we have explained why your proposed solution to make this 
configurable is not a sensible path forward.
  
  I know and i thank you (and the rest) for the constructive discussion!
  
  > We can still keep this Diff open for discussion for a couple of days, but 
at some point we'll have to make some progress. Your comments were really 
helpful in making us reflect even more use cases and situations, but in the end 
it turned out the patch should be able to handle all that just fine. Please let 
us know how to go forward from here.
  > 
  > In any case we plan to also discuss this with #Dolphin 
 before landing.
  
  Please let me stress that i'm very much **in favor** of having this! No 
mistake about that.
  I'm just quite firmly against a "in your face" icon changing state 
"automagically". If i press "show previews" then i expect them to be shown, 
however small they are. It's a choice i made and apparently want to have, A 
3-state button is imho still the way to go. It could for instance be a button 
with a diagonal line in it where half of the button is disabled and the other 
half is enabled. Quite graphically showing that "both" states are active (the 
auto state, the view knows best).
  
  Anyhow, i don't think we're going to agree on this. That's fine :)
  You folks have the vision and you've been doing a great job thus far so i 
ultimately have no reason to stop you from making progress, even if i don't 
like it.
  I will resign from this review in a few days to let it progress to wherever 
it wants to go.

REPOSITORY
  R241 KIO

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

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: elvisangelaccio, markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, 
michaelh, bruns

D12321: Hide file preview when icon is too small

[Mark Gaiser]

markg added a comment.


  In D12321#252393 , @rkflx wrote:
  
  > In D12321#252374 , @markg wrote:
  >
  > > The blown up way of looking at it (resized version) does not give you an 
accurate representation of how it will look like in the native size.
  >
  >
  > Have you actually tried the patch? Raster icons are not blown up, they are 
shown at their native size as long as it is smaller than the grid spacing (and 
the grid spacing is large enough to enable showing of previews).
  
  
  I've not tried this patch nor the svg patch.
  The perfect video of this patch clearly shows me what i need to see (big pros 
for that btw!).
  The svg patch unlikely influences my png icons ;)
  
  My comment was based on what i see in an icon folder right in front of me at 
this very moment.
  16x16 icons are scaled to whatever the zoom level is (which is fine imho). 
These icons are png ones.

REPOSITORY
  R241 KIO

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

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Mark Gaiser]

markg added a comment.


  In D12321#252366 , @ngraham wrote:
  
  > In D12321#252365 , @markg wrote:
  >
  > > > Browse... grid view... small size... previews... icons... so you want 
to be able to do this?
  > > > 
  > > > F5819296: Small, not useful.png 
  > >
  > > Sometimes, yes.
  > >  Like when looking for which icon to use for another button in the 
application i'm making for my job for instance
  >
  >
  > Are such tiny icons actually useful and distinguishable? Wouldn't it 
actually improve your productivity to increase the size of the icons and the 
window, such that you saw this instead?
  >
  > F5819375: Large, more useful.png 
  >
  > Why torture yourself with tiny icons in a tiny window?
  
  
  Not always. Sometimes you want to see how they look at the actual size. How 
they are represented.
  The blown up way of looking at it (resized version) does not give you an 
accurate representation of how it will look like in the native size. Granted, 
this is very developer specific and what i usually do is first look at the 
blown up version. Then single out a few i might like followed by looking at the 
real sizes to determine which one to use. But regardless of that, it's great to 
be able to do that, i see no reason why we should lose that ability.

REPOSITORY
  R241 KIO

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

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Mark Gaiser]

markg added a comment.


  In D12321#252369 , @rkflx wrote:
  
  > In D12321#252337 , @markg wrote:
  >
  > > In those cases where you just browse through a gazillion icons (nothing 
with an icon picker or selecting icons, i didn't say any of that) becomes 
impossible in your future patch.
  > >  This patch makes it slightly more "inconvenient" to browse folders like 
that.
  >
  >
  > What "future patch" are you referring to, though?
  >
  > As far as I've understood, we don't plan to prevent people from viewing 
thumbnails for tiny icons anywhere (as long as they select a reasonable/medium 
grid size).
  >
  > In fact, with D12306: Improve grid icon layout in filepicker dialog 
 there probably won't be much of a 
difference between slider position with regard to grid spacing anyway, as "this 
patch improves the grid spacing in icons-on-top mode by making it looser for 
small icons", to give more room for the filename label.
  
  
  You said:
  
  > This automatic logic is already there. You are simply objecting to not 
allowing previews for small icons. Yes, we will remove this possibility which 
was there before.
  
  Which i interpreted as "previews for small images/icons won't be possible 
anymore in the future".
  Did i interpret that wrong?

REPOSITORY
  R241 KIO

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

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Mark Gaiser]

markg added a comment.


  In D12321#252346 , @ngraham wrote:
  
  > In D12321#252337 , @markg wrote:
  >
  > > In D12321#252299 , @rkflx 
wrote:
  > >
  > > > > Then you make it impossible (ultimately, not with this patch though) 
to for instance browse through folders with small icons (say icon sets).
  > > >
  > > > We have an explicit icon chooser dialog for this task, using the file 
dialog is not the recommended way for apps to select an icon. Also, as you may 
have noticed, the file dialog does not show previews of SVGs for any size, 
making your point moot.
  > > >
  > > > As for choosing PNG icons, you can simply set the icon size large 
enough (38px for me) to see the previews anyway. This workaround is good 
enough, and should not prevent us from improving the handling for all other 
situations. Let's be honest here: How often do you want to select icons with 
<38px size, but cannot increase the size temporarily?
  > >
  > >
  > > I think you mis-interpreted what i said which then caused @ngraham to 
reply with apparently that in mind which is also not as i intended :)
  > >
  > > I intended specifically what i said.
  > >  **browse** a folder with lots of icons **like** an icon pack/theme. And 
yes, that is - sometimes - very handy to have!
  > >  I said nothing about the view mode (i meant the grid view though, not 
the list view).
  >
  >
  > Browse... grid view... small size... previews... icons... so you want to be 
able to do this?
  >
  > F5819296: Small, not useful.png 
  
  
  Sometimes, yes.
  Like when looking for which icon to use for another button in the application 
i'm making for my job for instance

REPOSITORY
  R241 KIO

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

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Mark Gaiser]

markg added a comment.


  In D12321#252299 , @rkflx wrote:
  
  > > Then you make it impossible (ultimately, not with this patch though) to 
for instance browse through folders with small icons (say icon sets).
  >
  > We have an explicit icon chooser dialog for this task, using the file 
dialog is not the recommended way for apps to select an icon. Also, as you may 
have noticed, the file dialog does not show previews of SVGs for any size, 
making your point moot.
  >
  > As for choosing PNG icons, you can simply set the icon size large enough 
(38px for me) to see the previews anyway. This workaround is good enough, and 
should not prevent us from improving the handling for all other situations. 
Let's be honest here: How often do you want to select icons with <38px size, 
but cannot increase the size temporarily?
  
  
  I think you mis-interpreted what i said which then caused @ngraham to reply 
with apparently that in mind which is also not as i intended :)
  
  I intended specifically what i said.
  **browse** a folder with lots of icons **like** an icon pack/theme. And yes, 
that is - sometimes - very handy to have!
  I said nothing about the view mode (i meant the grid view though, not the 
list view).
  
  In those cases where you just browse through a gazillion icons (nothing with 
an icon picker or selecting icons, i didn't say any of that) becomes impossible 
in your future patch.
  This patch makes it slightly more "inconvenient" to browse folders like that.
  
  As for SVG.. Not my words. png, jpg, any format really.

REPOSITORY
  R241 KIO

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

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Mark Gaiser]

markg added a comment.


  In D12321#252285 , @rkflx wrote:
  
  > In D12321#252283 , @markg wrote:
  >
  > > In D12321#252282 , @rkflx 
wrote:
  > >
  > > > In D12321#252281 , @markg 
wrote:
  > > >
  > > > > I agree but not for a button that the user controls.
  > > >
  > > >
  > > > We already disable options where they do not make sense, see the Icon 
position setting which is only available in select view modes.
  > > >
  > > > The ability to preview could similarly be disabled where it does not 
make sense. I highly doubt this will cause confusion for users.
  > >
  > >
  > > Ohh, but i'm not against that :)
  > >  I merely don't want one button to be changed by both the user and some 
code logic.
  >
  >
  > It's the same for Icon position: Even if the user does not change from 
Above filename to Next to filename the code logic when changing the radio 
button (which would equal moving the zoom slider) to another view mode will 
change the icon position anyway, because for Detailed View you cannot have 
icons above the filename.
  >
  > This automatic logic is already there. You are simply objecting to not 
allowing previews for small icons. Yes, we will remove this possibility which 
was there before. No, I don't think this is bad, unless you can come up with 
reasons why anyone would want totally undecipherable previews.
  
  
  Then you make it impossible (ultimately, not with this patch though) to for 
instance browse through folders with small icons (say icon sets).
  That cannot possibly be desired...
  
  What file browser should people then use to view small icons?
  Or should they just never do that and be forced to use gwenview (or some 
alternative)?

REPOSITORY
  R241 KIO

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

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Mark Gaiser]

markg added a comment.


  In D12321#252282 , @rkflx wrote:
  
  > In D12321#252281 , @markg wrote:
  >
  > > I agree but not for a button that the user controls.
  >
  >
  > We already disable options where they do not make sense, see the Icon 
position setting which is only available in select view modes.
  >
  > The ability to preview could similarly be disabled where it does not make 
sense. I highly doubt this will cause confusion for users.
  
  
  Ohh, but i'm not against that :)
  I merely don't want one button to be changed by both the user and some code 
logic.

REPOSITORY
  R241 KIO

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

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Mark Gaiser]

markg added a comment.


  In D12321#252264 , @rkflx wrote:
  
  > In D12321#252261 , @markg wrote:
  >
  > > That is unexpected behavior.
  >
  >
  > I disagree, adapting the interface dynamically is good design.
  
  
  I agree but not for a button that the user controls.
  It should either be a tri-state or a "auto preview" button.
  
  You should never change states that users had set explicitly (at least, 
that's my opinion).
  It will only cause confusion and becomes a nightmare when saving settings as 
you then already need to maintain if a button was changed by the user or code 
logic.
  In fact, the current patch already adds class-wide bools to check for this 
thus internally it already needs a tri state.
  
  > 
  > 
  >> A tri-state would prevent that.
  >>  For the record, if it becomes a tri-state i would set it to auto (this 
implementation, the view knows best) by default.
  > 
  > Suppose previews are on. Now the user clicks on the button, wanting to turn 
previews off. Sadly, nothing will happen, because the user just switched into 
"automatic" mode. That's not something we should implement!
  > 
  > Besides that, `tristate` is only a property of `QCheckBox`, but not of 
`QToolButton`. (And see also confusion here: 
https://www.reddit.com/r/kde/comments/8c5ael/there_are_several_settings_in_kde_that_have_a/.)
  > 
  > Thus this would need to be a separate config option, making everything even 
more complicated. I don't get why you want to see those tiny previews? Or is it 
simply out of principle that you want to control the thumbnails, even if they 
are not useful?
  
  With a two-state button you either give me control or you don't, anything in 
between is potentially unexpected behavior.
  If you want smarter ways then two-states then it should either be represented 
in a button that can handle it (a tri-state) or a new button altogether with 
the downside that you get two (at first sight seemingly the same) buttons.
  
  As for the vision, i'm perfectly fine with that :)
  Just as long as it doesn't cause unexpected behavior which i think this one 
as implemented now will cause. That doesn't make the vision wrong, it merely 
means the technical execution of some parts might need a little tweaking.

REPOSITORY
  R241 KIO

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

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Mark Gaiser]

markg added a comment.


  In D12321#252259 , @anemeth wrote:
  
  > In D12321#252238 , @markg wrote:
  >
  > > You are overwriting a setting that the user had explicitly set (show 
preview). That will result in a "huh, why is the preview off all of a sudden?" 
responses which will lead to bug reports.
  >
  >
  > That's why the tooltip changes when that is the case.
  
  
  Let me rephrase it then :)
  
  The user **explicitly** clicks on the preview button. If the user then zooms 
in/out that same preview button might change to what the view thinks is best.
  That is unexpected behavior.
  
  A tri-state would prevent that.
  For the record, if it becomes a tri-state i would set it to auto (this 
implementation, the view knows best) by default.

REPOSITORY
  R241 KIO

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

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Mark Gaiser]

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


  That looks really fancy! :)
  Yet i have to give it a -1..
  
  You are overwriting a setting that the user had explicitly set (show 
preview). That will result in a "huh, why is the preview off all of a sudden?" 
responses which will lead to bug reports.
  I would consider it a bug if the view is going to decide for me when i want 
to see previews and when not.
  
  Also, i really would not base this logic on fonts. I can have smaller fonts 
just because i might like that (or bigger, same argument) which would then 
determine when the preview is switched off.
  I get that this is a tricky area, but it would be much better if you can 
somehow determine if the preview is going to be smaller then - lets say - 1 by 
1 centimeters (in physical size, therefore you need to know the scaling ratio 
and the dpi) or in inch it would be 0.39 by 0.39. That would be a more logical 
decision to turn on/off previews.
  
  This logic breaks in really high resolution displays (take smartphones for 
example) where a 1x1 cm block can (and should!) easily show you an image. But 
lets lets consider that a out-of-scope issue as dolphin isn't on the smartphone 
(that i'm aware of), just something that might need to be considered in the 
future.
  
  That being said, having an "auto preview" (aka, the view knows best, your 
implementation) would be awesome! But not as overridden user setting.
  Perhaps the preview button needs to become a tri-state button:
  
  - Off
  - On (always a preview, the user knows best)
  - Auto (the view decides when you see a preview)
  
  I have no clue how a tri-state button would look in dolphin though :) Or even 
if the Breeze theme has a graphical representation for it.

REPOSITORY
  R241 KIO

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

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D11768: Add Desktop and Downloads to the default list of Places

[Mark Gaiser]

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


  Blocking it any longer seems rude to me :)

REPOSITORY
  R241 KIO

BRANCH
  add-desktop-and-downloads (branched from master)

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

To: ngraham, #dolphin, #gwenview, #frameworks, #vdg, markg, progwolff
Cc: abetts, huoni, markg, fabiank, progwolff, broulik, michaelh, bruns

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

[Mark Gaiser]

markg added a comment.


  In D12337#249814 , @rkflx wrote:
  
  > In D12337#249528 , @rkflx wrote:
  >
  > > F5812548: KIO-toolbar-sort-button.png 

  >
  >
  > @broulik  @markg Do you like the screenshot above better? The button does 
not float in the middle, and takes less space.
  >
  > @andreaska Any tips for a better sort icon? Would it be too much to ask for 
a new one? (Our current icons often contain characters or a sorting direction, 
while here we'd need a more generic icon describing a menu containing various 
sorting options.)
  
  
  Well, the icon certainly is better and without text is much better as well, 
but i still don't like (nor see the value) of having this sort option that 
prominent "in your face".
  You can sort right now via:
  
  - RMB on the list -> Sorting -> take your pick
  - Settings icon -> Sorting -> take your pick
  - one-click sorting on the headers (if you open a view with headers)
  
  You think we need a **fourth** option?
  
  I did also just notice that the arrow icon in a header is visible when the 
view is sorted on that column.
  On top of that, the default view (when you don't change anything as a user) 
is a the detailed view (thus with a visible sort indicator) and one-click 
sorting on the headers.
  You can't satisfy all users, in "my" opinion the current way (without this 
patch) is fine.
  
  Just because the space is there now doesn't mean the it should be filled with 
icons ;)

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

[Mark Gaiser]

markg added a comment.


  In D12337#249784 , @ngraham wrote:
  
  > In D12337#249782 , @markg wrote:
  >
  > > Sort already is "right in front" of the user. They merely have to click 
the column headers.
  >
  >
  > ...Only if the view has visible columns. Short View and Tree View don't.
  
  
  Right, then it should be view dependent.
  For the views you posted screenshots of (detailed view) the button would be a 
waste of space.
  To me this sounds more useful as a right mouse button menu (Sort by) on views 
that don't have header names. In a tree view that also allows for fine grained 
sorting control (aka, only sort the tree node you clicked on)

REPOSITORY
  R241 KIO

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

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

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

[Mark Gaiser]

markg added a comment.


  Sort already is "right in front" of the user. They merely have to click the 
column headers.
  These column headers used to (before the breeze theme, i think the air theme 
had it) show arrows indicating if something was sorted in ascending or 
descending.
  I don't know why that's gone but that was the universal way to immediately 
know which column was sorted and in which order. No need to add a button for 
that.
  
  If the view needs to stay as clean as it is now then you could only show the 
arrows in the headers when you hover the header (or view) or when it has focus 
and let them disappear when it loses focus.

REPOSITORY
  R241 KIO

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

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

D12218: Remove Reload button from the file dialogs' toolbar

[Mark Gaiser]

markg added a comment.


  In D12218#247481 , @ngraham wrote:
  
  > In D12218#247400 , @markg wrote:
  >
  > > As i said, it's all relative. I barely use any of the buttons but the one 
i do use is refresh.
  > >  I only need it when i'm impatient (for instance when wanting to click on 
a file that is still being copied)
  >
  >
  > Not really a valid use case; KDirWatcher should update the view for you 
automatically, and there's no practical benefit to mashing the reload button. 
The button isn't there to facilitate OCD. :) To do that, just hit [F5]. :)
  
  
  Not valid? I'd remove all those buttons on the left side except for refresh :)
  
  > 
  > 
  >> or when i'm browsing a slow network drive.
  > 
  > The Reload functionality is still available via the [F5] as well as the 
context menu. Are those not enough for this use case? And how common is this, 
really? I'm not sure that "file manipulation from the open/save dialog on a 
slow network drive" is a common enough use case to justify taking up space in 
an extremely constrained UI to show a button for reloading the view, especially 
once that functionality is available via another GUI method too (it's always 
available via [F5].
  > 
  > Ultimately the pressure here will be reduced by making the toolbar 
editable, so people who really want a visible Reload button can have one. But 
until then, we need to ask ourselves whether the //general user// benefits from 
having this button always visible, more than he or she might benefit from 
having something or more general usefulness visible in the toolbar (e.g. a 
dropdown menu button that exposes sorting options).
  
  I find refresh (much) more important then the preview toggle (that should 
imho be under the settings thingy, not as it's own button).
  Removing refresh is a mistake in my opinion.
  Note that F5  is for some users FN + F5 
 in case the user prefers to use the "FN" 
functions (in my case that is true on one keyboard where the volume keys are 
hidden under F8, F9 and F10). That's not an argument to either keep or remove 
it, but just to show that refresh doesn't always have to be a one-key thing. 
But i consider that to be a "it's your own fault" case as the user then 
knowingly chose this.
  
  But perhaps we need more opinions.

REPOSITORY
  R241 KIO

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

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

D12218: Remove Reload button from the file dialogs' toolbar

[Mark Gaiser]

markg added a comment.


  >> Refresh on the other hand is something i use relatively often, more then 
any of the other buttons.
  > 
  > Curious to know: What do you need Refresh for? Maybe there's need for some 
changes so you won't have to hit that button manually so often?
  
  As i said, it's all relative. I barely use any of the buttons but the one i 
do use is refresh.
  I only need it when i'm impatient (for instance when wanting to click on a 
file that is still being copied) or when i'm browsing a slow network drive.

REPOSITORY
  R241 KIO

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

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

D12218: Remove Reload button from the file dialogs' toolbar

[Mark Gaiser]

markg added a comment.


  I personally find the reload button much more useful to have there.
  
  ... (looking at the buttons) ...
  
  Why not removing back, forward and up? This is not an ironic or sarcastic 
suggestion! Just be frank, how often do you use back?
  How often do you use forward (close to never i assume).
  And up? Well, it's twice in there already. You have the breadcrumb bar which 
allows you to go up.
  
  Refresh on the other hand is something i use relatively often, more then any 
of the other buttons. But it depends on my hand position. If i'm already moving 
the mouse i'm inclined to hit the refresh button, if i'm already on the 
keyboard then i'm inclined to just press F5 .

REPOSITORY
  R241 KIO

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

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

D11204: Support NTFS hidden files

[Mark Gaiser]

markg added a comment.


  In D11204#247248 , @vonreth wrote:
  
  > The commit broke the build on mac.
  >
  >   11:43:10 
/Users/packaging/Craft/BinaryFactory/macos-64-clang/build/kde/frameworks/tier3/kio/work/kio-5.45.0/src/ioslaves/file/file_unix.cpp:421:19:
 error: no matching function for call to 'getxattr'
  >11:43:10 auto length = getxattr(filenameEncoded.data(), attrName, 
nullptr, 0);
  >11:43:10   ^~~~
  >11:43:10 /usr/include/sys/xattr.h:61:9: note: candidate function not 
viable: requires 6 arguments, but 4 were provided
  >11:43:10 ssize_t getxattr(const char *path, const char *name, void 
*value, size_t size, u_int32_t position, int options);
  >11:43:10 ^
  >11:43:10 
/Users/packaging/Craft/BinaryFactory/macos-64-clang/build/kde/frameworks/tier3/kio/work/kio-5.45.0/src/ioslaves/file/file_unix.cpp:427:14:
 error: no matching function for call to 'getxattr'
  >11:43:10 length = getxattr(filenameEncoded.data(), attrName, 
strAttr, xattr_size);
  >11:43:10  ^~~~
  >11:43:10 /usr/include/sys/xattr.h:61:9: note: candidate function not 
viable: requires 6 arguments, but 4 were provided
  >11:43:10 ssize_t getxattr(const char *path, const char *name, void 
*value, size_t size, u_int32_t position, int options);
  >11:43:10 ^
  >11:43:10 2 errors generated.
  
  
  Ha, apparently linux and mac implement the same principle (xattr) but with 
different arguments (same function names).
  Linux docs: http://man7.org/linux/man-pages/man2/getxattr.2.html
  Mac docs: 
https://developer.apple.com/legacy/library/documentation/Darwin/Reference/ManPages/man2/getxattr.2.html
  
  Note for the mac one, the link above is to their legacy stuff (found via 
google). I don't know if they have a "non-legacy" version or completely new 
alternative.

REPOSITORY
  R241 KIO

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

To: rominf, #dolphin, #frameworks, dfaure
Cc: vonreth, kossebau, dfaure, markg, elvisangelaccio, ltoscano, 
anthonyfieroni, broulik, #frameworks, #dolphin, michaelh, ngraham, bruns

D11768: Add Desktop and Downloads to the default list of Places

[Mark Gaiser]

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


  I don't know why, but i already have this by default in my Dolphin... 
(ArchLinux user here, i don't think they add it explicitly as they try to stay 
as true to upstream as possible).
  So, err, don't know why.
  
  Anyhow, having them by default is a big +1 from me.
  
  Yet i still give a -1.. The reason for that is simple. Ever since the 
placesmodel changes in KIO and Dolphin, a unittest for that fails. Fix that 
first.
  I did have a look at it, but quite some placesmodel magic changed so it's 
better if one of the authors of those changes takes a look.
  For reference:, look at all the recent builds of Dolphin and KIO.
  Dolphin: 
https://build.kde.org/job/Applications%20dolphin%20kf5-qt5%20SUSEQt5.9/
  KIO: https://build.kde.org/job/Frameworks%20kio%20kf5-qt5%20SUSEQt5.10/

REPOSITORY
  R241 KIO

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

To: ngraham, #dolphin, #gwenview, #frameworks, #vdg, markg
Cc: markg, fabiank, progwolff, broulik, michaelh, ngraham

D11204: Support NTFS hidden files

[Mark Gaiser]

markg resigned from this revision.
markg added a comment.
This revision is now accepted and ready to land.


  In D11204#233997 , @dfaure wrote:
  
  > Thanks !
  >
  > Do you have a contributor account for pushing the commit, or do you need 
someone else to do it?
  
  
  He can commit, he has done a bunch in Dolphin already :)

REPOSITORY
  R241 KIO

BRANCH
  ntfs-hidden (branched from master)

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

To: rominf, #dolphin, #frameworks, dfaure
Cc: dfaure, markg, elvisangelaccio, ltoscano, anthonyfieroni, broulik, 
#frameworks, #dolphin, michaelh, ngraham

D11204: Support NTFS hidden files

[Mark Gaiser]

markg added inline comments.

INLINE COMMENTS

> rominf wrote in file_unix.cpp:546-550
> The logic behind this is that `getxattr` exists only on Linux 
> (https://www.gnu.org/software/gnulib/manual/html_node/getxattr.html#getxattr) 
> and `file_unix.cpp` is more Linux-y than `file.cpp`. If you think that I'm 
> wrong, I can move it.

I "think" it belongs in createUDSEntry, but i'm not sure either. It would keep 
this else nice and clean. But i do see why you'd want to put it here... What's 
your take on this, @dfaure?

REPOSITORY
  R241 KIO

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

To: rominf, #dolphin, #frameworks, markg, dfaure
Cc: dfaure, markg, elvisangelaccio, ltoscano, anthonyfieroni, broulik, 
#frameworks, #dolphin, michaelh, ngraham

D11204: Support NTFS hidden files

[Mark Gaiser]

markg added a comment.


  Ping.

REPOSITORY
  R241 KIO

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

To: rominf, #dolphin, #frameworks, markg, dfaure
Cc: dfaure, markg, elvisangelaccio, ltoscano, anthonyfieroni, broulik, 
#frameworks, #dolphin, michaelh, ngraham

D11204: Support NTFS hidden files

[Mark Gaiser]

markg added a reviewer: dfaure.

REPOSITORY
  R241 KIO

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

To: rominf, #dolphin, #frameworks, markg, dfaure
Cc: dfaure, markg, elvisangelaccio, ltoscano, anthonyfieroni, broulik, 
#frameworks, #dolphin, michaelh, ngraham

D11204: Support NTFS hidden files

[Mark Gaiser]

markg requested changes to this revision.
This revision now requires changes to proceed.

REPOSITORY
  R241 KIO

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

To: rominf, #dolphin, #frameworks, markg
Cc: dfaure, markg, elvisangelaccio, ltoscano, anthonyfieroni, broulik, 
#frameworks, #dolphin, michaelh, ngraham

D11204: Support NTFS hidden files

[Mark Gaiser]

markg added a subscriber: dfaure.
markg added a comment.


  @dfaure I've been looking over the file.cpp and file_unix.cpp code a bit and 
i'm rather surprised that UDS_HIDDEN isn't being set at all here. Which makes 
me wonder, why is the hidden logic missing and how is it working now?
  I don't know for the "why", i'm hoping you can share some insight on this?
  I do know for the "how"; "KFileItem::isHidden()" is taking care of that. It 
checks the first character for a dot and returns true if it does (thus hidden 
for any app that uses KFileItem).
  
  Would it be OK to move this logic from KFileItem::isHidden to the file.cpp 
side? Imho, that is the right place to check as operating systems apparently 
have a different way of showing files as hidden.
  Note that this will cause regressions. IOSlaves that don't set UDS_HIDDEN 
will then show the hidden files. That imho is a bug for those respective 
IOSlaves not for KFileItem.

INLINE COMMENTS

> file_unix.cpp:546-550
> +#ifdef Q_OS_LINUX
> +if (isNtfsHidden(filename)) {
> +entry.insert(KIO::UDSEntry::UDS_HIDDEN, 1);
> +}
> +#endif

Why here?
This should be done inside the createUDSEntry function (it's in file.cpp).

REPOSITORY
  R241 KIO

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

To: rominf, #dolphin, #frameworks, markg
Cc: dfaure, markg, elvisangelaccio, ltoscano, anthonyfieroni, broulik, 
#frameworks, #dolphin, michaelh, ngraham

D11204: Support NTFS hidden files

[Mark Gaiser]

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


  -2 in my opinion.
  KFileItem should know nothing about NTFS.
  
  You already have isHidden in there which should handle it.
  If it doesn't then you might have found a bug.
  
  See how m_hidden is filled:
  
const int hiddenVal = m_entry.numberValue(KIO::UDSEntry::UDS_HIDDEN, -1);
m_hidden = hiddenVal == 1 ? Hidden : (hiddenVal == 0 ? Shown : Auto);
  
  If that gives the wrong result then the place that's filling UDS_HIDDEN (in 
the KIO slave that @broulik point you to.

REPOSITORY
  R241 KIO

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

To: rominf, #dolphin, #frameworks, markg
Cc: markg, elvisangelaccio, ltoscano, anthonyfieroni, broulik, #frameworks, 
#dolphin, michaelh

D10446: Add KLanguageName

[Mark Gaiser]

markg added a comment.


  Isn't this better suited for KCoreAddons?
  
  On a related note, i did make this bug report for Qt: 
https://bugreports.qt.io/browse/QTBUG-64942 with regard to ISO 3166 country 
names. But language names also came up, see this 

 comment.
  
  I don't know of any progress there though.

REPOSITORY
  R265 KConfigWidgets

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

To: aacid
Cc: markg, apol, #frameworks, michaelh

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

[Mark Gaiser]

markg added a comment.


  In D10742#218171 , @dfaure wrote:
  
  > Can someone explain to me how switching from pointers to values is making 
anything faster, or is a first step towards making anything faster? This step 
in itself can only make things slower due to more "copying" (refcount updating).
  
  
  I don't know what the ultimate goal of @jtamate is here (speeding things up, 
that i do know).
  But in general, putting something on the stack (aka, no new) is measurable 
faster. For small objects and in small quantities that doesn't matter much. But 
for large objects (KFileItem is large) and in large quantities (also fitting 
for KFileItem) it could very well be a nice speedup!
  
  The other side of this optimizing story is copies where you don't intent them 
to happen but they just do. For instance, a std::vector allocates a 
bunch ahead of time and then copies it in. Or moves since my move semantics 
commits and where applicable. If this were a std::vector you'd had 
no such copy or move.
  
  Back on this patch. It might allow faster routes, but it really depends on 
@jtamate next plans here if the current patch is worth it.
  Sounds like we need more info :)

REPOSITORY
  R241 KIO

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

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

D10857: Change qSort to std::sort in simplifiedUrlList

[Mark Gaiser]

markg added a comment.


  In D10857#216725 , @dfaure wrote:
  
  > I cannot confirm that stable_sort is faster, on the contrary. 
http://www.davidfaure.fr/2018/qsort_performance.cpp updated, says (repeatedly)
  >  "std::sort took: 5003 ms"
  >  "std::stable_sort took: 7490 ms"
  >
  > Maybe on specific data (the actual filenames you're testing this with), 
stable_sort ends up being faster, but this isn't true in general (with random 
data).
  
  
  "qSort took: 3308 ms"
  "std::sort took: 3383 ms"
  "std::stable_sort took: 5829 ms"
  
  My results. A direct copy-paste from your benchmark, no edits.
  Release mode and linux this time.
  
  @jtamate std::sort it is. Feel free to commit :)

REPOSITORY
  R241 KIO

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

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

D10857: Change qSort to std::sort in simplifiedUrlList

[Mark Gaiser]

markg added a comment.


  In D10857#214867 , @jtamate wrote:
  
  > In D10857#214607 , @dfaure wrote:
  >
  > > I'm not opposed to the idea, but measuring CPU usage is a rather 
misleading indicator. What if it takes 10 times longer, because it's 
progressing much more slowly? ;)
  > >
  > > Please at least measure with QElapsedTimer around the sorting (not to be 
committed, just to gather numbers about the actual performance of this from a 
user's point of view).
  > >  I'm interested in the result ;)
  >
  >
  > The results are strange. All the results are measured sorting 50.000 small 
files:
  >
  > Both Qt are the same version from opensuse.
  >
  > In an i5, why the difference is so big? recent cpu bugs?
  >  qSort in i3 
  >  274764, 276060 (with 3 directories), 365878, 424506  (without directories)
  >  std::sort in i3
  >  940, 995 (with 3 directories), 2472, 2539 (without directories)
  >
  > In AMD the results are closer, qSort wins
  >  qSort in AMD
  >  658, 726, 695, 683, 676, 666, 649, 684, 666 (without directories)
  >  std:sort in AMD
  >  843, 839, 878, 896,  925 (without directories)
  
  
  That surprises me a lot!
  I "thought" qSort was just a template or alias to std::sort, but it isn't: 
http://code.qt.io/cgit/qt/qtbase.git/tree/src/corelib/tools/qalgorithms.h#n340
  But it isn't which kinda makes my other expectations moot (qSort == 
std::sort... it isn't)
  
  So i did some tests as well.
  5 filenames sorting them (computer sorting, not the natural one) with 
std::string and QString (yes, it matters)
  Filenames, well, sorta.. Made them up in a loop :)
  
  QString version: https://p.sc2.nl/r1JTFaf_z
  qSort: ~220ms
  std::sort ~276ms
  
  std::string version: https://p.sc2.nl/HJ69Y6G_G
  qSort: ~100ms
  std::sort ~130ms
  
  Note that std::string might not be the fair comparison as it's 8 bit/char. 
QString is 16.
  My compiler (in this rare case) Visual Studio 2017 on an Intel CPU.
  
  I still think it's wise to replace all qSort, if only for it being 
deprecated. But it does suck a little that qSort beats std::sort.

REPOSITORY
  R241 KIO

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

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

D10857: Change qSort to std::sort in simplifiedUrlList

[Mark Gaiser]

markg added a comment.


  +1, also for what @apol said.

REPOSITORY
  R241 KIO

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

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

D10414: Add move semantics support to KIO::UDSEntry.

[Mark Gaiser]

markg added a comment.


  I seem to be making a mess of this commit. Sorry for that.
  https://p.sc2.nl/B13Etcldf with the changes.
  
  Regarding arc to push a change back here again, how do you do that for 
something that is already committed?

REPOSITORY
  R241 KIO

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

To: markg, dfaure
Cc: bcooksley, apol, #frameworks, michaelh, kmorwinski

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

[Mark Gaiser]

markg added a comment.


  In D10702#213845 , @dfaure wrote:
  
  > "unlink() in most of the modern filesystems is not affected by the size of 
the file" doesn't match my experience, I have seen konqueror/dolphin freeze for 
10s while deleting a 8GB file (on a somewhat old system, no SSD). And that 
would actually be the reason for this patch to go in. But on the other hand, I 
have a hard time believing that this patch doesn't make things slower for the 
case of many small files, due to the communication overhead with the kioslave 
(and that's the reason I wrote this code in the first place).
  >
  > Maybe the right solution is to use QFile::remove if the file is small, and 
use the kioslave if the file is big. But finding the file size in the first 
place takes a little bit of time too :-)
  
  
  Note that the issue here is the blocking part which @meven tried to solve :)
  The performance impact this would potentially have is merely the point i 
happen to notice.
  
  But the blocking issue remains, also with your suggestion of QFile::remove.
  An alternative approach (that does not involve std::async) is to pre-scan the 
list of files for local files and send them all at once to the kioslave, just 
as a list of files to be deleted. A downside in that approach would be the 
requirement to change the slave as well to handle this.

REPOSITORY
  R241 KIO

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

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

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

[Mark Gaiser]

markg added a comment.


  In D10702#212753 , @meven wrote:
  
  > In D10702#211236 , @markg wrote:
  >
  > > While this might give you the expected result, it feels like a workaround.
  > >
  > > I'm assuming the fast path is there for a reason and is really 
substantially faster then going through the job route.
  > >  If that is the case then the proper fix would be to make that code part 
async. That is obviously much more complex (otherwise it would've been done 
already).
  > >  Think of using std::async and a QEventLoop. Sounds difficult, right? It 
is :) But I've been playing with that kind of stuff lately so i'm happy to 
share an example that you can use as a starting point.
  > >  Here it is: https://p.sc2.nl/BygE-Oiwz
  > >
  > > I wanted to paste it inline, but that already got quite big so a link it 
is.
  > >  I've added a bunch of comments in the code to explains what it's doing.
  > >  Note that the example does make a "QEventLoop", you should **not** do 
that within the if statement, but rather outside the while loop and simply call 
exec() and quit() every time (not making a new QEventLoop for every delete)
  > >
  > > Lastly, please benchmark this fast pats (as it currently is) compared to 
your KIO version and my async version to see if the fast path really is the 
fast path. As we just don't know and that kinda influences which route to 
choose here.
  >
  >
  > Please correct if I am wrong but kio::file_delete will, in the end, calls 
FileProtocol::deleteRecursive(const QString ) which works the same way 
using QFile::remove as before.
  >  So I have the hypothesis that they should not be much difference in 
performance if any.
  >
  > It could be impactful because of the KIO::file_delete call and I don't know 
precisely how expensive such a kio call is.
  >  Or because they are more jobs class instanciated.
  >  But those KIO::file_* calls are already used in a lot of cases in 
CopyJobPrivate::copyNextFile for instance to recursively copy files, so I know 
that at least in some cases it is ok.
  >
  > Please correct me wherever I am wrong I am new in the KIO codebase. I am 
learning as I go.
  >
  > The main drawback in this version of the patch currently, is that we loose 
progressive status update when a lot of files are being removed (slotReport was 
called each 300 deletion before).
  >  And since needs to be fixed this at least.
  
  
  This is spot on: ..."//in the end, calls FileProtocol::deleteRecursive(const 
QString ) //"...
  But it takes some time to get there.
  KIO is process based. It opens a file slave and feeds it instructions via a 
socket connection. That architecture is why it doesn't block.
  This is heavily oversimplified, but that's roughly how it works.
  
  This communication (and subsequent parsing of the messages) just takes 
"time". Not a whole lot and it's quite efficient, but it starts to count with 
loads of files.
  I don't know how much this time is, you'd have to benchmark it to know. I'm 
guessing it's quite substantial otherwise the fast path wouldn't be there.
  
  As for losing progression status... I think that's a no-go. Users kinda lake 
to know how far something is.

REPOSITORY
  R241 KIO

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

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

D10771: Save some memory allocations by using the right API

[Mark Gaiser]

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


  Looks good to me.

REPOSITORY
  R237 KConfig

BRANCH
  master

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

To: apol, #frameworks, markg
Cc: markg, michaelh

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

[Mark Gaiser]

markg added a comment.


  I unfortunately have no clue how to answer your questions.

INLINE COMMENTS

> kcoredirlister.cpp:825-829
> +// If take remove the element from the list.
> +if (take) {
> +dirItem->lstItems.erase(it);
> +}
> +return retKFileItem;

Hmm, this looks weird to me.
Sure, it works. "KFileItem retKFileItem = *it;" makes a copy.

A more efficient way (but requires you to change the lists this is backed by to 
a std::vector) is to:

1. Take the element out of the vector. Something like "KFileItem item = 
std::move(*it);
2. Now the vector would be in a valid state but with one invalid object (will 
post a problem if you iterate over it later on) so you have to remove that 
element from the vector like you did.

I'm not sure if the benefits of this justify the path of changing the list 
(dirItem->lstItems) to a std::vector, if possible at all.
That's up to you :)

REPOSITORY
  R241 KIO

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

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

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

[Mark Gaiser]

markg added a comment.


  While this might give you the expected result, it feels like a workaround.
  
  I'm assuming the fast path is there for a reason and is really substantially 
faster then going through the job route.
  If that is the case then the proper fix would be to make that code part 
async. That is obviously much more complex (otherwise it would've been done 
already).
  Think of using std::async and a QEventLoop. Sounds difficult, right? It is :) 
But I've been playing with that kind of stuff lately so i'm happy to share an 
example that you can use as a starting point.
  Here it is: https://p.sc2.nl/BygE-Oiwz
  
  I wanted to paste it inline, but that already got quite big so a link it is.
  I've added a bunch of comments in the code to explains what it's doing.
  Note that the example does make a "QEventLoop", you should **not** do that 
within the if statement, but rather outside the while loop and simply call 
exec() and quit() every time (not making a new QEventLoop for every delete)
  
  Lastly, please benchmark this fast pats (as it currently is) compared to your 
KIO version and my async version to see if the fast path really is the fast 
path. As we just don't know and that kinda influences which route to choose 
here.

REPOSITORY
  R241 KIO

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

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

D10414: Add move semantics support to KIO::UDSEntry.

[Mark Gaiser]

markg added a comment.


  @bcooksley Ah, i see you **just** fixed this by using kiotesthelper.h (which 
includes kioglobal_p.h).
  According to Jenkins, all is OK now :)
  
  Thank you very much!

REPOSITORY
  R241 KIO

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

To: markg, dfaure
Cc: bcooksley, apol, #frameworks, michaelh

D10414: Add move semantics support to KIO::UDSEntry.

[Mark Gaiser]

markg added a comment.


  In D10414#208515 , @bcooksley 
wrote:
  
  > Looks like QT_LSTAT doesn't exist on Windows - see 
https://git.reviewboard.kde.org/r/127727/
  
  
  Would including "kioglobal_p.h" perhaps fix this?
  
#ifndef QT_LSTAT
#define QT_LSTAT kio_windows_lstat
#endif
  
  It's a private header, but it's just for the unit tests. That would probably 
be fine.
  
  If someone could test that on a windows setup?

REPOSITORY
  R241 KIO

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

To: markg, dfaure
Cc: bcooksley, apol, #frameworks, michaelh

D10617: Fix "ambiguous shortcut" issue introduced with D10314

[Mark Gaiser]

markg added a comment.


  In D10617#208703 , 
@elvisangelaccio wrote:
  
  > In D10617#208513 , @ngraham 
wrote:
  >
  > > The shortcut won't affect other apps since the file dialog its its own 
context. Even if the host app uses F12 for something, the file dialog will grab 
the key first, so there's no "ambiguous shortcut" issue.
  >
  >
  > Yes, you won't get the ambiguous shortcut dialog but it'd be a bad idea to 
use F12 for two different things within the same app (the file dialog //is// 
part of the app).
  
  
  If it were F11 (which it was) then this patch would be very intrusive as you 
could have potentially broken fullscreen support (with F11), but it isn't it's 
F12..
  Note that you can in fact see this "broken fullscreen". Open gwenview, open 
an image and press F11, then do CTRL+O to open the file open dialog. Now F11 
only has effect on that file open dialog.
  
  But for F12, i don't see anything wrong. As far as i'm aware it isn't bound 
to any really crucial actions (i would call fullscreen crucial aka F11).
  F12 in chrome opens the developer console.
  F12 in konsole prints the tilde sign (there is another dedicated key for 
that, right under escape)
  
  F12 seems fine to me. Yes, you'd get into the weird issue that F12 in the app 
might have a different behavior in a file open dialog opened by the very same 
app, but that is with every key you choose.
  As long as we are consistent in keys which i thing Nate is very much trying 
to do (good job!).
  
  Also, note that F11 in kwrite binds to something completely different (show 
line numbers).

REPOSITORY
  R241 KIO

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

To: ngraham, #frameworks, #dolphin, markg, elvisangelaccio
Cc: michaelh

D10414: Add move semantics support to KIO::UDSEntry.

[Mark Gaiser]

markg added a comment.


  Right, it didn't fix it... 
https://build.kde.org/job/Frameworks%20kio%20kf5-qt5%20WindowsMSVCQt5.9/195/console
  Could someone with a windows setup (compiler and kio) compile kio and tell me 
what is wrong here?

REPOSITORY
  R241 KIO

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

To: markg, dfaure
Cc: apol, #frameworks, michaelh

D10414: Add move semantics support to KIO::UDSEntry.

[Mark Gaiser]

markg added a comment.


  Right, this one is my fault: 
https://build.kde.org/job/Frameworks%20kio%20kf5-qt5%20WindowsMSVCQt5.9/193/
  Weird that QT_LSTAT is no problem on linux without including qplatformdefs.h, 
on windows it apparently is.
  I pushed the fix: 
https://cgit.kde.org/kio.git/commit/?id=11051f3709cdc651a68990a9c17a69c33d5812db

REPOSITORY
  R241 KIO

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

To: markg, dfaure
Cc: apol, #frameworks, michaelh

D10414: Add move semantics support to KIO::UDSEntry.

[Mark Gaiser]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R241:d8414425acc5: Add move semantics support to 
KIO::UDSEntry. (authored by markg).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10414?vs=27431=27432

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

AFFECTED FILES
  autotests/udsentrytest.cpp
  autotests/udsentrytest.h
  src/core/udsentry.cpp
  src/core/udsentry.h

To: markg, dfaure
Cc: apol, #frameworks, michaelh

D10414: Add move semantics support to KIO::UDSEntry.

[Mark Gaiser]

markg marked 3 inline comments as done.

REPOSITORY
  R241 KIO

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

To: markg, dfaure
Cc: apol, #frameworks, michaelh

D10414: Add move semantics support to KIO::UDSEntry.

[Mark Gaiser]

markg updated this revision to Diff 27431.
markg added a comment.


  Add @since lines where for the move assignment operator and constructor.
  Typo fix.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10414?vs=27028=27431

BRANCH
  udsentry_move

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

AFFECTED FILES
  autotests/udsentrytest.cpp
  autotests/udsentrytest.h
  src/core/udsentry.cpp
  src/core/udsentry.h

To: markg, dfaure
Cc: apol, #frameworks, michaelh

D10616: Add Ctrl+H to the list of shortcuts for "show/hide hidden files"

[Mark Gaiser]

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


  Fine by me.
  I do wonder why it used to be in an if statement though. It would have added 
a nullptr if the "if (showHidden)" check failed (which would fail when it's a 
nullptr).

REPOSITORY
  R241 KIO

BRANCH
  master

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

To: ngraham, #frameworks, #dolphin, elvisangelaccio, markg
Cc: markg, michaelh

D10617: Fix "ambiguous shortcut" issue introduced with D10314

[Mark Gaiser]

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


  Looks good to me.

REPOSITORY
  R241 KIO

BRANCH
  solve-file-dialog-ambiguous-shortcut-issue (branched from master)

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

To: ngraham, #frameworks, #dolphin, markg, elvisangelaccio
Cc: michaelh

D10380: Refactor KCoreDirLister(Cache) to use KFileItemListV2 where possible.

[Mark Gaiser]

markg abandoned this revision.
markg added a comment.


  As this is a no-go, rightfully so! Abandoning it.

REPOSITORY
  R241 KIO

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

To: markg, dfaure
Cc: #frameworks, michaelh

D10378: Deprecate KFileItemList and introduce KFileItemListV2

[Mark Gaiser]

markg added a comment.


  @dfaure What's your preference here?
  I do "want" to work on this, no promises though. It could take a very long 
while ;)

REPOSITORY
  R241 KIO

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

To: markg, dfaure
Cc: #frameworks, michaelh

D10414: Add move semantics support to KIO::UDSEntry.

[Mark Gaiser]

markg added a comment.


  Ahh, will fix that when i get home.
  I missed the @since lines in KFileITem as well. Is it OK if i just add them 
outside of phabricator? They will get an "@since 5.43" i think.

REPOSITORY
  R241 KIO

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

To: markg, dfaure
Cc: apol, #frameworks, michaelh

D10414: Add move semantics support to KIO::UDSEntry.

[Mark Gaiser]

markg marked 2 inline comments as done.
markg added a comment.


  I'm not afraid of the compiler generating wrong code. That is the least of my 
worries :)
  What i am afraid of (which is why i suggested the copy test) is a wrong 
optimization at some point (like adding a pointer member but forgetting to 
properly copy it) which would break the UDSEntry.
  But a test for that would probably deserves its own review independent of 
this.
  
  Forget it, i have no plans to change the internal data structure of UDSEntry. 
I might play with a HAT-trie  at some point 
but that is unlikely to be on the UDSEntry level. It would be much more 
suitable on the KCoreDirLister level.

REPOSITORY
  R241 KIO

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

To: markg, dfaure
Cc: apol, #frameworks, michaelh

D10414: Add move semantics support to KIO::UDSEntry.

[Mark Gaiser]

markg updated this revision to Diff 27028.
markg added a comment.


  Fix a typo and replace QCOMPARE with QVERIFY for the file open check.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10414?vs=26868=27028

BRANCH
  udsentry_reductions

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

AFFECTED FILES
  autotests/udsentrytest.cpp
  autotests/udsentrytest.h
  src/core/udsentry.cpp
  src/core/udsentry.h

To: markg, dfaure
Cc: apol, #frameworks, michaelh

D10437: Limit the use of file.so for privilege operation to one application

[Mark Gaiser]

markg added a comment.


  In D10437#204416 , @chinmoyr wrote:
  
  > In D10437#204402 , @markg wrote:
  >
  > > I just tried this:
  > >  kdesu gwenview (type root password and go to the root homefolder).
  > >  In gwenview i can now go to that folder.
  > >  In dolphin (started as user, not root) i can't get into that folder.
  > >
  > > I don't really see what you try to fix here as it seems to be working 
just fine here.
  > >  Or i don't understand it.. In that case, please provide reproducible 
steps.
  >
  >
  > Currently file ioslave does not support reading contents of a locked(is 
this the right word?) folder. So no file operations are possible.
  
  
  Could you provide steps to reproduce what you try to fix?

REPOSITORY
  R241 KIO

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

To: chinmoyr, #frameworks, dfaure, fvogt
Cc: markg, anthonyfieroni, michaelh

D10437: Limit the use of file.so for privilege operation to one application

[Mark Gaiser]

markg added a comment.


  I just tried this:
  kdesu gwenview (type root password and go to the root homefolder).
  In gwenview i can now go to that folder.
  In dolphin (started as user, not root) i can't get into that folder.
  
  I don't really see what you try to fix here as it seems to be working just 
fine here.
  Or i don't understand it.. In that case, please provide reproducible steps.

REPOSITORY
  R241 KIO

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

To: chinmoyr, #frameworks, dfaure, fvogt
Cc: markg, anthonyfieroni, michaelh

D10414: Add move semantics support to KIO::UDSEntry.

[Mark Gaiser]

markg added a comment.


  @dfaure i can make the test much more complete if you want.. But it's more 
complex thus i left it out.
  I can store the UDSEntry in a QDataStream which would be backed by a 
QByteArray. That can be hashed!
  Then i can do the same after moving operations and compare the hash. That way 
i would know for sure the UDSEntry objects match perfectly.
  
  But as said, that is more complex. I'd probably make a lambda within the 
testMove() function. I think with a signature like this:
  auto udsEntryHash = [](const KIO::UDSEntry ){
  // some foo..
  return hash; (would be the QCryptographicHash::result() output).
  };
  
  Your call :)
  
  Or i can add that as a separate test that just tests UDSEntry copy and move 
operations. That might actually be better and more generic.
  Do note that this doesn't fix or catch anything at the moment. It would just 
be an extra test to guarantee copying of UDSEntry objects is not broken.

REPOSITORY
  R241 KIO

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

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

D10414: Add move semantics support to KIO::UDSEntry.

[Mark Gaiser]

markg marked 7 inline comments as done.
markg added a comment.


  Seems my unittest skills have become a bit rusty over the years of basically 
not contributing patches to KDE ;)
  Thank you for the comments, David!

REPOSITORY
  R241 KIO

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

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

D10414: Add move semantics support to KIO::UDSEntry.

[Mark Gaiser]

markg updated this revision to Diff 26868.
markg added a comment.


  Update test.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10414?vs=26839=26868

BRANCH
  udsentry_reductions

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

AFFECTED FILES
  autotests/udsentrytest.cpp
  autotests/udsentrytest.h
  src/core/udsentry.cpp
  src/core/udsentry.h

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

D10414: Add move semantics support to KIO::UDSEntry.

[Mark Gaiser]

markg marked an inline comment as done.

REPOSITORY
  R241 KIO

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

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

D10414: Add move semantics support to KIO::UDSEntry.

[Mark Gaiser]

markg updated this revision to Diff 26839.
markg added a comment.


  Remove comment about compiler generated functions.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10414?vs=26838=26839

BRANCH
  udsentry_move

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

AFFECTED FILES
  autotests/udsentrytest.cpp
  autotests/udsentrytest.h
  src/core/udsentry.cpp
  src/core/udsentry.h

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

D10414: Add move semantics support to KIO::UDSEntry.

[Mark Gaiser]

markg added a comment.


  Just a note.
  The fact that currently no code is structured in a way that move semantics 
are used is... slightly annoying ;)
  No as in not when running dolphin with it and profiling it to see UDSEntry 
uses. There "might" be code that uses move semantics when this patch is 
applied, but i haven't seen it yet.
  
  So, more patches to come to fix that. I've already spotted one potential case 
in KCoreDirListerCache although that probably depends on more.

REPOSITORY
  R241 KIO

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

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

D10414: Add move semantics support to KIO::UDSEntry.

[Mark Gaiser]

markg updated this revision to Diff 26838.
markg added a comment.


  Fix indentation.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10414?vs=26836=26838

BRANCH
  udsentry_move

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

AFFECTED FILES
  autotests/udsentrytest.cpp
  autotests/udsentrytest.h
  src/core/udsentry.cpp
  src/core/udsentry.h

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

D10414: Add move semantics support to KIO::UDSEntry.

[Mark Gaiser]

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

REVISION SUMMARY
  Adds move semantics support and a testcase (only useful in callgrind).
  Right now there are 0 uses of these semantics.

TEST PLAN
  Relevant tests (udsentrytest and kfileitemtest) pass.
  Dolphin compiles and runs just fine with it.

REPOSITORY
  R241 KIO

BRANCH
  udsentry_move

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

AFFECTED FILES
  autotests/udsentrytest.cpp
  autotests/udsentrytest.h
  src/core/udsentry.cpp
  src/core/udsentry.h

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

D10378: Deprecate KFileItemList and introduce KFileItemListV2

[Mark Gaiser]

markg added a comment.


  Based on the comment you made in https://phabricator.kde.org/D10380 it's 
logical that this commit won't make it as it currently is implemented.
  The using line is probably also not the best option if you want to keep the 
convenience functions, inheriting would be better. But then again, inheriting 
from an std container gives it's own issues (like no virtual destructor...)
  
std::vector, for example, is not designed to be inherited and typically 
should not be inherited (very shaky design), as that will then be prone to this 
base pointer deletion issue (std::vector deliberately avoids a virtual 
destructor) in addition to clumsy object slicing issues if your derived class 
adds any new state.
  
  See: 
https://softwareengineering.stackexchange.com/questions/284561/when-not-to-use-virtual-destructors
  
  That kinda leaves me wondering how to proceed with this.
  Some options i can think of:
  
  1. Do nothing. Leave as is for now and the foreseeable time. No move 
semantics.
  2. Go for the using line (as KFileItemVector) but with a 
"KFileItemVectorHelper" namespace that contains the current helper function. 
(or just the KIO:: namespace).
1. In this case not deprecating the existing API before all other functions 
that currently use KFileItemList have a KFileItemVector counterpart
2. Also put this under a compile time define (say -DUSE_KFILEITEMVECTOR) 
which means the code will be littered with #if statements, but maintains 
performance as it currently stands. Using this define would disable 
KFileItemList so that too would be under #if statements.
3. This also allows for a more graceful transition as not everything has to 
be implemented at once. Once everything is implemented, add a whole slew of 
deprecated warnings to all KFileItemList uses.
4. In KF6 simply remove all #if and end up with only KFileItemVector code 
paths.
  3. Something else?
  
  What's your opinion?

REPOSITORY
  R241 KIO

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

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

D10341: Allow move semantics to be generated for KFileItem. The existing copy constructor, destructor and copy assignment operator are now also generated by the compiler.

[Mark Gaiser]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:80ccfabb06ff: Allow move semantics to be generated for 
KFileItem. The existing copy… (authored by markg).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10341?vs=26648=26755

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

AFFECTED FILES
  autotests/kfileitemtest.cpp
  autotests/kfileitemtest.h
  src/core/kfileitem.cpp
  src/core/kfileitem.h

To: markg, dfaure, mwolff
Cc: ngraham, #frameworks, michaelh

D10341: Allow move semantics to be generated for KFileItem. The existing copy constructor, destructor and copy assignment operator are now also generated by the compiler.

[Mark Gaiser]

markg added a comment.


  In https://phabricator.kde.org/D10341#202720, @markg wrote:
  
  > In https://phabricator.kde.org/D10341#202704, @dfaure wrote:
  >
  > > I like the idea of enabling moves for KFileItem very much.
  > >
  > > But here's a fun fact: your unittest passes even without the rest of the 
patch.
  > >
  > >   PASS   : KFileItemTest::testMove()
  > >   
  > >
  > > That's because std::move() doesn't move, it only makes the argument 
eligible for e.g. a move ctor, but will call a copy ctor if there's no move 
ctor. So that test is not really testing that move works ;)
  > >  That's always a bit tricky to test, I guess, because one can't really 
rely on the state of the moved-from object to be anything in particular. And we 
want =default, not to implement some counters in there.
  > >  Oh well, then maybe there's no real way to unittest that moving works.
  >
  >
  > I know, i - somewhat - mentioned it ;)
  >  //"New test for move semantics (it passes, would probably pass without as 
well but just be a copy)."//
  >
  > The test might be rather pointless as is, but running that test though 
callgrind does show move semantics which is why i added it.
  >  Do i just remove it?
  >
  > > Anyhow, the thing I'm wondering is the following: does the rule-of-zero 
lead to more opportunities for optimizations by the compiler, who can see "from 
the outside" that the 5 special members are the default generated ones, while 
your patch "hides" the implementation, lowering the visibility for the compiler 
in the rest of the code? Well, one could just move the 5 "=default" to the 
header to fix that.
  >
  > There was an issue with that... I don't quite remember what it was. Let me 
try again... (to be continued)
  
  
  - continued --
  
  It doesn't compile...
  I don't know why or how to fix it, you might :)
  This is the message:
  
In file included from /usr/include/qt/QtCore/QSharedData:1:0,
 from /home/mark/GitProjects/kio/src/core/udsentry.h:26,
 from 
/home/mark/GitProjects/kio_build/src/core/kio/udsentry.h:1,
 from /home/mark/GitProjects/kio/src/core/kfileitem.h:26,
 from 
/home/mark/GitProjects/kio/src/core/kcoredirlister.h:24,
 from 
/home/mark/GitProjects/kio/src/core/kcoredirlister.cpp:23:
/usr/include/qt/QtCore/qshareddata.h: In instantiation of 
‘QSharedDataPointer::QSharedDataPointer(const QSharedDataPointer&) [with 
T = KFileItemPrivate]’:
/home/mark/GitProjects/kio/src/core/kfileitem.h:121:5:   required from here
/usr/include/qt/QtCore/qshareddata.h:92:84: error: invalid use of 
incomplete type ‘class KFileItemPrivate’
 inline QSharedDataPointer(const QSharedDataPointer ) : d(o.d) { 
if (d) d->ref.ref(); }

 ~~~^~~
In file included from 
/home/mark/GitProjects/kio/src/core/kcoredirlister.h:24:0,
 from 
/home/mark/GitProjects/kio/src/core/kcoredirlister.cpp:23:
/home/mark/GitProjects/kio/src/core/kfileitem.h:35:7: note: forward 
declaration of ‘class KFileItemPrivate’
 class KFileItemPrivate;
   ^~~~
  
  kfileitem.h:121 is: KFileItem(const KFileItem&) = default; 
  It spawns that error about 14 times on various places...
  All errors: https://p.sc2.nl/BJviiMKUG
  And the code as diff on top of this change (should apply cleanly): 
https://p.sc2.nl/rJwAifKUM

REPOSITORY
  R241 KIO

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

To: markg, dfaure, mwolff
Cc: ngraham, #frameworks, michaelh

D10380: Refactor KCoreDirLister(Cache) to use KFileItemListV2 where possible.

[Mark Gaiser]

markg updated this revision to Diff 26739.
markg added a comment.


  Get rid of the temp file.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10380?vs=26738=26739

BRANCH
  refactor_dirlistercache

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

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

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

D10380: Refactor KCoreDirLister(Cache) to use KFileItemListV2 where possible.

[Mark Gaiser]

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

REVISION SUMMARY
  This refactors KFileItemList uses to KFileItemListV2 where possible.
  In terms of performance there isn't any change really. I did meassure it with 
callgrind but it seems roughtly the same when running Dolphin with it. If 
anything it's marginally faster, but that could just be the specific case.
  Another folder might be the same.

TEST PLAN
  Test Dolphin, run the tests. The relevant ones (kdirmodeltest and 
kfileitemtest) seem to pass just fine.

REPOSITORY
  R241 KIO

BRANCH
  refactor_dirlistercache

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

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

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

D10378: Deprecate KFileItemList and introduce KFileItemListV2

[Mark Gaiser]

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

REVISION SUMMARY
  Summary
  Deprecate KFileItemList. It's using QList and lacks move semantics.
  Introducing KFileItemListV2 which is just a using statement for 
std::vector.
  This new list allows for more efficient moving of KFileItem objects using 
move semantics.
  

  
  Also added (and immediately deprecated) a "toKFileItemList" function that 
converta a KFileItemListV2 back to a KFileItemList to keep the public API 
usable.

REPOSITORY
  R241 KIO

BRANCH
  kfileitemlist2

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

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

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

D10341: Allow move semantics to be generated for KFileItem. The existing copy constructor, destructor and copy assignment operator are now also generated by the compiler.

[Mark Gaiser]

markg added a comment.


  In https://phabricator.kde.org/D10341#202704, @dfaure wrote:
  
  > I like the idea of enabling moves for KFileItem very much.
  >
  > But here's a fun fact: your unittest passes even without the rest of the 
patch.
  >
  >   PASS   : KFileItemTest::testMove()
  >   
  >
  > That's because std::move() doesn't move, it only makes the argument 
eligible for e.g. a move ctor, but will call a copy ctor if there's no move 
ctor. So that test is not really testing that move works ;)
  >  That's always a bit tricky to test, I guess, because one can't really rely 
on the state of the moved-from object to be anything in particular. And we want 
=default, not to implement some counters in there.
  >  Oh well, then maybe there's no real way to unittest that moving works.
  
  
  I know, i - somewhat - mentioned it ;)
  //"New test for move semantics (it passes, would probably pass without as 
well but just be a copy)."//
  
  The test might be rather pointless as is, but running that test though 
callgrind does show move semantics which is why i added it.
  Do i just remove it?
  
  > Anyhow, the thing I'm wondering is the following: does the rule-of-zero 
lead to more opportunities for optimizations by the compiler, who can see "from 
the outside" that the 5 special members are the default generated ones, while 
your patch "hides" the implementation, lowering the visibility for the compiler 
in the rest of the code? Well, one could just move the 5 "=default" to the 
header to fix that.
  
  There was an issue with that... I don't quite remember what it was. Let me 
try again... (to be continued)
  My preference is = default in the header.
  
  > Of course in both cases (rule-of-zero or 5*=default in the header), it 
means we are tied to the 5 defaults for all of KF5, for BIC reasons. But that 
seems rather sensible in this case (the only member will always be the d 
pointer, and it's unlikely we'll move away from refcounting...).
  >  And in fact the other reason against rule-of-zero is that we can't just 
remove the copy-ctor and operator=, that would be BIC (existing code links to 
it).
  
  Ha, i tried and found out :) (before posting it here). That's why i went for 
the = default version (rule-of-five-defaults) to prevent that BIC.
  
  > In summary: this looks good to me, I'm just wondering if inlining the 5 
"=default" wouldn't be better, for optimization purposes.
  
  You know the real funny thing here? KFileItemList...
  It's a crappy QList with no move semantics.. I want to deprecate it (working 
on a patch for that now) by replacing it with a "using KFileItemListV2 = 
std::vector" The biggest improvements can be made by having move 
semantics in KFileItemListV2. It would also deprecate a load of KIO functions 
that all expose KFileItemList (either as argument or as return value). The 
downside? Well, you probably guessed it already. A lot of deprecated warnings 
and current code that needs to stay working for the KF5 lifetime thus in the 
short term this might actually be a performance loss :( I'll post a patch for 
this soon for you to judge :)

REPOSITORY
  R241 KIO

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

To: markg, dfaure, mwolff
Cc: ngraham, #frameworks, michaelh

Re: Possible regression in kio with data loss

[Mark Gaiser]

On Tue, Feb 6, 2018 at 4:17 PM, Jaime  wrote:

> Hi all, this is becoming urgent if next release is this weekend.
> It still happens to me (with kio just compiled from master)
>

If it is then add the KIO master (aka, David Faure) in cc :)
As i just did.

>
>
> 2018-02-05 8:18 GMT+01:00 Jaime :
>
>>
>>
>> 2018-02-04 23:42 GMT+01:00 Albert Astals Cid :
>>
>>> El dissabte, 20 de gener de 2018, a les 11:56:34 CET, Jaime va escriure:
>>> > Hi,
>>>
>>> Is it me or noone reacted to a "data loss regression" email?
>>>
>>> That's pretty sad.
>>>
>>> >
>>> >   Last weekend I did the following:
>>> >   * build kio using kdesrc-build
>>> >   * copy the resulting bin/kf5/file.so to
>>> /usr/lib64/qt5/plugins/kf5/kio
>>> >   * ldconfig
>>> >   * restart the session
>>> >   Just to be sure that all processes are using the new kio.
>>> >
>>> >   I copied files to an ntfs filesystem and the files were copied and
>>> the
>>> > messages about rights where shown.
>>>
>>> How are you copying the files? Dolphin? kdecp5? something else?
>>>
>>
>> With dolphin, drag
>>
>> Probably I'm wrong, but isn't this addressed in
>> https://phabricator.kde.org/D10233 ?
>>
>>
>>> Cheers,
>>>   Albert
>>>
>>> >
>>> >   Today I've done the same as last weekend, but when I copy a file to
>>> the
>>> > same filesystem, there is a message that the owner can't be changed,
>>> and
>>> > the resulting file has always 0 bytes (screenshot attached). If I move
>>> the
>>> > file, the original file content is lost, and also the destination has 0
>>> > bytes.
>>> >
>>> >   This can be reproduced also in a loopback vfat, created following the
>>> > next steps:
>>> >
>>> > dd if=/dev/zero of=fat.fs bs=1024 count=5120
>>> > (create the file fat.fs with only 5 MB)
>>> > /usr/sbin/mkfs.vfat fat.fs
>>> > (create the filesystem)
>>> > sudo mount -t vfat -o uid=,fmask=0007,dmask=,rw,noexec,loop
>>> fat.fs
>>> > /mnt
>>> > (mount being root the owner)
>>> >
>>> > try to copy any file to /mnt
>>> >
>>> > sudo umount /mnt
>>> > (don't forget to umount it when the tests are finished).
>>> >
>>> > Best Regards.
>>>
>>>
>>>
>>>
>>>
>>
>

D10341: Allow move semantics to be generated for KFileItem. The existing copy constructor, destructor and copy assignment operator are now also generated by the compiler.

[Mark Gaiser]

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

REVISION SUMMARY
  This allows the compiler to generate:
  
  - Move constructor
  - Move assingment
  - Copy constructor
  - Copy assignment
  - Destructor
  
  This in turn allows further KFileItem optimization throughout KIO and Dolphin.
  Also added a quite minimal test to see if move semantics work.
  
  As implemented now it roughly follows the "rule-of-five-default": 
http://scottmeyers.blogspot.nl/2014/03/a-concern-about-rule-of-zero.html
  I was tempted to go for the "rule-of-zero" which means not implementing any 
of those functions (thus the compiler generates them), but that - in my opinion 
- is not really clear as it's easy to add the destructor and then be surprised 
by not having move 
  semantics anymore.

TEST PLAN
  New test for move semantics (it passes, would probably pass without as well 
but just be a copy).
  Existing relevant tests (kfileitemtest and kdirmodeltest) all pass just fine.
  Running the new "testMove" through callgrind shows that the move constructor 
and assignment operator are really being used by that test.

REPOSITORY
  R241 KIO

BRANCH
  kfileitem_move

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

AFFECTED FILES
  autotests/kfileitemtest.cpp
  autotests/kfileitemtest.h
  src/core/kfileitem.cpp
  src/core/kfileitem.h

To: markg, dfaure, mwolff
Cc: #frameworks, michaelh, ngraham

D10325: [KFileWidget] Hide places frame and header

[Mark Gaiser]

markg added a comment.


  In https://phabricator.kde.org/D10325#201884, @broulik wrote:
  
  > > Would it be possible to show it as if it were locked? That would solve 
all the issues with it, right?
  >
  > I don't get it. That "lock" feature is entirely a Dolphin invention. It 
does exactly what I do here:
  >
  >   void DolphinDockWidget::setLocked(bool lock)
  >   {
  >   ...
  >   if (lock) {
  >   ...
  >   setTitleBarWidget(m_dockTitleBar);
  >   setFeatures(QDockWidget::NoDockWidgetFeatures);
  >
  >
  > with `m_dockTitleBar` being a custom widget for some added padding
  
  
  Looks like i was looking at the wrong picture. I was looking as your 
**after** image and comparing that to the "locked" state in dolphin.
  The image i was expecting is the one you call "crammed at the top" :)
  
  Imho, the "crammed at the top" version looks best as the "after" one just has 
some weird empty room above the panel now. But feel free to use the one you 
think fits best.
  
  A suggestion though if you do choose for the "after" version. Would it be 
possible to rearrange the layout then?
  So:
  
  - move the actions to the top, right above the panel.
  - move the location bar next to the actions
  
  I think that would look nice :)

REPOSITORY
  R241 KIO

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

To: broulik, #plasma, #vdg, #frameworks, ngraham, mart
Cc: markg, ngraham, plasma-devel, michaelh, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D10325: RFC: [KFileWidget] Hide places frame and header

[Mark Gaiser]

markg added a comment.


  Why does it show the panel as if it were unlocked?
  Your before image looks exactly like an unlocked frame in Dolphin.
  
  Would it be possible to show it as if it were locked? That would solve all 
the issues with it, right?

REPOSITORY
  R241 KIO

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

To: broulik, #plasma, #vdg, #frameworks
Cc: markg, ngraham, plasma-devel, michaelh, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D10314: Use F11 as the shortcut to toggle the aside preview

[Mark Gaiser]

markg added a comment.


  In https://phabricator.kde.org/D10314#201419, @ngraham wrote:
  
  > Fixed it!
  >
  > Changing Dolphin to Alt-P would open a can of worms since the other panels 
are triggered with Function keys, so I think F11 makes sense here.
  
  
  Nice!
  
  That could indeed be a "small issue" ;)
  Go for it. Commit (F11).

REPOSITORY
  R241 KIO

BRANCH
  master

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

To: ngraham, #frameworks, markg
Cc: markg, michaelh, ngraham