Use the new drag handle in the Language KCM
ClosedPublic

Authored by hein on May 29 2018, 1:12 PM.

Details

Summary

This makes the list items draggable.

To do this, the backend model was split up into three models so
the move() method could actually move a row as the new drag handle
expects. This also makes move and remove actions a little bit
faster since there's no proxy abstractions involved. The use of
Plasma.SortFilterModel with JS callbacks for filtering has been
dropped from the QML code.

The lists now use DelegateRecycler to be speedier.

The list of available languages is now guaranteed to be sorted in
a locale-aware manner and case-insensitively, using QCollator.
This likely works better than whatever PSFM was doing with the
Qt::DisplayRole sort role before.

Also drops a stray unused KConfigGroup member from
TranslationsModel.

Diff Detail

Repository
R119 Plasma Desktop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
hein created this revision.May 29 2018, 1:12 PM
Restricted Application added a project: Plasma. · View Herald TranscriptMay 29 2018, 1:12 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
hein requested review of this revision.May 29 2018, 1:12 PM
hein planned changes to this revision.May 29 2018, 1:12 PM
hein updated this revision to Diff 35207.May 30 2018, 5:57 PM
hein edited the summary of this revision. (Show Details)
hein added a reviewer: davidedmundson.
  • Rewrite model backend to make list drag handle happy.
  • Use QCollator for available languages.
  • Code cleanup.
hein updated this revision to Diff 35208.May 30 2018, 5:58 PM
hein edited the summary of this revision. (Show Details)

Mention the use of DelegateRecycler in the message.

hein added a comment.May 30 2018, 6:00 PM

This is now ready for review.

With the use of DelegateRecycler, I get many ReferenceErrors from particular the sheet list delegate during scroll:

kcmshell5(8088)/default unknown: file:///home/eike/devel/install/share/kpackage/kcms/kcm_translations/contents/ui/main.qml:41: TypeError: Cannot read property 'display' of undefined
kcmshell5(8088)/default unknown: file:///home/eike/devel/install/share/kpackage/kcms/kcm_translations/contents/ui/main.qml:37: TypeError: Cannot read property 'LanguageCode' of undefined

I suspect this is some sort of race condition. @mart needs to have a look. The code in the diff itself should be unrelated to this I hope.

hein added a comment.EditedMay 30 2018, 6:01 PM

Screenshot:

zzag added a subscriber: zzag.May 30 2018, 6:07 PM
zzag added inline comments.
kcms/translations/translations.cpp
97–98

Maybe,

const auto missingLanguages = m_selectedTransationsModel->missingLanguages();
for (const QString& lang : missingLanguages) {
   // ...
}

(to avoid detach)

kcms/translations/translationsmodel.cpp
103

Coding style nitpick: don't use else after return.

References:

195

Can be const.

234–236

How about early return? E.g.

if (index < 0) {
    return;
}

// ...
266

Dead code.

hein updated this revision to Diff 35209.May 30 2018, 6:19 PM
hein removed a subscriber: zzag.

Implement Vlad's code hygiene suggestions.

hein updated this revision to Diff 35247.May 31 2018, 12:26 PM

Uncheck sheet list items when the sheet is closed.

This now has to be done explicitly as the delegates no longer get
destroyed with the use of a DelegateRecycler.

hein updated this revision to Diff 35249.May 31 2018, 12:35 PM

Reference correct variable.

hein updated this revision to Diff 35260.EditedMay 31 2018, 2:58 PM

Drop height and anchors as per Marco's advice.

mart added inline comments.Jun 7 2018, 10:24 AM
kcms/translations/package/contents/ui/main.qml
77

this explicit reparenting shouldn't be necessary (and breaks in my branch for multi level kcms)

98

is this width: necessary?

183–184

this shouldn't be necessary

hein added inline comments.Jun 8 2018, 12:00 AM
kcms/translations/package/contents/ui/main.qml
77

I've told you a few times now that it's been necessary for me so far, since the sheet doesn't appear or in bizarre locations otherwise. Did you test?

98

I added it because you told me several times that it's necessary now. You tell me?

183–184

I'll remove it soon.

hein updated this revision to Diff 35814.Jun 8 2018, 12:39 AM

Remove width from RowLayout.

hein added a comment.Jun 8 2018, 12:43 AM

The other two are currently still needed:

  • Without the parenting, the sheet doesn't show up at all.
  • Without setting the width on the DelegateRecycler (which you told me was necessary, and it seems to be true!) the delegates in the sheet aren't full width so the seperator lines don't span the entire view.
mart accepted this revision.Jun 12 2018, 9:47 AM
This revision is now accepted and ready to land.Jun 12 2018, 9:47 AM
This revision was automatically updated to reflect the committed changes.