D12936: kcm_workspace is finished.

2018-05-16 Thread Furkan Tokac 
furkantokac updated this revision to Diff 34343.
furkantokac added a comment.


  Rewrite workspace KCM in QtQuick

REPOSITORY
  R119 Plasma Desktop

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

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
Cc: davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12936: kcm_workspace is finished.

2018-05-16 Thread Nathaniel Graham 
ngraham added inline comments.

INLINE COMMENTS

> kcm.cpp:133
> +// If yes, setNeedsSave(true).
> +void KCMWorkspaceOptions::handleIfNeedSave()
> +{

How about `handleNeedsSave()` instead?

> furkantokac wrote in ToolTip.qml:2
> Yeap it's Roman's implementation used in "input" kcm. Shouldn't I use it ?

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.

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: kcm_workspace is finished.

2018-05-16 Thread David Edmundson 
davidedmundson added inline comments.

INLINE COMMENTS

> furkantokac wrote in metadata.desktop:105-107
> I'm removing.
> Can you explain why ? Just want to learn.

Those are special hints which are used only by plasma applets. 
This isn't a plasma applet. So they do nothing.

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: kcm_workspace is finished.

2018-05-16 Thread Nathaniel Graham 
ngraham added a comment.


  Also, this patch doesn't actually solve 
https://bugs.kde.org/show_bug.cgi?id=393547; it just ports the KCM to QML in 
preparation for fixing it. So please remove the `BUG 393547` keyword and let's 
only add it to the patch that actually re-adds the missing feature.

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: kcm_workspace is finished.

2018-05-16 Thread Furkan Tokac 
furkantokac added a comment.


  In D12936#263939 , @ngraham wrote:
  
  > In D12936#263907 , @furkantokac 
wrote:
  >
  > > In D12936#263886 , 
@davidedmundson wrote:
  > >
  > > > Can you change the title to something more specific.
  > >
  > >
  > > There will be other options so that time it will make more sense I think 
(see the ss). Maybe we can change it to "General Workspace Settings". If you 
have any suggestion, you're welcome.
  > >  F5852492: Screenshot_20180517_015512.png 

  >
  >
  > David meant the title of the patch, not the title of the KCM. :)
  
  
  Got it :) I fixed "Default" button and some important things and applied the 
suggestions. Update is coming.

INLINE COMMENTS

> kcm.cpp:42
> +
> +setToolTip(true);
> +}

This is unnecessary. Just ignore it.

> ngraham wrote in ExclGroupBox.qml:1
> Is this file actually used at all?

ExclGroupBox.qml and ToolTip.qml are not used now but they'll be used. I forgot 
to remove them.

> davidedmundson wrote in ToolTip.qml:2
> is this copy pasted from somewhere?

Yeap it's Roman's implementation used in "input" kcm. Shouldn't I use it ?

> davidedmundson wrote in main.qml:24
> this is unused in every file

Actually it is used. It has "units" in it.

Example

  Layouts.ColumnLayout {
  id: maincol
  spacing: units.largeSpacing

> davidedmundson wrote in main.qml:37
> why is here a column inside a columnlayout?

Items under the "Column" has smallSpacing. Items under the "ColumnLayout" has 
largeSpacing. Otherwise, it looks bad.

> davidedmundson wrote in main.qml:43
> i18n

Thanks, fixing...

> davidedmundson wrote in metadata.desktop:105-107
> remove all X-Plasma lines

I'm removing.
Can you explain why ? Just want to learn.

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: kcm_workspace is finished.

2018-05-16 Thread Nathaniel Graham 
ngraham added a comment.


  In D12936#263907 , @furkantokac 
wrote:
  
  > In D12936#263886 , 
@davidedmundson wrote:
  >
  > > Can you change the title to something more specific.
  >
  >
  > There will be other options so that time it will make more sense I think 
(see the ss). Maybe we can change it to "General Workspace Settings". If you 
have any suggestion, you're welcome.
  >  F5852492: Screenshot_20180517_015512.png 

  
  
  David meant the title of the patch, not the title of the KCM. :)

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: kcm_workspace is finished.

2018-05-16 Thread Furkan Tokac 
furkantokac added a comment.


  In D12936#263894 , @ngraham wrote:
  
  > BTW, here are a couple of formatting rules you should follow: 
https://community.kde.org/Infrastructure/Phabricator#Formatting_your_patch
  
  
  Oh I know it but just forgot. Editing. Thanks.
  
  In D12936#263886 , @davidedmundson 
wrote:
  
  > Can you change the title to something more specific.
  
  
  There will be other options so that time it will make more sense I think (see 
the ss). Maybe we can change it to "General Workspace Settings". If you have 
any suggestion, you're welcome.
  F5852492: Screenshot_20180517_015512.png 


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: kcm_workspace is finished.

2018-05-16 Thread Nathaniel Graham 
ngraham added inline comments.

INLINE COMMENTS

> ExclGroupBox.qml:1
> +/*
> + * Copyright 2017 Roman Gilg 

Is this file actually used at all?

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: kcm_workspace is finished.

2018-05-16 Thread David Edmundson 
davidedmundson added a comment.


  Can you change the title to something more specific.

INLINE COMMENTS

> ToolTip.qml:2
> +/*
> + * Copyright 2017 Roman Gilg 
> + *

is this copy pasted from somewhere?

> main.qml:24
> +
> +import org.kde.plasma.core 2.0 as PlasmaCore
> +

this is unused in every file

> main.qml:37
> +// General Settings
> +Column {
> +spacing: units.smallSpacing

why is here a column inside a columnlayout?

> main.qml:43
> +id: generalSettings
> +text: "General Settings"
> +}

i18n

> metadata.desktop:105-107
> +X-Plasma-API=declarativeappletscript
> +
> +X-Plasma-MainScript=ui/main.qml

remove all X-Plasma lines

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: kcm_workspace is finished.

2018-05-16 Thread Nathaniel Graham 
ngraham added a comment.


  BTW, here are a couple of formatting rules you should follow: 
https://community.kde.org/Infrastructure/Phabricator#Formatting_your_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: kcm_workspace is finished.

2018-05-16 Thread 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
  kcm_workspaceoptions is changed as kcm_workspace.
  It is rewritten in QML, ConfigModule.
  ToolTip and VisualFeedback added.
  All features are tested.

REPOSITORY
  R119 Plasma Desktop

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/ExclGroupBox.qml
  kcms/workspaceoptions/package/contents/ui/ToolTip.qml
  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
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart