Display mounted file system type and mounted from fields in properties dialog
ClosedPublic

Authored by shubham on Aug 22 2018, 12:09 PM.

Details

Summary

BUG: 220976

Test Plan

Open properties dialog.

Diff Detail

Repository
R241 KIO
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
shubham created this revision.Aug 22 2018, 12:09 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald TranscriptAug 22 2018, 12:09 PM
shubham requested review of this revision.Aug 22 2018, 12:09 PM
shubham updated this revision to Diff 40219.Aug 22 2018, 12:20 PM
broulik added inline comments.
src/widgets/kpropertiesdialog.cpp
1161

Captailize: File System

1164

This leaks, you can probably create it on the stack:

QStorageInfo storageInfo(...);

Also, shouldn't you pass url.toLocalFile() instead of hardcoded /?
Also, move it in the isLocal check as QStorageInfo can only work on paths, not URLs.
Also, what about KMountPoint::mountType()?

1169

QString::fromLatin1?

shubham updated this revision to Diff 40233.Aug 22 2018, 3:09 PM
shubham edited the summary of this revision. (Show Details)
shubham added a reviewer: broulik.
shubham edited the summary of this revision. (Show Details)
  1. use
KMountPoint::mountPoint()
  1. Move everything into
isLocal
shubham updated this revision to Diff 40236.Aug 22 2018, 3:31 PM
shubham updated this revision to Diff 40312.EditedAug 23 2018, 4:07 PM
shubham edited the summary of this revision. (Show Details)

Add mounted from label
@ngraham Is it okay?

shubham edited the summary of this revision. (Show Details)Aug 23 2018, 4:10 PM
ngraham requested changes to this revision.Aug 24 2018, 8:42 PM

Let's move the new "Mounted from:" code to within the conditional that determines whether or not to show "Mounted on:". We should show both or neither, not just one.

This revision now requires changes to proceed.Aug 24 2018, 8:42 PM
shubham updated this revision to Diff 40397.Aug 25 2018, 3:28 AM

Move Mounted from into conditional.

shubham retitled this revision from Display mounted file system type in properties dialog to Display mounted file system type and mounted from fields in properties dialog.Aug 25 2018, 3:32 AM
ngraham accepted this revision.Aug 25 2018, 6:14 PM
ngraham added reviewers: Frameworks, cfeck.

Works great and looks fine to me, and the code seems sensible too given that this dialog is currently implemented with a GridLayout. I'd like to see this ported to use a FormLayout at some point, which is both more semantically appropriate, and would also simplify this type of code. But that's material for another patch of course. :)

Please wait for at least one more review before landing.

This revision is now accepted and ready to land.Aug 25 2018, 6:14 PM

@cfeck @broulik any comments on this?

broulik accepted this revision.Aug 26 2018, 1:34 PM
This revision was automatically updated to reflect the committed changes.