dfaure marked 5 inline comments as done. dfaure added a comment.
In D29385#662336 <https://phabricator.kde.org/D29385#662336>, @svuorela wrote: > I've been looking at it for a while, and after trying to decipher the long lambdas, which might have been better as named functions, I think it is, beside Kossebau's comments, fine. Interesting comment. I thought it was actually more readable to keep that code together, rather than having to jump around in the file to find the right slots. INLINE COMMENTS > kossebau wrote in openurljobtest.cpp:108 > why no simple range-based for loop? Good point, copy/pasted from another (old) test. > kossebau wrote in openurljob.h:73 > Makes me wonder if those runflags should not be rather part of KIO namespace, > to decouple classes here. Interesting point, let's discuss it quickly (because if we want to change that, I need to redo the kio RC1 tag for 5.70, which has ApplicationLauncherJob). But I'm not sure we should do that, anyway. On hindsight, I would rather change this one to be setTemporaryFiles(bool b); I don't like all the flag-conversion hell that we end up with otherwise. Stuff like RunFlags flags = tempFile ? KRun::DeleteTemporaryFiles : RunFlags(); if (runExecutables) { flags |= KRun::RunExecutables; } The input data, all the way up, is for sure not in the form of a set of job flags. So I find it more readable to have code like setSomething(conditionForSomething) than to have a bunch of flag manipulation code. > kossebau wrote in openurljob.h:96 > Could this and the following bool flags be turned into some flags instead? > Just wondering, not looked further. > > Also wondering if there should not be some getter as well, when using a > flagset that would be just a single method/symbol. With this one in particular, since it's on by default, we'd have to either have DefaultRunFlags that includes RunExecutables --- or we'd have to make the flag DoNotRunExecutables. I don't like either.... Technically yes it could all be flags, but I'd much rather have independent setters. The TempFile flag is a good example why: if it's shared with ApplicationLauncherJob by being in the KIO namespace, does that mean we also put all of those in there? But then people can write things that don't make sense like ApplicationLauncherJob *job = ...; job->setFlags(DoNotRunExecutables). What? That flag doesn't apply to that job. If there's one thing that job does, it's to run executables :-) [but via desktop files, while that flag is actually about what OpenUrlJob should do when opening an executable directly]. Sharing flags between jobs who need a different set, is a problem. It ends up with docu like "only these flags make sense here". Not great. I don't see a use case for getters, but we can certainly add them (either now or as needed). > kossebau wrote in openurljob.h:127 > -> "mimeTypeFound"? would match other casing. Well spotted, thanks. > kossebau wrote in openurljobhandlerinterface.h:33 > Please prepend a line > > * @class OpenUrlJobHandlerInterface openurljobhandlerinterface.h > <KIO/OpenUrlJobHandlerInterface> > > at the top, to trick doxygen into documenting the full CamelCase include. Ah! I was wondering what that was about... > kossebau wrote in openurljobhandlerinterface.h:84 > Prevent use of nested Private class here, declare a class > OpenUrlJobHandlerInterfacePrivate outside, also use QScopedPointer for > consistency and preparedness in case there ever is an actual object. LOL I was the one arguing against nested Private classes recently. Looks like I forgot that when copy/pasting here. Thanks! Hmm note that using QScopedPointer requires actually defining (in the .cpp) a dummy private class, even if there's no "new" anywhere (generated code wants to delete it). REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D29385 To: dfaure, ahmadsamir, broulik, meven, kossebau, davidedmundson, nicolasfella, svuorela Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
