Bug 271824 - Dolphin runs scripts in a wrong working directory
Summary: Dolphin runs scripts in a wrong working directory
Status: RESOLVED FIXED
Alias: None
Product: dolphin
Classification: Applications
Component: general (show other bugs)
Version: 16.12.2
Platform: Unlisted Binaries Linux
: NOR major with 20 votes (vote)
Target Milestone: ---
Assignee: Peter Penz
URL:
Keywords: investigated, reproducible
: 274256 282909 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-04-27 09:05 UTC by Nikita Churaev
Modified: 2014-06-25 22:07 UTC (History)
6 users (show)

See Also:
Latest Commit:
Version Fixed In: 4.7.3


Attachments
Fix for this problem (491 bytes, patch)
2011-07-23 14:31 UTC, Tautvydas Andrikys
Details
Additional patch to kdelibs (5.24 KB, patch)
2011-09-28 14:35 UTC, Tautvydas Andrikys
Details
New patch (1.67 KB, patch)
2011-09-30 18:46 UTC, Tautvydas Andrikys
Details
patch for kdelibs (985 bytes, patch)
2011-10-09 14:08 UTC, Tautvydas Andrikys
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Churaev 2011-04-27 09:05:32 UTC
Version:           unspecified
OS:                Linux

It's always ~/Documents, regardless of what directory you are in.

Reproducible: Always

Steps to Reproduce:
1. Do this in terminal:

cd $HOME
mkdir moomoo ; cd moomoo
echo '#!/bin/sh' > lol.sh
echo 'touch quxqux.txt' >> lol.sh
chmod u+x lol.sh

2. Run the lol.sh script in ~/moomoo by clicking it in Dolphin

3. Run find in terminal:

find $HOME -name 'quxqux.txt' 

Actual Results:  
Find outputs:

/home/you/Documents/moomoo.txt

Expected Results:  
Find outputs:

/home/you/moomoo/quxqux.txt
Comment 1 Frank Reininghaus 2011-04-28 12:24:11 UTC
Thanks for the bug report! I can reproduce this in the master branch (the file 'quxqux.txt' isn't created in 'Documents', but in  the home directory in my case, but that's still not what we want).
Comment 2 Frank Reininghaus 2011-05-27 20:51:04 UTC
*** Bug 274256 has been marked as a duplicate of this bug. ***
Comment 3 Frank Reininghaus 2011-05-28 14:56:16 UTC
The place in Dolphin where the script gets launched is in DolphinViewContainer::slotItemTriggered(const KFileItem& item); that function calls 'item.run()'. Then KFileItem::run(...) uses KRun to execute the script, where the working directory is set to KGlobalSettings::documentPath() in KRun::runCommand(...).

It seems that this was done intentionally (see bug 108510). It might be desirable to set the working directory to 'Documents' in some cases, but IMHO not for shell (or Perl/Python/...) scripts launched from Dolphin.

One possibility to resolve this would be to implement overloaded functions KFileItem::run(...) and KRun::KRun(...) that allow to pass the working directory from DolphinViewContainer.

@David: Do you have a better idea how to resolve this issue?
Comment 4 Georg 2011-05-31 09:20:15 UTC
Hi Folks,

the idea that the WM determines where the current directory is, is simply mad. The current directory and the Documents directory have nothing in common and it should not be mixed up, this is Unix and not M$. To determine the document save path is part of the application logic and cannot be worked around with the PWD.

Since this seems to be broken in KDE4 in general, I would at least vote for fixing this when applications are started from konqueror or dolphin, if possible.
A distinction by file type would be another source of bugs. Why does KDE not set an environment variable, say $DOCUMENTPATH, and if it is not set then the application uses $HOME. All applications can be started with the correct PWD and fine. 
Would this not be resolveable in the core FileOpen Dialogs and the like?

Why do we care so much? You cannot graphically call any script's, since they don't know where they are started. (even setting the path=. in the desktop file does not work).

Regards,
  Georg
Comment 5 Tautvydas Andrikys 2011-07-23 14:31:40 UTC
Created attachment 62120 [details]
Fix for this problem

This patch changes the working directory of launched file to currently active dolphin directory.
Comment 6 Frank Reininghaus 2011-09-27 23:23:19 UTC
Thanks for the patch, but it doesn't compile for me - there is no matching member function in KFileItem.
Comment 7 Frank Reininghaus 2011-09-27 23:25:06 UTC
*** Bug 282909 has been marked as a duplicate of this bug. ***
Comment 8 Tautvydas Andrikys 2011-09-28 14:35:48 UTC
Created attachment 64035 [details]
Additional patch to kdelibs

(In reply to comment #6)
> Thanks for the patch, but it doesn't compile for me - there is no matching
> member function in KFileItem.

Oops forgot to attach kdelibs patch :)
Comment 9 Frank Reininghaus 2011-09-29 19:23:05 UTC
Thanks for the quick reply and the kdelibs patch!

