Bug 15903 - LANG=en_US xclock -digital -font fixed has wrong size
Summary: LANG=en_US xclock -digital -font fixed has wrong size
Status: RESOLVED FIXED
Alias: None
Product: xorg
Classification: Unclassified
Component: App/other (show other bugs)
Version: git
Hardware: Other All
: medium normal
Assignee: Xorg Project Team
QA Contact: Xorg Project Team
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-05-11 19:02 UTC by Jay Berkenbilt
Modified: 2011-10-04 23:40 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
image of xclock that is too narrow (2.32 KB, image/png)
2008-05-11 19:02 UTC, Jay Berkenbilt
no flags Details
patch that corrects the problem (2.38 KB, patch)
2008-05-11 19:03 UTC, Jay Berkenbilt
no flags Details | Splinter Review
Patch to calculate text and window size from LC_CTYPE instead of LC_TIME (990 bytes, patch)
2010-08-25 19:23 UTC, Peter Stuge
no flags Details | Splinter Review

Description Jay Berkenbilt 2008-05-11 19:02:07 UTC
In a locale other than C, running xclock -digital with "fixed" font and no other X resources results in a window that is too narrow.  Looking at recent changes to xclock's Clock.c, there appears to be a subtle logic error for which I have attached a simple patch.

The first attachment shows an xclock started with "LANG=en_US.utf-8 xclock -digital" with no X resources.

Look at Clock.c from the xclock source code.

Find this block of code around line 680.  The "if (!no_locale)" statement below used to just be an "else" in an earlier version, and the "no_locale = True;" else statement was not present.  Clearly (looking at a wider section of code than pasted here) the lower block is supposed to be run in the event of a null value for w->clock.fontSet.

	       min_height = fse->max_logical_extent.height +
		 3 * w->clock.padding;
	   } else {
	       no_locale = True;
	   }
       }

       if (!no_locale)
