markg (Mark Gaiser)
User

Projects

User does not belong to any projects.

Today

  • Clear sailing ahead.

Tomorrow

  • Clear sailing ahead.

Tuesday

  • Clear sailing ahead.

User Details

User Since
Apr 23 2016, 4:37 PM (164 w, 7 h)
Availability
Available

Recent Activity

Jan 30 2019

markg added a comment to D18563: Don't create directory tree when a new folder has a '/' in the name.

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?

Jan 30 2019, 3:24 PM · Frameworks

Jan 29 2019

markg added a comment to D18563: Don't create directory tree when a new folder has a '/' in the name.

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

Jan 29 2019, 8:39 PM · Frameworks

Jan 27 2019

markg added a comment to D18380: KIO: make file dialog columns resizable again (and movable).

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

Jan 27 2019, 6:06 PM · Frameworks
markg added a comment to D18380: KIO: make file dialog columns resizable again (and movable).

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!

Jan 27 2019, 5:40 PM · Frameworks
markg requested changes to D18380: KIO: make file dialog columns resizable again (and movable).

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.

Jan 27 2019, 4:15 PM · Frameworks

Aug 29 2018

markg added a comment to D15138: Consider all thumbnailers for the information panel and tooltips.

Oke, right. Go for it :)

Aug 29 2018, 10:06 PM · Dolphin
markg added a comment to D15138: Consider all thumbnailers for the information panel and tooltips.

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

Aug 29 2018, 10:24 AM · Dolphin
markg added a comment to D15138: Consider all thumbnailers for the information panel and tooltips.

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.

Aug 29 2018, 9:40 AM · Dolphin
markg added a comment to D15138: Consider all thumbnailers for the information panel and tooltips.

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.

Aug 29 2018, 9:30 AM · Dolphin
markg requested changes to D15138: Consider all thumbnailers for the information panel and tooltips.

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.

Aug 29 2018, 9:22 AM · Dolphin

Aug 25 2018

markg added a comment to D15071: Don't draw frames and shadows around images with transparency.

(repost from D15069)

Aug 25 2018, 5:05 PM · Frameworks
markg added a comment to D15069: Make thumbnail frame-and-shadow drawing criteria match those of the file dialog.

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

Aug 25 2018, 4:50 PM · Dolphin
markg added a comment to D15069: Make thumbnail frame-and-shadow drawing criteria match those of the file dialog.

Hi Nate,

Aug 25 2018, 11:28 AM · Dolphin
markg added a comment to D14442: Unify window and tab title.

Closed?
What's going on in here?

Aug 25 2018, 10:28 AM · Dolphin

Aug 22 2018

markg accepted D14442: Unify window and tab title.
Aug 22 2018, 1:48 PM · Dolphin
markg added a comment to D14442: Unify window and tab title.

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

Aug 22 2018, 1:47 PM · Dolphin

Aug 19 2018

markg added a comment to D14893: [recentdocuments:/] Filter out files that can't be browsed with a file manager.

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

Aug 19 2018, 11:07 AM · Frameworks, Dolphin

Aug 12 2018

markg added a comment to D14442: Unify window and tab title.

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.

Aug 12 2018, 12:43 PM · Dolphin

Aug 11 2018

markg added a comment to D14442: Unify window and tab title.

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.

Aug 11 2018, 4:22 PM · Dolphin

Aug 10 2018

markg added inline comments to D14442: Unify window and tab title.
Aug 10 2018, 7:44 PM · Dolphin

Aug 9 2018

markg added a comment to D14442: Unify window and tab title.

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.

Aug 9 2018, 2:56 PM · Dolphin
markg added inline comments to D14442: Unify window and tab title.
Aug 9 2018, 2:51 PM · Dolphin

Jul 28 2018

markg resigned from D14442: Unify window and tab title.

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

Jul 28 2018, 9:30 PM · Dolphin
markg added a comment to D14443: Use nullptr.

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);
Jul 28 2018, 5:04 PM · Dolphin
markg requested changes to D14442: Unify window and tab title.

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

Jul 28 2018, 4:56 PM · Dolphin

Jul 11 2018

markg added a comment to D13814: Speedup sort.

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.

Jul 11 2018, 4:19 PM · Dolphin

Jul 8 2018

markg added a comment to D13814: Speedup sort.

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.

Jul 8 2018, 10:00 AM · Dolphin

Jul 7 2018

markg added a comment to D13814: Speedup sort.

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

Jul 7 2018, 1:27 PM · Dolphin

Jul 2 2018

markg added a comment to D13124: Add Share action to Dolphin context menu.

Check out KFileItemActions::addServiceActionsTo

Actually, KFileItemActions::addPluginActionsTo ;)

Jul 2 2018, 8:50 PM · Frameworks, Dolphin
markg added a comment to D13124: Add Share action to Dolphin context menu.

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?

Jul 2 2018, 1:41 PM · Frameworks, Dolphin
markg accepted D13814: Speedup sort.

2x +1 = +2
Ship it :)

Jul 2 2018, 1:07 PM · Dolphin

