Modernize View Properties window
ClosedPublic

Authored by ngraham on Jun 27 2018, 8:00 PM.

Details

Summary

Like D12571, but for the View Properties Window. Also did a little bit of re-organization. This allows us to use a QFormLayout as the top-level layout and simplify the code a lot, including no longer using the now-unnecessary paradigm of putting a layout inside a QWidget, and ending the use of QGridLayout to make a fake and more complicated form-style layout.

Depends on D13749

Test Plan

Window still resizes properly when the Additional Information content is shown or hidden.

Global view properties, additional information hidden:

Global view properties, additional information shown:

Per-folder view properties, additional information hidden:

Per-folder view properties, additional information shown:

Diff Detail

Repository
R318 Dolphin
Branch
modernize-view-properties-window
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 397
Build 397: arc lint + arc unit
ngraham requested review of this revision.Jun 27 2018, 8:00 PM
ngraham created this revision.
ngraham edited the summary of this revision. (Show Details)Jun 27 2018, 8:03 PM
ngraham edited the test plan for this revision. (Show Details)
ngraham added a reviewer: VDG.
ngraham edited the summary of this revision. (Show Details)
ngraham updated this revision to Diff 36796.Jun 27 2018, 9:01 PM

Merge master

Restricted Application added a project: Dolphin. · View Herald TranscriptJun 27 2018, 9:01 PM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
ngraham updated this revision to Diff 36797.Jun 27 2018, 9:04 PM

Remove unneeded #includes

ngraham edited the summary of this revision. (Show Details)Jun 27 2018, 9:09 PM
abetts added a comment.Jul 6 2018, 4:17 AM

Just two nitpicks in language:

"Current folder including all sub-folders"

to

Current folder and sub-folders

"Use these view properties as default"

to

Set properties as default

"Additional Information Shown"

to

Select/show Additional Information

Just to economize on length

"Current folder including all sub-folders"

to

Current folder and sub-folders

Sounds good.

"Use these view properties as default"

to

Set properties as default

This I don't like so much because the action verb "Set" is something I would expect to see on a button, not a checkbox (which usually has slightly more neutral language).

How about instead, something more like "Use as default view settings" or "Use as default view properties"?

"Additional Information Shown"

to

Select/show Additional Information

Maybe "Additional information to show" or "Show additional information" or even "Additional information"?I'm not yet convinced this really needs to be changed at all, though.

ngraham updated this revision to Diff 37258.Jul 6 2018, 2:18 PM

Merge master and shorten a string

abetts added a comment.Jul 6 2018, 2:22 PM

"Current folder including all sub-folders"

to

Current folder and sub-folders

Sounds good.

"Use these view properties as default"

to

Set properties as default

This I don't like so much because the action verb "Set" is something I would expect to see on a button, not a checkbox (which usually has slightly more neutral language).

How about instead, something more like "Use as default view settings" or "Use as default view properties"?

I like “use as default view settings”

"Additional Information Shown"

to

Select/show Additional Information

Maybe "Additional information to show" or "Show additional information" or even "Additional information"?I'm not yet convinced this really needs to be changed at all, though.

Here I was thinking that the triangle button caused an effect. It was like a button so I wanted the string to start with an active verb.

Maybe "Additional information to show" or "Show additional information" or even "Additional information"?I'm not yet convinced this really needs to be changed at all, though.

Here I was thinking that the triangle button caused an effect. It was like a button so I wanted the string to start with an active verb.

It's sort of both. Clicking on the triangle initiates an action of showing the list ("Show the list of additional information available"), but that list itself determines which of those pieces of information is shown for the current view ("Choose what additional information is shown"). Maybe the list itself needs its own label to help explain what it does?

abetts added a comment.Jul 6 2018, 7:25 PM

That would be good too. Yes!

Like this?

abetts added a comment.Jul 7 2018, 1:01 AM

Like this?

<3

ngraham updated this revision to Diff 37279.Jul 7 2018, 1:02 AM

Add a label to help people understand what the metadata list is for

ngraham edited the test plan for this revision. (Show Details)Jul 7 2018, 1:05 AM
elvisangelaccio requested changes to this revision.Jul 8 2018, 1:57 PM
elvisangelaccio added inline comments.
src/settings/viewpropertiesdialog.cpp
155

Please use VERTICAL_SPACER_HEIGHT.

This revision now requires changes to proceed.Jul 8 2018, 1:57 PM
ngraham updated this revision to Diff 37365.Jul 8 2018, 6:15 PM

Use Dolphin::VERTICAL_SPACER_HEIGHT instead of a hardcoded value

ngraham marked an inline comment as done.Jul 8 2018, 6:16 PM

Ping! @elvisangelaccio or anyone else in Dolphin? Tagging is fast approaching, and I think it would be nice to get this into 18.08.

elvisangelaccio accepted this revision.Jul 14 2018, 7:31 PM
This revision is now accepted and ready to land.Jul 14 2018, 7:31 PM
ngraham closed this revision.Jul 14 2018, 7:55 PM