D12974: Workspace KCM Code Improvement

[Furkan Tokac]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R119:de742972bf31: Workspace KCM Code Improvement (authored by 
furkantokac).

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12974?vs=34656=34759

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

AFFECTED FILES
  kcms/workspaceoptions/workspaceoptions.cpp
  kcms/workspaceoptions/workspaceoptions.h

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

D12974: Workspace KCM Code Improvement

[Roman Gilg]

romangg accepted this revision.
romangg added a comment.


  Looks good to me.  Only commit to master branch please.

REPOSITORY
  R119 Plasma Desktop

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

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

D12974: Workspace KCM Code Improvement

[Furkan Tokac]

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


  .qrc file is cancelled.

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12974?vs=34652=34656

BRANCH
  kcmworkspace-CodeFormatting

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

AFFECTED FILES
  kcms/workspaceoptions/workspaceoptions.cpp
  kcms/workspaceoptions/workspaceoptions.h

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

D12974: Workspace KCM Code Improvement

[Marco Martin]

mart requested changes to this revision.
mart added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> resources.qrc:1
> +
> +

I don't want this kcm done differently from all the others, i want  all modules 
using the same structure, on disk

REPOSITORY
  R119 Plasma Desktop

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

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

D12974: Workspace KCM Code Improvement

[Furkan Tokac]

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


  Changes are done according to feedbacks. For detailed information,
  please check the commit message.

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12974?vs=34499=34652

BRANCH
  kcmworkspace-CodeFormatting

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

AFFECTED FILES
  kcms/workspaceoptions/CMakeLists.txt
  kcms/workspaceoptions/resources.qrc
  kcms/workspaceoptions/workspaceoptions.cpp
  kcms/workspaceoptions/workspaceoptions.h

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

D12974: Workspace KCM Code Improvement

[Furkan Tokac]

furkantokac marked 8 inline comments as done.
furkantokac added inline comments.

INLINE COMMENTS

> davidedmundson wrote in workspaceoptions.cpp:97
> We want to batch our syncs to plasmarc, which the old code did better.
> 
> I wouldn't bother trying to be clever with checking if it's the original 
> state or not here, as KConfig will do all of that for us anyway at a more 
> correct lower level.

Nice point. I tested the situation for the old code. When I just change 
single/double click option, other option files (plasmarc) are not opened. That 
means KConfig handles the situation. Thank you. Correction will be done now.

REPOSITORY
  R119 Plasma Desktop

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

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

D12974: Workspace KCM Code Improvement

[Roman Gilg]

romangg added inline comments.

INLINE COMMENTS

> davidedmundson wrote in workspaceoptions.cpp:97
> We want to batch our syncs to plasmarc, which the old code did better.
> 
> I wouldn't bother trying to be clever with checking if it's the original 
> state or not here, as KConfig will do all of that for us anyway at a more 
> correct lower level.

For the sync yes.

For checking state: the old code wrote always all values to the config file, 
with or without the user changing them at all. It might make sense to only 
write config values which have been explicitly changed by the user, because 
this would mean that we can later on change the default of values of this user 
which have never been explicitly changed by the user before with an update of 
just the KCM code.

REPOSITORY
  R119 Plasma Desktop

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

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

D12974: Workspace KCM Code Improvement

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> workspaceoptions.cpp:97
> +
> +config->sync();
>  }

We want to batch our syncs to plasmarc, which the old code did better.

I wouldn't bother trying to be clever with checking if it's the original state 
or not here, as KConfig will do all of that for us anyway at a more correct 
lower level.

REPOSITORY
  R119 Plasma Desktop

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

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

D12974: Workspace KCM Code Improvement

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> workspaceoptions.cpp:51
>  
> -/*ConfigModule functions*/
> +// ***ConfigModule functions
>  void KCMWorkspaceOptions::load()

Why all the asterisks?

> workspaceoptions.h:58
>  // QML variables
> -bool m_ostateToolTip;   // Original state
> -bool m_stateToolTip;// Current state
> +bool m_toolTipOriginalState;   // Original state
> +bool m_toolTipCurrentState;// Current state

The inline comments are no longer needed now that the variable name is 
self-documenting.

REPOSITORY
  R119 Plasma Desktop

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

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

D12974: Workspace KCM Code Improvement

[Furkan Tokac]

furkantokac marked 3 inline comments as done.
furkantokac added inline comments.

INLINE COMMENTS

> zzag wrote in workspaceoptions.cpp:37
> I think you could do something like this in the header file
> 
>   class ...  {
>   ...
>   m_stateToolTip = true;
>   m_stateVisualFeedback = true;
>   m_stateSingleClick = true;
>   ...
>   };

This one is more clear imho because they are not optional initializations, they 
are must so we emphasize that.

> zzag wrote in workspaceoptions.cpp:180
> I would remove it. It doesn't add any useful information. I don't see any 
> word about commets in the kdelibs coding style(I assume Plasma follows it) 
> but as a rule of thumb: comment things that are not obvious.
> 
> For example, see 
> https://google.github.io/styleguide/cppguide.html#Implementation_Comments

Actually this is optional. Not a big issue. Makes it easier to follow. 
Specifically for this patch, it's okay imho.

REPOSITORY
  R119 Plasma Desktop

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

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

D12974: Workspace KCM Code Improvement

[Furkan Tokac]

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


  State variable names are changed. Some formatting improvements.

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12974?vs=34458=34499

BRANCH
  kcmworkspace-CodeFormatting

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

AFFECTED FILES
  kcms/workspaceoptions/workspaceoptions.cpp
  kcms/workspaceoptions/workspaceoptions.h

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

D12974: Workspace KCM Code Improvement

[Nathaniel Graham]

ngraham added a comment.


  In D12974#264991 , @furkantokac 
wrote:
  
  > In D12974#264920 , @ngraham 
wrote:
  >
  > > While we're doing some formatting and style cleanup work, how about 
renaming some variables? For example `m_ostateToolTip` and 
`m_ostateVisualFeedback` aren't very descriptive IMHO. Maybe instead, they 
could be `m_currentToolTipState` and `m_currentVisualFeedbackState`.
  >
  >
  > So they'll be changed as
  >  m_toolTipCurrentState m_toolTipOriginalState etc.
  
  
  Yep, sounds good!

REPOSITORY
  R119 Plasma Desktop

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

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

D12974: Workspace KCM Code Improvement

[Vlad Zagorodniy]

zzag added a comment.


  Also, about coding style. Why are you using `/*` for one line comments?

INLINE COMMENTS

> workspaceoptions.cpp:29
>  #include 
>  #include 
>  

https://community.kde.org/Policies/Kdelibs_Coding_Style#Qt_Includes

> workspaceoptions.cpp:37
>m_stateVisualFeedback(true),
>m_stateSingleClick(true)
>  {

I think you could do something like this in the header file

  class ...  {
  ...
  m_stateToolTip = true;
  m_stateVisualFeedback = true;
  m_stateSingleClick = true;
  ...
  };

> workspaceoptions.cpp:180
>  
>  /*SingleClick functions*/
>  bool KCMWorkspaceOptions::getSingleClick() const

I would remove it. It doesn't add any useful information. I don't see any word 
about commets in the kdelibs coding style(I assume Plasma follows it) but as a 
rule of thumb: comment things that are not obvious.

For example, see 
https://google.github.io/styleguide/cppguide.html#Implementation_Comments

REPOSITORY
  R119 Plasma Desktop

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

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

D12974: Workspace KCM Code Improvement

[Furkan Tokac]

furkantokac added a comment.


  In D12974#264920 , @ngraham wrote:
  
  > While we're doing some formatting and style cleanup work, how about 
renaming some variables? For example `m_ostateToolTip` and 
`m_ostateVisualFeedback` aren't very descriptive IMHO. Maybe instead, they 
could be `m_currentToolTipState` and `m_currentVisualFeedbackState`.
  
  
  So they'll be changed as
  m_toolTipCurrentState m_toolTipOriginalState etc.

REPOSITORY
  R119 Plasma Desktop

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

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

D12974: Workspace KCM Code Improvement

[Vlad Zagorodniy]

zzag removed a reviewer: zzag.
zzag added a comment.


  Thanks, but I'm not a member of #Plasma 
. ;-)

REPOSITORY
  R119 Plasma Desktop

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

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

D12974: Workspace KCM Code Improvement

[Nathaniel Graham]

ngraham added a comment.


  While we're doing some formatting and style cleanup work, how about renaming 
some variables? For example `m_ostateToolTip` and `m_ostateVisualFeedback` 
aren't very descriptive IMHO. Maybe instead, they could be 
`m_currentToolTipState` and `m_currentVisualFeedbackState`.

REPOSITORY
  R119 Plasma Desktop

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

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

D12974: Workspace KCM Code Improvement

[Furkan Tokac]

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

REVISION SUMMARY
  Code formatting is improved. "save" function is improved by preventing 
unnecessary file open and file write.

REPOSITORY
  R119 Plasma Desktop

BRANCH
  kcmworkspace-CodeFormatting

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

AFFECTED FILES
  kcms/workspaceoptions/workspaceoptions.cpp

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