Re: Review Request: Fix for stale permissions information in properties dialog
On Jan. 2, 2013, 3:18 p.m., David Faure wrote:
kio/kio/kdirlister.cpp, line 1080
http://git.reviewboard.kde.org/r/103555/diff/1/?file=44960#file44960line1080
I don't understand the if(). Why should the behavior differ depending
on whether we have this url currently shown (itemsInUse) or in the cache
(itemsCached), compared to when we don't know about this URL?
The if() is wrongly written anyway: if it should be there, then it
should be url.url(), not url.path(), since itemsInUse and itemsCached use
URLs as keys. I should change that to QUrl in KF5.
And in case you remove the if, then my old comment meant: in that case,
we can simplify the code to remove the if (isDir), and simply do
Q_FOREACH(const QString dir,
directoriesForCanonicalPath(url.directory())) {
handleDirDirty(dir);
}
in all cases (file or directory).
Because as you say, it's the parent that needs to be re-listed when the
permissions of anything changes.
Also, this changes misses a corresponding unittest in
kdirlistertest.cpp.
Ahh... I see what you mean. I should have realized what I was doing is utterly
unnecessary. I will make the change as you suggested and add a test case for
this. In fact, I will add the unittest first to test for the failure.
- Dawit
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/103555/#review24439
---
On Dec. 29, 2012, 8:31 p.m., Dawit Alemayehu wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/103555/
---
(Updated Dec. 29, 2012, 8:31 p.m.)
Review request for kdelibs and David Faure.
Description
---
If you open a directory that contains other directories in Konqueror or
Dolphin, change the permission of one of these directories from outside, say
the command line, and right click on the same directory to look at the
permission tab in the properties dialog, you will see that the permission
change has not been updated. This patch addresses that bug.
This addresses bug 173733.
http://bugs.kde.org/show_bug.cgi?id=173733
Diffs
-
kio/kio/kdirlister.cpp ec3d622
Diff: http://git.reviewboard.kde.org/r/103555/diff/
Testing
---
1. In konsole, create a test directory within another test directory:
mkdir -p test/test1
2. Open Dolphin or Konqueror and navigate to the top newly created directory,
i.e. test.
3. In konsole, cd into the first test directory:
cd test
4. In konsole, change the permission of 'test1' from konsole. For example,
chmod go-rx
5. In the open Dolphin or Konqueror, right click on test1, select
properties and click on permission tab.
6. Validate whether or not the permission shown in the GUI matches what you
get in the command line.
Thanks,
Dawit Alemayehu
Re: Review Request: Fix for stale permissions information in properties dialog
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/103555/#review24439
---
kio/kio/kdirlister.cpp
http://git.reviewboard.kde.org/r/103555/#comment18721
I don't understand the if(). Why should the behavior differ depending on
whether we have this url currently shown (itemsInUse) or in the cache
(itemsCached), compared to when we don't know about this URL?
The if() is wrongly written anyway: if it should be there, then it should
be url.url(), not url.path(), since itemsInUse and itemsCached use URLs as
keys. I should change that to QUrl in KF5.
And in case you remove the if, then my old comment meant: in that case, we
can simplify the code to remove the if (isDir), and simply do
Q_FOREACH(const QString dir,
directoriesForCanonicalPath(url.directory())) {
handleDirDirty(dir);
}
in all cases (file or directory).
Because as you say, it's the parent that needs to be re-listed when the
permissions of anything changes.
Also, this changes misses a corresponding unittest in kdirlistertest.cpp.
- David Faure
On Dec. 29, 2012, 8:31 p.m., Dawit Alemayehu wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/103555/
---
(Updated Dec. 29, 2012, 8:31 p.m.)
Review request for kdelibs and David Faure.
Description
---
If you open a directory that contains other directories in Konqueror or
Dolphin, change the permission of one of these directories from outside, say
the command line, and right click on the same directory to look at the
permission tab in the properties dialog, you will see that the permission
change has not been updated. This patch addresses that bug.
This addresses bug 173733.
http://bugs.kde.org/show_bug.cgi?id=173733
Diffs
-
kio/kio/kdirlister.cpp ec3d622
Diff: http://git.reviewboard.kde.org/r/103555/diff/
Testing
---
1. In konsole, create a test directory within another test directory:
mkdir -p test/test1
2. Open Dolphin or Konqueror and navigate to the top newly created directory,
i.e. test.
3. In konsole, cd into the first test directory:
cd test
4. In konsole, change the permission of 'test1' from konsole. For example,
chmod go-rx
5. In the open Dolphin or Konqueror, right click on test1, select
properties and click on permission tab.
6. Validate whether or not the permission shown in the GUI matches what you
get in the command line.
Thanks,
Dawit Alemayehu
Re: Review Request: Fix for stale permissions information in properties dialog
On Dec. 28, 2011, 7:37 a.m., David Faure wrote:
kio/kio/kdirlister.cpp, line 1078
http://git.reviewboard.kde.org/r/103555/diff/1/?file=44960#file44960line1078
Yes, but changing permissions is only one case for ending up here. The
most common case is that KDirWatch (in Stat mode) notifies us that a
directory has changed because files have been created in it, or deleted,
etc. In that case we do want to make the directory as dirty, not its parent.
I guess that means we need to do both, mark the parent and the
directory itself, as dirty. Sucks for performance, though.
The real issue is that KDirWatch's dirty() signal is pretty unspecific.
Ah, I know. This is called by KDirWatch so it's local only, no networ
transparency.
So we should just clear the permissions/owner in the KFileItem for the
directory, they'll be re-determined on demand by KFileItem.
Dawit Alemayehu wrote:
Yes, but changing permissions is only one case for ending up here. The
most common case is that KDirWatch (in Stat mode) notifies us that a
directory has changed because files have been created in it, or deleted,
etc. In that case we do want to make the directory as dirty, not its parent.
Yes, I got that. But when an item in a given directory is renamed,
deleted or created, slotFileDirty is invoked with the path set to that
directory and not the specific file or directory that was modified. IOW if I
have a directory called Test that contains two items, a directory named
New Folder and a file named New File.txt, then when either one of those
two items are renamed or deleted or a third item is created the parameter to
slotFileDirty is the full path to Test. However, if I simply touch or
change permission on either one of those items, then the parameter to
slotFileDirty is set to the full path for Test/New Folder or Test/New
File.txt.
Everything works fine except when you change permissions or timestamp on
the child directory, i.e New Folder. In that case because the parameter
passed to slotFileDirty is the full path to the directory that was modified,
Test/New Folder, the call to updateDirectory, called from handleDirtyDir,
does nothing. Why ? The call to checkUpdate always returns false since New
Folder is in neither itemsInUse nor itemsCached containers.
Dawit Alemayehu wrote:
@David: Tried this approach, i.e. calling refresh on the changed
directory's KFileItem if it is not in the cache, but it did not seem to work
for me. I am sure I am doing it wrong. Here is what I changed my patch into:
if (!itemsInUse.contains(url.path())
!itemsCached.contains(url.path())) {
KFileItem* item = findByUrl(0, url);
if (item) {
kDebug(7004) *** Refreshing item;
item-refresh();
return;
}
}
But that does not seem to work. The code path is definitely hit called
because the debug statement is printed out on the command line.
Oh, I forgot to answer this (ouch, exactly a year ago already).
Your testing is based on the inotify backend. When using stat (due to no
inotify support -- for instance on non-linux systems, or over NFS),
slotFileDirty isn't emitted. All that KDirWatch can find out is that something
changed inside directory parent.
As in your more recent patch, the condition inside the if() is wrong, the if()
will always pass since these contain urls, not paths.
Refreshing the item might have the problem that whichever code looks into the
item later on, is working on a copy. KDirLister needs to call
emitRefreshedItems in order to let the world know about the modified item.
- David
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/103555/#review9322
---
On Dec. 29, 2012, 8:31 p.m., Dawit Alemayehu wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/103555/
---
(Updated Dec. 29, 2012, 8:31 p.m.)
Review request for kdelibs and David Faure.
Description
---
If you open a directory that contains other directories in Konqueror or
Dolphin, change the permission of one of these directories from outside, say
the command line, and right click on the same directory to look at the
permission tab in the properties dialog, you will see that the permission
change has not been updated. This patch addresses that bug.
This addresses bug 173733.
http://bugs.kde.org/show_bug.cgi?id=173733
Diffs
-
kio/kio/kdirlister.cpp ec3d622
Diff: http://git.reviewboard.kde.org/r/103555/diff/
Testing
---
1. In konsole, create a test directory
Re: Review Request: Fix for stale permissions information in properties dialog
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103555/ --- (Updated Dec. 29, 2012, 8:31 p.m.) Review request for kdelibs and David Faure. Changes --- Added testing done. Description --- If you open a directory that contains other directories in Konqueror or Dolphin, change the permission of one of these directories from outside, say the command line, and right click on the same directory to look at the permission tab in the properties dialog, you will see that the permission change has not been updated. This patch addresses that bug. This addresses bug 173733. http://bugs.kde.org/show_bug.cgi?id=173733 Diffs - kio/kio/kdirlister.cpp ec3d622 Diff: http://git.reviewboard.kde.org/r/103555/diff/ Testing (updated) --- 1. In konsole, create a test directory within another test directory: mkdir -p test/test1 2. Open Dolphin or Konqueror and navigate to the top newly created directory, i.e. test. 3. In konsole, cd into the first test directory: cd test 4. In konsole, change the permission of 'test1' from konsole. For example, chmod go-rx 5. In the open Dolphin or Konqueror, right click on test1, select properties and click on permission tab. 6. Validate whether or not the permission shown in the GUI matches what you get in the command line. Thanks, Dawit Alemayehu
Re: Review Request: Fix for stale permissions information in properties dialog
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103555/ --- (Updated Aug. 3, 2012, 7:04 p.m.) Review request for kdelibs and David Faure. Changes --- @David any idea on how to resolve this issue ? I wanted to address it the way you suggested but it did not work for me. See my earlier response. Description --- If you open a directory that contains other directories in Konqueror or Dolphin, change the permission of one of these directories from outside, say the command line, and right click on the same directory to look at the permission tab in the properties dialog, you will see that the permission change has not been updated. This patch addresses that bug. This addresses bug 173733. http://bugs.kde.org/show_bug.cgi?id=173733 Diffs - kio/kio/kdirlister.cpp ec3d622 Diff: http://git.reviewboard.kde.org/r/103555/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request: Fix for stale permissions information in properties dialog
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103555/ --- (Updated Jan. 5, 2012, 9:48 p.m.) Review request for kdelibs and David Faure. Changes --- Updated bugs list. Description --- If you open a directory that contains other directories in Konqueror or Dolphin, change the permission of one of these directories from outside, say the command line, and right click on the same directory to look at the permission tab in the properties dialog, you will see that the permission change has not been updated. This patch addresses that bug. This addresses bugs 118136 and 173733. http://bugs.kde.org/show_bug.cgi?id=118136 http://bugs.kde.org/show_bug.cgi?id=173733 Diffs - kio/kio/kdirlister.cpp ec3d622 Diff: http://git.reviewboard.kde.org/r/103555/diff/diff Testing --- Thanks, Dawit Alemayehu
Re: Review Request: Fix for stale permissions information in properties dialog
On Dec. 28, 2011, 7:37 a.m., David Faure wrote:
kio/kio/kdirlister.cpp, line 1078
http://git.reviewboard.kde.org/r/103555/diff/1/?file=44960#file44960line1078
Yes, but changing permissions is only one case for ending up here. The
most common case is that KDirWatch (in Stat mode) notifies us that a
directory has changed because files have been created in it, or deleted,
etc. In that case we do want to make the directory as dirty, not its parent.
I guess that means we need to do both, mark the parent and the
directory itself, as dirty. Sucks for performance, though.
The real issue is that KDirWatch's dirty() signal is pretty unspecific.
Ah, I know. This is called by KDirWatch so it's local only, no networ
transparency.
So we should just clear the permissions/owner in the KFileItem for the
directory, they'll be re-determined on demand by KFileItem.
Dawit Alemayehu wrote:
Yes, but changing permissions is only one case for ending up here. The
most common case is that KDirWatch (in Stat mode) notifies us that a
directory has changed because files have been created in it, or deleted,
etc. In that case we do want to make the directory as dirty, not its parent.
Yes, I got that. But when an item in a given directory is renamed,
deleted or created, slotFileDirty is invoked with the path set to that
directory and not the specific file or directory that was modified. IOW if I
have a directory called Test that contains two items, a directory named
New Folder and a file named New File.txt, then when either one of those
two items are renamed or deleted or a third item is created the parameter to
slotFileDirty is the full path to Test. However, if I simply touch or
change permission on either one of those items, then the parameter to
slotFileDirty is set to the full path for Test/New Folder or Test/New
File.txt.
Everything works fine except when you change permissions or timestamp on
the child directory, i.e New Folder. In that case because the parameter
passed to slotFileDirty is the full path to the directory that was modified,
Test/New Folder, the call to updateDirectory, called from handleDirtyDir,
does nothing. Why ? The call to checkUpdate always returns false since New
Folder is in neither itemsInUse nor itemsCached containers.
@David: Tried this approach, i.e. calling refresh on the changed directory's
KFileItem if it is not in the cache, but it did not seem to work for me. I am
sure I am doing it wrong. Here is what I changed my patch into:
if (!itemsInUse.contains(url.path()) !itemsCached.contains(url.path())) {
KFileItem* item = findByUrl(0, url);
if (item) {
kDebug(7004) *** Refreshing item;
item-refresh();
return;
}
}
But that does not seem to work. The code path is definitely hit called because
the debug statement is printed out on the command line.
- Dawit
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/103555/#review9322
---
On Dec. 27, 2011, 6:26 p.m., Dawit Alemayehu wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/103555/
---
(Updated Dec. 27, 2011, 6:26 p.m.)
Review request for kdelibs and David Faure.
Description
---
If you open a directory that contains other directories in Konqueror or
Dolphin, change the permission of one of these directories from outside, say
the command line, and right click on the same directory to look at the
permission tab in the properties dialog, you will see that the permission
change has not been updated. This patch addresses that bug.
This addresses bug 173733.
http://bugs.kde.org/show_bug.cgi?id=173733
Diffs
-
kio/kio/kdirlister.cpp ec3d622
Diff: http://git.reviewboard.kde.org/r/103555/diff/diff
Testing
---
Thanks,
Dawit Alemayehu
Re: Review Request: Fix for stale permissions information in properties dialog
On Dec. 28, 2011, 7:37 a.m., David Faure wrote: kio/kio/kdirlister.cpp, line 1078 http://git.reviewboard.kde.org/r/103555/diff/1/?file=44960#file44960line1078 Yes, but changing permissions is only one case for ending up here. The most common case is that KDirWatch (in Stat mode) notifies us that a directory has changed because files have been created in it, or deleted, etc. In that case we do want to make the directory as dirty, not its parent. I guess that means we need to do both, mark the parent and the directory itself, as dirty. Sucks for performance, though. The real issue is that KDirWatch's dirty() signal is pretty unspecific. Ah, I know. This is called by KDirWatch so it's local only, no networ transparency. So we should just clear the permissions/owner in the KFileItem for the directory, they'll be re-determined on demand by KFileItem. Yes, but changing permissions is only one case for ending up here. The most common case is that KDirWatch (in Stat mode) notifies us that a directory has changed because files have been created in it, or deleted, etc. In that case we do want to make the directory as dirty, not its parent. Yes, I got that. But when an item in a given directory is renamed, deleted or created, slotFileDirty is invoked with the path set to that directory and not the specific file or directory that was modified. IOW if I have a directory called Test that contains two items, a directory named New Folder and a file named New File.txt, then when either one of those two items are renamed or deleted or a third item is created the parameter to slotFileDirty is the full path to Test. However, if I simply touch or change permission on either one of those items, then the parameter to slotFileDirty is set to the full path for Test/New Folder or Test/New File.txt. Everything works fine except when you change permissions or timestamp on the child directory, i.e New Folder. In that case because the parameter passed to slotFileDirty is the full path to the directory that was modified, Test/New Folder, the call to updateDirectory, called from handleDirtyDir, does nothing. Why ? The call to checkUpdate always returns false since New Folder is in neither itemsInUse nor itemsCached containers. On Dec. 28, 2011, 7:37 a.m., David Faure wrote: kio/kio/kdirlister.cpp, line 1080 http://git.reviewboard.kde.org/r/103555/diff/1/?file=44960#file44960line1080 Well, that's equivalent to looping over url.directory() instead of url.path(), then. Which then means both cases (directory and file) can be merged. Ahh... I do not follow. - Dawit --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103555/#review9322 --- On Dec. 27, 2011, 6:26 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103555/ --- (Updated Dec. 27, 2011, 6:26 p.m.) Review request for kdelibs and David Faure. Description --- If you open a directory that contains other directories in Konqueror or Dolphin, change the permission of one of these directories from outside, say the command line, and right click on the same directory to look at the permission tab in the properties dialog, you will see that the permission change has not been updated. This patch addresses that bug. This addresses bug 173733. http://bugs.kde.org/show_bug.cgi?id=173733 Diffs - kio/kio/kdirlister.cpp ec3d622 Diff: http://git.reviewboard.kde.org/r/103555/diff/diff Testing --- Thanks, Dawit Alemayehu
Review Request: Fix for stale permissions information in properties dialog
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103555/ --- Review request for kdelibs and David Faure. Description --- If you open a directory that contains other directories in Konqueror or Dolphin, change the permission of one of these directories from outside, say the command line, and right click on the same directory to look at the permission tab in the properties dialog, you will see that the permission change has not been updated. This patch addresses that bug. This addresses bug 173733. http://bugs.kde.org/show_bug.cgi?id=173733 Diffs - kio/kio/kdirlister.cpp ec3d622 Diff: http://git.reviewboard.kde.org/r/103555/diff/diff Testing --- Thanks, Dawit Alemayehu
