D12936: Rewrite workspace KCM in QtQuick

[Furkan Tokac]

furkantokac added inline comments.

INLINE COMMENTS

> zzag wrote in workspaceoptions.cpp:110
> Coding style nitpick: No whitespace after `(` and before `)`. Also, I 
> believe, there should be a whitespace between control statement keyword and 
> opening parentheses.

Done. Thanks. I avoid small changes for better diff revision so now I'm 
reviewing the whole patch.

REPOSITORY
  R119 Plasma Desktop

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

To: furkantokac, romangg, ngraham, #plasma
Cc: ltoscano, yurchor, zzag, davidedmundson, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12936: Rewrite workspace KCM in QtQuick

[Roman Gilg]

romangg added inline comments.

INLINE COMMENTS

> ltoscano wrote in Messages.sh:2
> Because they did not ask :)
> But in most cases the renamed KCMs kept the old name, and that's fine too.

Jeez, I won't say who these people were (Night Color KCM...).

Ok, I created a patch in D12956 . Pls 
quickly ack.

REPOSITORY
  R119 Plasma Desktop

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

To: furkantokac, romangg, ngraham, #plasma
Cc: ltoscano, yurchor, zzag, davidedmundson, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12936: Rewrite workspace KCM in QtQuick

[Luigi Toscano]

ltoscano added inline comments.

INLINE COMMENTS

> romangg wrote in Messages.sh:2
> Ok, I will upload a diff shortly in order to change it here and in the 
> TANSLATION_DOMAIN to `kcm5_workspace`. I was just a bit unsure about the `5` 
> in the name, because other QML rewrites of KCMs I saw didn't have it in their 
> TRANSLATION_DOMAIN/Messages.sh.

Because they did not ask :)
But in most cases the renamed KCMs kept the old name, and that's fine too.

REPOSITORY
  R119 Plasma Desktop

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

To: furkantokac, romangg, ngraham, #plasma
Cc: ltoscano, yurchor, zzag, davidedmundson, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12936: Rewrite workspace KCM in QtQuick

[Roman Gilg]

romangg added inline comments.

INLINE COMMENTS

> ltoscano wrote in Messages.sh:2
> Either the old name, or kcm5_, and it should be the same as 
> TRANSLATION_DOMAIN.

Ok, I will upload a diff shortly in order to change it here and in the 
TANSLATION_DOMAIN to `kcm5_workspace`. I was just a bit unsure about the `5` in 
the name, because other QML rewrites of KCMs I saw didn't have it in their 
TRANSLATION_DOMAIN/Messages.sh.

REPOSITORY
  R119 Plasma Desktop

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

To: furkantokac, romangg, ngraham, #plasma
Cc: ltoscano, yurchor, zzag, davidedmundson, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12936: Rewrite workspace KCM in QtQuick

[Luigi Toscano]

ltoscano added inline comments.

INLINE COMMENTS

> romangg wrote in Messages.sh:2
> Change this line as follows?
> 
>   $XGETTEXT `find . -name "*.cpp" -o -name "*.qml"` -o 
> $podir/kcm_workspace.pot

Either the old name, or kcm5_, and it should be the same as 
TRANSLATION_DOMAIN.

REPOSITORY
  R119 Plasma Desktop

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

To: furkantokac, romangg, ngraham, #plasma
Cc: ltoscano, yurchor, zzag, davidedmundson, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12936: Rewrite workspace KCM in QtQuick

[Roman Gilg]

romangg added inline comments.

INLINE COMMENTS

> Messages.sh:2
>  #! /usr/bin/env bash
> -$EXTRACTRC *.ui >> rc.cpp
> -$XGETTEXT *.cpp -o $podir/kcmworkspaceoptions.pot
> +$XGETTEXT `find . -name "*.cpp" -o -name "*.qml"` -o 
> $podir/kcm_workspaceoptions.pot

Change this line as follows?

  $XGETTEXT `find . -name "*.cpp" -o -name "*.qml"` -o $podir/kcm_workspace.pot

REPOSITORY
  R119 Plasma Desktop

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

To: furkantokac, romangg, ngraham, #plasma
Cc: ltoscano, yurchor, zzag, davidedmundson, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12936: Rewrite workspace KCM in QtQuick

