Modernize Settings window
ClosedPublic

Authored by ngraham on Apr 28 2018, 4:28 AM.

Details

Summary

This patch modernizes the appearance of Dolphin's configuration window by following the KDE HIG as much as possible and following design cues from Plasma and System Settings.

Test Plan

Tested all settings to make sure they still work; they do.

Startup page, before:

Startup page, after:

View page (Icons), before:

View page (Icons) after:

View page (Compact), before:

View page (Compact) after:

View page (Detailed), before:

View page: (Detailed), after:

Navigation page: no change

Trash page, before:

Trash page, after:


(Provided by D12986: [Trash KCM] Clean up and standardize UI to be in line with the KDE HIG)

General page (behavior), before:

General page (behavior) after:

General page (confirmations), before:

General page (confirmations), after:

General page (status bar): no change

Diff Detail

Repository
R318 Dolphin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes
ngraham updated this revision to Diff 35963.Jun 10 2018, 8:36 PM

Revert using form layouts for statur bar and navigation pages

ngraham updated this revision to Diff 35965.Jun 10 2018, 8:46 PM

Bring back the distinction between confirmations affecting Dolphin vs all KDE apps

ngraham updated this revision to Diff 35968.Jun 10 2018, 8:56 PM

Restored the "Use default location" button

ngraham updated this revision to Diff 35971.Jun 10 2018, 9:19 PM

Reduced diff for startupsettingspage.cpp and removed KMessageWidget change (will put it in a separate patch)

ngraham updated this revision to Diff 35972.Jun 10 2018, 9:23 PM

Missed one

ngraham edited the test plan for this revision. (Show Details)Jun 10 2018, 9:25 PM
ngraham edited the test plan for this revision. (Show Details)
ngraham updated this revision to Diff 35976.Jun 10 2018, 10:06 PM

Remove unnecessary line

@elvisangelaccio this is ready for review again. Now all it does is change the layout on pages that have mixed controls (checkboxes, radio buttons, and comboboxes etc) for which the new design has the label on the left. I've moved the Use Default Location button removal change to D13474. I've also abandoned the KMessageWidget change since I couldn't get it work work properly when pressing the OK button; the windows always closes and the message isn't displayed.

elvisangelaccio requested changes to this revision.Jun 13 2018, 8:36 PM

Don't they control how the view appears on startup?

Only "Split view mode" does. The other ones control things that are outside the view. How about using "On startup:" and rewording the sentences on the checkboxes where necessary?

