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

[David Faure]

dfaure added a comment.


  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.

REPOSITORY
  R241 KIO

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

To: shubham, rkflx, dfaure, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

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

[Henrik Fehlauer]

rkflx added a comment.


  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 ;)

REPOSITORY
  R241 KIO

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

To: shubham, rkflx, dfaure, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

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

[Nathaniel Graham]

ngraham added a comment.


  Late to the party here, but the read-only label needs to be selectable too. I 
submitted a patch for that: D14648 

REPOSITORY
  R241 KIO

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

To: shubham, rkflx, dfaure, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

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

[Nathaniel Graham]

ngraham added a task: T9297: Polish file/folder properties dialog.

REPOSITORY
  R241 KIO

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

To: shubham, rkflx, dfaure, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

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

[Shubham]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:e2c56f6ddc51: Use KLineEdit for folder name if folder has 
write access, else use QLabel (authored by shubham).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14610?vs=39164=39166

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

AFFECTED FILES
  src/widgets/kpropertiesdialog.cpp

To: shubham, rkflx, dfaure, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

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

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  I disagree that it's more readable, but let's not nitpick :-)

REPOSITORY
  R241 KIO

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

To: shubham, rkflx, dfaure, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

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

[David Faure]

dfaure accepted this revision.
dfaure added a comment.
This revision is now accepted and ready to land.


  (oops, misclicked)

REPOSITORY
  R241 KIO

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

To: shubham, rkflx, dfaure, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

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

[Shubham]

shubham updated this revision to Diff 39164.
shubham added a comment.


  Add check for bFromTemplate
  simplify expression (!d->m_bFromTemplate && !itemList.supportsMoving())  to 
!(d->m_bFromTemplate || itemList.supportsMoving()) for better readability

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14610?vs=39163=39164

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

AFFECTED FILES
  src/widgets/kpropertiesdialog.cpp

To: shubham, rkflx, dfaure, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

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

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  Better, but you missed the `(!m_bFromTemplate &&` bit. Make sure test "Create 
New / Text File" in dolphin.

REPOSITORY
  R241 KIO

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

To: shubham, rkflx, dfaure, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

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

[Shubham]

shubham updated this revision to Diff 39163.
shubham added a comment.


  Re factor
  Remove unnecessary indentations
  use only !itemList.supportsMoving()

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14610?vs=39112=39163

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

AFFECTED FILES
  src/widgets/kpropertiesdialog.cpp

To: shubham, rkflx, dfaure, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

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

[David Faure]

dfaure added a comment.


  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 :-)

REPOSITORY
  R241 KIO

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

To: shubham, rkflx, dfaure, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

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

[David Faure]

dfaure added a comment.


  `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()))`

REPOSITORY
  R241 KIO

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

To: shubham, rkflx, dfaure, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

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

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  Please re-read your diff and, indeed, revert any unnecessary changes like 
indentation or no-op moving of code.

INLINE COMMENTS

> shubham wrote in 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

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 
.

REPOSITORY
  R241 KIO

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

To: shubham, rkflx, dfaure, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

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

[Shubham]

shubham added a comment.


  In D14610#303706 , @rkflx wrote:
  
  > In D14610#303537 , @rkflx wrote:
  >
  > > `KPropertiesDialog::setFileNameReadOnly`
  > > `m_bFromTemplate`
  >
  >
  > @shubham Any comments on that (see my questions to @dfaure above)?
  
  
  sorry, don't know about it

INLINE COMMENTS

> rkflx wrote in 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.)

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

REPOSITORY
  R241 KIO

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

To: shubham, rkflx, dfaure, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

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

[Henrik Fehlauer]

rkflx added a comment.


  In 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

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

[Shubham]

shubham updated this revision to Diff 39112.
shubham added a comment.


  Re-use Qlabel

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14610?vs=39082=39112

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

AFFECTED FILES
  src/widgets/kpropertiesdialog.cpp

To: shubham, rkflx, dfaure, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

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

[Henrik Fehlauer]

rkflx added a comment.


  @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.

REPOSITORY
  R241 KIO

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

To: shubham, rkflx, dfaure, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

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

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

[Shubham]

shubham retitled this revision from "Use KLineEdit if folder has write access, 
else use QLabel" to "Use KLineEdit for folder name if folder has write access, 
else use QLabel".

REPOSITORY
  R241 KIO

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

To: shubham, rkflx, dfaure, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns