Re: Review Request: Reset time format upon user request

[Commit Hook]

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


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:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103434/
> ---
> 
> (Updated Dec. 19, 2011, 2:03 p.m.)
> 
> 
> 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.
> http://bugs.kde.org/show_bug.cgi?id=289094
> 
> 
> Diffs
> -
> 
>   plasma/generic/applets/digital-clock/clock.h 4aec3fd 
>   plasma/generic/applets/digital-clock/clock.cpp dd03692 
> 
> Diff: http://git.reviewboard.kde.org/r/103434/diff/diff
> 
> 
> 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

[Commit Hook]

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


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:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103434/
> ---
> 
> (Updated Dec. 19, 2011, 2:03 p.m.)
> 
> 
> 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.
> http://bugs.kde.org/show_bug.cgi?id=289094
> 
> 
> Diffs
> -
> 
>   plasma/generic/applets/digital-clock/clock.h 4aec3fd 
>   plasma/generic/applets/digital-clock/clock.cpp dd03692 
> 
> Diff: http://git.reviewboard.kde.org/r/103434/diff/diff
> 
> 
> 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

[Lamarque Vieira Souza]

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

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


Review request for kdelibs and Plasma.


Changes
---

Use patch http://git.reviewboard.kde.org/r/103469 (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.
http://bugs.kde.org/show_bug.cgi?id=289094


Diffs (updated)
-

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

Diff: http://git.reviewboard.kde.org/r/103434/diff/diff


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

[David Faure]

On Sunday 18 December 2011 20:26:42 Lamarque V. Souza wrote:
> Em Sunday 18 December 2011, David Faure escreveu:
> > On Sunday 18 December 2011 12:51:25 Lamarque Vieira Souza wrote:
> > > Another problem with this approach is that we cannot prevent anybody
> > > else
> > > from listening to
> > > KGlobalSettings::self()->emitChange(KGlobalSettings::SettingsChanged,
> > > KGlobalSettings::SETTING_LOCALE).
> > 
> > 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?
> 
>   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).

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.

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

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

> > If everyone only went for the "minimal lines of code" fix all the time, we
> > would have one hell of a mess by now...
> 
>   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.

This is what reviews are for.

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Sponsored by Nokia to work on KDE, incl. KDE Frameworks 5

Re: Review Request: Reset time format upon user request

[Lamarque V. Souza]

Em Sunday 18 December 2011, David Faure escreveu:
> On Sunday 18 December 2011 12:51:25 Lamarque Vieira Souza wrote:
> > Another problem with this approach is that we cannot prevent anybody else
> > from listening to
> > KGlobalSettings::self()->emitChange(KGlobalSettings::SettingsChanged,
> > KGlobalSettings::SETTING_LOCALE).
> 
> 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?

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 trying to minimize the number of patches needed to fix bug #289094.
> 
> If everyone only went for the "minimal lines of code" fix all the time, we
> would have one hell of a mess by now...

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.

-- 
Lamarque V. Souza
KDE's Network Management maintainer
http://planetkde.org/pt-br

Re: Review Request: Reset time format upon user request

[David Faure]

On Sunday 18 December 2011 12:51:25 Lamarque Vieira Souza wrote:
> Another problem with this approach is that we cannot prevent anybody else
> from listening to
> KGlobalSettings::self()->emitChange(KGlobalSettings::SettingsChanged,
> KGlobalSettings::SETTING_LOCALE).

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?

> I am trying to minimize the number of patches needed to fix bug #289094.

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

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Sponsored by Nokia to work on KDE, incl. KDE Frameworks 5

Re: Review Request: Reset time format upon user request

[Lamarque Vieira Souza]

> On Dec. 18, 2011, 8:59 a.m., David Faure wrote:
> > SETTINGS_COMPLETION !? This has nothing to do with completion. How about 
> > adding a SETTINGS_LOCALE if that's what you need?

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


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


On Dec. 16, 2011, 9:13 p.m., Lamarque Vieira Souza wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103434/
> ---
> 
> (Updated Dec. 16, 2011, 9:13 p.m.)
> 
> 
> 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.
> http://bugs.kde.org/show_bug.cgi?id=289094
> 
> 
> Diffs
> -
> 
>   plasma/generic/applets/digital-clock/clock.h 4aec3fd 
>   plasma/generic/applets/digital-clock/clock.cpp dd03692 
> 
> Diff: http://git.reviewboard.kde.org/r/103434/diff/diff
> 
> 
> 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

[Lamarque Vieira Souza]

> On Dec. 18, 2011, 8:33 a.m., Aaron J. Seigo wrote:
> > plasma/generic/applets/digital-clock/clock.cpp, lines 100-102
> > 
> >
> > this still realy ought to be done in klocale

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


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


On Dec. 16, 2011, 9:13 p.m., Lamarque Vieira Souza wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103434/
> ---
> 
> (Updated Dec. 16, 2011, 9:13 p.m.)
> 
> 
> 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.
> http://bugs.kde.org/show_bug.cgi?id=289094
> 
> 
> Diffs
> -
> 
>   plasma/generic/applets/digital-clock/clock.h 4aec3fd 
>   plasma/generic/applets/digital-clock/clock.cpp dd03692 
> 
> Diff: http://git.reviewboard.kde.org/r/103434/diff/diff
> 
> 
> 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

[David Faure]

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


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:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103434/
> ---
> 
> (Updated Dec. 16, 2011, 9:13 p.m.)
> 
> 
> 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.
> http://bugs.kde.org/show_bug.cgi?id=289094
> 
> 
> Diffs
> -
> 
>   plasma/generic/applets/digital-clock/clock.h 4aec3fd 
>   plasma/generic/applets/digital-clock/clock.cpp dd03692 
> 
> Diff: http://git.reviewboard.kde.org/r/103434/diff/diff
> 
> 
> 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

[Aaron J. Seigo]

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

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


this still realy ought to be done in klocale 


- Aaron J. Seigo


On Dec. 16, 2011, 9:13 p.m., Lamarque Vieira Souza wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103434/
> ---
> 
> (Updated Dec. 16, 2011, 9:13 p.m.)
> 
> 
> 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.
> http://bugs.kde.org/show_bug.cgi?id=289094
> 
> 
> Diffs
> -
> 
>   plasma/generic/applets/digital-clock/clock.h 4aec3fd 
>   plasma/generic/applets/digital-clock/clock.cpp dd03692 
> 
> Diff: http://git.reviewboard.kde.org/r/103434/diff/diff
> 
> 
> 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

[Lamarque Vieira Souza]

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

(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.
http://bugs.kde.org/show_bug.cgi?id=289094


Diffs (updated)
-

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

Diff: http://git.reviewboard.kde.org/r/103434/diff/diff


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

[Aaron J. Seigo]

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


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


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:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103434/
> ---
> 
> (Updated Dec. 16, 2011, 6:35 p.m.)
> 
> 
> 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.
> http://bugs.kde.org/show_bug.cgi?id=289094
> 
> 
> Diffs
> -
> 
>   plasma/generic/applets/digital-clock/clock.h 4aec3fd 
>   plasma/generic/applets/digital-clock/clock.cpp dd03692 
> 
> Diff: http://git.reviewboard.kde.org/r/103434/diff/diff
> 
> 
> 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
> 
>