Modernize View Properties window
ClosedPublic

Authored by ngraham on Wed, Jun 27, 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 681
Build 693: arc lint + arc unit
ngraham requested review of this revision.Wed, Jun 27, 8:00 PM
ngraham created this revision.
ngraham edited the summary of this revision. (Show Details)Wed, Jun 27, 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.Wed, Jun 27, 9:01 PM

Merge master

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

Remove unneeded #includes

ngraham edited the summary of this revision. (Show Details)Wed, Jun 27, 9:09 PM
abetts added a comment.Fri, Jul 6, 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.Fri, Jul 6, 2:18 PM

Merge master and shorten a string

abetts added a comment.Fri, Jul 6, 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.Fri, Jul 6, 7:25 PM

That would be good too. Yes!

Like this?

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

Like this?

<3

ngraham updated this revision to Diff 37279.Sat, Jul 7, 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)Sat, Jul 7, 1:05 AM
elvisangelaccio requested changes to this revision.Sun, Jul 8, 1:57 PM
elvisangelaccio added inline comments.
src/settings/viewpropertiesdialog.cpp
159

Please use VERTICAL_SPACER_HEIGHT.

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

Use Dolphin::VERTICAL_SPACER_HEIGHT instead of a hardcoded value

ngraham marked an inline comment as done.Sun, Jul 8, 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.Sat, Jul 14, 7:31 PM
This revision is now accepted and ready to land.Sat, Jul 14, 7:31 PM
ngraham closed this revision.Sat, Jul 14, 7:55 PM