T9297: 5th point
Use QLabel for "write" protected folder
Details
Details
- 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
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 grid->addWidget(d->nameArea, curRow++, 2); 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 if 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. | |
Comment Actions
@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 KPropertiesDialog::setFileNameReadOnly, which is used in Plasma's IconApplet and would still show the KLineEdit if I understand your proposal correctly?
Also, KFilePropsPlugin::setFileNameReadOnly checks for m_bFromTemplate. 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.
Comment Actions
| src/widgets/kpropertiesdialog.cpp | ||
|---|---|---|
| 988 | Without your patch, we only tested for !itemList.supportsMoving(). Now you also test for (itemList.isDirectory() & !itemList.supportsWriting(). 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 if and the else branch. Why not keep it here? | |
Comment Actions
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 | |
Comment Actions
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. !itemList.supportsMoving() is enough to detect the case of files in a non-writable directory. See the unittest I just pushed about that, commit 0799ca8f32. | |
Comment Actions
KFilePropsPlugin::setFileNameReadOnly 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:
if (d->bMultiple || isTrash || hasRoot || (!m_bFromTemplate && !itemList.supportsMoving()))
Comment Actions
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 :-)
Comment Actions
Better, but you missed the (!m_bFromTemplate && bit. Make sure test "Create New / Text File" in dolphin.
Comment Actions
Add check for bFromTemplate
simplify expression (!d->m_bFromTemplate && !itemList.supportsMoving()) to !(d->m_bFromTemplate || itemList.supportsMoving()) for better readability
Comment Actions
Late to the party here, but the read-only label needs to be selectable too. I submitted a patch for that: D14648
Comment Actions
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.
Interesting, for me only Link to Application will bring up the properties dialog ;)
Comment Actions
Done: https://phabricator.kde.org/D14662
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.