*** Bug 598040 has been marked as a duplicate of this bug. ***

Include the possibility to switch from a.m./p.m to 24 hours style.

I personally think that the date format should be configurable like in Evolution. 
For example in my locale the date format is usually dd.mm.yyyy or dd.mm, so "October 31st" is unnecessarily verbose.

The usual clock applet lets a user define their format in a gconf key.  Nautilus gives a few different formats.  Perhaps we could at least respect the Nautilus setting for consistency?

I suppose editing gconf is on a user level with editing panel.js's format string, though?

*** Bug 607889 has been marked as a duplicate of this bug. ***

I also think that this should be configurable. Where would be the "right" place in gnome-shell? A "right-click" context-menu on the current day?

@Florian: This might be not the right place for my question, but i did a git --pull bevor i worte my tiny "patch". What's wrong with it?

(In reply to comment #7)
> I also think that this should be configurable. Where would be the "right" place
> in gnome-shell? A "right-click" context-menu on the current day?

To my knowledge, there has been no decision on the "right" place yet. There is some relation to bug 603532 here, which is in need of another date/time preference.

I think what I'd like to see for getting to options is a small "Options" link at the bottom of the calendar (right-aligned). This would bring up a dialog containing all date/time related preferences, both for the calendar and for the display on the panel.

Implementation of this dialog is a bit tricky - you can't just pop up a normal toplevel window dialog in GTK+ from within the Mutter process because Mutter can't decorate its own windows due to X limitations.

Possibilities:

 A) Out-of-process dialog controlling GConf options written in Javascript. The hard thing about this is the way that you access GConf in the shell is based on glue code in the gnome-shell process not generic gobject-introspection. That's fixable by installing a "gnome-shell-utils.so" of some sort, but requires some expertise in modifying the build system.

 B) Out of process dialog controlling GConf options written in something else, like C.

 C) In process Clutter-based user interface (a preferences "screen" for the shell)

 D) In process GTK+ based user interface in a toplevel override-redirect (GTK_WINDOW_POPUP) "system modal" dialog. This will look a bit weird, but will work.

If someone wants to make an attempt at this and doesn't feel comfortable moving stuff around between shared libraries and modifying the build system, I'm going to suggest implementing as D) with the plan to move to A) later.

Another reason for doing something easy short-term is that in the slightly longer term we might want to to appearing to be part of the system control center, which would have different implementation technologies and is a bit in flux.

Another option is to have this kind of configuration be part of the needs-to-be-written Date/Time control center panel.

Maybe we should resurrect my attempt in bug 154085.

(In reply to comment #10)
> Another option is to have this kind of configuration be part of the
> needs-to-be-written Date/Time control center panel.
> 
> Maybe we should resurrect my attempt in bug 154085.

I was vaguely hinting at stuff like that in the last paragraph, but there are various questions there, and it's also hard a good job at panel options in a mixed gnome-panel/gnome-shell world, so I think it would be worth doing something more immediate for now. If it's back-ended off of GConf options, than driving it from the control-center later will be easy.

Oh yeah I see it now.  Sure, we can't really add it well in control center until shell is a standard component that is true.

Created attachment 154749 [details] [review]
[ShellUtils] Move ShellGConf into a separate library

Split ShellGConf into a separate library and modify the build system
to install it to a "public" location. This will allow out-of-process
gjs scripts to use ShellUtils.GConf.

Created attachment 154750 [details] [review]
Update code to use ShellUtils.GConf instead of Shell

This is a simple sed job, not sure this should be squashed into the previous patch - it seems a little bit cleaner to keep them separate.

Created attachment 154751 [details] [review]
[panel] Make clock configurable via gconf

Add keys for customizing the panel clock to the gconf schema and make
the clock use them. The settings are copied from gnome-panel's clock
applet, excluding all location/weather/appointment/... keys. In addition,
'internet' is no longer a supported value for the format key.

Created attachment 154752 [details] [review]
Add a simple preference dialog for the clock

Port the 'General' preferences tab of the panel's clock applet to
javascript and add it to the build system.

Owen: not sure if the split is a little too hackish, but having a gjs-wrapper for main() wrapped in a shell script seemed a little extreme ...

Created attachment 154754 [details] [review]
Temporarily add clock preferences to the control center

As our preferences will not have any effect when not running shell,
it would be confusing if they appeared in the control center. On
the other hand, when the shell is running the control center seems
like a good place to put them.
Until a proper solution for this problem is implemented, we symlink
the desktop file to the user's xdg application folder and remove the
link on exit.

If adding our preferences to the control center works out well, we'll probably want to add some key to the desktop file (maybe add GNOME-Shell to categories) and make control center ignore it when not running under shell - hopefully this hack proves helpful in the decision-making meanwhile.

(In reply to comment #15)
> In addition, 'internet' is no longer a supported value for the format key.

Why? I liked "internet time".

(In reply to comment #18)
> (In reply to comment #15)
> > In addition, 'internet' is no longer a supported value for the format key.
> 
> Why? I liked "internet time".

Not sure anyone would really want this (unless as additional clock - which may be implemented in an extension, but unlike with gnome-panel it's no longer supported in the core).

Of course, the same applies to the 'unix' format, but it's implemented in 5 lines (including one comment line!), while 'internet' would require adding quite some code - so yeah, it boils down to "wouldn't hurt to support it, but I'm waaaay too lazy to do it myself ..."

we don't need "internet time" or "unix time". it's a clock, not a "do whatever goofy thing anyone feels like"

(In reply to comment #20)
> we don't need "internet time" or "unix time". it's a clock, not a "do whatever
> goofy thing anyone feels like"

Agreed - "unix" is included because it's in gnome-panel and it's easy to support. That said, I have no problem whatsoever to remove that from the patch :)

*** Bug 610995 has been marked as a duplicate of this bug. ***

Review of attachment 154754 [details] [review]:

Eek.  Well this patch is fairly heroic, but I feel like we should avoid carrying this kind of thing.  I'd rather see a patch for control-center which knows how to ignore preferences that only apply to the shell (flag in .desktop file).

Also this patch seems to be missing the actual preferences executable.

Review of attachment 154749 [details] [review]:

Hmm.  Honestly would prefer to patch GConf upstream...would need a bit of thought though.  Libraries have a cost, and while we clearly link to a heck of a lot of stuff right now, we should be wary of adding more.

Review of attachment 154750 [details] [review]:

No specific comments but obviously depends on how we resolve the previous patch.

(In reply to comment #24)
> Hmm.  Honestly would prefer to patch GConf upstream...would need a bit of
> thought though.

Not sure - Shell.GConf is a very convenient, very shell-specific wrapper; I doubt it would fit in anywhere upstream.

What we could do to avoid an additional library:

 - make libshell public (I assume we agree that it's a bad idea, just listing
   for completeness)

 - allow gnome-shell-clock-preferences to access libshell - it lives in the
   shell source tree, so this sounds OKish to me

 - use GConf.Client directly in gnome-shell-clock-preferences - probably the
   cleanest solution (not sure if Shell.GConf works around gconf issues, it 
   looks pretty much like a convenience wrapper)



(In reply to comment #23)
> Review of attachment 154754 [details] [review]:
> 
> Eek.  Well this patch is fairly heroic,but I feel like we should avoid
> carrying this kind of thing.  I'd rather see a patch for control-center which
> knows how to ignore preferences that only apply to the shell (flag in .desktop
> file).

I disagree on the "heroic" part, the rest matches more or less what I had in mind with "proper solution" in the commit message. Is this something which we could expect to land in control-center-2.30 though?


> Also this patch seems to be missing the actual preferences executable.

Yes. The preference dialog is a lot less eeky (well, less eeky, not sure about the lot - the whole series is pretty much sorted by eekness) than this hack, so it's a separate patch (attachment 154752 [details] [review]).

(In reply to comment #26)
> 
> Not sure - Shell.GConf is a very convenient, very shell-specific wrapper; I
> doubt it would fit in anywhere upstream.

I meant that I'd at least like to scope out what would be involved in getting libgconf introspectable.  I'm sure it's missing a few (transfer nones), GConfValue probably needs to be a boxed, and the list types probably need special handling.

> What we could do to avoid an additional library:
> 
>  - make libshell public (I assume we agree that it's a bad idea, just listing
>    for completeness)

I'd be more in favor of this actually, with special areas of the library marked as OOP-friendly, rather than just making Shell.GConf public.  This gives us the flexibility to add more things that are useful for out-of-process extensions.  And actually we might as well use this for things like the app-not-responding dialog, etc.  (Though that specifically should likely be reworked to be part of the shell UI).
 
>  - use GConf.Client directly in gnome-shell-clock-preferences - probably the
>    cleanest solution (not sure if Shell.GConf works around gconf issues, it 
>    looks pretty much like a convenience wrapper)

It's a combination of things.  

Or really there is option 4 which is just write clock-preferences in C.

> (In reply to comment #23)

> I disagree on the "heroic" part, the rest matches more or less what I had in
> mind with "proper solution" in the commit message. Is this something which we
> could expect to land in control-center-2.30 though? 

It just strikes me as a bit too magical; I don't like the idea of dynamically changing parts of $HOME.  Among other things, this is going to lead to problems on NFS home dirs.

(In reply to comment #27)
> > Not sure - Shell.GConf is a very convenient, very shell-specific wrapper; I
> > doubt it would fit in anywhere upstream.
> 
> I meant that I'd at least like to scope out what would be involved in getting
> libgconf introspectable.  I'm sure it's missing a few (transfer nones),
> GConfValue probably needs to be a boxed, and the list types probably need
> special handling.

the two big problems that led to Shell.GConf were:

    * list-type handling (libgconf doesn't have existing methods that can
      be annotated into introspectability)

    * notifications (libgconf uses callbacks rather than signals, and gjs
      can't do callbacks)

we could add more-introspectable methods to libgconf, but we'd still need shellgconf to handle the notifications. (IMHO the way shellgconf handles this is a hack, not an improvement over the current gconf code, so it wouldn't make sense to upstream it.)

Created attachment 156464 [details] [review]
[panel] Make clock configurable via gconf

Add keys for customizing the panel clock to the gconf schema and make
the clock use them. The settings are copied from gnome-panel's clock
applet, excluding all location/weather/appointment/... keys. In addition,
'internet' is no longer a supported value for the format key.

Created attachment 156465 [details] [review]
Add a simple preference dialog for the clock

Updated patch to use gconf directly - depends on proper gconf introspection.

Hi,

not to undermine what Florian has written at all - in fact I was working from the main project page that said the configurable clock was a project that needed work, so I've written a preliminary patch to at least add date/seconds configurability to the clock; as it's a different approach to Florian's it may be worth looking at in case is resolves some of the concerns raised in the discussion;

Basically the patch as it stands is entirely in the javascript codebase, that could be a virtue according to how you see it :-)

it's a preliminary patch currently, more of a 'proof-of-concept' than a ready patch, needs define'd gconf key paths, notify_add listener for changes to the key, plus handling of configurable 24-hour / custom fields; it's a pull-down from right-clicking the clock

git://github.com/MarkieMark/gnome-shell.git

Created attachment 159428 [details] [review]
js patch to add customizability to gnome-shell clock

preliminary patch - 'proof-of-concept' stage

Created attachment 159432 [details] [review]
preliminary patch to js codebase to add customizability to the clock

clock preferences preliminary 'proof-of-concept' patch; merge git commits

Created attachment 160468 [details] [review]
Add a simple preference dialog for the clock

Extract the 'General' preferences tab of the panel's clock applet into
a small stand-alone application and add it to the build system.

The previous JS implementation stopped working as of commit 26f89d6db which limits the number of callbacks in GJS to one - as GConf does not use GDestroyNotify but GFreeFunc, the destroy_notify parameter is interpreted as additional callback. Not sure if it's worth fixing with the transition to GSettings in sight, so to get some UI review here's a quick port to C (sorry for chickening out here :)

(In reply to comment #34)
> Created an attachment (id=160468) [details] [review]
> Add a simple preference dialog for the clock
> 
> Extract the 'General' preferences tab of the panel's clock applet into
> a small stand-alone application and add it to the build system.
> 
> The previous JS implementation stopped working as of commit 26f89d6db which
> limits the number of callbacks in GJS to one - as GConf does not use
> GDestroyNotify but GFreeFunc, the destroy_notify parameter is interpreted as
> additional callback. Not sure if it's worth fixing with the transition to
> GSettings in sight, so to get some UI review here's a quick port to C (sorry
> for chickening out here :)

See bug 617972 (gjs fix), bug 617973 (annotation fix in GConf), bug 617974 (same fix in gir-repository)

Created attachment 160595 [details] [review]
Add a simple preference dialog for the clock

(In reply to comment #35)
> See bug 617972 (gjs fix), bug 617973 (annotation fix in GConf), bug 617974
> (same fix in gir-repository)

OK, great! Reverting to the previous patch then (rebased on master that is) ...

Review of attachment 156464 [details] [review]:

Looks good to commit with a few fixes.

::: data/gnome-shell.schemas
@@ +90,3 @@
+	  <short>Custom format of the clock</short>
+	  <long>
+	    This key specifies the format used by the clock applet when the

'clock applet'

@@ +93,3 @@
+            format key is set to "custom". You can use conversion specifiers
+            understood by strftime() to obtain a specific format. See the
+            strftime() manual for more information.

Would be nice to say here that when set show_seconds and show_date are ignored. (Improving on the docs that gnome-panel had)

::: js/ui/panel.js
@@ +651,3 @@
+        let msecRemaining;
+        if (showSeconds) {
+            msecRemaining = 1000 - displayDate.getMilliseconds();

This has the same problem as the minutes code path of the possibility of waking up a millsecond before the second, displaying the old second, sleeping for a millsecond, then showing the new second.  I'd repeat the same logic with 50 rather than 500.

Review of attachment 160595 [details] [review]:

This generally looks good to me. 

My biggest concern is that nobody will find the preferences dialog. In fact, in a jhbuild it doesn't even show up for me under System Preferences.. (maybe the control-center shell doesn't use standard .desktop file reading using XDG_DATA_DIRS)

In general, going from "I'd like to have 24 hour time on my clock" to browsing through a big listof preference applications is very non-immediate. I think people generally do like to have the ability to link preferences to the object being customized - if you want to change your background, you probably right click on the background, not go to "Appearance" in the control center.

I would probably expect to find this on right click/Preferences on the clock. i don't know if any of our menu abstractions allow that to be easily implemented.

Another option that is definitely easier to implement would be a small preferences link at the lower right of the calendar. It obviously doesn't deserve such a promiment placement for this small level of customizability but if (if) we are extending the clock into more of a "widget layer" or at least something like the gnome 2 intlclock with weather, multiple timezones, todo list, etc, then customizing that will be a pretty common activity and should be easy for the user to find.

::: js/prefs/clockPreferences.js
@@ +21,3 @@
+
+        this._dialog = builder.get_object('prefs-dialog');
+        this._dialog.connect('response', Lang.bind(this, this._onDestroy));

This isn't onDestroy - it's onResponse

@@ +34,3 @@
+        this._gconf.notify_add(GCONF_DIR,
+                               Lang.bind(this, this._updateDialog),
+                               null, null);

You don't need the null, null here

If you save the returned connection ID and remove the notify using gconf_client_notify_remove(), you'll fix the "**ERROR **: after destroying context: JavasScript objects were leaked" problem on quit.

::: src/clock-preferences.ui
@@ +1,2 @@
+<?xml version="1.0"?>
+<interface>

This file should be in data/ not src/

::: src/gnome-shell-clock-preferences.in
@@ +4,3 @@
+then
+  uiDir="`dirname $0`"
+  jsDir="@topsrcdir@/js"

Do something like:

 scriptDir="`dirname $0`"
 uiDir="$scriptDir/../data"
 jsDir="$scriptDir/../js"

Otherwise it will break when run as, say, src/gnome-shell-clock-preferences

@@ +14,3 @@
+export GJS_PATH="$jsDir"
+
+@GJS_CONSOLE@ >/dev/null <<EOF

To avoid a mesage about the interactive console, better to use @GJS_CONSOLE@ -c '
'

To make editing the JS easier. Do it as:

@GJS_CONSOLE@ -c "const ClockPreferences = imports.prefs.clockPreferences;
ClockPreferences.main({ progName, '$progName', uiFile: 'uiFile' })"

Let's keep the JS embedded in shell to a minimum.

Created attachment 160610 [details] [review]
[panel] Make clock configurable via gconf

(In reply to comment #37)
> @@ +93,3 @@
> +            format key is set to "custom". You can use conversion specifiers
> +            understood by strftime() to obtain a specific format. See the
> +            strftime() manual for more information.
> 
> Would be nice to say here that when set show_seconds and show_date are ignored.
> (Improving on the docs that gnome-panel had)

Not quite true - whether show_seconds and show_date are ignored depends on the format key, so I documented this there.

Review of attachment 160610 [details] [review]:

Looks good

Created attachment 160611 [details] [review]
Add a simple preference dialog for the clock

(In reply to comment #38)
> I think people generally do like to have the ability to link preferences to the object
> being customized - if you want to change your background, you probably right
> click on the background, not go to "Appearance" in the control center.
> 
> I would probably expect to find this on right click/Preferences on the clock.

Yeah, me too - I'm just not sure how acceptable it is from a design POV (I understand one of the main complaints about the notification area is the inconsistency of click handling). I'll try to come up with a patch, though it might be worth waiting for bug 613804.


> Another option that is definitely easier to implement would be a small
> preferences link at the lower right of the calendar.

Nah, at least right now that doesn't sound right. If we decide to extend the clock/calendar we can reconsider the option.

Review of attachment 160611 [details] [review]:

Looks good

::: js/prefs/clockPreferences.js
@@ +78,3 @@
+function main(params) {
+    if (params['progName'])
+        GLib.set_prgname(params['progName']);

Maybe just slightly better as 

  if ('progName' in params)

Attachment 160610 [details] pushed as e6b9141 - [panel] Make clock configurable via gconf
Attachment 160611 [details] pushed as 266a0fb - Add a simple preference dialog for the clock

*** Bug 580945 has been marked as a duplicate of this bug. ***

Created attachment 160627 [details] [review]
[panel] Add a right-click menu to the clock

The clock preferences are hardly discoverable, so allow launching
them from a right-click menu.

This is a simple GTK menu, based on the status menu - long term, we probably want to switch both to a clutter-based version. Meanwhile, this should do ...

Review of attachment 160627 [details] [review]:

Should be ported to the new boxpointer widget.

Created attachment 160914 [details] [review]
[ClockButton] Add a right-click menu to the clock

The clock preferences are hardly discoverable, so allow launching
them from a right-click menu.

I'm in doubt whether to add the ClockButton to the PanelMenuBar, feedback appreciated.

Created attachment 160916 [details] [review]
[ClockButton] Add a right-click menu to the clock

The clock preferences are hardly discoverable, so allow launching
them from a right-click menu.

Make sure the :pressed pseudo class is set properly on left click

Created attachment 161070 [details] [review]
[ClockButton] Add a right-click menu to the clock

Rebased on master. Made the right-clock menu part of the menu bar, while the calendar popup remains independent.

Created attachment 161572 [details] [review]
[ClockButton] Add a right-click menu to the clock

Rebased on master.

Review of attachment 161572 [details] [review]:

This looks fine.

Attachment 161572 [details] pushed as 63d8453 - [ClockButton] Add a right-click menu to the clock