D13194: Use the new drag handle in the Language KCM

[Eike Hein]

This revision was automatically updated to reflect the committed changes.
Closed by commit R119:59d0458bd09e: Use the new drag handle in the Language KCM 
(authored by hein).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D13194?vs=35814=36040#toc

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13194?vs=35814=36040

REVISION DETAIL
  https://phabricator.kde.org/D13194

AFFECTED FILES
  kcms/translations/package/contents/ui/main.qml
  kcms/translations/translations.cpp
  kcms/translations/translations.h
  kcms/translations/translationsmodel.cpp
  kcms/translations/translationsmodel.h

To: hein, mart, davidedmundson
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D13194: Use the new drag handle in the Language KCM

[Eike Hein]

hein added a comment.


  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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D13194

To: hein, mart, davidedmundson
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D13194: Use the new drag handle in the Language KCM

[Eike Hein]

hein updated this revision to Diff 35814.
hein added a comment.


  Remove width from RowLayout.

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13194?vs=35260=35814

BRANCH
  arcpatch-D13194

REVISION DETAIL
  https://phabricator.kde.org/D13194

AFFECTED FILES
  applets/kickoff/package/contents/ui/FullRepresentation.qml
  applets/kickoff/package/contents/ui/KickoffButton.qml
  kcms/translations/package/contents/ui/main.qml
  kcms/translations/translations.cpp
  kcms/translations/translations.h
  kcms/translations/translationsmodel.cpp
  kcms/translations/translationsmodel.h

To: hein, mart, davidedmundson
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D13194: Use the new drag handle in the Language KCM

[Eike Hein]

hein added inline comments.

INLINE COMMENTS

> mart wrote in main.qml:77
> this explicit reparenting shouldn't be necessary (and breaks in my branch for 
> multi level kcms)

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?

> mart wrote in main.qml:98
> is this width: necessary?

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

> mart wrote in main.qml:184
> this shouldn't be necessary

I'll remove it soon.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D13194

To: hein, mart, davidedmundson
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D13194: Use the new drag handle in the Language KCM

[Marco Martin]

mart added inline comments.

INLINE COMMENTS

> main.qml:77
>  
>  parent: root.parent
>  

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

