Re: [PATCH] Help Viewer - incorrect behaviour of <up> arrrow key

[Grigory Trenin <gtrenin gmail com>]

Hello Pavel,

Pavel Tsekov wrote:

> I think the correct solution would be to keep both
> 'startpoint' and 'currentpoint' in the history but I am
> open to suggestions.


Yes, I thought about it too. This is a good and safe fix.
But I have a suggestion.
Actually, 'startpoint' is used only once at line 315 of help.c:

   p = current_link - 1;
   if (p <= start)
       return 0;
   p = search_char_node (p, CHAR_LINK_START, -1);
   return p;

Here 'current_link' is the pointer to the currently
selected (highlighted) link, and 'start' == 'startpoint'.
Since there can be no situation when currently selected link
is outside of the current topic, (p <= start) expression
becomes true only in one rare case - when the first text in the node
is a link, for example:
^D[Topic name]^ASome link^BSome link^C

In that case 'p' will point to ']' character, and 'start_point' will
point to ^A, so (p <= start) condition will be true. But
this check for (p <= start) condition is not really necessary,
because search_char_node() will not search beyond the topic
boundaries, it will stop at ^D character and return NULL.

So, if that check and the 'startpoint' variable itself are not
really necessary and can be eliminated, why should we mess with
saving/restoring 'startpoint' in the history?

Please look at a new patch - what do you think?
There I removed 'startpoint' as well as the first argument of
select_prev_link() function. And after calling it there is no sense to
check if (selected_item >= last_shown) - this will never happen,
so I removed it also.


Regards,
 Grigory.

--- help.c.orig	2005-07-22 13:29:50.000000000 +0400
+++ help.c	2006-12-02 16:35:18.000000000 +0300
@@ -72,7 +72,7 @@
 static const char *main_node;	/* The main node */
 static const char *last_shown = NULL;	/* Last byte shown in a screen */
 static int end_of_node = 0;	/* Flag: the last character of the node shown? */
-static const char *currentpoint, *startpoint;
+static const char *currentpoint;
 static const char *selected_item;
 
 /* The widget variables */
@@ -305,19 +305,11 @@
     return p - 1;
 }
 
-static const char *select_prev_link (const char *start, const char *current_link)
+static const char *select_prev_link (const char *current_link)
 {
-    const char *p;
-
     if (!current_link)
 	return 0;
-    
-    p = current_link - 1;
-    if (p <= start)
-	return 0;
-    
-    p = search_char_node (p, CHAR_LINK_START, -1);
-    return p;
+    return search_char_node (current_link - 1, CHAR_LINK_START, -1);
 }
 
 static void start_link_area (int x, int y, const char *link_name)
@@ -489,7 +481,7 @@
     event->y -= 2;
 
     if (event->buttons & GPM_B_RIGHT){
-	currentpoint = startpoint = history [history_ptr].page;
+	currentpoint = history [history_ptr].page;
 	selected_item = history [history_ptr].link;
 	history_ptr--;
 	if (history_ptr < 0)
@@ -527,7 +519,7 @@
 	history_ptr = (history_ptr+1) % HISTORY_SIZE;
 	history [history_ptr].page = currentpoint;
 	history [history_ptr].link = current_area->link_name;
-	currentpoint = startpoint = help_follow_link (currentpoint, current_area->link_name);
+	currentpoint = help_follow_link (currentpoint, current_area->link_name);
 	selected_item = NULL;
     } else{
 	if (event->y < 0)
@@ -561,7 +553,7 @@
     if (p == NULL)
 	return;
 
-    currentpoint = startpoint = p + 1;
+    currentpoint = p + 1;
     selected_item = NULL;
     help_callback (h, DLG_DRAW, 0);
 }
@@ -582,7 +574,7 @@
     history[history_ptr].page = currentpoint;
     history[history_ptr].link = selected_item;
 
-    currentpoint = startpoint = new_item + 1;
+    currentpoint = new_item + 1;
     selected_item = NULL;
     help_callback (h, DLG_DRAW, 0);
 }
@@ -595,7 +587,7 @@
 static void prev_node_cmd (void *vp)
 {
     Dlg_head *h = vp;
-    currentpoint = startpoint = history [history_ptr].page;
+    currentpoint = history [history_ptr].page;
     selected_item = history [history_ptr].link;
     history_ptr--;
     if (history_ptr < 0)
@@ -672,14 +664,14 @@
 	    if (history_ptr < 0)
 		history_ptr = HISTORY_SIZE-1;
 	    
-	    currentpoint = startpoint = history [history_ptr].page;
+	    currentpoint = history [history_ptr].page;
 	    selected_item   = history [history_ptr].link;
 #endif
 	} else {
 	    history_ptr = (history_ptr+1) % HISTORY_SIZE;
 	    history [history_ptr].page = currentpoint;
 	    history [history_ptr].link = selected_item;
-	    currentpoint = startpoint = help_follow_link (currentpoint, selected_item) + 1;
+	    currentpoint = help_follow_link (currentpoint, selected_item) + 1;
 	}
 	selected_item = NULL;
 	break;
@@ -704,9 +696,8 @@
     case KEY_UP:
     case ALT ('\t'):
 	/* select previous link */
-	new_item = select_prev_link (startpoint, selected_item);
-	selected_item = new_item;
-	if (selected_item < currentpoint || selected_item >= last_shown){
+	selected_item = select_prev_link (selected_item);
+	if (selected_item == NULL || selected_item < currentpoint){
 	    if (c == KEY_UP)
 		move_backward (1);
 	    else{
@@ -835,7 +826,7 @@
 		    DLG_TRYUP | DLG_CENTER | DLG_WANT_TAB);
 
     selected_item = search_string_node (main_node, STRING_LINK_START) - 1;
-    currentpoint = startpoint = main_node + 1;
+    currentpoint = main_node + 1;
 
     for (history_ptr = HISTORY_SIZE; history_ptr;) {
 	history_ptr--;