makis marimpis

unread,

Mar 13, 2012, 2:53:29 PM3/13/12

to makis marimpis, KDE Runtime

This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104261/

Review request for KDE Runtime.

By makis marimpis.

Description

Patches kactivitymanagerd to store (and restore back) the working current directory when switching activities.

The activity-changing-behavior is as follows:
1.  Say you have two (or more activities) A and B.
2.  You are working on activity A on Desktop 4.
3.  You switch to activity B (and by default to Desktop 4).
4.  Change to Desktop 1.
5.  Go back to activity A and (by default) to Desktop 1, while it should move you to Desktop 4 (this is where my patch kicks in).

I hope it makes sense :-)

Bugs: 241864, 265015

Diffs

service/ActivityManager.cpp (7af2049)
service/ActivityManager_p.h (d054eb7)

View Diff

Lamarque Vieira Souza

unread,

Mar 13, 2012, 5:44:06 PM3/13/12

to Lamarque Vieira Souza, makis marimpis, KDE Runtime

This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104261/

service/ActivityManager.cpp (Diff revision 1)
bool ActivityManagerPrivate::setCurrentActivity(const QString & id)
		277

remove trailing whitespace.

You should also save the desktop id in disk so that on next reboot kamd sets the currect desktop id. Just do

mainConfig().writeEntry("currentDesktop", desktopId);

here and read the value in ActivityManager::ActivityManager().

service/ActivityManager.cpp (Diff revision 1)
bool ActivityManagerPrivate::setCurrentActivity(const QString & id)
		289

remove trailing whitespace.

service/ActivityManager.cpp (Diff revision 1)
bool ActivityManagerPrivate::setCurrentActivity(const QString & id)
		292

remove trailing whitespace.

service/ActivityManager.cpp (Diff revision 1)
bool ActivityManagerPrivate::setCurrentActivity(const QString & id)
		296

remove trailing whitespace.

- Lamarque Vieira

On March 13th, 2012, 6:53 p.m., makis marimpis wrote:

Review request for KDE Runtime.

By makis marimpis.

Updated March 13, 2012, 6:53 p.m.

makis marimpis

unread,

Mar 14, 2012, 3:28:48 PM3/14/12

to makis marimpis, KDE Runtime, Lamarque Vieira Souza

This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104261/

Review request for KDE Runtime.

By makis marimpis.

Updated March 14, 2012, 7:28 p.m.

Changes

Added support for KConfigGroup storage and removed whitespaces.

Description

Patches kactivitymanagerd to store (and restore back) the working current directory when switching activities.

The activity-changing-behavior is as follows:
1.  Say you have two (or more activities) A and B.
2.  You are working on activity A on Desktop 4.
3.  You switch to activity B (and by default to Desktop 4).
4.  Change to Desktop 1.
5.  Go back to activity A and (by default) to Desktop 1, while it should move you to Desktop 4 (this is where my patch kicks in).

I hope it makes sense :-)

Bugs: 241864, 265015

Diffs (updated)

makis marimpis

unread,

Mar 15, 2012, 2:13:13 PM3/15/12

to makis marimpis, KDE Runtime, Lamarque Vieira Souza

This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104261/

Review request for KDE Runtime.

By makis marimpis.

Updated March 15, 2012, 6:13 p.m.

Description

Patches kactivitymanagerd to store (and restore back) the working current directory when switching activities.

The activity-changing-behavior is as follows:
1.  Say you have two (or more activities) A and B.
2.  You are working on activity A on Desktop 4.
3.  You switch to activity B (and by default to Desktop 4).
4.  Change to Desktop 1.
5.  Go back to activity A and (by default) to Desktop 1, while it should move you to Desktop 4 (this is where my patch kicks in).

I hope it makes sense :-)

Bugs: 241864, 265015

Diffs

Christoph Feck

unread,

Mar 15, 2012, 6:15:50 PM3/15/12

to makis marimpis, KDE Runtime, Christoph Feck

This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104261/

Could you please add group "Plasma" to reviewers?

- Christoph

makis marimpis

unread,

Mar 16, 2012, 3:40:00 AM3/16/12

to makis marimpis, KDE Runtime, Lamarque Vieira Souza, Plasma, Christoph Feck

This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104261/

Review request for KDE Runtime and Plasma.

By makis marimpis.

Updated March 16, 2012, 7:40 a.m.

Changes

Added group "Plasma" to reviewers group as requested.

Aaron J. Seigo

unread,

Mar 16, 2012, 7:49:15 AM3/16/12

to makis marimpis, KDE Runtime, Aaron J. Seigo, Plasma

This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104261/

generally looks good to me, save for one small issue.

service/ActivityManager.cpp (Diff revision 2)

bool ActivityManagerPrivate::setCurrentActivity(const QString & id)

		303	if (desktopId <= KWindowSystem::numberOfDesktops())
		304	KWindowSystem::setCurrentDesktop(desktopId);

should also check for >= 0. also, this statement needs {}s, we use them even for single line conditionals :)

- Aaron J.

On March 16th, 2012, 7:40 a.m., makis marimpis wrote:

Review request for KDE Runtime and Plasma.

By makis marimpis.

Updated March 16, 2012, 7:40 a.m.

Description

makis marimpis

unread,

Mar 16, 2012, 7:55:56 AM3/16/12

to Aaron J. Seigo, Lamarque Vieira Souza, Christoph Feck, Plasma, makis marimpis, KDE Runtime

This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104261/

Review request for KDE Runtime and Plasma.

By makis marimpis.

Updated March 16, 2012, 11:55 a.m.

Changes

Update "if condition".

Description

Patches kactivitymanagerd to store (and restore back) the working current directory when switching activities.

The activity-changing-behavior is as follows:
1.  Say you have two (or more activities) A and B.
2.  You are working on activity A on Desktop 4.
3.  You switch to activity B (and by default to Desktop 4).
4.  Change to Desktop 1.
5.  Go back to activity A and (by default) to Desktop 1, while it should move you to Desktop 4 (this is where my patch kicks in).

I hope it makes sense :-)

Bugs: 241864, 265015

Diffs (updated)

Ivan Čukić

unread,

Mar 16, 2012, 8:49:15 AM3/16/12

to makis marimpis, KDE Runtime, Plasma, Ivan Čukić

This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104261/

