D12974: Workspace KCM Code Improvement

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 SINC

D12974: Workspace KCM Code Improvement

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

D12974: Workspace KCM Code Improvement

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&id=34656 BRANCH kcmworkspace-CodeFormatting REVISION DETAIL https://phabricator.

D12974: Workspace KCM Code Improvement

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

D12974: Workspace KCM Code Improvement

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&id=34652

D12974: Workspace KCM Code Improvement

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

D12974: Workspace KCM Code Improvement

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 t

D12974: Workspace KCM Code Improvement

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

D12974: Workspace KCM Code Improvement

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

D12974: Workspace KCM Code Improvement

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; >

D12974: Workspace KCM Code Improvement

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&id=34499 BRANCH kcmworkspace-CodeFormatting

D12974: Workspace KCM Code Improvement

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 variable

D12974: Workspace KCM Code Improvement

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(

D12974: Workspace KCM Code Improvement

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 IMH

D12974: Workspace KCM Code Improvement

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, ragr

D12974: Workspace KCM Code Improvement

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_currentVisualFeedbackStat

D12974: Workspace KCM Code Improvement

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 ope