Other comments on the UI before looking at the code:

  • In the Startup page, what does "Location" refer to? The groupbox made it clear it was the Home folder, now we are hiding this information.
  • In the View Modes page: "Expandable folders: [] Enable" => do we really have to? :(
  • The Confirmation tab now is better but still confusing. I don't see why we need to touch it at all honestly, given that there are only checkboxes in there.
This revision now requires changes to proceed.Jun 13 2018, 8:36 PM

Don't they control how the view appears on startup?

Only "Split view mode" does. The other ones control things that are outside the view. How about using "On startup:" and rewording the sentences on the checkboxes where necessary?

I was using the term "View" from a user perspective. I think to people who aren't Dolphin developers, these are view settings. The technical distinction between the view, its container, and the main window are not relevant to users, or important to understand.

Other comments on the UI before looking at the code:

  • In the Startup page, what does "Location" refer to? The groupbox made it clear it was the Home folder, now we are hiding this information.

"Home folder" has an established meaning: it's your user's own home folder. Using the label "Home folder" always seemed to imply that you could change your home folder, which was not true. In fact, this setting simply controls what folder dolphin shows on startup. The fact that you're on the "Startup" tab I think does a good job of suggesting that the settings on this page control Dolphin's behavior and appearance when it launches. Maybe I could add a small label at the top of the window explaining that?

  • In the View Modes page: "Expandable folders: [] Enable" => do we really have to? :(

Yeah, I'm not thrilled with this, either. Checkboxes like these would look much better if they were switches (which always have their labels on the left), but we can't use switches until and unless we move Dolphin to an instant-apply paradigm.

  • The Confirmation tab now is better but still confusing. I don't see why we need to touch it at all honestly, given that there are only checkboxes in there.

I feel like there's resistance to the visual design conception itself. I'm getting the feeling that you simply don't like the labels-on-the-left paradigm itself. Perhaps we should address that rather than arguing over individual examples of its application.

abetts added a comment.EditedJun 15 2018, 2:03 PM

Guys, this work has been going on for a long time on something rather simple in aligning the settings design to what Plasma is doing. There should not be this much resistance to the change. Can we please move forward with the approval?

Well, I don't want to force anything down anyone's throat, but it's true that this is the style we're already using all over Plasma...

Don't they control how the view appears on startup?

Only "Split view mode" does. The other ones control things that are outside the view. How about using "On startup:" and rewording the sentences on the checkboxes where necessary?

I was using the term "View" from a user perspective. I think to people who aren't Dolphin developers, these are view settings. The technical distinction between the view, its container, and the main window are not relevant to users, or important to understand.

I disagree, but that's not the point anyway. Using "view" here would be just wrong. Why not using "window" instead?

Other comments on the UI before looking at the code:

  • In the Startup page, what does "Location" refer to? The groupbox made it clear it was the Home folder, now we are hiding this information.

"Home folder" has an established meaning: it's your user's own home folder. Using the label "Home folder" always seemed to imply that you could change your home folder, which was not true. In fact, this setting simply controls what folder dolphin shows on startup. The fact that you're on the "Startup" tab I think does a good job of suggesting that the settings on this page control Dolphin's behavior and appearance when it launches. Maybe I could add a small label at the top of the window explaining that?

Fair enough. How about "Start in:" ?

  • The Confirmation tab now is better but still confusing. I don't see why we need to touch it at all honestly, given that there are only checkboxes in there.

I feel like there's resistance to the visual design conception itself. I'm getting the feeling that you simply don't like the labels-on-the-left paradigm itself. Perhaps we should address that rather than arguing over individual examples of its application.

Guys, this work has been going on for a long time on something rather simple in aligning the settings design to what Plasma is doing. There should not be this much resistance to the change. Can we please move forward with the approval?

Honestly I'm quite disappointed to read this.

I can assure you there is no resistance to the visual design change from me.
Is it true that I don't particularly like it and I'm sad that my feedback was ignored, but if that's what the VDG wants, so be it.

But this patch currently has still issues (in particular, the Confirmations tab) and will be approved only when it's ready.

Don't they control how the view appears on startup?

Only "Split view mode" does. The other ones control things that are outside the view. How about using "On startup:" and rewording the sentences on the checkboxes where necessary?

I was using the term "View" from a user perspective. I think to people who aren't Dolphin developers, these are view settings. The technical distinction between the view, its container, and the main window are not relevant to users, or important to understand.

I disagree, but that's not the point anyway. Using "view" here would be just wrong. Why not using "window" instead?

Other comments on the UI before looking at the code:

  • In the Startup page, what does "Location" refer to? The groupbox made it clear it was the Home folder, now we are hiding this information.

"Home folder" has an established meaning: it's your user's own home folder. Using the label "Home folder" always seemed to imply that you could change your home folder, which was not true. In fact, this setting simply controls what folder dolphin shows on startup. The fact that you're on the "Startup" tab I think does a good job of suggesting that the settings on this page control Dolphin's behavior and appearance when it launches. Maybe I could add a small label at the top of the window explaining that?

Fair enough. How about "Start in:" ?

  • The Confirmation tab now is better but still confusing. I don't see why we need to touch it at all honestly, given that there are only checkboxes in there.

I feel like there's resistance to the visual design conception itself. I'm getting the feeling that you simply don't like the labels-on-the-left paradigm itself. Perhaps we should address that rather than arguing over individual examples of its application.

Guys, this work has been going on for a long time on something rather simple in aligning the settings design to what Plasma is doing. There should not be this much resistance to the change. Can we please move forward with the approval?

Honestly I'm quite disappointed to read this.

I can assure you there is no resistance to the visual design change from me.
Is it true that I don't particularly like it and I'm sad that my feedback was ignored, but if that's what the VDG wants, so be it.

But this patch currently has still issues (in particular, the Confirmations tab) and will be approved only when it's ready.

Thanks for that Elvis. Nate, are you able to adress Elvis’ feedbavk and move the patch forward?

Thanks for that Elvis. Nate, are you able to adress Elvis’ feedback and move the patch forward?

Yep, I'll do that.

One thing that this discussion makes clear is that with this new style, we have an issue with checkboxes in mixed-control layouts. I think the subject needs more discussion, so I've opened T9036: Checkboxes in mixed-control layouts for discussion purposes.

ngraham updated this revision to Diff 36260.Jun 17 2018, 7:33 PM

Review suggestions

ngraham edited the test plan for this revision. (Show Details)Jun 17 2018, 7:33 PM
ngraham updated this revision to Diff 36299.Jun 19 2018, 2:41 AM
  • Improve awkward "Expandable folders: [] Enable" string
  • Replace "General:" with "Miscellaneous:" so we don't have three instances of the word "general" visible at once
  • Use a bit more padding between sections to improve their distinctiveness
ngraham edited the test plan for this revision. (Show Details)Jun 19 2018, 2:44 AM

Any opinions on the latest version? The screenshots are all up to date.

I like it! No objections on looks from me.

src/settings/viewmodes/viewsettingstab.cpp
81–88

Why the change from KComboBox to QComboBox?

108–109

How about "Expandable on click" ?

ngraham added inline comments.Mon, Jun 25, 9:24 PM
src/settings/viewmodes/viewsettingstab.cpp
81–88

Because KComboBox didn't display correctly within a QFormLayout. QComboBox is a drop-in replacement, so it seemed like an acceptable way to fix the issue.

I also found references in commit messages for several other KDE projects to replacing KComboBox with QComboBox, so it seemed to align with the overall code direction anyway.

108–109

That wording would imply that the folders themselves are clickable to expand them, which isn't accurate.

cfeck added a subscriber: cfeck.Mon, Jun 25, 9:41 PM

You only need KComboBox if you use the KCompletion features.

Could we have a consistent "Type *var" vs. "Type* var" style? I prefer the former, but I haven't checked what Dolphin maintainers prefer.

ngraham added a comment.EditedTue, Jun 26, 3:39 AM

You only need KComboBox if you use the KCompletion features.

Yeah, and we're not using them here.

Could we have a consistent "Type *var" vs. "Type* var" style? I prefer the former, but I haven't checked what Dolphin maintainers prefer.

Good point. "Type* var" seems to be more common in Dolphin's codebase, so I'll use that.

ngraham updated this revision to Diff 36664.Tue, Jun 26, 3:43 AM

Standardize asterisk placement

Sorry to be pedantic, but I'd appreciate if the port to QComboBox were moved to another commit.

Then we would be almost ready, I swear ;)

ngraham added a comment.EditedTue, Jun 26, 10:57 PM

Sorry to be pedantic, but I'd appreciate if the port to QComboBox were moved to another commit.

Then we would be almost ready, I swear ;)

Sure, no problem! :)

Done: D13749: Port from KComboBox to QComboBox

ngraham updated this revision to Diff 36792.Wed, Jun 27, 8:52 PM

Rebase on master

ngraham marked 2 inline comments as done.Wed, Jun 27, 8:53 PM
ngraham updated this revision to Diff 36793.Wed, Jun 27, 8:54 PM

Fix dumb merge error

ngraham updated this revision to Diff 36795.Wed, Jun 27, 9:00 PM

Remove Qlabel and QGroupBox #includes for pages that no longer use them

elvisangelaccio requested changes to this revision.Mon, Jul 2, 9:01 PM

I'm ok with the UI changes now.
Just some small nitpicks left and then I think we can ship this.

src/settings/general/behaviorsettingspage.cpp
28–32

Please keep the sorting of includes

78

Please add a global variable instead of hardcoding 18 in different places.

/OT
Does this 18 come from some HIG? If yes, it would be a good idea to put it in some global config that can be shared across all KDE apps.
So that if one day we realize 18 is not good anymore and we want to use 19, we will have just one thing to update.

src/settings/startup/startupsettingspage.cpp
52

Please add a parent to this layout

69

Please add a parent to this layout

This revision now requires changes to proceed.Mon, Jul 2, 9:01 PM
fabianr added a subscriber: fabianr.Tue, Jul 3, 6:11 AM
fabianr added inline comments.
src/settings/general/behaviorsettingspage.cpp
78

Yes, 18 comes from the HIG. That's the "default px representation" for plasmas and kirigamis gridUnit.

ngraham updated this revision to Diff 37112.Tue, Jul 3, 3:22 PM
ngraham marked an inline comment as done.

Keep #includes alphabetically ordered and use a global for the 18px spacer height

ngraham added inline comments.Tue, Jul 3, 3:23 PM
src/settings/general/behaviorsettingspage.cpp
78

@elvisangelaccio

I've added this to a Dolphin-wide global for now, but it seems like it might make more sense to put it in an even more global location. Maybe in KXMLGui? Or somewhere else? I can submit a patch for it if you think that makes sense.

src/settings/startup/startupsettingspage.cpp
52

My understanding of Qt's layout system is that any item (including a layout) that will be added to a layout doesn't need to be given a parent, since Qt automatically re-parents it to the layout it's added to.

https://doc.qt.io/qt-5/layout.html#tips-for-using-layouts

69

Same ^^

ngraham marked an inline comment as done.Tue, Jul 3, 3:24 PM
elvisangelaccio added inline comments.Wed, Jul 4, 9:29 PM
src/settings/general/behaviorsettingspage.cpp
25

Please put it before viewproperties.h (again, to preserver sorting of includes)

src/settings/startup/startupsettingspage.cpp
52

Oh, right!

ngraham updated this revision to Diff 37187.Thu, Jul 5, 12:21 PM

Preserve alphabetical sorting of local includes

ngraham marked 4 inline comments as done.Thu, Jul 5, 12:21 PM
elvisangelaccio accepted this revision.Thu, Jul 5, 6:56 PM

Thanks for the patience :)

This revision is now accepted and ready to land.Thu, Jul 5, 6:56 PM

Thanks! If you're not totally sick of my UI changes yet, there's also D13768: Modernize View Properties window. ;-)

This revision was automatically updated to reflect the committed changes.