Re: Review Request 111272: Correctly handle executable names typed into KOpenWithDialog
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111272/#review35490 --- Ship it! - David Faure 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
Re: Review Request 111272: Correctly handle executable names typed into KOpenWithDialog
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
Re: Review Request 111272: Correctly handle executable names typed into KOpenWithDialog
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? David Faure wrote: 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. Well I changed my mind about this in the meantime so I converted the special constructor to a setExec function and marked it internal as you suggested originally. You are correct, it makes much easier to grep for usage. - Dawit --- 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
Re: Review Request 111272: Correctly handle executable names typed into KOpenWithDialog
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111272/#review35509 --- This review has been submitted with commit d27838a49e958c7c22eed8f0348ee3bccfb4e24e by Dawit Alemayehu to branch master. - Commit Hook 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
Re: Review Request 111272: Correctly handle executable names typed into KOpenWithDialog
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111272/#review35416 --- kdecore/services/kservice.h http://git.reviewboard.kde.org/r/111272/#comment25945 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. kio/kfile/kopenwithdialog.cpp http://git.reviewboard.kde.org/r/111272/#comment25944 this comment should move with the code that it affected - David Faure On July 2, 2013, 2:41 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111272/ --- (Updated July 2, 2013, 2:41 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
Re: Review Request 111272: Correctly handle executable names typed into KOpenWithDialog
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. 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? - Dawit --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111272/#review35416 --- On July 2, 2013, 2:41 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111272/ --- (Updated July 2, 2013, 2:41 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
Re: Review Request 111272: Correctly handle executable names typed into KOpenWithDialog
--- 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. Changes --- Updated the previous iteration of the patch to - Remove the entryPath() from the custom KService we create so as not to confuse KRun. - If the typed executable does not contain a %U or %F, but the matching service does, append those options so that KRun won't end up using kioexec. 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 (updated) - 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
Re: Review Request 111272: Correctly handle executable names typed into KOpenWithDialog
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111272/#review35391 --- This will create further trouble, I would say. What if an application installs several .desktop files, like: kmail --attach %U kmail --view %U kmail --check kmail --erase-my-harddisk By typing kmail in the open-with dialog, you would now randomly pick any of these? That doesn't sound like it will do what one would expect. Either we can kmail %U (directly or via a .desktop file), or we can't, but we certainly shouldn't take any existing .desktop file with any sort of stuff in the Exec line. - David Faure On June 27, 2013, 1:05 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111272/ --- (Updated June 27, 2013, 1:05 p.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 - 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
Re: Review Request 111272: Correctly handle executable names typed into KOpenWithDialog
On July 1, 2013, 6:17 p.m., David Faure wrote: This will create further trouble, I would say. What if an application installs several .desktop files, like: kmail --attach %U kmail --view %U kmail --check kmail --erase-my-harddisk By typing kmail in the open-with dialog, you would now randomly pick any of these? That doesn't sound like it will do what one would expect. Either we can kmail %U (directly or via a .desktop file), or we can't, but we certainly shouldn't take any existing .desktop file with any sort of stuff in the Exec line. That is indeed a problem. I did not think about that when I wrote the fix. The reason for this bug is actually simple. The service that currently gets created, when the typed in executable does not match the one from the service Exec= line, does not have the necessary property for KIO to be able to tell it is a KDE application. More specifically Categories= is not set. Hence, KIO thinks it is opening the file with a non KDE application. Anyhow, I will see if I can fix this as correctly as I can. I will probably need to add a new ctor to KService though. - Dawit --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111272/#review35391 --- On June 27, 2013, 1:05 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111272/ --- (Updated June 27, 2013, 1:05 p.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 - 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
Re: Review Request 111272: Correctly handle executable names typed into KOpenWithDialog
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111272/ --- (Updated July 2, 2013, 2:41 a.m.) Review request for kdelibs and David Faure. Changes --- Properly address this problem such that it is fixed for use cases the previous version of the patch ignored. 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 (updated) - 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
Review Request 111272: Correctly handle executable names typed into KOpenWithDialog
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111272/ --- 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 - 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
