[PATCH] move functionality of font_path() directly to util/grub-mkconfig.in and prefer unicode.pf2 over ascii.pf2

[PATCH] move functionality of font_path() directly to util/grub-mkconfig.in and prefer unicode.pf2 over ascii.pf2

[Felix Zielcke]

[-- Attachment #1: Type: text/plain, Size: 307 bytes --]

As discussed with Robert on IRC I moved the font_path function directly
to grub-mkconfig where it's only used.
And now the unicode font gets prefered over the ascii one.
And LANG=C gets exported if ascii.pf2 gets used, which is the main
reason for the move of it.

-- 
Felix Zielcke
Proud Debian Maintainer

[-- Attachment #2: font.diff --]
[-- Type: text/x-patch, Size: 2668 bytes --]

2009-08-06  Felix Zielcke  <fzielcke@z-51.de>

	* util/grub-mkconfig_lib.in (font_path): Move the functionality
	of it to ...
	* util/grub-mkconfig.in: ... here.  Prefer unicode.pf2 and
	unifont.pf2 over ascii.pf2.  Export LANG=C in case ascii.pf2 gets used.

Index: util/grub-mkconfig.in
===================================================================
--- util/grub-mkconfig.in	(revision 2469)
+++ util/grub-mkconfig.in	(working copy)
@@ -24,6 +24,8 @@ sbindir=@sbindir@
 libdir=@libdir@
 sysconfdir=@sysconfdir@
 package_version=@PACKAGE_VERSION@
+datadir=@datadir@
+pkgdatadir=${datadir}/`echo @PACKAGE_TARNAME@ | sed "${transform}"`
 grub_prefix=`echo /boot/grub | sed ${transform}`
 grub_cfg=""
 grub_mkconfig_dir=${sysconfdir}/grub.d
@@ -151,22 +153,30 @@ esac
 # check for terminals that require fonts
 case ${GRUB_TERMINAL_OUTPUT} in
   gfxterm)
-    if path=`font_path` ; then
-      GRUB_FONT_PATH="${path}"
-    else
-      # fallback to the native terminal for this platform
-      unset GRUB_TERMINAL_OUTPUT
+    for dir in ${pkgdatadir} /boot/grub /usr/share/grub ; do
+      for basename in unicode unifont ascii; do
+	path="${dir}/${basename}.pf2"
+        if is_path_readable_by_grub ${path} > /dev/null ; then
+	  GRUB_FONT_PATH=${path}
+	else
+	  continue
+	fi
+	if [ "${basename}" = "ascii"] ; then
+	  # make sure all our children behave in conformance with ascii..
+	  export LANG=C
+	fi
+	break 2
+      done
+    done
+      if [ -z "${GRUB_FONT_PATH}" ] ; then
+        # fallback to the native terminal for this platform
+        unset GRUB_TERMINAL_OUTPUT
+      fi
     fi
   ;;
-esac
-
-# does our terminal support utf-8 ?
-case ${GRUB_TERMINAL_OUTPUT} in
-  gfxterm) ;;
   *)
     # make sure all our children behave in conformance with ascii..
     export LANG=C
-  ;;
 esac
 
 # These are defined in this script, export them here so that user can
Index: util/grub-mkconfig_lib.in
===================================================================
--- util/grub-mkconfig_lib.in	(revision 2470)
+++ util/grub-mkconfig_lib.in	(working copy)
@@ -148,23 +148,6 @@ prepare_grub_to_access_device ()
   fi
 }
 
-font_path ()
-{
-  for dir in ${pkgdatadir} /boot/grub /usr/share/grub ; do
-    # FIXME: We prefer ascii because loading complete fonts is too slow (and
-    # we don't yet provide the gettext magic that would make unicode useful).
-    for basename in ascii unicode unifont ; do
-      path="${dir}/${basename}.pf2"
-      if is_path_readable_by_grub ${path} > /dev/null ; then
-        echo "${path}"
-        return 0
-      fi
-    done
-  done
-
-  return 1
-}
-
 grub_file_is_not_garbage ()
 {
   if test -f "$1" ; then

Re: [PATCH] move functionality of font_path() directly to util/grub-mkconfig.in and prefer unicode.pf2 over ascii.pf2

[Robert Millan]

On Thu, Aug 06, 2009 at 11:39:11AM +0200, Felix Zielcke wrote:
> +    for dir in ${pkgdatadir} /boot/grub /usr/share/grub ; do
> +      for basename in unicode unifont ascii; do
> +	path="${dir}/${basename}.pf2"
> +        if is_path_readable_by_grub ${path} > /dev/null ; then
> +	  GRUB_FONT_PATH=${path}
> +	else
> +	  continue
> +	fi
> +	if [ "${basename}" = "ascii"] ; then
> +	  # make sure all our children behave in conformance with ascii..
> +	  export LANG=C
> +	fi
> +	break 2
> +      done
> +    done
> +      if [ -z "${GRUB_FONT_PATH}" ] ; then
> +        # fallback to the native terminal for this platform
> +        unset GRUB_TERMINAL_OUTPUT
> +      fi
>      fi
>    ;;
> -esac
> -
> -# does our terminal support utf-8 ?
> -case ${GRUB_TERMINAL_OUTPUT} in
> -  gfxterm) ;;
>    *)
>      # make sure all our children behave in conformance with ascii..
>      export LANG=C
> -  ;;
>  esac

It seems that "export LANG=C" is issued twice?

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] move functionality of font_path() directly to util/grub-mkconfig.in and prefer unicode.pf2 over ascii.pf2

[Felix Zielcke]

Am Freitag, den 07.08.2009, 13:39 +0200 schrieb Robert Millan:
> On Thu, Aug 06, 2009 at 11:39:11AM +0200, Felix Zielcke wrote:
> > +    for dir in ${pkgdatadir} /boot/grub /usr/share/grub ; do
> > +      for basename in unicode unifont ascii; do
> > +	path="${dir}/${basename}.pf2"
> > +        if is_path_readable_by_grub ${path} > /dev/null ; then
> > +	  GRUB_FONT_PATH=${path}
> > +	else
> > +	  continue
> > +	fi
> > +	if [ "${basename}" = "ascii"] ; then
> > +	  # make sure all our children behave in conformance with ascii..
> > +	  export LANG=C
> > +	fi
> > +	break 2
> > +      done
> > +    done
> > +      if [ -z "${GRUB_FONT_PATH}" ] ; then
> > +        # fallback to the native terminal for this platform
> > +        unset GRUB_TERMINAL_OUTPUT
> > +      fi
> >      fi
> >    ;;
> > -esac
> > -
> > -# does our terminal support utf-8 ?
> > -case ${GRUB_TERMINAL_OUTPUT} in
> > -  gfxterm) ;;
> >    *)
> >      # make sure all our children behave in conformance with ascii..
> >      export LANG=C
> > -  ;;
> >  esac
> 
> It seems that "export LANG=C" is issued twice?
> 
Once if gfxterm gets used and once like before for all other output
terminals.

-- 
Felix Zielcke
Proud Debian Maintainer

Re: [PATCH] move functionality of font_path() directly to util/grub-mkconfig.in and prefer unicode.pf2 over ascii.pf2

[Felix Zielcke]

Am Freitag, den 07.08.2009, 14:02 +0200 schrieb Felix Zielcke:
> Am Freitag, den 07.08.2009, 13:39 +0200 schrieb Robert Millan:
> > On Thu, Aug 06, 2009 at 11:39:11AM +0200, Felix Zielcke wrote:
> > > +    for dir in ${pkgdatadir} /boot/grub /usr/share/grub ; do
> > > +      for basename in unicode unifont ascii; do
> > > +	path="${dir}/${basename}.pf2"
> > > +        if is_path_readable_by_grub ${path} > /dev/null ; then
> > > +	  GRUB_FONT_PATH=${path}
> > > +	else
> > > +	  continue
> > > +	fi
> > > +	if [ "${basename}" = "ascii"] ; then
> > > +	  # make sure all our children behave in conformance with ascii..
> > > +	  export LANG=C
> > > +	fi
> > > +	break 2
> > > +      done
> > > +    done
> > > +      if [ -z "${GRUB_FONT_PATH}" ] ; then
> > > +        # fallback to the native terminal for this platform
> > > +        unset GRUB_TERMINAL_OUTPUT
> > > +      fi
> > >      fi
> > >    ;;
> > > -esac
> > > -
> > > -# does our terminal support utf-8 ?
> > > -case ${GRUB_TERMINAL_OUTPUT} in
> > > -  gfxterm) ;;
> > >    *)
> > >      # make sure all our children behave in conformance with ascii..
> > >      export LANG=C
> > > -  ;;
> > >  esac
> > 
> > It seems that "export LANG=C" is issued twice?
> > 
> Once if gfxterm gets used and once like before for all other output
> terminals.

