Created attachment 63435 [details] [review]
the patch for this bug.

Update the patch.

Currently gnome-system-log do not support Solaris well, because some log names and locations are different to Linux. And gnome-vfs is not fully functioned in Solaris, so monitor can not work in Solaris.

Additionally, there is some nits in logview which may cause it to crash and  display incorrect character.

Thanks for the patch.  Here's some nitpicks:

  * there's a way to check if we are using solaris when launching the configure
    script, so that we can use AC_SUBST and have a more descriptive macro? I'd
    like to have a #ifdef ON_SUN_OS ... #endif instead of a #ifdef sun ... #endif
    pair - something that sticks out a bit more;

  * the coding style mandates lowercase function names, and a bit of namespacing
    doesn't harm - so:

    +static gboolean
    +WasModified (Log *log)

    should become:

    +static gboolean
    +log_was_modified (Log *log)
 
  * also, you should avoid placing more than one statement on the same line,
    or the branch of an if (...) on the same line as the if;

  * finally, the indentation should be consistent, and always be done using
    tab (set at 8 characters); I know, the consistency inside the code base
    is not good, but I plan to do some clean up in the next months.

other that these minor remarks the patch looks good.

Comment on attachment 63435 [details] [review]
the patch for this bug.

Update the patch.

>Index: ChangeLog
>===================================================================
>RCS file: /cvs/gnome/gnome-utils/logview/ChangeLog,v
>retrieving revision 1.385
>diff -u -p -r1.385 ChangeLog
>--- ChangeLog	5 Feb 2006 23:19:16 -0000	1.385
>+++ ChangeLog	14 Apr 2006 06:31:15 -0000
>@@ -1,3 +1,13 @@
>+2006-04-14  Lin Ma  <Lin.Ma@Sun.COM>
>+
>+	* log_repaint.c: (logview_update_statusbar), remove '\n' from ctime returned string.
>+	* logrtns.c: (log_open), (log_read_new_lines): let displayed lines equal total lines to avoid bolding all the active log lines.
>+	* logview.c: (logview_add_log): Monitor the log at last to avoid calling a nil logview pointer.
>+	* monitor.c: unfortunately gnomve-vfs in Solaris don't support monitor, so in order to introduce logview to Solaris, we should still use glib timeout routine. I will track the change of gnome-vfs in Solaris to deal with it properly.
>+	* userprefs.c: (parse_syslog), (prefs_create_defaults), add some Solaris default log files.
>+	(prefs_load): err always nil, so we can use it to judge to initialize the logfile list.
>+
>+
> 2006-02-06  Emmanuele Bassi  <ebassi@cvs.gnome.org>
> 
> 	Codebase clean up patch (based on a patch by Paolo Borelli).
>Index: log_repaint.c
>===================================================================
>RCS file: /cvs/gnome/gnome-utils/logview/log_repaint.c,v
>retrieving revision 1.118
>diff -u -p -r1.118 log_repaint.c
>--- log_repaint.c	6 Dec 2005 19:42:06 -0000	1.118
>+++ log_repaint.c	14 Apr 2006 06:31:15 -0000
>@@ -164,23 +164,26 @@ static void
> logview_update_statusbar (LogviewWindow *logview)
> {
>    char *statusbar_text;
>-   char *size, *modified;
>+   char *size, *modified, *index;
>    Log *log;
> 
>    g_assert (LOGVIEW_IS_WINDOW (logview));
> 
>    log = logview->curlog;
> 
>-   if (log == NULL) { 
>+   if (log == NULL) {
>        gtk_statusbar_pop (GTK_STATUSBAR (logview->statusbar), 0);
>        return;
>    }
>-
>-   modified = g_strdup_printf (_("last update : %s"), ctime (&(log->stats->file_time)));   
>+   
>+   /* ctime returned string has "\n\0" causes statusbar display a invalid char */
>+   modified = ctime (&(log->stats->file_time));
>+   index = strrchr (modified, '\n'); *index = '\0';
>+   modified = g_strdup_printf (_("last update : %s"), modified);
>    size = gnome_vfs_format_file_size_for_display (log->stats->file_size);
>    statusbar_text = g_strdup_printf (_("%d lines (%s) - %s"), 
>                                      log->total_lines, size, modified);
>-
>+                                     
>    if (statusbar_text) {
>        gtk_statusbar_pop (GTK_STATUSBAR(logview->statusbar), 0);
>        gtk_statusbar_push (GTK_STATUSBAR(logview->statusbar), 0, statusbar_text);
>@@ -305,7 +308,7 @@ log_add_new_log_lines (Log *log)
>     } else {
>       iter_ptr = NULL;
>     }  
>-                
>+ 
>     for (i=log->displayed_lines; i<log->total_lines; i++) {
> 
>         line = log->lines[i];
>Index: logrtns.c
>===================================================================
>RCS file: /cvs/gnome/gnome-utils/logview/logrtns.c,v
>retrieving revision 1.104
>diff -u -p -r1.104 logrtns.c
>--- logrtns.c	23 Nov 2005 19:30:00 -0000	1.104
>+++ logrtns.c	14 Apr 2006 06:31:15 -0000
>@@ -410,7 +410,7 @@ log_open (char *filename, gboolean show_
>    g_free (buffer);   
> 
>    log->total_lines = g_strv_length (log->lines);
>-   log->displayed_lines = 0;
>+   log->displayed_lines = log->total_lines;
>    log->first_time = TRUE;
>    log->stats = stats;
>    log->model = NULL;
>@@ -486,15 +486,15 @@ log_read_new_lines (Log *log)
>     GnomeVFSResult result;
>     gchar *buffer;
>     GnomeVFSFileSize newfilesize, read;
>-    int size, newsize;
>+    guint64 size, newsize;
>     
>     g_return_val_if_fail (log!=NULL, FALSE);
>     
>     result = gnome_vfs_seek (log->mon_file_handle, GNOME_VFS_SEEK_END, 0L);
>     result = gnome_vfs_tell (log->mon_file_handle, &newfilesize);
>-    size = (int) log->mon_offset;
>-    newsize = (int) newfilesize;
>-    
>+    size = log->mon_offset;
>+    newsize = newfilesize;
>+
>     if (newsize > size) {
>       buffer = g_malloc (newsize - size);
>       if (!buffer)
>Index: logview.c
>===================================================================
>RCS file: /cvs/gnome/gnome-utils/logview/logview.c,v
>retrieving revision 1.228
>diff -u -p -r1.228 logview.c
>--- logview.c	5 Feb 2006 23:19:16 -0000	1.228
>+++ logview.c	14 Apr 2006 06:31:15 -0000
>@@ -398,9 +398,9 @@ logview_add_log (LogviewWindow *logview,
>     g_return_if_fail (log);
> 
>     logview->logs = g_slist_append (logview->logs, log);
>-    monitor_start (log);
>     loglist_add_log (LOG_LIST(logview->loglist), log);
>-    log->window = logview;    
>+    log->window = logview;
>+    monitor_start (log);
> }
> 
> 
>Index: monitor.c
>===================================================================
>RCS file: /cvs/gnome/gnome-utils/logview/monitor.c,v
>retrieving revision 1.58
>diff -u -p -r1.58 monitor.c
>--- monitor.c	16 Nov 2005 20:22:41 -0000	1.58
>+++ monitor.c	14 Apr 2006 06:31:15 -0000
>@@ -26,15 +26,22 @@
> #include "monitor.h"
> #include <libgnomevfs/gnome-vfs-ops.h>
> 
>+#ifdef sun
>+const guint check_file_period=1000;
>+#endif
>+
> void
> monitor_stop (Log *log)
> {
>   g_return_if_fail (log);
> 
>+#ifdef sun
>+  if (log->monitored) {
>+#else
>   if (log->mon_handle != NULL) {
>       gnome_vfs_monitor_cancel (log->mon_handle);
>+#endif
>       log->mon_handle = NULL;
>-      log->mon_offset = 0;
>       
>       gnome_vfs_close (log->mon_file_handle);
>       log->mon_file_handle = NULL;
>@@ -44,9 +51,13 @@ monitor_stop (Log *log)
> }
> 
> static void
>+#ifndef sun
> monitor_callback (GnomeVFSMonitorHandle *handle, const gchar *monitor_uri,
>                   const gchar *info_uri, GnomeVFSMonitorEventType event_type,
>                   gpointer data)
>+#else
>+monitor_callback (gpointer data)
>+#endif /* sun */
> {
>   LogviewWindow *logview;
>   Log *log = data;
>@@ -60,6 +71,31 @@ monitor_callback (GnomeVFSMonitorHandle 
>     logview_repaint (logview);
> }
> 
>+#ifdef sun
>+/* ----------------------------------------------------------------------
>+   NAME:	WasModified
>+   DESCRIPTION:	Returns true if modified flag in log changed. It also
>+                changes the modified flag.
>+   ---------------------------------------------------------------------- */
>+
>+static gboolean
>+WasModified (Log *log)
>+{
>+	GnomeVFSResult result;
>+	GnomeVFSFileInfo info;
>+    GnomeVFSFileSize newfilesize;
>+
>+	if (log->monitored == FALSE) return FALSE;
>+    result = gnome_vfs_seek (log->mon_file_handle, GNOME_VFS_SEEK_END, 0L);
>+    result = gnome_vfs_tell (log->mon_file_handle, &newfilesize);
>+
>+	if (newfilesize > log->mon_offset) {
>+		monitor_callback(log);
>+	}
>+	return TRUE;
>+}
>+#endif /* sun */
>+
> void
> monitor_start (Log *log)
> {
>@@ -71,19 +107,31 @@ monitor_start (Log *log)
> 
>   result = gnome_vfs_open (&(log->mon_file_handle), log->name, 
>                            GNOME_VFS_OPEN_READ);
>-  result = gnome_vfs_seek (log->mon_file_handle, GNOME_VFS_SEEK_END, 0L);
>-  result = gnome_vfs_tell (log->mon_file_handle, &size);
>-  log->mon_offset = size;
> 
>+  if (log->mon_offset == 0) {
>+		result = gnome_vfs_seek (log->mon_file_handle, GNOME_VFS_SEEK_END, 0L);
>+		result = gnome_vfs_tell (log->mon_file_handle, &size);
>+		log->mon_offset = size;
>+  }
>+  
>   result = gnome_vfs_monitor_add (&(log->mon_handle), log->name,
>                          GNOME_VFS_MONITOR_FILE, monitor_callback,
>                          log);
>+#ifdef sun
>+  if (result == GNOME_VFS_ERROR_NOT_SUPPORTED)
>+  {
>+		log->monitored = TRUE;
>+		g_timeout_add (check_file_period, WasModified, log);
>+		return;
>+  }
>+#endif
> 
>   if (result != GNOME_VFS_OK) {
>     main = g_strdup (_("This file cannot be monitored."));
>     switch (result) {
>     case GNOME_VFS_ERROR_NOT_SUPPORTED :
>         second = g_strdup (_("File monitoring is not supported on this file system.\n"));
>+        break;
>     default:
>         second = g_strdup (_("Gnome-VFS Error.\n"));
>     }
>Index: userprefs.c
>===================================================================
>RCS file: /cvs/gnome/gnome-utils/logview/userprefs.c,v
>retrieving revision 1.30
>diff -u -p -r1.30 userprefs.c
>--- userprefs.c	5 Feb 2006 23:19:16 -0000	1.30
>+++ userprefs.c	14 Apr 2006 06:31:15 -0000
>@@ -59,45 +59,58 @@ parse_syslog(gchar *syslog_file) {
> 	char *cline, *p;
> 	FILE *cf;
> 	GSList *logfiles = NULL;
>-
> 	if ((cf = fopen(syslog_file, "r")) == NULL) {
> 		return NULL;
> 	}
> 	cline = cbuf;
> 	while (fgets(cline, sizeof(cbuf) - (cline - cbuf), cf) != NULL) {
>-		for (p = cline; isspace(*p); ++p);
>-		if (*p == '\0' || *p == '#')
>+	    gchar **list;
>+	    gint i;
>+	    for (p = cline; g_ascii_isspace(*p); ++p);
>+		    if (*p == '\0' || *p == '#' || *p == '\n')
> 			continue;
>-		for (;*p && !strchr("\t ", *p); ++p);
>-		while (*p == '\t' || *p == ' ')
>-			p++;
>-		if (*p == '-')
>-			p++;
>-		if (*p == '/') {
>-			logfile = g_strdup (p);
>-			/* remove trailing newline */
>-			if (logfile[strlen(logfile)-1] == '\n')
>-				logfile[strlen(logfile)-1] = '\0';
>-			logfiles = g_slist_append (logfiles, logfile);
>-		}
>+		    list = g_strsplit_set (p, ", -\t()\n", 0);
>+		    for (i = 0; list[i]; ++i) {
>+				if (*list[i] == '/'
>+				    && g_slist_find_custom(logfiles, list[i],
>+							   g_ascii_strcasecmp) == NULL) {
>+				    logfiles = g_slist_insert (logfiles, g_strdup(list[i]), 0);
>+			}
>+	    }
>+	    g_strfreev(list);
> 	}
>-	return logfiles; 
>+	fclose(cf);
>+	return logfiles;
> }
> 
> static void
> prefs_create_defaults (UserPrefs *p)
> {
> 	int i;
>-	gchar *logfiles[] = {"/var/adm/messages",
>-                         "/var/log/messages",
>-                         "/var/log/sys.log",
>-                         "/var/log/secure",
>-                         "/var/log/maillog",
>-                         "/var/log/cron",
>-                         "/var/log/Xorg.0.log",
>-                         "/var/log/XFree86.0.log",
>-                         "/var/log/auth.log",
>-                         "/var/log/cups/error_log"};
>+	gchar *logfiles[] = {
>+	    "/var/log/sys.log",
>+#ifndef sun
>+		"/var/log/messages",
>+		"/var/log/secure",
>+		"/var/log/maillog",
>+		"/var/log/cron",
>+		"/var/log/Xorg.0.log",
>+		"/var/log/XFree86.0.log",
>+		"/var/log/auth.log",
>+	    "/var/log/cups/error_log",
>+#else
>+	    "/var/adm/messages",
>+	    "/var/adm/sulog",
>+	    "/var/log/authlog",
>+	    "/var/log/brlog",
>+	    "/var/log/postrun.log",
>+	    "/var/log/scrollkeeper.log",
>+	    "/var/log/snmpd.log",
>+	    "/var/log/sysidconfig.log",
>+	    "/var/log/swupas/swupas.log",
>+	    "/var/log/swupas/swupas.error.log",
>+#endif
>+	NULL};
> 	struct stat filestat;
> 	GSList *logs = NULL;
> 	GnomeVFSResult result;
>@@ -114,9 +127,10 @@ prefs_create_defaults (UserPrefs *p)
> 		logs = parse_syslog ("/etc/syslog.conf");
> 	}
> 	
>-	for (i=0; i<10; i++) {
>-		if (file_is_log (logfiles[i], FALSE))
>-			logs = g_slist_append (logs, g_strdup(logfiles[i]));
>+	for (i=0; logfiles[i]; i++) {
>+	    if (g_slist_find_custom(logs, logfiles[i], g_ascii_strcasecmp) == NULL &&
>+		file_is_log (logfiles[i], FALSE))
>+		logs = g_slist_insert (logs, g_strdup(logfiles[i]), 0);
> 	}
> 
> 	if (logs != NULL) {
>@@ -142,7 +156,7 @@ prefs_load (void)
> 					 GCONF_LOGFILES,
> 					 GCONF_VALUE_STRING,
> 					 &err);
>-	if (err)
>+	if (p->logs == NULL)
> 		prefs_create_defaults (p);
> 	
> 	logfile = NULL;

Created attachment 64265 [details] [review]
update the patch.

What about this one? For the TAB indent, I only change two files: monitor.c and userprefs.c, because the two files are changed the most. For others I think it's better to do a thoroughly re-formatting.

Created attachment 66865 [details] [review]
Base on the old patch, fix some UI bugs.

Fixed: No horizontal scrolledbar even a log line is extremely long.
Added: Sort logs in alphabet order.
Fixed: Change menu bar "Log" to "File"
Moved: From "Log"->"Monitor" to "View"->"Monitor" which is more reasonable.

Removed: Nil char from the status bar.

The Patch in attachment 64265 [details] [review] looks good to me. Please commit this one.

About the other enhancements that you've added in attachment 66865 [details] [review], could you please
extract those in seperate patches and file them in individual bugs ? I'm not comfortable committing
various changes in the same patch.

Note that the "no horizontal scrollbar" part is bug 333862, so you can attach the relevant patch
there. Apparently lots of people are requesting this, so thanks a lot !

Created attachment 66879 [details] [review]
Patch for $top/configure.in and logview/Makefile.am

Make the app can be built on Solaris.

Created attachment 66880 [details] [review]
Added: Sort logs in alphabet order.

Created attachment 66881 [details] [review]
Fixed: Bold the whole log lines and can't get the corrent size in x86_64 and Sparc system.

Created attachment 66882 [details] [review]
Fixed: no horizontal scrollbar

Created attachment 66883 [details] [review]
Fixed: can't monitor logs on Solaris dur to no FAM

Created attachment 66884 [details] [review]
Removed: Nil char from the status bar.

Created attachment 66885 [details] [review]
add some Solaris default log files.

(prefs_load): err always nil, so we can't use it to initialize the logfile list.

Lin Ma : I was expecting you to create seperate bugs for each patch. Sorry if that wasn't clear.

For the time being, you can commit patch 66879. Do you have CVS commit rights ?

heh, sorrry.
I haven't CVS commit rights :(
So, please help me commit it.

Lin Ma, please open a different bug for each patch so we can track them.

I've applied #66885, #66884, #66882, #66881, #66880 and #66879 to HEAD.

Patch #66883 has some bits wrong, like not reporting the error from gnome-vfs and the special casing of the timeout; I'd that the timed stat() on the file is used if/when no gnome-vfs monitoring is supported - not only on solaris.

I just filed the following bugs for each patches except #66882, because bug#333862
 is filed against the bug.

#344648	can't monitor logs on Solaris dur to no FAM
#344647 Add macro ON_SUN_OS in configure.in and update Makefile.am
#344645 Can not display any logs at the first run
#344644 Bold the whole log lines when booting up logview, and can not get the correct length of large files.
#344641 Status bar displays an incorrect charater at the end of the message.
#344640 Sort logs in alphabet order.

So Emmanuele Bassi, you may close the bug and track those bugs. For bug#333862, Vincent, I hope you can allow my mistake this time, because it's hard for me to recreate the patch, forgive me please ;-) I will take care it next time.

closing this bug.