> On July 2, 2013, 7:48 a.m., David Faure wrote: > > kdecore/services/kservice.h, line 82 > > <http://git.reviewboard.kde.org/r/111272/diff/2/?file=167013#file167013line82> > > > > How about a setExec() method rather than a special constructor? This > > seems a lot clearer to me (and e.g. easier to grep for in order to find > > usages, etc.). > > > > > > But anyway, I have my doubts that this really works: returning a > > KService that isn't exactly like the one in ksycoca seems fragile to me. > > E.g. when krun uses that service, it will call KToolInvocation with the > > entry path to the service - i.e. just the path to the desktop file. The > > modified Exec field (in memory) won't be used in that case. > > > > What KRun can do, though, is work with a "temp" kservice, created like > > this: > > > > KService::Ptr service(new KService(_name, _exec, _icon)); > > > > This is detected as "not in ksycoca" and therefore not given to > > KToolInvocation (but to runTempService). > > > > I think this would recreate the issue you saw though, since indeed > > nowadays it's not enough to have %U in the exec line, one must also have > > Categories or X-KDE-Protocols indicating that KIO protocols are supported. > > But this could be copied from the orig file, using a new setter in > > KService. Hmm. > > > > Maybe it would be simpler to just clear entryPath() in the > > KService::setExec() I'm suggesting initially. i.e.: if we're modifying the > > KService then the path to the file that has been cached in ksycoca, no > > longer applies. > > And then KRun will go to runTempService, when running it. > > > > Dawit Alemayehu wrote: > Well I created a special constructor instead of a setter because I wanted > to limit the scope of change to KService's behavior as low as possible. If I > added a setExec, it would allow the "Exec=" to be changed at anytime > regardless of how KService was created. To me that did not make sense as the > need to change the "Exec=" line is very specific scenarios like the one we > have in the openWith dialog. Moreover, we already have several different > ctors for different use cases right now. Adding one that could be merged with > an existing on in KF5 would be the lowest impact change that could be made to > KService itself. Are you sure adding a setExec() is the better way to go in > this case?
I agree that from the KService point of view, this is really a "very specific scenario". That was actually my reason for a setExec, documented as internal, and easy to find again later. But OK, this mostly works with a separate constructor too. Please mark it as internal, only for use by KOpenWithDialog. - David ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111272/#review35416 ----------------------------------------------------------- On July 3, 2013, 2:22 a.m., Dawit Alemayehu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/111272/ > ----------------------------------------------------------- > > (Updated July 3, 2013, 2:22 a.m.) > > > Review request for kdelibs and David Faure. > > > Description > ------- > > The attached patch addresses a bug where a user enters the name of a KDE > application in OpenWith dialog to open a remote file and the file is opened > as if the user requested to open it with a non KDE application. That is a > local copy of the file is created first. Currently this problem can be > reproduced with kate because the "Exec=" line in its desktop file contains an > additional option, "-b". > > Note that this patch only addresses the specific condition where the user > only typed in the KDE executable name. Other scenarios, like the user typing > in not only the name of the KDE app but also additional command line options, > will still produce this same issue. > > > This addresses bug 222519. > http://bugs.kde.org/show_bug.cgi?id=222519 > > > Diffs > ----- > > kdecore/services/kservice.h 3843bad > kdecore/services/kservice.cpp e2cc86f > kio/kfile/kopenwithdialog.cpp 84465cd > > Diff: http://git.reviewboard.kde.org/r/111272/diff/ > > > Testing > ------- > > Try to open a remote text or source file by typing "kate" in the open with > dialog. > > > Thanks, > > Dawit Alemayehu > >
