[Trash KCM] Clean up and standardize UI to be in line with the KDE HIG
ClosedPublic

Authored by ngraham on May 19 2018, 9:18 PM.

Details

Summary

Clean up and standardize the UI to be in line with the KDE HIG.

Test Plan
  • All functionality still works
  • Checking and unchecking the checkboxes still enables and disables companion items appropriately
  • No unit test regressions, and trash test still passes
  • Visual comparison:

Before:

After:

Diff Detail

Repository
R241 KIO
Branch
clean-up-trash-kcm (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
ngraham created this revision.May 19 2018, 9:18 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMay 19 2018, 9:18 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
ngraham requested review of this revision.May 19 2018, 9:18 PM
ngraham edited the test plan for this revision. (Show Details)May 19 2018, 9:20 PM
ngraham added a subscriber: kfm-devel.
rkflx added a subscriber: rkflx.May 19 2018, 10:29 PM

I don't think putting whole phrases as a label before the checkbox works very well, and I would be surprised if that's what the HIG now recommends. I assume at most a single subject should be used there?

For example, instead of the awkwardly placed checkbox in

Limit to maximum size: [ ] 10 %

…it would work much better this way:

Size: [ ] Limit to 10 %

Not only is scanning the dialog much quicker if you only have to read a single word when looking for an option in that vertical row, but for languages with longer translations it's the only way to make the dialog not fall apart in the horizontal direction.

(Same reasoning applies to the other options too, of course.)

abetts added a subscriber: abetts.May 19 2018, 10:58 PM

I don't think putting whole phrases as a label before the checkbox works very well, and I would be surprised if that's what the HIG now recommends. I assume at most a single subject should be used there?

For example, instead of the awkwardly placed checkbox in

Limit to maximum size: [ ] 10 %

…it would work much better this way:

Size: [ ] Limit to 10 %

Not only is scanning the dialog much quicker if you only have to read a single word when looking for an option in that vertical row, but for languages with longer translations it's the only way to make the dialog not fall apart in the horizontal direction.

(Same reasoning applies to the other options too, of course.)

+1

Yeah, that makes sense. Suggestions welcome, of course. Is this any better?

Still not super happy with "When limit reached:

Yeah, that makes sense. Suggestions welcome, of course. Is this any better?

Still not super happy with "When limit reached:

Would it make sense to flip it? Start with:

Warn me - when limit is reached

Seems more natural to me

rkflx added a comment.EditedMay 20 2018, 6:17 AM

Yeah, that makes sense. Suggestions welcome, of course. Is this any better?

Hm, I think after the checkbox there should some sort of sentence, with a noun in front:

Cleanup: [ ] Automatically empty after 7 days

Warn me - when limit is reached

Putting the combobox in front would violate the HIG, wouldn't it?

@ngraham Please change your language to both Greek and German, and notice the width of the label as well as the width of the combobox. Your current proposal will blow up the width of the dialog massively, and create a huge amount of whitespace on the left as well.

Perhaps you need something like this:

Noun: Explanations are good
      Combobox

Alternatively:

Noun: ( ) Option 1
      ( ) Option 2
      ( ) Option 3

Good idea, perhaps some radio buttons would make more sense here. I'll play around with it and try other languages.

Yeah, that makes sense. Suggestions welcome, of course. Is this any better?

Hm, I think after the checkbox there should some sort of sentence, with a noun in front:

Cleanup: [ ] Automatically empty after 7 days

Warn me - when limit is reached

Putting the combobox in front would violate the HIG, wouldn't it?

@ngraham Please change your language to both Greek and German, and notice the width of the label as well as the width of the combobox. Your current proposal will blow up the width of the dialog massively, and create a huge amount of whitespace on the left as well.

Perhaps you need something like this:

Noun: Explanations are good
      Combobox

Alternatively:

Noun: ( ) Option 1
      ( ) Option 2
      ( ) Option 3

Much better!

Like this?

Like this?

That looks really good IMHO. Somewhat unrelated, I have seen a few UIs recently that ask the user to automatically empty their trash bin, can, container, whatever... every X amount of TIME UNIT. Would it make sense to add a feature like that in this KCM?

That looks really good IMHO.

Thanks! A lot of the credit goes to Henrik, with his excellent eye for UI design.

Somewhat unrelated, I have seen a few UIs recently that ask the user to automatically empty their trash bin, can, container, whatever... every X amount of TIME UNIT. Would it make sense to add a feature like that in this KCM?

I think that would be enabled by the first checkbox, no? Or were you asking for the ability to change the time unit to hours, weeks, months, etc? An interesting idea, though out of scope for this patch.

rkflx added a comment.May 20 2018, 7:43 PM

Thanks, "Size" and "Full trash" are looking good now.

Yeah, that makes sense. Suggestions welcome, of course. Is this any better?

Hm, I think after the checkbox there should some sort of sentence, with a noun in front:

Cleanup: [ ] Automatically empty after 7 days

Just noticed that I based my suggestion on your screenshot, but missed to see that you changed the semantics compared to the original dialog: The trash won't be emptied completely after 7 days, but files older than 7 days will be deleted (based on the wording, you might want to check the code what's correct).

Therefore I think we might have to keep the original wording and only add the label in front.

(Which is why I'm not too fond of changing every UI to the new style, because it is much work but creates little value.)

What's your plan for landing this change, do you want to convert the other pages first?

ngraham added a comment.EditedMay 20 2018, 8:21 PM

Just noticed that I based my suggestion on your screenshot, but missed to see that you changed the semantics compared to the original dialog: The trash won't be emptied completely after 7 days, but files older than 7 days will be deleted (based on the wording, you might want to check the code what's correct).

Therefore I think we might have to keep the original wording and only add the label in front.

Still looks good IMHO.

(Which is why I'm not too fond of changing every UI to the new style, because it is much work but creates little value.)

The value is in standardizing on something. Plasma, System Settings KCMs, and Gwenview already use this style. Many others don't (see T8655). We had many VDG discussions on the subject, and decided to standardize on the labels-on-the-left style instead of the labels-on-top style. If we had decided to standarize on the other one, we would have simply had a different set of work converting Plasma, System Settings, and Gwenview.

What's your plan for landing this change, do you want to convert the other pages first?

Yes, I'm doing all the other pages in D12571 (which is WIP).

rkflx added a comment.May 20 2018, 8:35 PM

The value is in standardizing on something.

Personally I'd be okay with a consistent style per application, with the new style applied to new applications, and allowing exceptions where it makes sense. You've already witnessed the amount of bikeshedding in Gwenview when we needed to add options, because with the new style it is much more challenging to get the wording right than with the traditional style.

Anyway, I'm not complaining too much (but please be aware that every change you propose creates additional work for reviewers and translators).

The value is in standardizing on something.

Personally I'd be okay with a consistent style per application, with the new style applied to new applications, and allowing exceptions where it makes sense. You've already witnessed the amount of bikeshedding in Gwenview when we needed to add options, because with the new style it is much more challenging to get the wording right than with the traditional style.

Right, but the end result was better (IMHO). Good design usually requires more work, and I'm very happy that you have high standards!

Anyway, I'm not complaining too much (but please be aware that every change you propose creates additional work for reviewers and translators).

I'm quite aware, but this is just the nature of submitting patches, right? Nobody's making you review mine, and I hope you don't feel like it's your responsibility to babysit me to prevent things from getting broken or anything like that. :)

rkflx added a comment.May 20 2018, 8:45 PM

Anyway, I'm not complaining too much (but please be aware that every change you propose creates additional work for reviewers and translators).

I'm quite aware, but this is just the nature of submitting patches, right? Nobody's making you review mine, and I hope you don't feel like it's your responsibility to babysit me to prevent things from getting broken or anything like that. :)

Well, I'd say you should use the resources available (which are quite scarce, as you are aware) wisely. I'd rather have the translators translate strings in Gwenview (there are many missing spots where we added things for 18.04) and work on my review backlog. Also note that fighting an uphill battle, i.e. more stuff getting broken than you are able to fix by yourself, is not much fun.