I commited it now with an ack from Robert on IRC.


-- 
Felix Zielcke
Proud Debian Maintainer

Re: [PATCH] move functionality of font_path() directly to util/grub-mkconfig.in and prefer unicode.pf2 over ascii.pf2

[Pavel Roskin]

On Fri, 2009-08-07 at 14:57 +0200, Felix Zielcke wrote:

> I commited it now with an ack from Robert on IRC.

Sorry, I'm commenting after it has been committed.  Anyway, please note
that having an approval doesn't absolve you from testing the code on
your own.  Reviews are not testing.  There was a warning introduced by
your change, and there was a syntax error after "ascii".  Also, the
formatting of the moved code should have been changed to use the same
indentation as the target file.  I have fixed all that.

Could you please explain what I should do to keep using ascii.pf2?  I
checked the script, but don't see any variable controlling that.  "make
install" would install both unicode.pf2 and ascii.pf2, so unicode.pf2
would always be preferred.

If changing the default, it's a good style to provide an easy way for
users to keep the old setting, and I just don't see it, short or
removing /usr/src/unifont.bdf and /usr/local/share/grub/unicode.pf2 so
that they are never reinstalled or detected by GRUB.

-- 
Regards,
Pavel Roskin

Re: [PATCH] move functionality of font_path() directly to util/grub-mkconfig.in and prefer unicode.pf2 over ascii.pf2

[Felix Zielcke]

