Re: [PATCH] Choose syntax

[Roland Illig <roland illig gmx de>]

Leonard den Ottolander wrote:
> +#include <config.h>

#include "../src/global.h"
(see HEAD/maint/headers.txt)

> +#include "edit.h"
> +#include "syntax.h"
> +#include "../src/wtools.h"
> +
> +#define MAX_ENTRY_LEN 40
> +#define LIST_LINES 14
> +#define N_DFLT_ENTRIES 1
> +
> +int
> +exec_syntax_dialog (const char **names) {

This function must know the size of the ''names'' buffer. I suggest a 
second parameter: size_t names_size.

> +    int i;
> +
> +    Listbox *syntaxlist = create_listbox_window (MAX_ENTRY_LEN, LIST_LINES,
> +	N_(" Choose syntax highlighting "), NULL);
> +    LISTBOX_APPEND_TEXT (syntaxlist, 'A', N_("< Auto >"), NULL);
> +
> +    for (i = 0; names[i]; i++) {
> +	LISTBOX_APPEND_TEXT (syntaxlist, 0, names[i], NULL);
> +	if (! option_auto_syntax && option_syntax_type &&

I would have written option_syntax_type != NULL.

> +	    ! strcmp (names[i], option_syntax_type))

don't use !strcmp(...). This function does not return a boolean, but a 
"comparison result", which should be checked using the relational 
operators (==, <=, >=, <, >).

> +    	    listbox_select_by_number (syntaxlist->list, i + N_DFLT_ENTRIES);
> +    }
> +
> +    return run_listbox (syntaxlist);
> +}
> +
> +void
> +syntax_dialog () {

syntax_dialog (void)
{

> +    char *old_syntax_type;
> +    int old_auto_syntax, syntax;
> +    char *names[MAX_SYNTAX_FILES + 1];
> +
> +    /* We fill the list of syntax files every time the editor is invoked.
> +       Instead we could save the list to a file and update it once the syntax
> +       file gets updated (either by testing or by explicit user command). */
> +    edit_load_syntax (NULL, names, 0);
> +
> +    if ((syntax = exec_syntax_dialog ((const char**) names)) < 0)
> +	return;
> +
> +    old_auto_syntax = option_auto_syntax;
> +    old_syntax_type = g_strdup (option_syntax_type);
> +
> +    /* Using a switch as we might want to define more specific commands, f.e.
> +       "Refill syntax list" (compare N_DFLT_ENTRIES). */
> +    switch (syntax) {
> +	case 0: /* auto syntax */
> +	    option_auto_syntax = 1;

TRUE

> +	    break;
> +	default:
> +	    option_auto_syntax = 0;

FALSE

> +	    g_free (option_syntax_type);
> +	    option_syntax_type = g_strdup (names[syntax - N_DFLT_ENTRIES]);
> +    }
> +
> +    /* Load or unload syntax rules if the option has changed */
> +    if (option_auto_syntax) {
> +	if (!old_auto_syntax) 
> +	    edit_load_syntax (wedit, 0, 0);

edit_load_syntax (wedit, NULL, NULL);

or, for the C purists: edit_load_syntax (wedit, (char **) 0, NULL);

> +    } else
> +	if (old_auto_syntax || old_syntax_type && option_syntax_type &&
> +	    strcmp (old_syntax_type, option_syntax_type))

strcmp(...) != 0

> +	    edit_load_syntax (wedit, 0, option_syntax_type);
> +
> +    g_free (old_syntax_type);
> +}
> diff -pruN MC_4_6_1_PRE/mc/edit/edit.c MC_4_6_1_PRE.syntax.input/mc/edit/edit.c
> --- MC_4_6_1_PRE/mc/edit/edit.c	2005-05-29 12:04:00.000000000 +0200
> +++ MC_4_6_1_PRE.syntax.input/mc/edit/edit.c	2005-07-10 11:42:55.000000000 +0200
> @@ -499,6 +499,7 @@ edit_init (WEdit *edit, int lines, int c
>  	   long line)
>  {
>      int to_free = 0;
> +    option_auto_syntax = 1; /* Resetting to auto on every invokation */

replace 1 with TRUE.

>  
>      if (!edit) {
>  #ifdef ENABLE_NLS
> diff -pruN MC_4_6_1_PRE/mc/edit/editcmd.c MC_4_6_1_PRE.syntax.input/mc/edit/editcmd.c
> --- MC_4_6_1_PRE/mc/edit/editcmd.c	2005-05-29 12:04:00.000000000 +0200
> +++ MC_4_6_1_PRE.syntax.input/mc/edit/editcmd.c	2005-07-10 11:07:02.000000000 +0200
> @@ -512,7 +512,10 @@ edit_save_as_cmd (WEdit *edit)
>  		edit->modified = 0;
>  		edit->delete_file = 0;
>  		if (different_filename)
> -		    edit_load_syntax (edit, 0, 0);
> +		    if (option_auto_syntax)
> +			edit_load_syntax (edit, 0, 0);

edit_load_syntax (edit, 0, NULL);

> +		    else
> +			edit_load_syntax (edit, 0, option_syntax_type);
>  		edit->force |= REDRAW_COMPLETELY;
>  		return 1;
>  	    } else {
> diff -pruN MC_4_6_1_PRE/mc/edit/edit.h MC_4_6_1_PRE.syntax.input/mc/edit/edit.h
> --- MC_4_6_1_PRE/mc/edit/edit.h	2005-05-29 12:04:00.000000000 +0200
> +++ MC_4_6_1_PRE.syntax.input/mc/edit/edit.h	2005-07-10 11:07:02.000000000 +0200
> @@ -252,6 +252,7 @@ int line_is_blank (WEdit *edit, long lin
>  int edit_indent_width (WEdit *edit, long p);
>  void edit_insert_indent (WEdit *edit, int indent);
>  void edit_options_dialog (void);
> +void syntax_dialog (void);

What about the name edit_choose_syntax_dialog, which fits much better to 
the ones surrounding it? (see below, too)

>  void edit_mail_dialog (WEdit *edit);
>  void format_paragraph (WEdit *edit, int force);
>  
> @@ -311,6 +312,8 @@ extern int option_save_position;
>  extern int option_backup_ext_int;
>  extern int option_max_undo;
>  extern int option_syntax_highlighting;
> +extern int option_auto_syntax;

better use gboolean for that.

> +extern char *option_syntax_type;
>  extern int editor_option_check_nl_at_eof;
>  
>  extern int option_edit_right_extreme;
> diff -pruN MC_4_6_1_PRE/mc/edit/editmenu.c MC_4_6_1_PRE.syntax.input/mc/edit/editmenu.c
> --- MC_4_6_1_PRE/mc/edit/editmenu.c	2005-05-29 12:04:00.000000000 +0200
> +++ MC_4_6_1_PRE.syntax.input/mc/edit/editmenu.c	2005-07-10 11:07:02.000000000 +0200
> @@ -272,6 +272,13 @@ menu_options (void)
>  {
>      edit_options_dialog ();
>  }
> +
> +static void
> +menu_syntax (void)
> +{
> +    syntax_dialog ();
> +}
> +

Why this extra function? You can use syntax_dialog directly below. It 
should be called edit_choose_syntax_cmd then. syntax_dialog seems a bit 
too unspecific to me.

>  static void
>  menu_user_menu_cmd (void)
>  {
> @@ -394,7 +401,8 @@ static menu_entry OptMenu[] =
>  {
>      {' ', N_("&General...  "), 'G', menu_options},
>      {' ', N_("&Save mode..."), 'S', menu_save_mode_cmd},
> -    {' ', N_("learn &Keys..."), 'K', learn_keys}
> +    {' ', N_("Learn &Keys..."), 'K', learn_keys},
> +    {' ', N_("Syntax &Highlighting..."), 'H', menu_syntax}
>  };
>  
>  #define OptMenuEmacs OptMenu
> diff -pruN MC_4_6_1_PRE/mc/edit/editoptions.c MC_4_6_1_PRE.syntax.input/mc/edit/editoptions.c
> --- MC_4_6_1_PRE/mc/edit/editoptions.c	2005-05-29 12:04:00.000000000 +0200
> +++ MC_4_6_1_PRE.syntax.input/mc/edit/editoptions.c	2005-07-10 11:07:02.000000000 +0200
> @@ -199,6 +199,10 @@ edit_options_dialog (void)
>      edit_key_emulation = tedit_key_emulation;
>  
>      /* Load or unload syntax rules if the option has changed */
> -    if (option_syntax_highlighting != old_syntax_hl)
> -	edit_load_syntax (wedit, 0, 0);
> +    if (option_syntax_highlighting != old_syntax_hl) {
> +	if (option_auto_syntax)
> +	    edit_load_syntax (wedit, 0, 0);

see above.

> +	else
> +	    edit_load_syntax (wedit, 0, option_syntax_type);
> +    }
>  }
> diff -pruN MC_4_6_1_PRE/mc/edit/Makefile.am MC_4_6_1_PRE.syntax.input/mc/edit/Makefile.am
> --- MC_4_6_1_PRE/mc/edit/Makefile.am	2003-04-02 21:36:10.000000000 +0200
> +++ MC_4_6_1_PRE.syntax.input/mc/edit/Makefile.am	2005-07-10 11:07:02.000000000 +0200
> @@ -9,6 +9,6 @@ endif
>  libedit_a_SOURCES = \
>  	bookmark.c edit.c editcmd.c editwidget.c editdraw.c editkeys.c \
>  	editmenu.c editoptions.c editcmddef.h edit.h edit-widget.h \
> -	editlock.c editlock.h syntax.c wordproc.c
> +	editlock.c editlock.h syntax.c wordproc.c choosesyntax.c
>  
>  EXTRA_DIST = ChangeLog
> diff -pruN MC_4_6_1_PRE/mc/edit/syntax.c MC_4_6_1_PRE.syntax.input/mc/edit/syntax.c
> --- MC_4_6_1_PRE/mc/edit/syntax.c	2005-05-29 12:04:00.000000000 +0200
> +++ MC_4_6_1_PRE.syntax.input/mc/edit/syntax.c	2005-07-10 11:27:20.000000000 +0200
> @@ -23,6 +23,7 @@
>  #include <config.h>
>  #include "edit.h"
>  #include "edit-widget.h"
> +#include "syntax.h"
>  #include "../src/color.h"	/* use_colors */
>  #include "../src/main.h"	/* mc_home */
>  #include "../src/wtools.h"	/* message() */
> @@ -85,6 +86,8 @@ struct _syntax_marker {
>  };
>  
>  int option_syntax_highlighting = 1;
> +int option_auto_syntax = 1;

gboolean, as above.

> +char *option_syntax_type = NULL;
>  
>  #define syntax_g_free(x) do {g_free(x); (x)=0;} while (0)
>  
> @@ -1007,6 +1010,11 @@ edit_read_syntax_file (WEdit * edit, cha
>  	}
>  	if (names) {
>  /* 1: just collecting a list of names of rule sets */
> +/* This list could be allocated here dynamically as we don't know it's size yet */

s/could/should/
s/it's/its/
You could also add a TODO tag to the comment.

> +	    if (count == MAX_SYNTAX_FILES) {
> +		names[count] = 0;

NULL.

> +		break;
> +	    }
>  	    names[count++] = g_strdup (args[2]);
>  	    names[count] = 0;
>  	} else if (type) {
> diff -pruN MC_4_6_1_PRE/mc/edit/syntax.h MC_4_6_1_PRE.syntax.input/mc/edit/syntax.h
> --- MC_4_6_1_PRE/mc/edit/syntax.h	1970-01-01 01:00:00.000000000 +0100
> +++ MC_4_6_1_PRE.syntax.input/mc/edit/syntax.h	2005-07-10 11:11:56.000000000 +0200
> @@ -0,0 +1,6 @@
> +#ifndef __SYNTAX_H
> +#define __SYNTAX_H

All names starting with an underscore are reserved for the C 
implementation (that is, the compiler and the standard library). 
Application programmers shall not use them. If you had written your 
patch for -HEAD, you would have seen the change.

> +
> +#define MAX_SYNTAX_FILES 1024
> +
> +#endif				/* !__SYNTAX_H */
> diff -pruN MC_4_6_1_PRE/mc/src/wtools.h MC_4_6_1_PRE.syntax.input/mc/src/wtools.h
> --- MC_4_6_1_PRE/mc/src/wtools.h	2004-09-17 05:19:21.000000000 +0200
> +++ MC_4_6_1_PRE.syntax.input/mc/src/wtools.h	2005-07-10 11:07:02.000000000 +0200
> @@ -1,8 +1,7 @@
>  #ifndef __WTOOLS_H
>  #define __WTOOLS_H
>  
> -struct Dlg_head;
> -struct WListbox;
> +#include "widget.h"
>  
>  /* Listbox utility functions */
>  typedef struct {

Roland