In D7563#674682, @sitter wrote:

This really cannot land right now IMHO. Dolphin can actually deadlock itself because it uses way too much nested event looping and will be entirely unresponsive to mouse inputs when certain timers happen to trigger. A trivial way to reproduce this is to try and duplicate a file in file:/

Interesting... Yes if so that's a serious blocker.

Also making folders is actually not even implemented as the relevant mkdirjob seems to lack privilege enablement. That's not strictly speaking blocking but renders the actual UX broken.

I can confirm that I noticed this too. I also noticed that sometimes the dialog wouldn't show at all because the slave kept state that said that permission was denied (and hence it believed showing the dialog was redundant) when in fact my permission was never asked. Although this only happened for mkdir for me IIRC. So the slave might be mishandling state. At the same time, I'm not sure if the slave is in a position to handle such state (they're short-lived?).

In T12101#232196, @dfaure wrote:

Alternative solution: we port apps to libqt5keychain, which itself supports KWallet, gnome-keyring and libsecret (which I assume uses "secrets service").
See task T12219

In D29634#670419, @meven wrote:

In D29634#670377, @ngraham wrote:

Nice work.

In D29634#670159, @feverfew wrote:

I imagine something similar should be done for FileJob::write?

Yeah.

I guess you meant FileProtocol::write.
There is no need there, it uses QIODevice::write directly.

Hmmmm, this looks like a poor man's priority queue, would std::priority_queue not be appropriate here? (I'm not blocking here, idm tbh but it just seems more natural here to me). Also means that you don't have to explicitly enforce it all the time the queues are used, the priority is decided only in one place.

I imagine something similar should be done for FileJob::write?

Seems like something similar should also occur in FileJob::write?

In D29385#662435, @dfaure wrote:

In D29385#662422, @feverfew wrote:

Quick question, how does this affect D23384? Previously KRun used KIO::DesktopExecParser::resultingArguments() which handled the conversion of URLs to local KIOFuse URLs if needed, but now I believe this new API doesn't?

No worries, this should still happen.
KRun is actually split into three classes: OpenUrlJob, CommandLauncherJob and ApplicationLauncherJob. The last two delegate the work to an internal class, KProcessRunner (like KRun did).
So (to roll out the common case of a document-type file), once OpenUrlJob finds out which application to start, it calls ApplicationLauncherJob, which creates a KProcessRunner, which uses KIO::DesktopExecParser::resultingArguments().

Quick question, how does this affect D23384? Previously KRun used KIO::DesktopExecParser::resultingArguments() which handled the conversion of URLs to local KIOFuse URLs if needed, but now I believe this new API doesn't?

In D7563#653171, @elvisangelaccio wrote:

In D7563#650117, @ngraham wrote:

[insert I-have-no-idea-what-I'm-doing dog meme here]

When trying to create items in root-owned locations, I'm getting an errors saying "The process for the file protocol died unexpectedly." or else Dolphin simply crashes with a totally unhelpful backtrace.

If it's still happening, please have a look at https://community.kde.org/Guidelines_and_HOWTOs/Debugging/Debugging_IOSlaves

In D28535#642298, @feverfew wrote:

In D28535#642289, @elvisangelaccio wrote:

In D28535#640833, @fvogt wrote:

I assume there is a reason why MTPDevice::getDevice() has code for handling this very specific case, so I wouldn't just remove it without figuring out why: https://i.redd.it/hfnl7xo8yovy.gif

If not, that would indeed be the best option.

Unfortunately git blame doesn't seem to help us here.

I suggest to push this fix to master only and see what happens.

By pushing this to master would we still be able to throw it up to 20.04.* if we decide it's stable enough? (also need to know to know what to put down as the FIXED-IN in the commit message)?

Just an idea: can Baloo figure out this for us? This sounds like a feature that would fit at the baloo level, as it recurses through directories anyway and goes through their attributes.

Apologies for the delay in reviewing this. I believe FIXED-IN will unfortunately have to change to 20.08. Also looking at the screenshot, the text and the buttons/input box are misaligned, is this resolvable in this patch, or is that a wider issue with settings? Also note in the commit message "thatfunctionality" -> "that functionality".

In D28535#642289, @elvisangelaccio wrote:

In D28535#640833, @fvogt wrote:

I assume there is a reason why MTPDevice::getDevice() has code for handling this very specific case, so I wouldn't just remove it without figuring out why: https://i.redd.it/hfnl7xo8yovy.gif

If not, that would indeed be the best option.

Unfortunately git blame doesn't seem to help us here.

I suggest to push this fix to master only and see what happens.

Ok so I've updated the code as can be seen. Mucked around with disconnecting/connecting, no issues on my side at least.

• Don't try to release device in get method

In D28535#640703, @anthonyfieroni wrote:

In D28535#640699, @feverfew wrote:

So to be succinct, the only correct fix here is to change getDevice() to return m_mtpdevice?

Yes, then check if it's crash, in all other places LIBMTP_xxx should take care of and return false or nullptr depend of function returning value.

If you want to, feel free, I'm a bit tight on time. But I will say this. the whole getDevice() function confuses me. I'm not entirely sure why this if check is necessary at all. The lifetime of devices and its children (i.e storage) should be managed by the KMTPD daemon AFAICT. A device shouldn't be trying to re-open itself anywhere IMO. To me the getDevice() function should simply be a simple return to avoid this NULL issue happening. Even if a device doesn't exist anymore, no segfaults should happen when passing an "invalid" LIBMTP_mtpdevice_t to any other LIBMTP function?

In D28535#640674, @fvogt wrote:

What you're suggesting is to change MTPDevice::getDevice to return the old device if reopening fails - but reopening without releasing might not work.

@apol just found another one where this occurs, forgot about it but noticed it earlier as well. This doesn't affect KIOFuse as much though (as we don't listen to the processedSize() signal).