[Luigi Toscano]

ltoscano added inline comments.

INLINE COMMENTS

> romangg wrote in workspaceoptions.cpp:34
> So do we need to change this here, in the CMake file, or can the translation 
> catalog be renamed (or a new one created) with the name `kcm_workspace` to 
> make the naming consistent?
> 
> Before in this KCM we used `kcm_workspaceoptions`, `workspaceoptions` and 
> `kcmworkspaceoptions` at different places. I want to use `kcm_workspace` only 
> now everywhere.

Either you keep the old name, or you change it to kcm5_ (which may help 
avoiding headaches in future). The values in TRANSLATION_DOMAIN and Messages.sh 
should be consistent.

REPOSITORY
  R119 Plasma Desktop

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

To: furkantokac, romangg, ngraham, #plasma
Cc: ltoscano, yurchor, zzag, davidedmundson, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12936: Rewrite workspace KCM in QtQuick

[Roman Gilg]

romangg added inline comments.

INLINE COMMENTS

> ltoscano wrote in workspaceoptions.cpp:34
> Iirc that's unrelated. The catalog name is not set by this call.

So do we need to change this here, in the CMake file, or can the translation 
catalog be renamed (or a new one created) with the name `kcm_workspace` to make 
the naming consistent?

Before in this KCM we used `kcm_workspaceoptions`, `workspaceoptions` and 
`kcmworkspaceoptions` at different places. I want to use `kcm_workspace` only 
now everywhere.

REPOSITORY
  R119 Plasma Desktop

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

To: furkantokac, romangg, ngraham, #plasma
Cc: ltoscano, yurchor, zzag, davidedmundson, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12936: Rewrite workspace KCM in QtQuick

[Luigi Toscano]

ltoscano added inline comments.

INLINE COMMENTS

> yurchor wrote in workspaceoptions.cpp:34
> This might not work with translations as the translation catalog is named 
> "kcm_workspaceoptions".

Iirc that's unrelated. The catalog name is not set by this call.

REPOSITORY
  R119 Plasma Desktop

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

To: furkantokac, romangg, ngraham, #plasma
Cc: ltoscano, yurchor, zzag, davidedmundson, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12936: Rewrite workspace KCM in QtQuick

[Yuri Chornoivan]

yurchor added inline comments.

INLINE COMMENTS