> main.qml:98
> +delegate: Kirigami.DelegateRecycler {
> +width: parent.width
>  

is this width: necessary?

> main.qml:184
>  contentItem: RowLayout {
>  width: implicitWidth
>  

this shouldn't be necessary

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D13194

To: hein, mart, davidedmundson
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D13194: Use the new drag handle in the Language KCM

[Eike Hein]

hein updated this revision to Diff 35260.
hein added a comment.


  Drop height and anchors as per Marco's advise.

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13194?vs=35249=35260

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D13194

AFFECTED FILES
  kcms/translations/package/contents/ui/main.qml
  kcms/translations/translations.cpp
  kcms/translations/translations.h
  kcms/translations/translationsmodel.cpp
  kcms/translations/translationsmodel.h

To: hein, mart, davidedmundson
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D13194: Use the new drag handle in the Language KCM

[Eike Hein]

hein updated this revision to Diff 35249.
hein added a comment.


  Reference correct variable.

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13194?vs=35247=35249

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D13194

AFFECTED FILES
  kcms/translations/package/contents/ui/main.qml
  kcms/translations/translations.cpp
  kcms/translations/translations.h
  kcms/translations/translationsmodel.cpp
  kcms/translations/translationsmodel.h

To: hein, mart, davidedmundson
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D13194: Use the new drag handle in the Language KCM

[Eike Hein]

hein updated this revision to Diff 35247.
hein added a comment.


  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.

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13194?vs=35209=35247

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D13194

AFFECTED FILES
  kcms/translations/package/contents/ui/main.qml
  kcms/translations/translations.cpp
  kcms/translations/translations.h
  kcms/translations/translationsmodel.cpp
  kcms/translations/translationsmodel.h

To: hein, mart, davidedmundson
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D13194: Use the new drag handle in the Language KCM

[Eike Hein]

hein updated this revision to Diff 35209.
hein removed a subscriber: zzag.
hein added a comment.


  Implement Vlad's code hygiene suggestions.

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13194?vs=35208=35209

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D13194

AFFECTED FILES
  kcms/translations/package/contents/ui/main.qml
  kcms/translations/translations.cpp
  kcms/translations/translations.h
  kcms/translations/translationsmodel.cpp
  kcms/translations/translationsmodel.h

To: hein, mart, davidedmundson
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart, zzag

D13194: Use the new drag handle in the Language KCM

[Vlad Zagorodniy]

zzag added inline comments.

INLINE COMMENTS

> translations.cpp:97
>  
> -for (const QString& lang : m_translationsModel->missingLanguages()) {
> +for (const QString& lang : 
> m_selectedTranslationsModel->missingLanguages()) {
>  m_configuredLanguages.removeOne(lang);

Maybe,

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

(to avoid detach)

> translationsmodel.cpp:103
> +return QLocale(QStringLiteral("pt_PT")).nativeLanguageName();
> +} else {
> +qWarning() << "Language code morphed into another existing 
> language code, please report!" << languageCode << locale.name();

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

References:

- https://releases.llvm.org/2.7/docs/CodingStandards.html#hl_else_after_return 
(Kdelibs coding style doesn't say anything about early returns)

> translationsmodel.cpp:197
>  
> -m_selectedLanguages.move(from, to);
> +int modelTo = to + (to > from ? 1 : 0);
>  

Can be const.

> translationsmodel.cpp:218
>  
> -emit selectedLanguagesChanged();
> +if (index != -1) {
> +beginRemoveRows(QModelIndex(), index, index);

How about early return? E.g.

  if (index < 0) {
  return;
  }
  
  // ...

> translationsmodel.cpp:254
> +
> +return TranslationsModel::data(index, role);
> +}

Dead code.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D13194

To: hein, mart, davidedmundson
Cc: zzag, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D13194: Use the new drag handle in the Language KCM

[Eike Hein]

hein added a comment.


  Screenshot:
  
  F5878558: Screenshot_20180531_030134.png 


REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D13194

To: hein, mart, davidedmundson
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D13194: Use the new drag handle in the Language KCM

[Eike Hein]

hein added a comment.


  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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D13194

To: hein, mart, davidedmundson
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D13194: Use the new drag handle in the Language KCM

[Eike Hein]

hein updated this revision to Diff 35208.
hein edited the summary of this revision.
hein added a comment.


  Mention the use of DelegateRecycler in the message.

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13194?vs=35207=35208

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D13194

AFFECTED FILES
  kcms/translations/package/contents/ui/main.qml
  kcms/translations/translations.cpp
  kcms/translations/translations.h
  kcms/translations/translationsmodel.cpp
  kcms/translations/translationsmodel.h

To: hein, mart, davidedmundson
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D13194: Use the new drag handle in the Language KCM

[Eike Hein]

hein updated this revision to Diff 35207.
hein edited the summary of this revision.
hein added a reviewer: davidedmundson.
hein added a comment.


  - Rewrite model backend to make list drag handle happy.
  - Use QCollator for available languages.
  - Code cleanup.

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13194?vs=35110=35207

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D13194

AFFECTED FILES
  kcms/translations/package/contents/ui/main.qml
  kcms/translations/translations.cpp
  kcms/translations/translations.h
  kcms/translations/translationsmodel.cpp
  kcms/translations/translationsmodel.h

To: hein, mart, davidedmundson
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D13194: Use the new drag handle in the Language KCM

[Eike Hein]

hein created this revision.
hein added a reviewer: mart.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.
hein requested review of this revision.

REVISION SUMMARY
  WIP.

REPOSITORY
  R119 Plasma Desktop

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D13194

AFFECTED FILES
  kcms/translations/package/contents/ui/main.qml

To: hein, mart
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart