rkflx added a comment.
In D14610#303537 <https://phabricator.kde.org/D14610#303537>, @rkflx wrote: > `KPropertiesDialog::setFileNameReadOnly` > `m_bFromTemplate` @shubham Any comments on that (see my questions to @dfaure above)? INLINE COMMENTS > kpropertiesdialog.cpp:988 > + KFileItemListProperties itemList(KFileItemList() << item); > + if (d->bMultiple || isTrash || hasRoot || !itemList.supportsMoving() || > (itemList.isDirectory() & !itemList.supportsWriting())) { > QLabel *lab = new QLabel(d->m_frame); 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.) > kpropertiesdialog.cpp:999-1019 > + d->m_lined = new KLineEdit(d->m_frame); > + d->m_lined->setObjectName("KFilePropsPlugin::nameLineEdit"); > + d->m_lined->setText(filename); > + d->nameArea = d->m_lined; > + d->m_lined->setFocus(); > + grid->addWidget(d->nameArea, curRow++, 2); > + Please don't add additional indentation here. > kpropertiesdialog.cpp:1021 > > - grid->addWidget(d->nameArea, curRow++, 2); > - You are moving that line to both the `if` and the `else` branch. Why not keep it here? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D14610 To: shubham, rkflx, dfaure, ngraham Cc: kde-frameworks-devel, michaelh, ngraham, bruns