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?

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.

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).

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):

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!

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.

Oke, right. Go for it :)

@ngraham could you chime in from a usability point of view?

In D15138#317083, @broulik wrote:

I don't think we should want that.

Why not? The tooltip is shown on-demand when you hover a file for a while. There you get more information when the thumbnailer is available. Just showing a bigger file icon makes no sense for the tooltip.

In D15138#317081, @fvogt wrote:

Of course, that's the main and only point of this patch.

I think it would be best to use the thumbnailers that are enabled in Dolphin for the preview as well.

No, because (quoting the description):

Having just a larger version of the placeholder icon from the theme
doesn't make a lot of sense, so try the best to get a proper preview.

This will give an inconsistency with the thumbnailes that are enabled in Dolphin's settings and the preview tooltip.
The preview tooltip would now always show a generated thumbnail regardless of the dolphin settings.

(repost from D15069)

In D15069#315402, @ngraham wrote:

@markg. Thanks for your comments. Do you think we could take the discussion to D15071? I imagine that making Dolphin's frame drawing behavior consistent with the file dialog is probably not very controversial, but I can see how the determination regarding what we draw a frame around in the first place might require more discussion.

Therefore, I'd like to use this patch to make Dolphin use whatever we decide for the file dialog. Therefore do you think we can take the discussion regarding regarding what we draw a frame around over to D15071 so we can have it on one place?

Hi Nate,

Closed?
What's going on in here?

In D14442#311970, @ngraham wrote:

I'm pretty much okay with the current approach. @markg, thoughts?

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

In D14442#306663, @markg wrote:

A possible fix for you would be to listen for the KCoreDirLister::started signal, but that object alone is hidden quite deeply (It's a member of KFileItemModel) and i don't even know if that would work. You're best of making a small test project to test this out.
Also, if that is not working, it would probably be accepted as a patch to have the rootitem set when started is emitted. That would be a patch to KCoreDirLister.

Oke, that is quite a bit of layers to figure out...
DolphinView::rootItem, which you can access in DolphinViewContainer::getCaption as m_view->rootItem(); is what you want to use.

In D14442#305753, @hallas wrote:

I would really like to have used the "Pretty Name" from the UDSEntry of the
current URL, but it is not easily available. I have tried to retrive it using
KIO::stat but not all protocols handle this very well, i.e. remote protocol.

That looks much more sensible to me :)
Removing the -1.

It's good to have that!
Also note that you "can" do that in a one-liner with C++14 with a little known library feature.

delete std::exchange(object, nullptr);

I think the logic you've chosen is wrong.
Lets take this fruit store analogy. You have a fruit store with an owner and fruit.
The fruit is dumb and can't do anything outside itself (this would be a tab in dolphin).
The owner (dolphinmainwindow) can look at fruit en change whatever it wants to change in the store (whole of dolphin).

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.

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.

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 0x00007fc0ba4e5904 in icu_61_1::RuleBasedCollator::getAttribute(UColAttribute, UErrorCode&) const () from /usr/lib64/libicui18n.so.61.1
#7 0x00007fc0ba4e6dc6 in icu_61_1::RuleBasedCollator::setAttribute(UColAttribute, UColAttributeValue, UErrorCode&) () from /usr/lib64/libicui18n.so.61.1
#8 0x00007fc0c0dfec25 in QCollatorPrivate::init() () at tools/qcollator_icu.cpp:82
#9 0x00007fc0c0dfedab in QCollator::compare (this=0x1928070, s1=0x1bd6b98, len1=19, s2=0x1d70af8, len2=17) at tools/qcollator_icu.cpp:109
#10 0x00007fc0c880ac30 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 0x00007fc0c880b2a2 in KFileItemModel::lessThan (this=this@entry=0x1928040, a=0x1c0e1f0, b=0x1cfbdf0, collator=...) at /g/5kde/kde/applications/dolphin/src/kitemviews/kfileitemmodel.cpp:1707
#12 0x00007fc0c8811715 in KFileItemModel::<lambda(const KFileItemModel::ItemData*, const KFileItemModel::ItemData*)>::operator() (__closure=0x7ffe8d4307a8, b=<optimized out>, a=<optimized out>) at /g/5kde/kde/applications/dolphin/src/kitemviews/kfileitemmodel.cpp:1717

In D13124#286263, @elvisangelaccio wrote:

In D13124#286115, @broulik wrote:

Check out KFileItemActions::addServiceActionsTo

Actually, KFileItemActions::addPluginActionsTo ;)

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?

2x +1 = +2
Ship it :)

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.

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.

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.

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.

In D13474#277933, @elvisangelaccio wrote:

I'm not 100% convinced. The Defaults button resets all dolphin settings, while this one resets only this particular option.
The problem is we don't know if someone relies on this "feature", and in theory we could break someone else's workflow (unlikely, but still).

Some more reasons why we might want to keep the button around:

• Nobody filed a bug to remove it, AFAIK.
• Firefox has the very same button.
• The after screenshot imho looks worse, since now we have a huge button that stands out.

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 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

In D13124#272698, @nicolasfella wrote:

