[kpropertiesdialog] adjust ui for root directory
AcceptedPublic

Authored by tienisto on Oct 17 2019, 7:49 PM.

Details

Reviewers
ngraham
GB_2
Group Reviewers
VDG
Frameworks
Summary

special ui changes if root directory is selected

Old:

New:

Diff Detail

Repository
R241 KIO
Branch
kpropertiesdialog-root-directory (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 17877
Build 17895: arc lint + arc unit
tienisto created this revision.Oct 17 2019, 7:49 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptOct 17 2019, 7:49 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
tienisto requested review of this revision.Oct 17 2019, 7:49 PM
tienisto edited the summary of this revision. (Show Details)Oct 17 2019, 7:50 PM
tienisto added a reviewer: ngraham.
GB_2 added a subscriber: GB_2.Oct 17 2019, 8:05 PM

Can we also set the root folder icon?

ahmadsamir added inline comments.
src/widgets/kpropertiesdialog.cpp
1177

Probably not so obvious for new Linux users; especially those coming from windows where things are represented as C:\, D:\ ... etc.

tienisto added inline comments.Oct 17 2019, 9:25 PM
src/widgets/kpropertiesdialog.cpp
1177

I think new linux users don't even know what mounting means :)

tienisto updated this revision to Diff 68206.Oct 18 2019, 12:31 AM

use root folder icon

tienisto edited the summary of this revision. (Show Details)Oct 18 2019, 12:32 AM
GB_2 accepted this revision.Oct 18 2019, 7:37 AM
GB_2 added a reviewer: Frameworks.
This revision is now accepted and ready to land.Oct 18 2019, 7:38 AM
meven added a subscriber: meven.Oct 18 2019, 10:36 AM

I would suggest keeping "Location" and "Mounted on" fields to actually display '/' to the user.
Especially for newbies users who might not know what "Root Directory" means.
But for more advanced user it would make sense too as / is currently visible only in the icon, it would feel amiss.
/ is not such a special case as other mount points, and consistency matter, even if redundant on this edge case.
To me the size, the name and icon change are sufficient here.

src/widgets/kpropertiesdialog.cpp
1113

Replace with Unkwown

tienisto updated this revision to Diff 68228.Oct 18 2019, 11:10 AM

use 'Unknown' instead of 'unknown'

tienisto marked an inline comment as done.Oct 18 2019, 11:10 AM

I would suggest keeping "Location" and "Mounted on" fields to actually display '/' to the user.

To me users should know that / is the root folder and has the highest level in the directory structure. And that the location of '/' is '/' is quite confusing.

In principle I like the idea of customizing the display to be more appropriate. However I feel like the best way to do this might be to go in a different direction: instead of adding overrides for /, maybe we should take advantage of the fact that / is always a mounted volume and instead make the properties dialog reflect the name and icon of the disk in question. Otherwise when you look at the properties for that disk, the dialog that appears shows a different name and icon from the thing that was clicked on.

If we improve the properties dialog for disks in general, that will also improve the display for other kinds of disks and mounted volumes.

What do you think?

tienisto updated this revision to Diff 68263.Oct 18 2019, 8:47 PM

show 'Mounted on' again

tienisto edited the summary of this revision. (Show Details)Oct 18 2019, 8:49 PM
tienisto added a comment.EditedOct 18 2019, 8:54 PM

[...]
If we improve the properties dialog for disks in general, that will also improve the display for other kinds of disks and mounted volumes.
What do you think?

I like this idea. However this is exceeds my skill set :)
But I think that my change is an improvement and maybe we can use your idea for another commit

tienisto marked 2 inline comments as done.Oct 18 2019, 8:55 PM