[KPropertiesDialog] Add octal permissions
Needs RevisionPublic

Authored by shubham on Mon, Apr 22, 12:02 PM.

Details

Reviewers
ngraham
pino
Group Reviewers
VDG
Summary

This patch adds octal format for permissions to the KPropertiesDialog

Test Plan
  1. Right click any file/folder/link
  2. Select Properties
  3. Octal permissions displayed in Permissions tab

Diff Detail

Repository
R241 KIO
Branch
octal
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 11432
Build 11450: arc lint + arc unit
shubham created this revision.Mon, Apr 22, 12:02 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMon, Apr 22, 12:02 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
shubham requested review of this revision.Mon, Apr 22, 12:02 PM
shubham edited the test plan for this revision. (Show Details)Mon, Apr 22, 12:03 PM
shubham edited the summary of this revision. (Show Details)Mon, Apr 22, 2:16 PM
pino requested changes to this revision.Mon, Apr 22, 3:35 PM
pino added a subscriber: pino.

Parsing the result of KFileItem::permissionsString() is a rather bad idea, especially considering that KFileItem already provides mode() and permissions().

Also, the placing of the label is completely off.

This revision now requires changes to proceed.Mon, Apr 22, 3:35 PM
shubham added a comment.EditedMon, Apr 22, 3:58 PM

I had used permission() to get the mode_t variable, which I later type casted to qint64, still the permissions resulted were completed wrong. Any suggestions, how this may be done better in efficient manner?

pino added a comment.Mon, Apr 22, 4:10 PM

I had used permission() to get the mode_t variable, which I later type casted to qint64, still the permissions resulted were completed wrong.

Tried to print it as octal-based value?

Any suggestions, how this may be done better in efficient manner?

http://lmgtfy.com/?q=unix+mode_t

In D20735#454263, @pino wrote:

Tried to print it as octal-based value?

No, now I get that those permissions were base 10.
How to print it as octal?

ngraham requested changes to this revision.Mon, Apr 22, 5:10 PM
ngraham added inline comments.
src/widgets/kpropertiesdialog.cpp
1869

You don't need to create a whole new grid layout; just add the label to the existing one.

pino added a comment.Mon, Apr 22, 5:21 PM

Also: what is the use case of this feature? For casual users the octal permissions make no sense, they are mostly useful for power users (if they don't even just use the terminal for these things).
At most, this would fit in the "advanced permissions" dialog.

I think it makes sense to show octal permissions somewhere in the GUI. Power users use the GUI, too. :) But yeah, where exactly to put it needs some thought.

We could also make the permissions interactive and explaining at the same time.
I could imagine something like this:

shubham updated this revision to Diff 56979.Thu, Apr 25, 3:18 PM
  1. Use mode_t to get the permissions
  2. Position the octal permissions inside "Advanced Permissions" tab
shubham edited the test plan for this revision. (Show Details)Thu, Apr 25, 3:19 PM
shubham marked an inline comment as done.
ngraham added a reviewer: VDG.Thu, Apr 25, 5:13 PM
ngraham requested changes to this revision.Thu, Apr 25, 6:00 PM

Code looks sane now. But the UI needs polish. Putting it in the advanced page feels awkward to me, and even on that page, its location isn't right. It feels arbitrary, disconnected from the rest of the UI. I feel like it might sit better on the main page, to the left of the Advanced Permissions button. Alternatively, if it has to stay on the advanced page, it needs to feel better integrated with the rest of the layout.

Adding VDG for more commentary.

This revision now requires changes to proceed.Thu, Apr 25, 6:00 PM
pino requested changes to this revision.Wed, May 1, 2:00 PM

Also, there was feedback it was still not taken care.

Please do not ping on pathed when you are requested for changes, and still do not do them.

src/widgets/kpropertiesdialog.cpp
2111

why the cast to qint64?

2112

why this string includes the untranslated label?

2115

the translatable label must be here instead

shubham updated this revision to Diff 57355.Thu, May 2, 5:55 AM

No type casting

shubham marked 2 inline comments as done.Thu, May 2, 5:55 AM
shubham updated this revision to Diff 57356.Thu, May 2, 5:59 AM

constantanize permissions

pino requested changes to this revision.Thu, May 2, 8:32 PM

The permission changes to ktelnetservice5.desktop are unrelated, please revert them.

Mostly important, the value shown in the advanced permissions dialog is not taking into account the changes done in the properties dialog of the file. The other widgets in this advanced dialog already do, so the shown octal value ought to as well.

src/widgets/kpropertiesdialog.cpp
2114

just use the static QString::number?

This revision now requires changes to proceed.Thu, May 2, 8:32 PM