The approach looks good overall (but note that I'm not the maintainer of that code, so this still needs to be reviewed by other people).

There is a little problem though: If this patch should be included in kdelibs 4.x (and not only in the frameworks branch), binary compatibility must be maintained (see [1] for details).

Essentially, this means that you cannot add new parameters to a function, even if they have default values -> you need to add a new function, which has one parameter more. In the .cpp files, you can keep your modified functions though, and just add a small function with the old signature that calls the new function.

If you have any questions about that, don't hesitate to ask :-)

Could you try to update your kdelibs patch and submit it to http://reviewboard.kde.org/ ?

[1] http://techbase.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B
Comment 10 Tautvydas Andrikys 2011-09-30 18:46:58 UTC
Created attachment 64104 [details]
New patch

Change my patch so that kdelibs changes are not required.

> Thanks for the quick reply and the kdelibs patch!
>
> The approach looks good overall (but note that 
> I'm not the maintainer of that code, so this
> still needs to be reviewed by other people).
>
> There is a little problem though: If this
> patch should be included in kdelibs
> 4.x (and not only in the frameworks branch), 
> binary compatibility must be maintained (see [1] for details).
>
> Essentially, this means that you cannot add new parameters 
> to a function, even if they have default values -> you need
> to add a new function, which has one parameter more. In 
> the .cpp files, you can keep your modified functions though,
> and just add a small function with the old signature
> that calls the new function.
>
> If you have any questions about that, don't hesitate to ask :-)
> 
> Could you try to update your kdelibs patch and submit it to
> http://reviewboard.kde.org/ ?
>
> [1] http://techbase.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B
Comment 11 Frank Reininghaus 2011-09-30 22:20:52 UTC
Hm, actually I think that it is actually better to add the functionality to kdelibs, but I'm not the one who has the final say over that.

@Peter, David: do you have ideas about this?
Comment 12 Peter Penz 2011-10-01 14:19:05 UTC
I'm not sure - I'd also prefer if this is done in kdelibs instead of doing it on application level. David is very busy and usually cannot reply to bug-reports as he gets too much of them each day. The best thing to get this discussed by people who are more familiar with this topic than me is to provide a merge-request for kdelibs like Frank mentioned in comment #9. If the feedback there is that it should be done on application level then we will do this, otherwise interface issues etc. will get discussed on the reviewboard so that this can be pushed into kdelibs.
Comment 13 Tautvydas Andrikys 2011-10-02 08:00:19 UTC
(In reply to comment #12)
> I'm not sure - I'd also prefer if this is done in kdelibs instead of doing it
> on application level. David is very busy and usually cannot reply to
> bug-reports as he gets too much of them each day. The best thing to get this
> discussed by people who are more familiar with this topic than me is to provide
> a merge-request for kdelibs like Frank mentioned in comment #9. If the feedback
> there is that it should be done on application level then we will do this,
> otherwise interface issues etc. will get discussed on the reviewboard so that
> this can be pushed into kdelibs.

I don`t think it is good idea to fix application bug/feature by creating API changes in some library(especially kdelibs). 
Plus if kdelibs changes are done they would require changes in dolphin too, so anyways we would have to fix it on dolphin too.
Comment 14 Peter Penz 2011-10-02 13:47:16 UTC
> I don`t think it is good idea to fix application bug/feature by
> creating API changes in some library(especially kdelibs). 

And this is exactly what is not clear for me: To me it looks like a general issue and not an application bug. Whether a fix in Dolphin is necessary or not is secondary to me: I'm fine doing a Dolphin only fix or doing a fix based on an added interface in kdelibs, but I need to be sure to do the "right thing" instead of introducing a workaround that might fail in other cases. For this I need the input of the maintainers from the KRun-interface (David) and we only get this by submitting a merge-request. It is also OK to submit a merge-request for Dolphin and add David Faure as review-person (as Konqueror is affected too by this).
Comment 15 David Faure 2011-10-06 08:22:23 UTC
Well, since this is only meaningful for local files, I would say that KFileItem could determine its parent directory all by itself, no need for an API change nor for changes in Dolphin.

Just do
if (d->m_bIsLocalUrl) {
    KUrl parent = url;
    parent.setPath(parent.directory());
    and pass parent.toLocalFile() to KRun.
}
Comment 16 Peter Penz 2011-10-06 08:40:47 UTC
Thanks David for the feedback, this sounds good.

@Tautvydas: Would you like to take care do apply David's proposal and submit a merge-request? That would be great but if you don't have the time I would take care for this.
Comment 17 Tautvydas Andrikys 2011-10-06 14:39:21 UTC
(In reply to comment #16)
> Thanks David for the feedback, this sounds good.
> 
> @Tautvydas: Would you like to take care do apply David's proposal and submit a
> merge-request? That would be great but if you don't have the time I would take
> care for this.

Better you do that, couse I dont even have account at reviewboard.

Also wouldnt recommend changing kfileitem functionality this way couse probably lots of software depends on it.
Comment 18 David Faure 2011-10-07 19:11:16 UTC
Don't worry about reviewboard, what would really help is if you can make and test the patch :)

And don't worry about the KFileItem::run fix affecting other apps - it will, but that's the whole point. Anyone calling KFileItem::run should benefit from this bugfix (using the right current directory). I definitely wouldn't want a different behavior in other apps where the user can "run" a file item.
Comment 19 Tautvydas Andrikys 2011-10-09 14:08:18 UTC
Created attachment 64363 [details]
patch for kdelibs

(In reply to comment #18)
> Don't worry about reviewboard, what would really help is if you can make and
> test the patch :)
> 
> And don't worry about the KFileItem::run fix affecting other apps - it will,
> but that's the whole point. Anyone calling KFileItem::run should benefit from
> this bugfix (using the right current directory). I definitely wouldn't want a
> different behavior in other apps where the user can "run" a file item.

Here is the patch. But it could only be done in KRun and not in KFileItem, so almost everything that uses KRun will be affected.
Comment 20 Peter Penz 2011-10-10 20:28:35 UTC
To me doing the patch in KRun instead of KFileItem::run() looks fine. @David: Is it OK if I commit the following change to KDE/4.7?

------------------------------- kio/kio/krun.cpp -------------------------------
index 0141a7c..c978576 100644
@@ -135,7 +135,7 @@ bool KRun::runUrl(const KUrl& u, const QString& _mimetype, QWidget* window, bool
     else if (isExecutableFile(u, _mimetype)) {
         if (u.isLocalFile() && runExecutables) {
             if (KAuthorized::authorize("shell_access")) {
-                return (KRun::runCommand(KShell::quoteArg(u.toLocalFile()), QString(), QString(), window, asn)); // just execute the url as a command
+                return (KRun::runCommand(KShell::quoteArg(u.toLocalFile()), QString(), QString(), window, asn, u.directory())); // just execute the url as a command
Comment 21 Tautvydas Andrikys 2011-10-11 09:18:33 UTC
I think it is very bad idea to include this patch to 4.7, it should be included in at least 4.8 or 4.9
Comment 22 David Faure 2011-10-12 12:10:25 UTC
Patch looks good. I can't really think of a case where we might not want the url's directory as current directory in runUrl, so this looks safe.

Funny that you're pushing for it to be merged in later rather than sooner, usually people ask for the other way round :-)
But there's only 4.7.x and 5.x branches for kdelibs, so this should go into 4.7.x., it's a bugfix after all.
Comment 23 Tautvydas Andrikys 2011-10-12 15:09:59 UTC
(In reply to comment #22)
> Patch looks good. I can't really think of a case where we might not want the
> url's directory as current directory in runUrl, so this looks safe.

Simple example is this shell script: touch test.txt

Before my fix it will create file in ~/Documents directory, after it will create in same directory as shell script

> Funny that you're pushing for it to be merged in later rather than sooner,
> usually people ask for the other way round :-)
> But there's only 4.7.x and 5.x branches for kdelibs, so this should go into
> 4.7.x., it's a bugfix after all.

It looks more like a feature to me, thats why it shouldnt be merged to stable version(4.7).
Comment 24 David Faure 2011-10-13 14:34:49 UTC
You misunderstood my first paragraph. I meant "yes, runUrl should probably always use the url's directory as current directory". Sorry for the double negation.
Comment 25 Peter Penz 2011-10-13 18:04:20 UTC
Git commit 693ca44eb71be65171aec8ecec6eee92e8c055b4 by Peter Penz.
Committed on 13/10/2011 at 20:00.
Pushed by ppenz into branch 'KDE/4.7'.

KRun: Use the URL-directory as current directory

Thanks to Tautvydas Andrikys for the patch!

BUG: 271824
FIXED-IN: 4.7.3

M  +1    -1    kio/kio/krun.cpp

http://commits.kde.org/kdelibs/693ca44eb71be65171aec8ecec6eee92e8c055b4
Comment 26 cj.wijtmans 2014-06-24 14:10:15 UTC
i just want to say i am experiencing this bug more or less. Whatever i am opening, binaries or scripts. They always are executed in the a completely different working directory then i am currently in dolphin.
I am on gentoo ~amd64. 

# kde4-config -v
Qt: 4.8.5
KDE Development Platform: 4.13.2
kde4-config: 1.0
Comment 27 cj.wijtmans 2014-06-24 14:12:14 UTC
I had to make scripts that change the working directory to the where the script is located before clicking .exe files. Just one example.
Comment 28 cj.wijtmans 2014-06-25 22:07:06 UTC
Yeah everything starts in $home even though i dont start the binaries or scripts in $home