[-- Attachment #1: Type: text/plain, Size: 1349 bytes --]

Am Samstag, den 08.08.2009, 01:12 -0400 schrieb Pavel Roskin:
> On Fri, 2009-08-07 at 14:57 +0200, Felix Zielcke wrote:
> 
> > I commited it now with an ack from Robert on IRC.
> 
> Sorry, I'm commenting after it has been committed.  Anyway, please note
> that having an approval doesn't absolve you from testing the code on
> your own.  Reviews are not testing.  There was a warning introduced by
> your change, and there was a syntax error after "ascii".  Also, the
> formatting of the moved code should have been changed to use the same
> indentation as the target file.  I have fixed all that.

Sorry.

> Could you please explain what I should do to keep using ascii.pf2?  I
> checked the script, but don't see any variable controlling that.  "make
> install" would install both unicode.pf2 and ascii.pf2, so unicode.pf2
> would always be preferred.

With the old code ascii.pf2 would be always preferred.
There wasn't either a way to specify it.

> If changing the default, it's a good style to provide an easy way for
> users to keep the old setting, and I just don't see it, short or
> removing /usr/src/unifont.bdf and /usr/local/share/grub/unicode.pf2 so
> that they are never reinstalled or detected by GRUB.

Here's now a patch which allows users to specifiy the used font with
GRUB_FONT=ascii


-- 
Felix Zielcke
Proud Debian Maintainer

[-- Attachment #2: font.diff.2 --]
[-- Type: text/plain, Size: 1078 bytes --]

2009-08-08  Felix Zielcke  <fzielcke@z-51.de>

	* util/grub-mkconfig.in: Allow the user to specify the used font
	with GRUB_FONT.

diff --git a/util/grub-mkconfig.in b/util/grub-mkconfig.in
index 7756b49..55135f8 100644
--- a/util/grub-mkconfig.in
+++ b/util/grub-mkconfig.in
@@ -155,6 +155,19 @@ esac
 case ${GRUB_TERMINAL_OUTPUT} in
   gfxterm)
     for dir in ${pkgdatadir} /boot/grub /usr/share/grub ; do
+      if [ -n "$GRUB_FONT" ] ; then
+        path="${dir}/${GRUB_FONT}.pf2"
+        if is_path_readable_by_grub ${path} > /dev/null ; then
+          GRUB_FONT_PATH=${path}
+	  if [ "${GRUB_FONT}" = "ascii" ]
+	    export LANG=C
+	  fi
+	break
+	else
+	  echo "No such font ${GRUB_FONT}.pf2" >&2
+	  exit 1
+	fi
+      else
       for basename in unicode unifont ascii; do
 	path="${dir}/${basename}.pf2"
         if is_path_readable_by_grub ${path} > /dev/null ; then
@@ -168,6 +181,7 @@ case ${GRUB_TERMINAL_OUTPUT} in
 	fi
 	break 2
       done
+    fi
     done
     if [ -z "${GRUB_FONT_PATH}" ] ; then
       # fallback to the native terminal for this platform

Re: [PATCH] move functionality of font_path() directly to util/grub-mkconfig.in and prefer unicode.pf2 over ascii.pf2

[Felix Zielcke]

Am Samstag, den 08.08.2009, 07:36 +0200 schrieb Felix Zielcke:
> Am Samstag, den 08.08.2009, 01:12 -0400 schrieb Pavel Roskin:
> > On Fri, 2009-08-07 at 14:57 +0200, Felix Zielcke wrote:
> > 
> > > I commited it now with an ack from Robert on IRC.
> > 
> > Sorry, I'm commenting after it has been committed.  Anyway, please note
> > that having an approval doesn't absolve you from testing the code on
> > your own.  Reviews are not testing.  There was a warning introduced by
> > your change, and there was a syntax error after "ascii".  Also, the
> > formatting of the moved code should have been changed to use the same
> > indentation as the target file.  I have fixed all that.
> 
> Sorry.
> 
> > Could you please explain what I should do to keep using ascii.pf2?  I
> > checked the script, but don't see any variable controlling that.  "make
> > install" would install both unicode.pf2 and ascii.pf2, so unicode.pf2
> > would always be preferred.
> 
> With the old code ascii.pf2 would be always preferred.
> There wasn't either a way to specify it.
> 
> > If changing the default, it's a good style to provide an easy way for
> > users to keep the old setting, and I just don't see it, short or
> > removing /usr/src/unifont.bdf and /usr/local/share/grub/unicode.pf2 so
> > that they are never reinstalled or detected by GRUB.
> 
> Here's now a patch which allows users to specifiy the used font with
> GRUB_FONT=ascii
> 

Oh I forgot to mention that I left the indent changes with intent out,
so it's more readable.

-- 
Felix Zielcke
Proud Debian Maintainer

Re: [PATCH] move functionality of font_path() directly to util/grub-mkconfig.in and prefer unicode.pf2 over ascii.pf2

[Pavel Roskin]

On Sat, 2009-08-08 at 07:36 +0200, Felix Zielcke wrote:

> > Could you please explain what I should do to keep using ascii.pf2?  I
> > checked the script, but don't see any variable controlling that.  "make
> > install" would install both unicode.pf2 and ascii.pf2, so unicode.pf2
> > would always be preferred.
> 
> With the old code ascii.pf2 would be always preferred.
> There wasn't either a way to specify it.

I see. Maybe that's what we should have fixed first.

> > If changing the default, it's a good style to provide an easy way for
> > users to keep the old setting, and I just don't see it, short or
> > removing /usr/src/unifont.bdf and /usr/local/share/grub/unicode.pf2 so
> > that they are never reinstalled or detected by GRUB.
> 
> Here's now a patch which allows users to specifiy the used font with
> GRUB_FONT=ascii

First of all, I hope that the patch you will actually commit will use
sane formatting.  Diffs that ignore spacing changes are OK for review,
but not for applying as is.

I think it would be more natural to let the user specify the full path
to the file.  Setting LANG=C seems unneeded in this case.  After all,
it's the user's choice, and we cannot examine the font file to check
which characters it has.

-- 
Regards,
Pavel Roskin

Re: [PATCH] move functionality of font_path() directly to util/grub-mkconfig.in and prefer unicode.pf2 over ascii.pf2

[Felix Zielcke]

[-- Attachment #1: Type: text/plain, Size: 1431 bytes --]

Am Samstag, den 08.08.2009, 01:49 -0400 schrieb Pavel Roskin:
> On Sat, 2009-08-08 at 07:36 +0200, Felix Zielcke wrote:
> 
> > > Could you please explain what I should do to keep using ascii.pf2?  I
> > > checked the script, but don't see any variable controlling that.  "make
> > > install" would install both unicode.pf2 and ascii.pf2, so unicode.pf2
> > > would always be preferred.
> > 
> > With the old code ascii.pf2 would be always preferred.
> > There wasn't either a way to specify it.
> 
> I see. Maybe that's what we should have fixed first.
> 
> > > If changing the default, it's a good style to provide an easy way for
> > > users to keep the old setting, and I just don't see it, short or
> > > removing /usr/src/unifont.bdf and /usr/local/share/grub/unicode.pf2 so
> > > that they are never reinstalled or detected by GRUB.
> > 
> > Here's now a patch which allows users to specifiy the used font with
> > GRUB_FONT=ascii
> 
> First of all, I hope that the patch you will actually commit will use
> sane formatting.  Diffs that ignore spacing changes are OK for review,
> but not for applying as is.

Yes.

> I think it would be more natural to let the user specify the full path
> to the file.  Setting LANG=C seems unneeded in this case.  After all,
> it's the user's choice, and we cannot examine the font file to check
> which characters it has.

Ok here's a new one.


-- 
Felix Zielcke
Proud Debian Maintainer

[-- Attachment #2: font.diff.3 --]
[-- Type: text/plain, Size: 1032 bytes --]

2009-08-08  Felix Zielcke  <fzielcke@z-51.de>

	* util/grub-mkconfig.in: Allow the user to specify the used font
	with GRUB_FONT.

Index: util/grub-mkconfig.in
===================================================================
--- util/grub-mkconfig.in	(revision 2482)
+++ util/grub-mkconfig.in	(working copy)
@@ -154,6 +154,14 @@ esac
 # check for terminals that require fonts
 case ${GRUB_TERMINAL_OUTPUT} in
   gfxterm)
+    if [ -n "$GRUB_FONT" ] ; then
+      if is_path_readable_by_grub ${GRUB_FONT} > /dev/null ; then
+        GRUB_FONT_PATH=${GRUB_FONT}
+	else
+	  echo "No such font or not readable by grub: ${GRUB_FONT}" >&2
+	  exit 1
+	fi
+    else
     for dir in ${pkgdatadir} /boot/grub /usr/share/grub ; do
       for basename in unicode unifont ascii; do
 	path="${dir}/${basename}.pf2"
@@ -169,6 +177,7 @@ case ${GRUB_TERMINAL_OUTPUT} in
 	break 2
       done
     done
+    fi
     if [ -z "${GRUB_FONT_PATH}" ] ; then
       # fallback to the native terminal for this platform
       unset GRUB_TERMINAL_OUTPUT

Re: [PATCH] move functionality of font_path() directly to util/grub-mkconfig.in and prefer unicode.pf2 over ascii.pf2

[Pavel Roskin]

On Sat, 2009-08-08 at 08:04 +0200, Felix Zielcke wrote:

> > I think it would be more natural to let the user specify the full path
> > to the file.  Setting LANG=C seems unneeded in this case.  After all,
> > it's the user's choice, and we cannot examine the font file to check
> > which characters it has.
> 
> Ok here's a new one.

Looks good to me.

-- 
Regards,
Pavel Roskin

Re: [PATCH] move functionality of font_path() directly to util/grub-mkconfig.in and prefer unicode.pf2 over ascii.pf2

[Felix Zielcke]

Am Samstag, den 08.08.2009, 02:35 -0400 schrieb Pavel Roskin:
> On Sat, 2009-08-08 at 08:04 +0200, Felix Zielcke wrote:
> 
> > > I think it would be more natural to let the user specify the full path
> > > to the file.  Setting LANG=C seems unneeded in this case.  After all,
> > > it's the user's choice, and we cannot examine the font file to check
> > > which characters it has.
> > 
> > Ok here's a new one.
> 
> Looks good to me.
> 

Ok commited with style changed.

-- 
Felix Zielcke
Proud Debian Maintainer