Details
T9297: 5th point
Use QLabel for "write" protected folder
- Open dolphin
- Create two folders, one with write access and other with write access denied
Before:
All folders used KLineEdit for folder name
After:
Folder with write access uses KLineEdit
Folder with write access denied uses QLabel instead
Diff Detail
- Repository
- R241 KIO
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
| src/widgets/kpropertiesdialog.cpp | ||
|---|---|---|
| 994 | What about this code path? You broke it by removing the call to in that case. | |
| 998 | This method accesses d->m_lined which isn't created yet (you moved that below), so it's null always, and this call would now be dead code. The proper fix, I think, is to reuse the above code that creates a QLabel instead of a KLineEdit (currently that's for other cases like selecting multiple files, or selecting the root directory, etc.), and go into that condition (the on line 987) also when the directory doesn't support renaming, or the files can't be moved. No point in having two different code paths that both create the same QLabel, differently, and with bugs in both, in this version of the patch... In fact I think your patch should end up removing this call here, no? | |
| 1000 | That test is about writing to the files themselves. To test if the folder forbids renaming, it's the folder that you should be testing for write access, not the file(s). Testcase: create a dr-xr-xr-x (0555) folder, and create a -rw-r--r-- (0644) file in it. Renaming is forbidden, if you can write to the file itself, so you should get the label and not the lineedit.. Arguably a possible ("proper") fix would be for KFileItemListProperties::supportsMoving to return false when the directory is readonly, in fact. If that was the case, you would get a readonly lineedit already. | |
@shubham Thanks for helping out with T9297!
reuse the above code that creates a QLabel
@dfaure Thanks for the review! Any advice on how to handle , which is used in Plasma's and would still show the if I understand your proposal correctly?
Also, checks for . Would this be relevant for your suggestion too?
the patch description is unclear
The main motivation for the patch is to also indicate visually that an item's name cannot be changed. Currently users only notice that by chance due to typing into the line edit not being allowed.
In D14610#303537, @rkflx wrote:
@shubham Any comments on that (see my questions to @dfaure above)?
| src/widgets/kpropertiesdialog.cpp | ||
|---|---|---|
| 988 | Without your patch, we only tested for . Now you also test for . Could you explain in what cases we need that new test? As far as I can see this blocks renaming the folder you removed the write permission from (while only its children should be prevented from being renamed). ( instead of also caught my eye.) | |
| 999–1019 | Please don't add additional indentation here. | |
| 1021 | You are moving that line to both the and the branch. Why not keep it here? | |
In D14610#303706, @rkflx wrote:In D14610#303537, @rkflx wrote:
@shubham Any comments on that (see my questions to @dfaure above)?
sorry, don't know about it
| src/widgets/kpropertiesdialog.cpp | ||
|---|---|---|
| 988 | this test was intended for blocking directories without write access to rename , but as you say it should be done to it's child I missed that one & by mistake | |
Please re-read your diff and, indeed, revert any unnecessary changes like indentation or no-op moving of code.
| src/widgets/kpropertiesdialog.cpp | ||
|---|---|---|
| 988 | You can remove the last condition completely. is enough to detect the case of files in a non-writable directory. See the unittest I just pushed about that, commit 0799ca8f32. | |
checks for m_bFromTemplate for the case where the properties dialog is used by the "Create New / ..." context menu action.
In that case, the source file (the template from /usr) isn't movable, but we still want to let the user type a filename -- for the copied file.
Doing this inside that method won't work anymore, we need to incorporate it into the new if() for using the label. For instance like this:
Hmm, well, for IconApplet's use case, if you never want a readonly line-edit then a major redesign is needed, switching from the lineedit to the qlabel inside of setFileNameReadOnly itself...
(and using that in the code modified by this patch...). Or adding a ctor flag, to avoid creating+destroying widgets right away...
Given the number of iterations just to get the simple case right, let's do that separately, ok? I could do it myself, faster than doing 10 reviews :-)
Re factor
Remove unnecessary indentations
use only !itemList.supportsMoving()
Better, but you missed the bit. Make sure test "Create New / Text File" in dolphin.
Add check for bFromTemplate
simplify expression (!d->m_bFromTemplate && !itemList.supportsMoving()) to !(d->m_bFromTemplate || itemList.supportsMoving()) for better readability
I disagree that it's more readable, but let's not nitpick :-)
Late to the party here, but the read-only label needs to be selectable too. I submitted a patch for that: D14648
In D14610#303987, @dfaure wrote:Hmm, well, for IconApplet's use case
I'd say it would be nice to be consistent everywhere.
I could do it myself, faster than doing 10 reviews :-)
No doubt about that, feel free to work on it ;) If you are busy, we can also add this to T9297 so we don't forget about it.
In D14610#304109, @dfaure wrote:Make sure test "Create New / Text File" in dolphin.
Interesting, for me only Link to Application will bring up the properties dialog ;)
In D14610#304619, @rkflx wrote:In D14610#303987, @dfaure wrote:Hmm, well, for IconApplet's use case
I'd say it would be nice to be consistent everywhere.
I could do it myself, faster than doing 10 reviews :-)
No doubt about that, feel free to work on it ;) If you are busy, we can also add this to T9297 so we don't forget about it.
Done: https://phabricator.kde.org/D14662
In D14610#304109, @dfaure wrote:Make sure test "Create New / Text File" in dolphin.
Interesting, for me only Link to Application will bring up the properties dialog ;)
Ah yes oops you're right, that's what this is about.
Revision Contents
| Path | Packages | |||
|---|---|---|---|---|
| M | src/widgets/kpropertiesdialog.cpp (7 lines) |