• Fix another slot lifetime

I'd like this, would make debugging a million times easier to. Probably faster as well...

@elvisangelaccio Done. I don't know how you choose for fixes in release branches to get into master so I'll let you sort that out (and what is it, for future reference)?

• Only do DBus call if mount is KIOFuse mount
• make interface member of class
• switch to camel case

Demo of feature:

Quick testing with fish protocol doesn't break anything KIOFuse side. Going to page in @ngraham, if it works for him with smb it's an all good from the "KIOFuse people" ;)

don't merge yet, looking at the code I sense something might break here but I need some time to do my own testing first.

I'll review this when I have a chance (sometime this week).

In D27810#621427, @sitter wrote:

I am pretty sure I picked that up somewhere, might be worth asking lxr for possible other places we set ACCESS to -1

I have extensive experience in Django, maybe in the summer I can help work in this direction.

• Fix &
• Use enum class
• Use enum instead of string comparison

I believe most of your (@sitter) comments (apart from the misaligned & probably were caused by me forgetting to rebase), lmk if otherwise.

• Merge branch 'master' into arcpatch-D21795_1
• Update version

Ok, I see what's going on here. Earlier I mucked up the diff a bit and had to go back to different diff id and reapply my changes. In the process I forgot to rebase onto master. Once I do that David's copyright will be back in (and the event loop goes with it).

With a brief look the ideal scenario is to be able to use DesktopExecParser::resultingArguments, or a class that calls it KRun. That should do the conversion. I'm just not sure if you have the information required to use it.

• Fix the diff

Ok I've commandeered this on request, as we're so close to getting this done. I've simply addressed sitter's comments here seeming as they're simple enough to do so without understanding the code that well. I haven't actually tested this in any capacity, as again, I'm not too familiar with this code and what it's trying to accomplish. If someone could point me in the correct direction I'll test as well.

• add const &

• Rebase
• Respond to sitter's comments

• Remote unnecessary newline

• Actually fix the bug

Good stuff, I'll admit I kind of skimmed over the bits that were rewrites error(...); return; -> Result::fail(), I assume you've mapped correctly there. Only minor comments from me.

In D27291#609358, @dfaure wrote:

feverfew: you're probably looking at master while this is a patch for the 19.12 branch.
See https://phabricator.kde.org/D26358 which happened in master.

I guess that makes this commit ok for 19.12, but it has to be redone differently in master.

I'm a bit confused here, isn't the protocol file now a JSON file? I'm looking at the repo and it appears to be the case, so I'm not seeing how this diff is seeing this as a rename?

• Merge branch 'master' into arcpatch-D26191
• adhere to protcol -> json switch

Cleanup code according to comments

Fix introduction of BIC method

In D26148#585958, @dfaure wrote:

In D26148#585944, @ngraham wrote:

Thanks @dfaure! Would you be able to also review D26191?

Yes, once it has been ported to virtual_hook as well.

Fix introduction of BIC method

Add fact that protocol supports truncation

Add feature to determine if protocol supports truncation

Avoid code duplication

Avoid free(NULL)

In D26148#582365, @apol wrote:

It would be interesting to explain why it's important or how it's to be used in the commit message.