> CMakeLists.txt:2
>  # KI18N Translation Domain for this library
> -add_definitions(-DTRANSLATION_DOMAIN=\"kcmworkspaceoptions\")
> -### next target ###
> +add_definitions(-DTRANSLATION_DOMAIN=\"kcm_workspace\")
>  

That will not work. The name of the translation catalog is 
"kcm_workspaceoptions" (see below).

> workspaceoptions.cpp:34
>  {
> -KAboutData *about =
> -new KAboutData(QStringLiteral("kcmworkspaceoptions"), i18n("Global 
> options for the Plasma Workspace"),
> -   QStringLiteral("1.0"), QString(), KAboutLicense::GPL,
> -   i18n("(c) 2009 Marco Martin"));
> -
> -about->addAuthor(i18n("Marco Martin"), i18n("Maintainer"), 
> QStringLiteral("notm...@gmail.com"));
> +KAboutData* about = new KAboutData(QStringLiteral("kcm_workspace"),
> +i18n("Plasma Workspace global options"),

This might not work with translations as the translation catalog is named 
"kcm_workspaceoptions".

REPOSITORY
  R119 Plasma Desktop

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

To: furkantokac, romangg, ngraham, #plasma
Cc: yurchor, zzag, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12936: Rewrite workspace KCM in QtQuick

[Furkan Tokac]

This revision was automatically updated to reflect the committed changes.
Closed by commit R119:856f58955aec: Rewrite workspace KCM in QtQuick (authored 
by furkantokac).

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12936?vs=34357=34387

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

AFFECTED FILES
  kcms/workspaceoptions/CMakeLists.txt
  kcms/workspaceoptions/Messages.sh
  kcms/workspaceoptions/kcm_workspace.desktop
  kcms/workspaceoptions/mainpage.ui
  kcms/workspaceoptions/package/contents/ui/main.qml
  kcms/workspaceoptions/package/metadata.desktop
  kcms/workspaceoptions/workspaceoptions.cpp
  kcms/workspaceoptions/workspaceoptions.desktop
  kcms/workspaceoptions/workspaceoptions.h

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

D12936: Rewrite workspace KCM in QtQuick

[Vlad Zagorodniy]

zzag added inline comments.

INLINE COMMENTS

> workspaceoptions.cpp:110
> +// Prevent from binding loop
> +if( m_stateToolTip == state ) {
> +return;

Coding style nitpick: No whitespace after `(` and before `)`. Also, I believe, 
there should be a whitespace between control statement keyword and opening 
parentheses.

REPOSITORY
  R119 Plasma Desktop

BRANCH
  bug393547-SingleDoubleClickFunc

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

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

D12936: Rewrite workspace KCM in QtQuick

[Furkan Tokac]

furkantokac updated this revision to Diff 34357.
furkantokac added a comment.


  Bug fixed about checkbox check states.

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12936?vs=34354=34357

BRANCH
  bug393547-SingleDoubleClickFunc

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

AFFECTED FILES
  kcms/workspaceoptions/CMakeLists.txt
  kcms/workspaceoptions/Messages.sh
  kcms/workspaceoptions/kcm_workspace.desktop
  kcms/workspaceoptions/mainpage.ui
  kcms/workspaceoptions/package/contents/ui/main.qml
  kcms/workspaceoptions/package/metadata.desktop
  kcms/workspaceoptions/workspaceoptions.cpp
  kcms/workspaceoptions/workspaceoptions.desktop
  kcms/workspaceoptions/workspaceoptions.h

To: furkantokac, romangg, ngraham, #plasma
Cc: davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12936: Rewrite workspace KCM in QtQuick

[Furkan Tokac]

furkantokac updated this revision to Diff 34354.
furkantokac added a comment.


  kcm.cpp and kcm.h files are renamed as workspaceoptions.cpp and 
workspaceoptions.h to preserve the git history better.

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12936?vs=34353=34354

BRANCH
  bug393547-SingleDoubleClickFunc

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

AFFECTED FILES
  kcms/workspaceoptions/CMakeLists.txt
  kcms/workspaceoptions/Messages.sh
  kcms/workspaceoptions/kcm_workspace.desktop
  kcms/workspaceoptions/mainpage.ui
  kcms/workspaceoptions/package/contents/ui/main.qml
  kcms/workspaceoptions/package/metadata.desktop
  kcms/workspaceoptions/workspaceoptions.cpp
  kcms/workspaceoptions/workspaceoptions.desktop
  kcms/workspaceoptions/workspaceoptions.h

To: furkantokac, romangg, ngraham, #plasma
Cc: davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12936: Rewrite workspace KCM in QtQuick

[Furkan Tokac]

furkantokac updated this revision to Diff 34353.
furkantokac added a comment.


  kcm.cpp and kcm.h files are renamed as workspaceoptions.cpp and 
workspaceoptions.h to preserve the git history better.

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12936?vs=34351=34353

BRANCH
  bug393547-SingleDoubleClickFunc

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

AFFECTED FILES
  kcms/workspaceoptions/CMakeLists.txt
  kcms/workspaceoptions/Messages.sh
  kcms/workspaceoptions/kcm_workspace.desktop
  kcms/workspaceoptions/mainpage.ui
  kcms/workspaceoptions/package/contents/ui/main.qml
  kcms/workspaceoptions/package/metadata.desktop
  kcms/workspaceoptions/workspaceoptions.cpp
  kcms/workspaceoptions/workspaceoptions.desktop
  kcms/workspaceoptions/workspaceoptions.h

To: furkantokac, romangg, ngraham, #plasma
Cc: davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12936: Rewrite workspace KCM in QtQuick

[Roman Gilg]

romangg added a comment.


  Please don't rename `workspaceoptions.h` and `workspaceoptions.cpp` to 
`kcm.h` and `kcm.cpp` in this patch to preserve the git history better. But imo 
you can remove Marco's copyright from these files, because you basically 
rewrote the files from scratch.

INLINE COMMENTS

> CMakeLists.txt:14
> +KF5::I18n
> +KF5::KIOCore
> +KF5::ConfigWidgets

link not needed

> CMakeLists.txt:18
>  
> -target_link_libraries(kcm_workspaceoptions  KF5::KIOCore KF5::ConfigWidgets 
> KF5::I18n)
> +Qt5::QuickWidgets
> +)

link not needed

> kcm.cpp:108
> +// Prevent from binding loop
> +if( m_stateToolTip == state ) {
> +return;

And a whitespace between `if` and `(`: 
https://community.kde.org/Policies/Kdelibs_Coding_Style#Whitespace

REPOSITORY
  R119 Plasma Desktop

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

To: furkantokac, romangg, ngraham, #plasma
Cc: davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12936: Rewrite workspace KCM in QtQuick

[Furkan Tokac]

furkantokac updated this revision to Diff 34351.
furkantokac marked 16 inline comments as done.
furkantokac added a comment.


  Bracked style correction. Removed translation data in ".desktop" files.

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12936?vs=34343=34351

BRANCH
  bug393547-SingleDoubleClickFunc

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

AFFECTED FILES
  kcms/workspaceoptions/CMakeLists.txt
  kcms/workspaceoptions/Messages.sh
  kcms/workspaceoptions/kcm.cpp
  kcms/workspaceoptions/kcm.h
  kcms/workspaceoptions/kcm_workspace.desktop
  kcms/workspaceoptions/mainpage.ui
  kcms/workspaceoptions/package/contents/ui/main.qml
  kcms/workspaceoptions/package/metadata.desktop
  kcms/workspaceoptions/workspaceoptions.cpp
  kcms/workspaceoptions/workspaceoptions.desktop
  kcms/workspaceoptions/workspaceoptions.h

To: furkantokac, romangg, ngraham, #plasma
Cc: davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12936: Rewrite workspace KCM in QtQuick

[Roman Gilg]

romangg added inline comments.

INLINE COMMENTS

> kcm.cpp:107
> +{
> +if( m_stateToolTip == state ) return;
> +

needs braces. see https://community.kde.org/Policies/Kdelibs_Coding_Style#Braces

> metadata.desktop:2
> +[Desktop Entry]
> +Name=Workspace
> +Name[ar]=مساحة العمل

I believe you should remove all `Name[...]=...` and only leave the 
`Name=Workspace` line. Same for `Comment`. This stuff is added by the 
translation team.

REPOSITORY
  R119 Plasma Desktop

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

To: furkantokac, romangg, ngraham, #plasma
Cc: davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12936: Rewrite workspace KCM in QtQuick

[Furkan Tokac]

furkantokac added a comment.


  In D12936#263969 , @ngraham wrote:
  
  > Please follow standard KDE style for braces in new code:
  >
  >   void myFunction() {
  >   int stuff = 1;
  >   }
  >
  
  
  According to Kdelibs Coding Style 
, functions and 
classes should be
  
void fun()
{
}
  
  like this. Did I miss a point ?

INLINE COMMENTS

> ngraham wrote in kcm.cpp:51
> Why are these within braces?

To make the

  const KConfigGroup cg(config, QStringLiteral("PlasmaToolTips"));

this definition local.

> ngraham wrote in kcm.cpp:133
> How about `handleNeedsSave()` instead?

Makes more sense :) Done.

> ngraham wrote in ToolTip.qml:2
> If this code is used in multiple KCMs, we shouldn't duplicate it; we should 
> upstream it so that it only needs to exist in one place at a time.

It can be reused. I'll delete it now (new patch is coming) then we can talk 
about it for the other patch.

REPOSITORY
  R119 Plasma Desktop

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

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

D12936: Rewrite workspace KCM in QtQuick

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> kcm.cpp:51
> +// Load toolTip
> +{
> +const KConfigGroup cg(config, QStringLiteral("PlasmaToolTips"));

Why are these within braces?

REPOSITORY
  R119 Plasma Desktop

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

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

D12936: Rewrite workspace KCM in QtQuick

[Nathaniel Graham]

ngraham added a comment.


  Please follow standard KDE style for braces in new code:
  
void myFunction() {
int stuff = 1;
}

REPOSITORY
  R119 Plasma Desktop

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

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