Rewrite workspace KCM in QtQuick
ClosedPublic

Authored by furkantokac on May 16 2018, 10:43 PM.

Details

Summary

This patch renews the System Settings -> Desktop Behaviour -> Workspace.

Basically it is rewritten in QML, ConfigModule. Also, its naming has been
changed. It was called as kcm_workspaceoptions and workspaceoptions before
but now, it is kcm_workspace like other renewed KCMs' names.

Currently, ToolTip and VisualFeedback working fine. All the functionalities
are tested. "Default", "Reset" buttons works like a charm.

Diff Detail

Repository
R119 Plasma Desktop
Branch
bug393547-SingleDoubleClickFunc
Lint
No Linters Available
Unit
No Unit Test Coverage
furkantokac created this revision.May 16 2018, 10:43 PM
Restricted Application added a project: Plasma. · View Herald TranscriptMay 16 2018, 10:43 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
furkantokac requested review of this revision.May 16 2018, 10:43 PM
furkantokac edited projects, added Plasma: KCM Redesign; removed Plasma.
Restricted Application edited projects, added Plasma; removed Plasma: KCM Redesign. · View Herald TranscriptMay 16 2018, 10:46 PM

Can you change the title to something more specific.

kcms/workspaceoptions/package/contents/ui/ToolTip.qml
2 ↗(On Diff #34332)

is this copy pasted from somewhere?

kcms/workspaceoptions/package/contents/ui/main.qml
25

this is unused in every file

38

why is here a column inside a columnlayout?

44

i18n

kcms/workspaceoptions/package/metadata.desktop
106–108

remove all X-Plasma lines

furkantokac edited the summary of this revision. (Show Details)May 16 2018, 10:49 PM

BTW, here are a couple of formatting rules you should follow: https://community.kde.org/Infrastructure/Phabricator#Formatting_your_patch

ngraham added inline comments.May 16 2018, 10:52 PM
kcms/workspaceoptions/package/contents/ui/ExclGroupBox.qml
1 ↗(On Diff #34332)

Is this file actually used at all?

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.

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.

furkantokac edited the summary of this revision. (Show Details)May 16 2018, 11:07 PM
furkantokac edited the summary of this revision. (Show Details)May 16 2018, 11:22 PM

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.

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

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.

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.

kcms/workspaceoptions/kcm.cpp
42 ↗(On Diff #34332)

This is unnecessary. Just ignore it.

kcms/workspaceoptions/package/contents/ui/ExclGroupBox.qml
1 ↗(On Diff #34332)

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

kcms/workspaceoptions/package/contents/ui/ToolTip.qml
2 ↗(On Diff #34332)

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

kcms/workspaceoptions/package/contents/ui/main.qml
25

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

Example

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

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

44

Thanks, fixing...

kcms/workspaceoptions/package/metadata.desktop
106–108

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

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.

kcms/workspaceoptions/package/metadata.desktop
106–108

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

ngraham added inline comments.May 17 2018, 12:51 AM
kcms/workspaceoptions/kcm.cpp
133 ↗(On Diff #34332)

How about handleNeedsSave() instead?

kcms/workspaceoptions/package/contents/ui/ToolTip.qml
2 ↗(On Diff #34332)

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.

furkantokac edited the summary of this revision. (Show Details)May 17 2018, 1:25 AM

Rewrite workspace KCM in QtQuick

furkantokac retitled this revision from kcm_workspace is finished. to Rewrite workspace KCM in QtQuick.May 17 2018, 1:42 AM
furkantokac edited the summary of this revision. (Show Details)May 17 2018, 1:58 AM
furkantokac edited the summary of this revision. (Show Details)

Please follow standard KDE style for braces in new code:

void myFunction() {
    int stuff = 1;
}
ngraham added inline comments.May 17 2018, 2:25 AM
kcms/workspaceoptions/kcm.cpp
51 ↗(On Diff #34343)

Why are these within braces?

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 ?

kcms/workspaceoptions/kcm.cpp
51 ↗(On Diff #34343)

To make the

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

this definition local.

133 ↗(On Diff #34332)

Makes more sense :) Done.

kcms/workspaceoptions/package/contents/ui/ToolTip.qml
2 ↗(On Diff #34332)

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

romangg added inline comments.May 17 2018, 5:19 AM
kcms/workspaceoptions/kcm.cpp
107 ↗(On Diff #34343)
kcms/workspaceoptions/package/metadata.desktop
3

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.

furkantokac marked 16 inline comments as done.

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

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.

kcms/workspaceoptions/CMakeLists.txt
11–16

link not needed

12

link not needed

kcms/workspaceoptions/kcm.cpp
108 ↗(On Diff #34351)

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

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

romangg accepted this revision.May 17 2018, 9:45 AM
This revision is now accepted and ready to land.May 17 2018, 9:45 AM

Bug fixed about checkbox check states.

romangg edited the summary of this revision. (Show Details)May 17 2018, 11:17 AM
furkantokac marked 2 inline comments as done.May 17 2018, 5:34 PM
zzag added a subscriber: zzag.May 17 2018, 5:36 PM
zzag added inline comments.
kcms/workspaceoptions/workspaceoptions.cpp
12

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

This revision was automatically updated to reflect the committed changes.
yurchor added inline comments.
kcms/workspaceoptions/CMakeLists.txt
2

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

kcms/workspaceoptions/workspaceoptions.cpp
0

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

ltoscano added inline comments.
kcms/workspaceoptions/workspaceoptions.cpp
0

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

romangg added inline comments.May 18 2018, 8:01 AM
kcms/workspaceoptions/workspaceoptions.cpp
0

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.

ltoscano added inline comments.May 18 2018, 8:05 AM
kcms/workspaceoptions/workspaceoptions.cpp
0

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

romangg added inline comments.May 18 2018, 8:07 AM
kcms/workspaceoptions/Messages.sh
2

Change this line as follows?

$XGETTEXT `find . -name "*.cpp" -o -name "*.qml"` -o $podir/kcm_workspace.pot
ltoscano added inline comments.May 18 2018, 8:10 AM
kcms/workspaceoptions/Messages.sh
2

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

romangg added inline comments.May 18 2018, 8:16 AM
kcms/workspaceoptions/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.

ltoscano added inline comments.May 18 2018, 8:17 AM
kcms/workspaceoptions/Messages.sh
2

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

romangg added inline comments.May 18 2018, 8:28 AM
kcms/workspaceoptions/Messages.sh
2

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

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

furkantokac marked 11 inline comments as done.May 18 2018, 12:40 PM
furkantokac added inline comments.May 18 2018, 3:38 PM
kcms/workspaceoptions/workspaceoptions.cpp
12

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

furkantokac marked an inline comment as done.Jun 28 2018, 5:31 AM