What are you actually asking me to do? Stop changing strings? Stop advocating for design changes? Stop submitting patches that implement design changes? Submit better patches that don't require so much reviewer time? Stop submitting patches entirely? Do less of those things instead of stopping entirely?

Anyway, I'm not complaining too much (but please be aware that every change you propose creates additional work for reviewers and translators).

I'm quite aware, but this is just the nature of submitting patches, right? Nobody's making you review mine, and I hope you don't feel like it's your responsibility to babysit me to prevent things from getting broken or anything like that. :)

Well, I'd say you should use the resources available (which are quite scarce, as you are aware) wisely. I'd rather have the translators translate strings in Gwenview (there are many missing spots where we added things for 18.04) and work on my review backlog. Also note that fighting an uphill battle, i.e. more stuff getting broken than you are able to fix by yourself, is not much fun.

It's the unfortunate life of working in development. Changes are going to happen and others will have to make adjustments accordingly. There is nothing inherently wrong with that. If the argument is, "don't change things because they cause too much work for others" then we have to determine if we are a software provider that wants to keep the first iteration of their software and not hear customers, or if we want to hear customers and change our approach overtime, and evolve. I prefer the latter, no matter how many headaches I get. Let's take that approach and welcome change. Let's be sensible but not inflexible. I am sure there is a middle point for all of this.

rkflx added a comment.EditedMay 20 2018, 9:08 PM

In my personal experience spending more time on a limited set of items leads to better results and less churn (as in reverting commits, number of iterations per patch etc.) than only lightly touching a large amount of issues everywhere. Of course you can do whatever you feel is right, I was merely hinting at a potential issue (in particular with mass-changing UI everywhere).

In my personal experience spending more time on a limited set of items leads to better results and less churn (as in reverting commits, number of iterations per patch etc.) than only lightly touching a large amount of issues everywhere. Of course you can do whatever you feel is right, I was merely hinting at a potential issue (in particular with mass-changing UI everywhere).

Thanks for your clarification. Let's keep moving forward. What should be the end result of this patch?

What should be the end result of this patch?

Hopefully something like our latest mockup:

Do folks approve of this, or do we need some more visual design revision?

ngraham updated this revision to Diff 34815.Thu, May 24, 1:06 PM

Tweaks per the review process

ngraham edited the test plan for this revision. (Show Details)Thu, May 24, 1:06 PM

This revision improves the labels per review comments, but stays with a combobox instead of radio buttons in the interest of avoiding too many code changes in this patch, since the glue code changes quite a bit when you switch controls. If folks feel strongly that we need radio buttons instead of a combobox, that might be best to do in a follow-up patch.

davidedmundson accepted this revision.Thu, May 24, 1:13 PM
davidedmundson added a subscriber: davidedmundson.
davidedmundson added inline comments.
src/ioslaves/trash/kcmtrash.cpp
303–304

There's no benefit in changing the initial parent of these.

It's all moot as QFormLayout::addItem takes ownership

This revision is now accepted and ready to land.Thu, May 24, 1:13 PM
ngraham added inline comments.Thu, May 24, 1:22 PM
src/ioslaves/trash/kcmtrash.cpp
303–304

Ah right, I think I wrote this before I found that out. In fact, you can omit the parent entirely, so I think I'll do that.

ngraham updated this revision to Diff 34818.Thu, May 24, 1:37 PM

Don't need to set a parent for mLimitReachedAction, since the QFormLayout it's in will just re-parent it anyway

ngraham marked 2 inline comments as done.Thu, May 24, 11:13 PM

Friendly ping! If nobody objects, I will commit this on June 3rd, or sooner if I get more explicit approvals.

Please wait till the 3rd.

Otherwise you give practically no days for the i18n changes to happen.

But then yes go for it. For future if I give an acceptance with a really minor comment feel free to push straight away after.

Got it, need to remember about the string freeze...

ngraham closed this revision.Sun, Jun 3, 3:24 PM