Jul 1 2018

markg added a comment to D13814: Speedup 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.

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.

Jul 1 2018, 7:38 PM · Dolphin
markg added a comment to D13814: Speedup 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.

Jul 1 2018, 12:48 PM · Dolphin
markg added a comment to D13814: Speedup sort.

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.

Jul 1 2018, 12:41 PM · Dolphin

Jun 13 2018

markg added a comment to D13211: Enable comparing KFileItems by url.

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.

Jun 13 2018, 8:42 PM · Frameworks
markg added a comment to D13474: Remove "Use Default Location" button in settings window.

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.
Jun 13 2018, 8:20 PM · Dolphin

Jun 5 2018

markg added a comment to D13358: Add new class that is a model of numbers between two values.

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

Jun 5 2018, 12:18 PM · Frameworks

Jun 3 2018

markg added a comment to D10702: Always use a job to delete files to avoid freezing process waiting on IO.

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

Jun 3 2018, 3:28 PM · Frameworks
markg added a comment to D13124: Add Share action to Dolphin context menu.

Friendly ping?

Jun 3 2018, 2:06 PM · Frameworks, Dolphin
markg added a comment to D10702: Always use a job to delete files to avoid freezing process waiting on IO.

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.

Jun 3 2018, 1:55 PM · Frameworks

May 31 2018

markg added a comment to D12218: Remove Reload button from the file dialogs' toolbar.

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.
May 31 2018, 7:41 PM · Frameworks
markg accepted D12732: Show a warning when running as the root user.

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?

May 31 2018, 7:36 PM · Dolphin

May 28 2018

markg added a comment to D13124: Add Share action to Dolphin context menu.

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

May 28 2018, 4:13 PM · Frameworks, Dolphin

May 27 2018

markg accepted D12795: Re-allow running Dolphin as the root user (but still not using sudo).
May 27 2018, 12:44 PM · Dolphin

May 26 2018

markg added a comment to D12464: Fix the test that broke after recent changes to the default Places items.

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

May 26 2018, 4:32 PM · Dolphin

May 22 2018

Restricted Application added a project to D12464: Fix the test that broke after recent changes to the default Places items: Dolphin.

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

May 22 2018, 5:48 PM · Dolphin

May 20 2018

markg added a comment to D12795: Re-allow running Dolphin as the root user (but still not using sudo).

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

Then could you help review it, at least?

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.

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.

May 20 2018, 7:57 PM · Dolphin
markg added a comment to D12795: Re-allow running Dolphin as the root user (but still not using sudo).

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

May 20 2018, 7:33 PM · Dolphin
markg added a comment to D12795: Re-allow running Dolphin as the root user (but still not using sudo).
  • "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.

May 20 2018, 7:04 PM · Dolphin
markg added inline comments to D12795: Re-allow running Dolphin as the root user (but still not using sudo).
May 20 2018, 1:43 PM · Dolphin

May 17 2018

markg accepted D12795: Re-allow running Dolphin as the root user (but still not using sudo).

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.

May 17 2018, 11:45 PM · Dolphin
markg added a comment to D12911: Add 'Sort By' and 'View Mode' into Dolphin file context menus.

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

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

May 17 2018, 11:41 PM · Dolphin
markg added a comment to D12911: Add 'Sort By' and 'View Mode' into Dolphin file context menus.

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

May 17 2018, 6:58 PM · Dolphin

May 14 2018

markg added a comment to D12795: Re-allow running Dolphin as the root user (but still not using sudo).

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?

May 14 2018, 4:32 PM · Dolphin

May 11 2018

markg added a comment to D12795: Re-allow running Dolphin as the root user (but still not using sudo).
In D12795#260849, @zzag 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.

May 11 2018, 6:00 PM · Dolphin

May 10 2018

markg accepted D12795: Re-allow running Dolphin as the root user (but still not using sudo).

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

May 10 2018, 7:30 PM · Dolphin
markg added a comment to D12795: Re-allow running Dolphin as the root user (but still not using sudo).

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!

May 10 2018, 6:41 PM · Dolphin
markg added a comment to D12795: Re-allow running Dolphin as the root user (but still not using sudo).

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

May 10 2018, 12:28 AM · Dolphin

May 9 2018

markg added inline comments to D12605: added 'add_to_places' action to file menu and as a shortcut.
May 9 2018, 1:46 PM · Dolphin
markg added a comment to D12765: Use a more intuitive, user-friendly name for Samba share access.

Which ones, and where do they live?

May 9 2018, 1:15 PM
markg accepted D12765: Use a more intuitive, user-friendly name for Samba share access.

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

May 9 2018, 12:46 PM
markg added a comment to D12732: Show a warning when running as the root user.

I missed this reply so responding on it now.

May 9 2018, 12:42 PM · Dolphin

May 8 2018

markg added a comment to D12732: Show a warning when running as the root user.

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.

May 8 2018, 10:43 PM · Dolphin
markg added a comment to T8655: Settle on a single style for radio buttons with a title.

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