#endif /* NO_I18N */
       {
	   if (w->clock.font == NULL)
	       w->clock.font = XQueryFont( XtDisplay(w),


The problem is that this used to be an if (...) { ... } else if (...) { ... } else { ... } and has now become an if (...) { ... } else if (...) { ... } if (...) { ... }.  The last if block (the if (!no_locale) block, the beginning of which is shown above) should not be run if the first if block (if (w->clock.render)) was true.  This is a simple case of a missing set of braces.  It's pretty obvious studying the code, but it's not that easy to see because of all the #ifdefs.

The second attachment is a patch that fixes the problem.  The patch just adds a missing set of braces and reindents the intervening code.  I hope you will consider inclusion of this patch.  I have also reported this in both Red Hat's and Debian's bug tracking systems.

Thanks.

And yes, this proves that some people still run xclock!  I like my minimalistic desktop with the xclock in the lower-right hand corner where it's been for a long time.  (I started running X with X10R4.) :-)
Comment 1 Jay Berkenbilt 2008-05-11 19:02:47 UTC
Created attachment 16474 [details]
image of xclock that is too narrow
Comment 2 Jay Berkenbilt 2008-05-11 19:03:30 UTC
Created attachment 16475 [details] [review]
patch that corrects the problem
Comment 3 James Cloos 2008-05-12 09:46:44 UTC
The patch does not apply as is to git master.  Could you clone master
and update the patch for it?

Thanks.

git clone git://anongit.freedesktop.org/git/xorg/app/xclock.git

If you only have http access, you can use:

git clone http://anongit.freedesktop.org/git/xorg/app/xclock.git
Comment 4 Jay Berkenbilt 2008-05-12 13:47:10 UTC
Heh....the patch doesn't apply because someone already fixed the problem.  The version in git doesn't have the bug.  Someone already added the missing set of braces.  I should have checked that. :-)
Comment 5 Peter Stuge 2010-08-25 19:22:23 UTC
Sorry, I'm reopening this. The source was fixed for many cases, but it seems there's still one problem case left even in git HEAD eefa0405.

Clock.c:Initialize() calculates incorrect extents if LC_CTYPE is set to a UTF-8 locale and LC_TIME is not, e.g. LC_CTYPE=en_US.utf8 LC_TIME=C.

--8<-- I added some tracing
diff --git a/Clock.c b/Clock.c
index 8f78cab..79cf919 100644
--- a/Clock.c
+++ b/Clock.c
@@ -591,6 +591,8 @@ Initialize (Widget request, Widget new, ArgList args, Cardinal *num_args)
        w->clock.utf8 = False;
 
+       printf("no_locale=%d\n",no_locale);
        if (!no_locale) {
           char *time_locale = setlocale(LC_TIME, NULL);
+          printf("time_locale='%s'\n",time_locale);
 
           if (strstr(time_locale, "UTF-8") || strstr(time_locale, "utf8")) {
@@ -613,4 +615,5 @@ Initialize (Widget request, Widget new, ArgList args, Cardinal *num_args)
 #endif /* NO_I18N */
 
+       printf("utf8=%d\n",w->clock.utf8);
        (void) time(&time_value);
        tm = *localtime(&time_value);
-->8--

which gets me the following output:

# OK
$ LC_CTYPE=C LC_TIME=C ./xclock 
no_locale=0
time_locale='C'
utf8=0

# also OK
$ LC_CTYPE=en_US.utf8 LC_TIME=en_US.utf8 ./xclock 
no_locale=0
time_locale='en_US.utf8'
utf8=1

# this is OK too
$ LC_CTYPE=C LC_TIME=en_US.utf8 ./xclock 
no_locale=0
time_locale='en_US.utf8'
utf8=1

# but this calculates the wrong size
$ LC_CTYPE=en_US.utf8 LC_TIME=C ./xclock 
no_locale=0
time_locale='C'
utf8=0

The double negations in Clock.c made me instinctively work around the problem by setting LC_CTYPE=C for my xclock invocation, but I guess it would be nice to handle also this case in the code.

I believe the correct solution is to determine character extent calculation method based on LC_CTYPE rather than LC_TIME.

--8<-- http://www.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap07.html
7.3.1 LC_CTYPE
The LC_CTYPE category shall define character classification, case conversion, and other character attributes.

..

7.3.5 LC_TIME
The LC_TIME category shall define the interpretation of the conversion specifications supported by the date utility and shall affect the behavior of the strftime(), wcsftime(), strptime(), XSI and nl_langinfo() functions.
-->8--

I interpret this to mean that LC_TIME determines what specifiers and formats are be output by the various functions, while LC_CTYPE determines encoding.

I'm attaching a patch which works for me, regardless of what I set LC_CTYPE to.
Comment 6 Peter Stuge 2010-08-25 19:23:25 UTC
Created attachment 38165 [details] [review]
Patch to calculate text and window size from LC_CTYPE instead of LC_TIME
Comment 7 Julien Cristau 2010-08-26 03:16:01 UTC
Review of attachment 38165 [details] [review]:

Looks correct to me.

Reviewed-by: Julien Cristau <jcristau@debian.org>
Comment 8 Jeremy Huddleston Sequoia 2011-10-04 23:40:06 UTC
commit 251461d84de4220f05ffa35a3138aad1cd2e0302
Author: Peter Stuge <peter@stuge.se>
Date:   Thu Aug 26 04:14:27 2010 +0200

    Calculate text and window size from LC_CTYPE instead of LC_TIME
    
    LC_TIME only defines what will be displayed, LC_CTYPE is what defines
    the character encoding.
    
    References:
    
      Bug 15903 - LANG=en_US xclock -digital -font fixed has wrong size
      https://bugs.freedesktop.org/show_bug.cgi?id=15903
    
    Reviewed-by: Julien Cristau <jcristau@debian.org>
    Reviewed-by: Jeremy Huddleston <jeremyhu@apple.com>


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.