Friendly ping?

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 100000 files of 1 byte, 100000 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
1b		1,586	1,738	1,684	1,66933333333333
1k	        1,719	1,777	1,649	1,715
3mb	        9,206	9,164	8,419	8,92966666666667
1gb	        30,736	20,559	29,981	27,092

after
1b 		4,637	1,721	1,599	2,65233333333333
1k	        32,186	1,726	1,685	11,8656666666667
3mb	        2,491	7,287	7,896	5,89133333333333
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.

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.

In D12732#271563, @ngraham wrote:

Now that D12795 has landed, what do Dolphin folks think about reviving this (without any of the main.cpp changes) so that Dolphin shows a warning when run as the root user?

I like this :)
Thanks a lot for working on it!

In D12464#268656, @elvisangelaccio wrote:

@markg Problem is, we can't bump the KIO minimum version in the stable branch.

In D12464#253281, @elvisangelaccio wrote:

This will only fix the test if you have a recent enough version of KIO. I wonder if we can find a more robust solution (which we'd also need in the 18.04 branch).

In D12795#265634, @ngraham wrote:

In D12795#265631, @graesslin wrote:

The ideas of sandbox baloo_file_extractor are after all based on my sandboxing for kscreenlocker.

Then could you help review it, at least?

In D12795#265631, @graesslin wrote:

I never used an exploit. What I would use is the chrome to download behavior. That is not fixed, it's still the default.

Ah, so the problem is that the user actually opened a malicious file. On macOS at least, Finder prompts the user before they can open a file that was auto-downloaded. Perhaps we need to do the same.

In D12795#265619, @graesslin wrote:

baloo is just one example. Every program on the user's system can be abused to it. You can also hope that the user just clicks it. Download a video, which uses a vulnerability in vlc, download a zip file which uses a vulnerability in gzip. There are just so many ways. All you need is a simple bug in an application.

Sounds like you've just described why security is hard. :) You've also described the proper response to a security threat: doing the hard work to harden apps, not the easy and lazy approach of simply disabling a feature that's potentially vulnerable to them. It's not like we should disable opening videos in VLC or zip files in Ark just because there are security vulnerabilities.

Since you've said you prefer to stay in the KWin world, ultimately this is the Dolphin maintainers' decision. We've heard lots of arguments on both sides, now I think it's fine for someone with some authority here to make a decision. However I would note that while not a maintainer, I'm someone who's actively involved in Dolphin's development and who submits a lot of patches, so I hope that counts for more than nothing.

In D12795#265616, @graesslin wrote:

You would also have to run a malicious application which is quite unlikely if you stick to vendor packages (but sure, there probably is a very small chance that a malicious package lands in the dist repository).

nope, sorry. The exploit I wrote would work through a drive-by download through an Internet browser. The world we live in sucks :-(

In D12795#265590, @graesslin wrote:

In D12795#265587, @ngraham wrote:

• "You broke my workflow using sudo dolphin to edit root-owned files."
• "You broke my use of a root GUI session in $DISTRO."

These are good reactions. Not so good reactions are sending hate mails to developers. If that happens I cannot take them serious. I tried to look up one of the incidents, but I cannot find it on google+ anymore. This has nothing to do that we change something users dislike. We do that constantly. Any change angers users. You will someday also notice this. You are relatively young in the KDE business. With time you will notice that you do changes that angers your users. You do something which improves things and it angers some other users. That's something which happens and we have to live with. We just cannot please all users. The important question is how you react to the change: explain the use cases you have or just let out your anger at random persons. In the latter case it's to me a clear sign that you don't have real arguments.

I did the change in kate with the information about sudoedit which motivated the change in dolphin AFAIK. This broke peoples workflows. It broke it really hard. Nevertheless the overwhelming feedback I got was: "wow awesome I didn't know that sudoedit exists, that's much better than my workflow before." It's totally fine to question users' workflows. With the change in kate I questioned the workflow of running the gui to edit as root in general. That would also be the answer to the first question. You need to edit files as root? Use sudoedit.

Now instead of trying to support the users' workflow of running the complete session as root I would question it. Why do they run it as root? What's the real usecase they have for it? Maybe there is no need, maybe they don't need it and their workflow improves when we can suggest them something better? If I get a request for a new feature I normally ask what's the actual usecase for it. Very often I have a feeling things are requested out of ignorance on how else something can be achieved. We don't need to follow and implement everything in the sake of usability. Usability is also questioning the users and provide the best workflow for them. And that might not be the workflow they are used to. I question that running a session as root is the best workflow for the issues user's try to solve with it.

Yes I used to do that 15 years ago as well. I logged into the suse session as root to do stuff. That was before I learned about sudo, before polkit existed. Workflows change and that's good so. I learned that using Kubuntu with sudo is much easier to use than logging into as root. It was new to me, I wasn't used to the concept of not having a root user. But my workflows improved. It improved because the distribution took away a part I used to have.

I think this one has been open for long enough for people to catch it if they want to voice their opinion.
@elvisangelaccio still needs to make his red light green but after that i'd just ship it.

In D12911#264341, @rkflx wrote:

I already voiced my opinion, but I'm also in favour of arguing with facts on not assumptions ;)

In D12911#264323, @markg wrote:

But things are going to change.
The sort mode is going to be in the next dolphin toolbar by default (right?) so the toolbar then has view and sort. Right there with less clicks then the RMB would have been.

Source? Currently this is planned for the file dialog, applying the same change for Dolphin would be possible (but needs a separate discussion).

In D12911#263862, @elvisangelaccio wrote:

Hmm, 'Sort By' could make sense, but why also 'View Mode'? What's wrong with the toolbar buttons?

Just curious, am i right in assuming that this "run as root" is ultimately going to ge gone as soon as KAuth is in place?
And if so, how would Dolphin - as root - then work on non-plasma environments?

In D12795#260849, @zzag wrote:

In D12795#260848, @ngraham wrote:

The problem is that USER is always set to "root" when you use sudo. That's why SUDO_USER exists, as far as I know.

It looks like kdesu sets KDESU_USER.

In D12795#260779, @ngraham wrote:

I'm sorry, I think I confused you with a stupid typo in the test plan section. :-( Fixed now.

In D12795#260449, @ngraham wrote:

Right: if our uid is zero, but $SUDO_USER is set to something, then we're actually a non-root user using sudo, not the root user itself. We have to check $SUDO_USER and not $USER because $USER is set to "root" for the sudo use case.

Does it not work for you? It works for me. Though if there's a better way, I'm all ears. As I'm sure you've noticed by know, I'm a terrible programmer, though I'm trying to improve all the time!

Log in as the root user and run `sudo dolphin; you can do it.

In D12765#260054, @ngraham wrote:

Which ones, and where do they live?

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

I missed this reply so responding on it now.

In D12732#259773, @nicolasfella wrote:

AFAIU the exploit works like this:

A user has a normal, non-root session running which is infected with a malicious program. The program now waits until the user is running 'sudo dolphin' and now can use the exploit to gain elevated privileges.

If the user is running as root anyway there is no additional harm, because most likely the malicious program will have root privileges anyway.

I prefer with groupboxes, but not the breeze and oxygen groupbox style.
The styles with the groupbox title embedded within the top horizontal line strikes me as really fancy.
For reference: http://www.beginning-kdevelop-programming.co.uk/Chapter%20Four/BKPChapterFour_html_46b5f46a.png

In D12732#259720, @elvisangelaccio wrote:

In D12732#259335, @ngraham wrote:

3. It is illogical to damage the user experience in the name of security; we fail at the goal of securing the user if the user becomes unable to use our software in the first place (e.g. for the Kali distro). If you lose your house key, you don't barricade the door so that nobody can go in or out until the locksmith comes. It is user-hostile, inappropriate, and illogical to remove a feature before its replacement is ready.

Finding the right trade-off between security and usability is a hard problem, yes. Note that running Dolphin as root is not a "feature". It's just something that happened to be possible but it has all sort of problems and it's never been supported by design.

In D12732#259134, @ngraham wrote:

In D12732#259043, @markg wrote:

I wonder if the check is accurate.

I think you need to replace the == root string check to either the one it was before or something like:
KUser(getuid()).isSuperUser() (this basically is getuid() == 0)

It's accurate. getuid() == 0 returns true for a regular user using sudo as well as the actual root user. Capturing that distinction is a part of this patch.

Hahaha, that screenshot made me laugh (new actions in the trash, i never noticed that but it's a nice bug).
Ship it!

I wonder if the check is accurate.

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

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.

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).

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?

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?

Why torture yourself with tiny icons in a tiny window?

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.

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?

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?

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.

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.

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.

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.

Ha, awesome :)

That looks really fancy! :)
Yet i have to give it a -1..

(writing it in this revision as well ;))

Blocking it any longer seems rude to me :)

In D11884#249992, @ngraham wrote:

In D11884#249989, @markg wrote:

In D11884#249797, @ngraham wrote:

Thanks! @elvisangelaccio?

I'm increasingly doubting whether i like this change.. Sure, it gives more room.
But the downside is that the actions (who were directly above the view they were controlling) are now above the places.
That might not be ideal as it now can be interpreted that the places are reloaded, or once place back/forward...

Just voicing my concern, not changing my +1 :) (yet)

Is this comment intended for this patch, or a different one...?

In D11884#249797, @ngraham wrote:

Thanks! @elvisangelaccio?

In D12337#249814, @rkflx wrote:

In D12337#249528, @rkflx wrote:

@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.)

+1
I like it :)

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.

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.

In D12202#247571, @abalaji wrote:

I "abandonded" it, does that not do that?

In D12202#247505, @abalaji wrote:

Sorry @markg, I created this revision by accident while setting up arc, D12162 is the leading one

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. :)

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?

I personally find the reload button much more useful to have there.

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.

Which change is the leading one now btw?
This or D12162?

I have a small request before i review and test it.
The code has some commented code in it, a redundant QTimer include (in the header where it also forward declares it) and a commented todo. I'm guessing you still need to do some cleanup?

The only reason i can think of (but it's not that since it's not in the code) is to only do this for local drives, not network mounted drives.
But as that's not it, +1 :)