May 8 2018, 10:37 PM · VDG
markg added a comment to D12732: Show a warning when running as the root user.
  1. 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.

May 8 2018, 10:25 PM · Dolphin
markg updated subscribers of D12732: Show a warning when running as the root user.

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.

May 8 2018, 10:39 AM · Dolphin

May 7 2018

markg accepted D12731: Disable all the 'create new' items when in the Trash.

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

May 7 2018, 10:53 AM
markg added a comment to D12732: Show a warning when running as the root user.

I wonder if the check is accurate.

May 7 2018, 10:43 AM · Dolphin

Apr 30 2018

markg resigned from D12321: Hide file preview when icon is too small.

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

Apr 30 2018, 3:08 PM · Frameworks

Apr 25 2018

markg added a comment to D12321: Hide file preview when icon is too small.

@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.
Apr 25 2018, 10:18 PM · Frameworks

Apr 23 2018

markg added a comment to D12321: Hide file preview when icon is too small.

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

Apr 23 2018, 5:04 PM · Frameworks
markg added a comment to D12321: Hide file preview when icon is too small.

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?

Apr 23 2018, 3:38 PM · Frameworks
markg added a comment to D12321: Hide file preview when icon is too small.

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.

Apr 23 2018, 3:36 PM · Frameworks
markg added a comment to D12321: Hide file preview when icon is too small.

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?

Apr 23 2018, 2:40 PM · Frameworks
markg added a comment to D12321: Hide file preview when icon is too small.

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?

Apr 23 2018, 2:03 PM · Frameworks
markg added a comment to D12321: Hide file preview when icon is too small.

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.

Apr 23 2018, 12:44 PM · Frameworks
markg added a comment to D12321: Hide file preview when icon is too small.

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.

Apr 23 2018, 12:24 PM · Frameworks
markg added a comment to D12321: Hide file preview when icon is too small.

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.

Apr 23 2018, 12:12 PM · Frameworks
markg added a comment to D12321: Hide file preview when icon is too small.

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.

Apr 23 2018, 11:29 AM · Frameworks
markg accepted D12464: Fix the test that broke after recent changes to the default Places items.

Ha, awesome :)

Apr 23 2018, 11:07 AM · Dolphin
markg requested changes to D12321: Hide file preview when icon is too small.

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

Apr 23 2018, 11:03 AM · Frameworks

Apr 22 2018

markg added a comment to D8208: [WIP] Add Duplicate feature.

(writing it in this revision as well ;))

Apr 22 2018, 3:27 PM · Dolphin
markg added inline comments to D8206: Add Duplicate feature.
Apr 22 2018, 3:23 PM · Dolphin
markg accepted D11768: Add Desktop and Downloads to the default list of Places.

Blocking it any longer seems rude to me :)

Apr 22 2018, 3:15 PM · Frameworks

Apr 20 2018

markg added a comment to D11884: Move "Open" actions to the top of the context menu for files.

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

Apr 20 2018, 2:17 PM · Dolphin

Apr 19 2018

markg added a comment to D11884: Move "Open" actions to the top of the context menu for files.
Apr 19 2018, 7:24 PM · Dolphin
markg added a comment to D12337: Give the file dialogs a "Sort by" menu button on the toolbar.

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

Apr 19 2018, 7:20 PM · Frameworks
markg added a comment to D11884: Move "Open" actions to the top of the context menu for files.

+1
I like it :)

Apr 19 2018, 3:07 PM · Dolphin
markg added a comment to D12337: Give the file dialogs a "Sort by" menu button on the toolbar.

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.

Apr 19 2018, 3:04 PM · Frameworks
markg added a comment to D12337: Give the file dialogs a "Sort by" menu button on the toolbar.

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.

Apr 19 2018, 2:58 PM · Frameworks

Apr 16 2018

markg added a comment to D12202: Add support for touch scrolling.

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

Apr 16 2018, 7:18 PM
markg added a comment to D12202: Add support for touch scrolling.

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

Apr 16 2018, 6:43 PM
markg added a comment to D12218: Remove Reload button from the file dialogs' toolbar.

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

Apr 16 2018, 6:42 PM · Frameworks
markg added a comment to D12218: Remove Reload button from the file dialogs' toolbar.

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?

Apr 16 2018, 3:25 PM · Frameworks
markg added a comment to D12218: Remove Reload button from the file dialogs' toolbar.

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

Apr 16 2018, 2:34 PM · Frameworks
markg added a comment to D11204: Support NTFS hidden files.

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.
Apr 16 2018, 12:31 PM · Frameworks
markg added a comment to D12202: Add support for touch scrolling.

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

Apr 16 2018, 10:10 AM
markg added a comment to D12202: Add support for touch scrolling.

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?

Apr 16 2018, 10:08 AM

Apr 11 2018

markg accepted D11245: infopanel: Add choice of date display formats.
Apr 11 2018, 9:49 AM · KDE Applications, Dolphin

Apr 3 2018

markg accepted D11894: Show folder previews for items that have a Places Panel entry.

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

Apr 3 2018, 1:51 PM · Dolphin