On Thu, Apr 05, 2018 at 08:05:47PM +0200, Hans de Goede wrote: > Hi, > > On 05-04-18 14:34, Daniel Kiper wrote: > >On Wed, Mar 28, 2018 at 04:50:28PM +0200, Hans de Goede wrote: > >>If we're running with a hidden menu we may never need text mode, so do not > >>change the video-mode to text until we actually need it. > >> > >>Signed-off-by: Hans de Goede <hdego...@redhat.com> > >>--- > >> grub-core/term/efi/console.c | 72 +++++++++++++++++++++++------------- > >> 1 file changed, 47 insertions(+), 25 deletions(-) > >> > >>diff --git a/grub-core/term/efi/console.c b/grub-core/term/efi/console.c > >>index 02f64ea74..d9fd7cf48 100644 > >>--- a/grub-core/term/efi/console.c > >>+++ b/grub-core/term/efi/console.c > >>@@ -24,6 +24,11 @@ > >> #include <grub/efi/api.h> > >> #include <grub/efi/console.h> > >> > >>+static grub_err_t grub_prepare_for_text_output(struct grub_term_output > >>*term); > > > >Please drop this forward declaration and put the function definition before > >the callers. > > I did not do that initially because that requires also moving > grub_console_setcolorstate() and grub_console_setcursor() to > higher in the file (or do a forward declaration for those). > > I'll add a preparation patch to v2 of the series moving just > those 2 up to higher in the file.
Great! > >>+static int text_mode_available = -1; > > > >Could you use bool type here? If yes please define grub_bool_t as stdbool.h > >does (grub/bool.h?) and replace its usage in grub-core/term/tparm.c as > >separate > >patch. If not bool please use enum here. > > There are 3 states, so a bool is not going to work I will > switch to an enum for v2. Perfect! > >>+static int text_colorstate = -1; > > There already is a grub_term_color_state enum for this, which > defines values 0-2, I want to know if setcolorstate has been > called and text_colorstate contains a valid value, so I made > this an int and use -1 to mean not set / invalid. > > I can see a number of solutions here: > > 1) Leave as is > 2) Use: > > static grub_term_color_state text_colorstate = -1; > > Assuming the compiler will not warn about putting -1 in there. > > 3) Extend the grub_term_color_state like this: > > typedef enum > { > /* Used for uninitialized grub_term_color_state variables */ > GRUB_TERM_COLOR_UNDEFINED = -1, > /* The color used to display all text that does not use the > user defined colors below. */ > GRUB_TERM_COLOR_STANDARD = 0, > /* The user defined colors for normal text. */ > GRUB_TERM_COLOR_NORMAL, > /* The user defined colors for highlighted text. */ > GRUB_TERM_COLOR_HIGHLIGHT > } > grub_term_color_state; > > Which solution would you prefer? 3rd, enum please. Thanks, Daniel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel