DevHeads.net

Review Request: Reset time format upon user request

Review request for kdelibs and Plasma.

Description
The patch resets time format in digital clock plasmoid when the user changes the 24h configuration in active-settings.

The reset part is from kdelibs/kdecore/localization/klocale_kde.cpp. I am wondering if I should add this change to kdelibs instead of kde-workspace to avoid duplicating code. Anyway, I wanted someone to review the code to see if there can be any side effect.

This addresses bug 289094.
<a href="http://bugs.kde.org/show_bug.cgi?id=289094" title="http://bugs.kde.org/show_bug.cgi?id=289094">http://bugs.kde.org/show_bug.cgi?id=289094</a>

Diffs
plasma/generic/applets/digital-clock/clock.h 4aec3fd
plasma/generic/applets/digital-clock/clock.cpp dd03692

Diff: <a href="http://git.reviewboard.kde.org/r/103434/diff/diff" title="http://git.reviewboard.kde.org/r/103434/diff/diff">http://git.reviewboard.kde.org/r/103434/diff/diff</a>

Testing
Works in Plasma Active. In Plasma Desktop kcmlocale does not call KGlobalSettings::self()->emitChange(KGlobalSettings::SettingsChanged) so it does not take effect. Other kcm modules (e.g. keyboard), call emitChange.

Thanks,

Lamarque Vieira Souza

Comments

Re: Review Request: Reset time format upon user request

By Lamarque V. Souza at 12/16/2011 - 17:13

(Updated Dec. 16, 2011, 9:13 p.m.)

Review request for kdelibs and Plasma.

Changes
Use KGlobal::locale()->setLanguage() to force reading the configuration file instead of duplicating kdelibs code.

Use KGlobalSettings::SETTINGS_COMPLETION category to decide when take action. I have not found a better category, the available are: SETTINGS_MOUSE, SETTINGS_COMPLETION, SETTINGS_PATHS, SETTINGS_POPUPMENU, SETTINGS_QT, SETTINGS_SHORTCUTS. There is no documentation about how use those categories in kglobalsettings.h.

There are several other set*() methods in klocale_kde.h. Adding a signal only for TimeFormat change will not solve the problem for them, specialy for set*Date*(), which are other calls that the clock plasmoid is interested in.

Maybe KLocale could have a localeChanged(changedSetting) signal, indicating what has changed. On the other hand... sending a signal for every KLocale's setting change sounds a little overkill.

Description
The patch resets time format in digital clock plasmoid when the user changes the 24h configuration in active-settings.

The reset part is from kdelibs/kdecore/localization/klocale_kde.cpp. I am wondering if I should add this change to kdelibs instead of kde-workspace to avoid duplicating code. Anyway, I wanted someone to review the code to see if there can be any side effect.

This addresses bug 289094.
<a href="http://bugs.kde.org/show_bug.cgi?id=289094" title="http://bugs.kde.org/show_bug.cgi?id=289094">http://bugs.kde.org/show_bug.cgi?id=289094</a>

Diffs (updated)
plasma/generic/applets/digital-clock/clock.h 4aec3fd
plasma/generic/applets/digital-clock/clock.cpp dd03692

Diff: <a href="http://git.reviewboard.kde.org/r/103434/diff/diff" title="http://git.reviewboard.kde.org/r/103434/diff/diff">http://git.reviewboard.kde.org/r/103434/diff/diff</a>

Testing
Works in Plasma Active. In Plasma Desktop kcmlocale does not call KGlobalSettings::self()->emitChange(KGlobalSettings::SettingsChanged) so it does not take effect. Other kcm modules (e.g. keyboard), call emitChange.

Thanks,

Lamarque Vieira Souza

Re: Review Request: Reset time format upon user request

By Lamarque V. Souza at 12/19/2011 - 10:03

(Updated Dec. 19, 2011, 2:03 p.m.)

Review request for kdelibs and Plasma.

Changes
Use patch <a href="http://git.reviewboard.kde.org/r/103469" title="http://git.reviewboard.kde.org/r/103469">http://git.reviewboard.kde.org/r/103469</a> (against kdelibs). This patch is trivial now, if review 103469 is approved I will commit this one as well since kde-workspace will not compile with this one applied and without patch 103469 applied against kdelibs.

Description
The patch resets time format in digital clock plasmoid when the user changes the 24h configuration in active-settings.

The reset part is from kdelibs/kdecore/localization/klocale_kde.cpp. I am wondering if I should add this change to kdelibs instead of kde-workspace to avoid duplicating code. Anyway, I wanted someone to review the code to see if there can be any side effect.

This addresses bug 289094.
<a href="http://bugs.kde.org/show_bug.cgi?id=289094" title="http://bugs.kde.org/show_bug.cgi?id=289094">http://bugs.kde.org/show_bug.cgi?id=289094</a>

Diffs (updated)
plasma/generic/applets/digital-clock/clock.h 4aec3fd
plasma/generic/applets/digital-clock/clock.cpp dd03692

Diff: <a href="http://git.reviewboard.kde.org/r/103434/diff/diff" title="http://git.reviewboard.kde.org/r/103434/diff/diff">http://git.reviewboard.kde.org/r/103434/diff/diff</a>

Testing
Works in Plasma Active. In Plasma Desktop kcmlocale does not call KGlobalSettings::self()->emitChange(KGlobalSettings::SettingsChanged) so it does not take effect. Other kcm modules (e.g. keyboard), call emitChange.

Thanks,

Lamarque Vieira Souza

Re: Review Request: Reset time format upon user request

By Commit Hook at 12/27/2011 - 08:50

This review has been submitted with commit 8bd86323cdbd11ffafb71b6d8f4836d0d4339af3 by Lamarque V. Souza to branch KDE/4.8.

- Commit Hook

On Dec. 19, 2011, 2:03 p.m., Lamarque Vieira Souza wrote:

Re: Review Request: Reset time format upon user request

By Commit Hook at 12/22/2011 - 09:24

This review has been submitted with commit 68ec5f077e20afe09def364d6e76ecbe989db02d by Lamarque V. Souza to branch ksmserver/qml-shutdowndlg.

- Commit Hook

On Dec. 19, 2011, 2:03 p.m., Lamarque Vieira Souza wrote:

Re: Review Request: Reset time format upon user request

By David Faure at 12/18/2011 - 04:59

SETTINGS_COMPLETION !? This has nothing to do with completion. How about adding a SETTINGS_LOCALE if that's what you need?

- David Faure

On Dec. 16, 2011, 9:13 p.m., Lamarque Vieira Souza wrote:

Re: Review Request: Reset time format upon user request

By Lamarque V. Souza at 12/18/2011 - 08:51

I am trying to minimize the number of patches needed to fix bug #289094. This review is patch number two and a third patch against kdelibs will be necessary to add KGlobalSettings::SETTINGS_COMPLETION in kglobalsettings.h. As I explained in my last review I have not found any better category.

- Lamarque Vieira

On Dec. 16, 2011, 9:13 p.m., Lamarque Vieira Souza wrote:

Re: Review Request: Reset time format upon user request

By Aaron J. Seigo at 12/18/2011 - 04:33

Ship it!

i'm ok with this fix for now, but we need to look at making KLocale work better for us here :)

plasma/generic/applets/digital-clock/clock.cpp
<http://git.reviewboard.kde.org/r/103434/#comment7475>

this still realy ought to be done in klocale

- Aaron J. Seigo

On Dec. 16, 2011, 9:13 p.m., Lamarque Vieira Souza wrote:

Re: Review Request: Reset time format upon user request

By Lamarque V. Souza at 12/18/2011 - 08:51

Well, we could add a KLocale::commit() call to make any changes in KLocale visible to everybody. That call would replace the KGlobalSettings::self()->emitChange(KGlobalSettings::SettingsChanged, KGlobalSettings::SETTING_COMPLETION) that I am using in plasma-mobile. KLocale::commit() would use KGlobalSettings::self()->emitChange() internally, we can add KGlobalSetings::SETTINGS_LOCALE as David suggested below. There would be a private slot KLocale::updateChanges() listening to emitChange that would reparse the config files. Of course we will need to update the documentation about emitChange and that would not work for KDE SC <= 4.7.4 because of the addition of KGlobalSetings::SETTINGS_LOCALE.

In bug #289094 particular case the will be the problem of synchronizing KLocale::updateChanges() with the Clock::generatePixmap(); Clock::update() calls in the clock plasmoid. KLocale::updateChanges() would have to emit a signal to indicate the changes has been updated or the changes will take place only on the next update, which can take almost a minute in Plasma Active. The current patch update the time format instantaneously.

Another problem with this approach is that we cannot prevent anybody else from listening to KGlobalSettings::self()->emitChange(KGlobalSettings::SettingsChanged, KGlobalSettings::SETTING_LOCALE). Hmmm we could add a private enum to accomodate KGlobalSettings::SETTING_LOCALE?

- Lamarque Vieira

On Dec. 16, 2011, 9:13 p.m., Lamarque Vieira Souza wrote:

Re: Review Request: Reset time format upon user request

By David Faure at 12/18/2011 - 18:03

On Sunday 18 December 2011 12:51:25 Lamarque Vieira Souza wrote:
What would be wrong with that? It would be the way to have a GUI that reacts
to changes in the locale settings. Every app and in particular date/time
widgets might want to listen to that and adapt.
Or did I misunderstand what this was about?

If everyone only went for the "minimal lines of code" fix all the time, we
would have one hell of a mess by now...

Re: Review Request: Reset time format upon user request

By Lamarque V. Souza at 12/18/2011 - 18:26

Em Sunday 18 December 2011, David Faure escreveu:
You misunderstood what I meant. You removed that paragraph from the
context of the first paragraph of my last e-mail. What I meant is that if we
use something like KLocale::commit() then we should not let others use
KGlobalSettings::self()>emitChange(KGlobalSettings::SettingsChanged,
KGlobalSettings::SETTING_LOCALE). If we just add
KGlobalSettings::SETTING_LOCALE to kglobalsettings.h then it is correct that
everybody uses it.

The problem with KGlobalSettings::SETTING_LOCALE is that is solves only
half the problem. I am using a hack to force the local KLocale instance in the
clock plasmoid to reload the configuration files. There is no API in kdelibs
to reload KLocale's configuration files and that is why I suggested using
something like KLocale::commit() when Aaron complained about my second review
of this patch.

I am not familiar with this part of kdelibs and it is very clear it is a
very sensible part of kdelibs. Anything wrong here will be noticed by
everybody, that is why I am trying to do the minimum in this case.

Re: Review Request: Reset time format upon user request

By David Faure at 12/19/2011 - 04:57

On Sunday 18 December 2011 20:26:42 Lamarque V. Souza wrote:
Minor issue. It would be harmless to signal a change when nothing changed.

The same is true for every other SETTING_* in there. Anyone can emit "the
desktop path has changed", but if it wasn't changed, then nothing will happen
anyway.

Adding a commit() that is called after setFoo() calls, sounds good.

This is what reviews are for.

Re: Review Request: Reset time format upon user request

By Aaron J. Seigo at 12/16/2011 - 15:07

i've made it so that kcmlocale also emit the signal ... now we just need to put the code into the right place, wh ich i imagine is klocale_kde.cpp. it would be nice if KLocale also had a signal for when it changed the time format so that we could know for sure when it was doing with the changes instead of listening to the generic KGlobalSettings signal?

plasma/generic/applets/digital-clock/clock.cpp
<http://git.reviewboard.kde.org/r/103434/#comment7470>

this part belongs in klocale for sure. since this will affect all time keeping bits. the clock will still need to call generatePixmap() and update() though.

also, this should be checking what has been changed and only changing on SettingsChanged. (we don't need to do this on, for instance, icon or cursor changes ;)

- Aaron J. Seigo

On Dec. 16, 2011, 6:35 p.m., Lamarque Vieira Souza wrote: