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.
Details
- Reviewers
broulik elvisangelaccio - Group Reviewers
Dolphin VDG - Commits
- R318:02c94b228a3a: Modernize Settings window
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
- Branch
- no-more-group-boxes (branched from master)
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 82 Build 82: arc lint + arc unit
Reduced diff for startupsettingspage.cpp and removed KMessageWidget change (will put it in a separate patch)
@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.
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.
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.
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...
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.
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?
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.
- 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
src/settings/viewmodes/viewsettingstab.cpp | ||
---|---|---|
82–89 | 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. | |
109–110 | That wording would imply that the folders themselves are clickable to expand them, which isn't accurate. |
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.
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.
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 ;)
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 | ||
---|---|---|
27–31 | Please keep the sorting of includes | |
79 | Please add a global variable instead of hardcoding 18 in different places. /OT | |
src/settings/startup/startupsettingspage.cpp | ||
54 | Please add a parent to this layout | |
71 | Please add a parent to this layout |
src/settings/general/behaviorsettingspage.cpp | ||
---|---|---|
79 | Yes, 18 comes from the HIG. That's the "default px representation" for plasmas and kirigamis gridUnit. |
src/settings/general/behaviorsettingspage.cpp | ||
---|---|---|
79 | 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 | ||
54 | 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. | |
71 | Same ^^ |
Thanks! If you're not totally sick of my UI changes yet, there's also D13768: Modernize View Properties window. ;-)