D14610: Use KLineEdit for folder name if folder has write access, else use QLabel

[David Faure]

dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> kpropertiesdialog.cpp:994
>          }
>          d->nameArea = lab;
>      } else {

What about this code path? You broke it by removing the call to `grid->addWidget(d->nameArea, curRow++, 2);` in that case.

> kpropertiesdialog.cpp:998
>          KFileItemListProperties itemList(KFileItemList() << item);
>          setFileNameReadOnly(!itemList.supportsMoving());
> +        // if supports writing then KLineEdit, else QLabel

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?
If we catch all cases of "renaming forbidden" upfront, this isn't needed anymore.

> kpropertiesdialog.cpp:1000
> +        // if supports writing then KLineEdit, else QLabel
> +        if (itemList.supportsWriting()) {
> +            d->m_lined = new KLineEdit(d->m_frame);

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).
[at least on Unix; I don't remember the exact semantics on Windows].

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.
(or do you? the patch description is unclear)

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14610

To: shubham, rkflx, dfaure, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20180804/2ebe0e15/attachment.html>