service/ActivityManager.cpp (Diff revision 3)
ActivityManagerPrivate::ActivityManagerPrivate(ActivityManager * parent,
		116	foreach (const QString & activity, activitiesDesktopsConfig().keyList()) {

Why do you want to keep these in memory?

- Ivan

On March 16th, 2012, 11:55 a.m., makis marimpis wrote:

Review request for KDE Runtime and Plasma.

By makis marimpis.

Updated March 16, 2012, 11:55 a.m.

Description

Patches kactivitymanagerd to store (and restore back) the working current directory when switching activities.

The activity-changing-behavior is as follows:
1.  Say you have two (or more activities) A and B.
2.  You are working on activity A on Desktop 4.
3.  You switch to activity B (and by default to Desktop 4).
4.  Change to Desktop 1.
5.  Go back to activity A and (by default) to Desktop 1, while it should move you to Desktop 4 (this is where my patch kicks in).

I hope it makes sense :-)

Bugs: 241864, 265015

Diffs

makis marimpis

unread,

Mar 16, 2012, 9:00:40 AM3/16/12

to makis marimpis, KDE Runtime, Plasma, Ivan Čukić

This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104261/

Hm, i did that in order to restore the desktop ids from a previous run of kamd (let's say, in case of log out).

- makis

Ivan Čukić

unread,

Mar 17, 2012, 7:51:58 AM3/17/12

to makis marimpis, KDE Runtime, Plasma, Ivan Čukić

This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104261/

On March 16th, 2012, 1 p.m., makis marimpis wrote:

Hm, i did that in order to restore the desktop ids from a previous run of kamd (let's say, in case of log out).

You misunderstood, I don't mind saving it in the config file, I don't understand the need to keep all those in memory.

For example, Bob has 20 activities, usually uses only 3 of them. Why would you want to keep the rest of the VD IDs in memory?

Just read the VD ID when necessary.

As you can see, we are not keeping anything that is saved to a config file in memory apart from the list of activities. The names, icons etc. are read from the config when needed.

makis marimpis

unread,

Mar 17, 2012, 12:18:17 PM3/17/12

to makis marimpis, KDE Runtime, Plasma, Ivan Čukić

This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104261/

On March 16th, 2012, 1 p.m., makis marimpis wrote:

Hm, i did that in order to restore the desktop ids from a previous run of kamd (let's say, in case of log out).

On March 17th, 2012, 11:52 a.m., Ivan Čukić wrote:

You misunderstood, I don't mind saving it in the config file, I don't understand the need to keep all those in memory.

For example, Bob has 20 activities, usually uses only 3 of them. Why would you want to keep the rest of the VD IDs in memory?

Just read the VD ID when necessary.

As you can see, we are not keeping anything that is saved to a config file in memory apart from the list of activities. The names, icons etc. are read from the config when needed.

Now i see your point.
I have implemented the same patch using only KConfigGroup but because of the "scheduleConfigSync" there is a case where the sync cannot keep up with the change of the activities - leading to a weird behavior (the changes are not yet made volatile).
That could be solved by calling "configSync" explicitly but that could affect the performace? (no idea).

To sum up: i think it is faster and less error-prone to store in memory and sync whenever sync is scheduled to, than to read/write whenever an activity is changed.

- makis

David Faure

unread,

Mar 18, 2012, 3:54:35 AM3/18/12

to makis marimpis, KDE Runtime, David Faure, Plasma, Ivan Čukić

This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104261/

On March 16th, 2012, 1 p.m., makis marimpis wrote:

Hm, i did that in order to restore the desktop ids from a previous run of kamd (let's say, in case of log out).

On March 17th, 2012, 11:52 a.m., Ivan Čukić wrote:

You misunderstood, I don't mind saving it in the config file, I don't understand the need to keep all those in memory.

For example, Bob has 20 activities, usually uses only 3 of them. Why would you want to keep the rest of the VD IDs in memory?

Just read the VD ID when necessary.

As you can see, we are not keeping anything that is saved to a config file in memory apart from the list of activities. The names, icons etc. are read from the config when needed.

On March 17th, 2012, 4:18 p.m., makis marimpis wrote:

Now i see your point.
I have implemented the same patch using only KConfigGroup but because of the "scheduleConfigSync" there is a case where the sync cannot keep up with the change of the activities - leading to a weird behavior (the changes are not yet made volatile).
That could be solved by calling "configSync" explicitly but that could affect the performace? (no idea).

To sum up: i think it is faster and less error-prone to store in memory and sync whenever sync is scheduled to, than to read/write whenever an activity is changed.

Not knowing the code around this, I would just like to point out that KConfig/KConfigGroup is already a "cache in memory". So, reading/writing should be fast. KConfig::sync is the slow bit (reading from disk if it was changed from the outside, then writing to disk).

Can't comment on "less error-prone" though, I don't know the activity code -- but indeed there's the general risk of a partial config read followed by a "save all", which loses everything that hadn't been read yet. Happened in old kmail far too many times.

- David

makis marimpis

unread,

Mar 18, 2012, 5:22:07 AM3/18/12

to Aaron J. Seigo, Lamarque Vieira Souza, David Faure, Christoph Feck, Plasma, makis marimpis, KDE Runtime, Ivan Čukić

This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104261/

Review request for KDE Runtime and Plasma.

By makis marimpis.

Updated March 18, 2012, 9:22 a.m.

Changes

Updated to store/retrieve "activity <=> desktop id" only to/from KConfigGroup.
Seems to work just fine :-)

Description

Patches kactivitymanagerd to store (and restore back) the working current directory when switching activities.

The activity-changing-behavior is as follows:
1.  Say you have two (or more activities) A and B.
2.  You are working on activity A on Desktop 4.
3.  You switch to activity B (and by default to Desktop 4).
4.  Change to Desktop 1.
5.  Go back to activity A and (by default) to Desktop 1, while it should move you to Desktop 4 (this is where my patch kicks in).

I hope it makes sense :-)

Bugs: 241864, 265015

Diffs (updated)

service/ActivityManager_p.h (d054eb7)
service/ActivityManager.cpp (7af2049)

View Diff

Ivan Čukić

unread,

Mar 21, 2012, 2:29:00 PM3/21/12

to makis marimpis, KDE Runtime, Plasma, Ivan Čukić

This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104261/

Ship it!

Ship It!

- Ivan

On March 18th, 2012, 9:22 a.m., makis marimpis wrote:

Review request for KDE Runtime and Plasma.

By makis marimpis.

Updated March 18, 2012, 9:22 a.m.

Description

Patches kactivitymanagerd to store (and restore back) the working current directory when switching activities.

The activity-changing-behavior is as follows:
1.  Say you have two (or more activities) A and B.
2.  You are working on activity A on Desktop 4.
3.  You switch to activity B (and by default to Desktop 4).
4.  Change to Desktop 1.
5.  Go back to activity A and (by default) to Desktop 1, while it should move you to Desktop 4 (this is where my patch kicks in).

I hope it makes sense :-)

Bugs: 241864, 265015

Diffs

makis marimpis

unread,

Mar 21, 2012, 2:52:45 PM3/21/12

to makis marimpis, KDE Runtime, Plasma, Ivan Čukić

This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104261/

On March 21st, 2012, 6:29 p.m., Ivan Čukić wrote:
Ship It!

please forgive my ignorance, but should i close it as submitted or wait? :-)

- makis

Reply all

Reply to author

Forward

Review Request: Remember current desktop when changing activity

makis marimpis

Description

Diffs

Lamarque Vieira Souza

makis marimpis

Changes

Description

Diffs (updated)

makis marimpis

Description

Diffs

Christoph Feck

makis marimpis

Changes

Aaron J. Seigo

Description

makis marimpis

Changes

Description

Diffs (updated)

Ivan Čukić

Description

Diffs

makis marimpis

Ivan Čukić

makis marimpis

David Faure

makis marimpis

Changes

Description

Diffs (updated)

Ivan Čukić

Description

Diffs

makis marimpis