Port Language KCM to Qt Quick
ClosedPublic

Authored by hein on Apr 10 2018, 10:34 PM.

Details

Summary
  • Changed the overall design from two lists to one list with a modal sheet to add more languages.
  • Replaced a modal "You need to relogin for changes" dialog with a MessageType.Positive InlineMessage.
  • Reworked the way missing languages are handled: The old KCM silently rewrote config and showed a warning. The new design shows an informative warning and removes the missing languages on the next save. Until then they're flagged as missing in the list.
  • Manages Apply button state correctly (or rather at all ...).

This depends on D12097.

Closes T7247.

This is currently not final code. It's a WIP upload to give Marco
something to work with to fix various Kirigami and SimpleKCM problems.

Currently known issues:

  • Does not save (code is from old KCM, might have been broken there)
  • Disabled SwipeListItem actions do not show disabled
  • Placement of actions button in SwipeListItem is wonky if the contentItem is a RowLayout
  • SwipeListItem spews errors about positionAnimation after using an action
  • SwipeListItem is awkward to use, we need a drag-reorderable list delegate
  • OverlaySheet spews numerous warnings about not being able to find applicationWindow and activeFocusItem
  • The sheet is parented to the SimpleKCM's parent since there's no app window to be modal too
  • The footer inside an OverlaySheet sometimes moves up above the content instead of staying down
  • Even though SimpleKCM is just a Kirigami.ScrollablePage like Kirigami Gallery pages, an InlineMessage that fills the page width gets cut off on the left and right, so wonky code to insert margins next to them
  • List has window bg color as background instead of view background color

Diff Detail

Repository
R119 Plasma Desktop
Branch
arcpatch-D12102_1
Lint
No Linters Available
Unit
No Unit Test Coverage
There are a very large number of changes, so older changes are hidden. Show Older Changes
hein added a comment.Apr 19 2018, 6:53 PM

Fix logic bug in managing the sheet's Add button enabled state.

hein planned changes to this revision.Apr 19 2018, 6:53 PM

Looking at the screenshots, it's unclear which language do the up/down buttons belong to. Can we use drag'n'drop? Can we embed the buttons into the language row next to the language name?

aspotashev added inline comments.Apr 20 2018, 8:07 AM
kcms/translations/package/contents/ui/main.qml
60

Please add context:

i18nc("@title:window", "Add Languages")
109

Please add context for translators:

i18nc("@action:button", "Add")
146

Please add context:

i18nc("@info", "There are no languages available on this system.")
158

Please add context "@info"

171

Please add context "@info":

i18ncp("@info %2 is the language code",
272

Please add context "@action:button"

hein updated this revision to Diff 32647.Apr 20 2018, 1:28 PM

Add context to strings.

hein planned changes to this revision.Apr 20 2018, 1:31 PM

Looking at the screenshots, it's unclear which language do the up/down buttons belong to. Can we use drag'n'drop? Can we embed the buttons into the language row next to the language name?

Here's an updated screenshot that reflects the latest changes in Kirigami upstream:

On desktops SwipeListItem now shows the actions without the swipe thing on hover.

I'd also probably prefer DND though.

I'm not a huge fan of the fact that the list items take up the entire horizontal width and have no side borders. Makes things feel rather visually muddy, like things are too big and flowing into one another. Could we put that whole list inside a frame with a reasonable maximum width to give it some visual containment and structure, and have at least a bit of side padding between the edges of that frame and the edges of the KCM?

And if this is a Kirigami convergence thing, IMHO we should think about having Kirigami automatically add a frame and some side padding when on desktop platforms, where having a list span the entire horizontal width rarely looks good.

Could the list be enclosed by a white field?

hein added a comment.Apr 20 2018, 2:51 PM

I think we can do a white field.

About the padding, I think it'd be cool if FormLayout could do this somehow. So it's automatic. (This code doesn't use FormLayout currently, though.) @mart?

hein added a comment.EditedApr 20 2018, 2:59 PM

I'm actually having no luck with the white field thing currently. I tried various combos of setting Kirigami.Theme.colorSet: Kirigami.Theme.View in either the LV or the delegate and relying on backgroundColor use in DefaultListItemBackground, or setting SwipeListItem.backgroundColor explicitly, or even using the deprecared Theme.viewBackgroundColor. None of them produced white.

Perhaps I'm blind, but in the latest screenshot it looks like there's already a white background.

If anything, what we need is for the general background to be light gray like all the other KCMs, and for the list itself to retain its current white background and have a simple frame around it to separate it from the gray background.

hein added a comment.Apr 20 2018, 3:06 PM

There's no white bg in the latest screenshot, the entire thing is #fcfcfc (window bg color).

hein added a comment.Apr 20 2018, 3:08 PM

Note btw that it's similarly broken for me in the new grid-based KCMs. We had the same discussion there where we want the grids to have the view background color behind them (white), but the grid in the Loon and Feel KCM for example is #fcfcfc for me.

hein added a comment.Apr 20 2018, 3:12 PM

OK, the plot thickens. Turns out this is a color scheme problem.

We have "Default" and "Breeze" color schemes. They used to be (and are supposed to be) identical. But right now, in my Colors KCM, switching between Default and Breeze produces different results.

In "Breeze", the window background color is #eff0f1 and the view background color is #fcfcfc, so you get a noticable contrast.

In "Default", they're both #fcfcfc.

So the Kirigami colorSet stuff actually works fine. It's the "Default" color scheme making all the colors the same.

hein updated this revision to Diff 32652.Apr 20 2018, 4:04 PM

Set color set, add padding, wrap in ScrollView.

This is all a bit hacky and mostly to get VDG feedback.

Known problems:

  • When the lang list grows out of bounds you get double-scrollbars because I couldn't find a way to access the ScrollView background item frame width to size it by.
  • The padding is hard-coded.
hein planned changes to this revision.Apr 20 2018, 4:05 PM

Updated screenshot, this time using the "Breeze" color scheme which has some contrast between window bg and view bg colors:

rkflx added a subscriber: rkflx.Apr 20 2018, 5:02 PM

Hm, I could imagine regular users will have a hard time selecting their preferred language. They'll see a list of languages, and look for some kind of checkbox to "select what they want". How should they realize that the top-most language will be used as the default, with the languages below as a fallback?

This either needs some kind of visual indication, or at least a help text above. For reference, the language control panel in Windows has this text written above the list: "Add languages you want to use to this list. The language at the top of your list is your primary language (the one you want to see and use most often)."

Also, having a Set as default action to move a language to the top in one go would be a bit more pleasant than being forced to use the endless loop of "click on Up button, button moves away, move mouse to target button again, click again…". Drag-reordering is also hard to discover for some type of users. Alternatively, move the Up button out of the list into a static position.

Just my 2ct, no hard feelings if you'll choose to simply ignore me ;)

Hm, I could imagine regular users will have a hard time selecting their preferred language. They'll see a list of languages, and look for some kind of checkbox to "select what they want". How should they realize that the top-most language will be used as the default, with the languages below as a fallback?

This either needs some kind of visual indication, or at least a help text above. For reference, the language control panel in Windows has this text written above the list: "Add languages you want to use to this list. The language at the top of your list is your primary language (the one you want to see and use most often)."

Also, having a Set as default action to move a language to the top in one go would be a bit more pleasant than being forced to use the endless loop of "click on Up button, button moves away, move mouse to target button again, click again…". Drag-reordering is also hard to discover for some type of users. Alternatively, move the Up button out of the list into a static position.

Just my 2ct, no hard feelings if you'll choose to simply ignore me ;)

The interaction will be a little bit different than that. Correct me anyone if I am wrong. The list of languages you see, are the ones selected. There is another window that has a list of languages to select from.

hein added a comment.Apr 20 2018, 5:14 PM

I think @rkflx gets that, he's talking just about the first list. I agree with his comments. Some more design mockups to work from would be nice :).

I can make more

hein added a comment.Apr 20 2018, 5:15 PM

Currently we use the following inline messages to communicate stuff btw:

  • An info box shown when there's no languages available to add (i.e. not installed)
  • An info box shown no languages have been added (but some are available)
  • An error box when some added languages have gone missing (i.e. uninstalled) saying they'll be removed from the list on the next OK/Apply
  • A positive box after adding langs and doing OK/Apply informing the changes need a restart

Here are additional mockups for thought.

rkflx added a comment.EditedApr 21 2018, 6:33 AM

Here are additional mockups for thought.

Quite nice (the helpful text label in particular, and how the default option is emphasized), but how will setting the fallback language work, e.g. how to switch English and Spanish around in your screenshot? In the previous iteration this was done by allowing to order the list (which IMO was fine, only needed some polishing).

hein added a comment.Apr 21 2018, 12:49 PM

I like the mockup, but yeah, still need to figure out ordering.

As for the top text label, let's go with "Preferred languages" over "Installed languages" (also what the old KCM used).

hein added a comment.Apr 21 2018, 2:23 PM

An early attempt at implementing @abetts new mockup:

I didn't do the icons because:

(a) I don't think SwipeListItem allows me to show the actions permanently currently
(b) The circle-with-slash icon is semantically wrong (list-remove looks like a minus in Breeze)
(c) This is not about enabling and disabling, it's a prioritized list

Pick your default language by moving topmost. To add more languages, click on the right.

How about this?

The language at the top of this list is the one you want to see and use most often. To add more languages, click the "Add Languages..." button.

Perhaps we could even remove the second sentence entirely, since the button is right there...

hein added a comment.EditedApr 21 2018, 2:32 PM

Yeah, I think duplicating the button text is goofy. I'm OK with the first sentence.

Otherwise, personally I would like the list to be like this:

(a) Have a drag handle and be drag-reorderable (missing in Kirigami)
(b) On hover, show icon-based "Move to top" (using the go-top icon) and remove actions

That'd address @rkflx "make changing the default super easy" thing.

hein added a comment.Apr 21 2018, 2:33 PM

BTW: I think SwipeListItem actions should have support for tooltips, so those actions can have "Make default" and "Remove" tooltips.

hein added a comment.Apr 21 2018, 2:34 PM

Another issue I have with SwipeListItem: In addition to the hover effect, there's always a fancy flashing effect on clicks, even though it doesn't do anything in my list.

In D12102#251008, @hein wrote:

Yeah, I think duplicating the button text is goofy. I'm OK with the first sentence.

Otherwise, personally I would like the list to be like this:

(a) Have a drag handle and be drag-reorderable (missing in Kirigami
(b) On hover, show icon-based "Move to top" (using the go-top icon) and remove actions

That'd address @rkflx "make changing the default super easy" thing.

+1, that would be excellent.

Also +1 for further refining the usability and presentation of the SwipeListItems in the way you bring up.

hein added a comment.Apr 21 2018, 2:40 PM

Turns out tooltip support is there already:

hein added a comment.Apr 21 2018, 2:45 PM

With the right text and disabling the default action for the first item:

Lookin' pretty sharp!

hein added a comment.Apr 21 2018, 2:56 PM

So, how are we gonna approach this drag thing? Three dots?

For this list, where the items themselves don't do anything on click, I think it's sane to allow the whole list item to be a drag area. There's more of a challenge to allow re-ordering items that do something when you click on them, but we don't have that issue here.

hein added a comment.Apr 21 2018, 3:12 PM

For this list, where the items themselves don't do anything on click, I think it's sane to allow the whole list item to be a drag area. There's more of a challenge to allow re-ordering items that do something when you click on them, but we don't have that issue here.

We need some sort of cue so people can tell it's draggable though.

rkflx added a comment.Apr 21 2018, 6:24 PM

Looking better now ;)

One more note: Users read the dialog from the top. Having "Default" added to the topmost entry like in @abetts' mockup would be great, if possible at all, because most users won't really get to the text at the bottom.

As for the handle, maybe something like http://clauderic.github.io/react-sortable-hoc/, (see "Drag Handle" and "Lock axis", observe cursor changing). This icon is pretty standard in both web and Android, unfortunately overloaded with many other meanings, too. I guess this needs to be a Kirigami-wide decision, or do we have an existing example?

+1 for putting the text on the top rather than the bottom. Once we make the list draggable, it might be nice to incorporate that into the text too, e.g "drag languages into your preferred order" or something like that.

Unfortunately, our use of the hamburger button precludes use of that elsewhere-fairly-universal "three bar" symbol to mean "this is draggable!" Maybe a very light gray rendition of this, or a variant?

Or we could finally banish the hamburger menu once and for all...

hein added a comment.Apr 21 2018, 7:22 PM

Adding 'Default' is pretty easy, no problem.

mart added a comment.Apr 21 2018, 7:33 PM

+1 for putting the text on the top rather than the
Unfortunately, our use of the hamburger button precludes use of that elsewhere-fairly-universal "three bar" symbol to mean "this is draggable!" Maybe a very light gray rendition of this, or a variant?

I like it tough it should still be consistent with the drag icons for the handles of swipelistitem which are now not used in desktop but still needed . On that review there are still some handles proposals (of which none look like an hamburger icon)

Or we could finally banish the hamburger menu once and for all...

No :)

mart added a comment.Apr 21 2018, 7:49 PM

+1 for putting the text on the top rather than the bottom. Once we make the list draggable, it might be nice to incorporate that into the text too, e.g "drag languages into your preferred order" or something like that.

Unfortunately, our use of the hamburger button precludes use of that elsewhere-fairly-universal "three bar" symbol to mean "this is draggable!" Maybe a very light gray rendition of this, or a variant?

Or we could finally banish the hamburger menu once and for all...

I'm thinking about a vertical version of https://phabricator.kde.org/D10980

which means we need versions for handles:
horizontal left
hirizontal, right
horizontal, both
vertical, top
vertical bottom
vertical, both (fot this particular thing)
https://phabricator.kde.org/D10980

hein updated this revision to Diff 33150.Apr 26 2018, 3:12 PM
  • Port to ScrollVieWKCM
  • Add list label at the top and show conditionally
  • Actions are now 'Make default' and 'Remove' with appropriate tooltips
  • Topmost language has a "(Default)" behind it
  • Code cleanups

It's now done aside from dragging stuff and ... uh ... saving settings.

hein added a comment.Apr 26 2018, 3:13 PM

New screenshot:

hein added a comment.Apr 26 2018, 3:18 PM

I also just realized we seem to have zero keyboard nav support ...

hein updated this revision to Diff 33742.May 7 2018, 6:08 AM
  • Fix settings saving
  • String tweak

This is done now from my end, other than DND which still requires
Kirigami work.

abetts added a comment.May 7 2018, 2:30 PM
In D12102#254322, @hein wrote:

New screenshot:

Is the Add Languages button able to be right aligned?

Did we want to include the helper text from previous iterations?

ngraham edited the summary of this revision. (Show Details)May 7 2018, 4:22 PM
ngraham added a task: T7247: Language.
hein added a comment.May 8 2018, 5:45 AM

Right-aligned button: Can do, sure.

Helper text: It's at the top now, AIUI the discussion had evolved to leave it like this? Not sure.

rkflx added a comment.May 8 2018, 10:39 AM
In D12102#259446, @hein wrote:

Helper text: It's at the top now, AIUI the discussion had evolved to leave it like this? Not sure.

LGTM.

mart added a comment.May 8 2018, 11:47 AM

+1 for moving the add language button bottom-right

kcms/translations/package/contents/ui/main.qml
67

same consideration for Layout.* in delegates

186

Layout.* attached properties are useless there, as items in a listview are not in a layout.
the correct thing to do here (as documented by qt) is even is not pretty:

width: languagesList.width

hein updated this revision to Diff 33817.May 8 2018, 11:51 AM
  • Right-align 'Add languages ...' button
  • Clean up cruft from a pre-ListView revision, thanks Marco
hein updated this revision to Diff 33818.May 8 2018, 11:52 AM

Set explicit width for Material style

mart accepted this revision.May 9 2018, 3:37 PM

on my side, code is all fine now

This revision is now accepted and ready to land.May 9 2018, 3:37 PM
hein added a comment.May 10 2018, 6:09 AM

@mart Do you think I should push this before we work on the DND stuff?

mart requested changes to this revision.May 11 2018, 11:38 AM
mart added inline comments.
kcms/translations/package/contents/ui/main.qml
67

sorry, counter what i told you before, setting an explicit width: in lists in overlaysheets actually breaks them, https://imgur.com/a/CPMhtYX
this line should be removed

This revision now requires changes to proceed.May 11 2018, 11:38 AM
mart added a comment.May 11 2018, 4:21 PM

if i select more than one language, only one will be added, the others will be added as "unknown"

hein added a comment.May 12 2018, 1:21 PM
In D12102#261174, @mart wrote:

if i select more than one language, only one will be added, the others will be added as "unknown"

I can't reproduce this, but will have a look.

hein updated this revision to Diff 34051.May 13 2018, 7:42 AM

Remove width from delegate in sheet again.

hein updated this revision to Diff 34053.May 13 2018, 8:26 AM

Don't mutate source model while building list of langs to add to selection.

This will likely fix Marco's problem.

hein updated this revision to Diff 34120.May 14 2018, 9:25 AM

Add back Move up/down actions.

DND won't be ready in Kirigami for 5.13, but as this port fixes
critical bugs vs. the old version - such as not actually saving
settings ... - we still want this in 5.13.

To prepare for DND, the model now has a method to do the moves.
Previously these were implemented by manipulating the list in
JS and setting it as a whole in the prop, which caused a
costly model reset. This is much smoother in the view.

hein updated this revision to Diff 34121.May 14 2018, 9:31 AM

Move removal into the C++ model code to avoid another model reset
and speed up the UX.

Please re-review now for 5.13 inclusion.

mart accepted this revision.May 14 2018, 9:54 AM
This revision is now accepted and ready to land.May 14 2018, 9:54 AM
This revision was automatically updated to reflect the committed changes.

Please, can I ask why is it impossible now to configure the keyboard layout as a result of this?

https://cgit.kde.org/plasma-desktop.git/diff/kcms/CMakeLists.txt?id=6055cfb94e1e8e42278cd4bea7d4debe4e745c7b

That was a mistake. It has been restored.

hein added a comment.May 16 2018, 9:39 AM

Sorry, accident! The code in the Keyboard KCM is ... interesting and literally breaks my system trying to build it (it needs a two-digit amount of GB of memory to build thanks to a boost-based keyboard layout preview generator it even changes the LLVM template recursion depth for), so I usually disable it so I don't have to hard reset my PC and lose data.

As part of porting can you please go through: https://bugs.kde.org/buglist.cgi?bug_status=UNCONFIRMED&bug_status=CONFIRMED&bug_status=ASSIGNED&bug_status=REOPENED&component=kcm_language&list_id=1517561&product=systemsettings&query_format=advanced

One about drag & drop is probably fixed by this, the majority need triaging to kcm_formats.
Interestingly no mention of it not saving.

hein added a comment.May 17 2018, 10:48 AM

I've cleaned this up/out now. About half or so got reassigned to either systemsettings:kcm_formats or frameworks-sonnet. Some got closed as FIXED and WORKSFORME. There's a bunch left. 2-3 have a point (e.g. the the language names), the rest is weird stuff.

Re saving: The part of saving that was broken is writing the list to the rc file so that the next time the KCM opens it shows the selection. But the env vars startkde sources did get written. So applying the settings worked but UI state was borked. At least I think so, because I copied over the rc saving part and it didn't work until I fixed it up. It seemed like KDE 4 code - it was passing KConfig the option to write to kdeglobals, when there's a seperate plasma-localerc now.