Re: [PATCH] Help Viewer - incorrect behaviour of up arrrow key
Hello Grigory, On Mon, 2006-12-04 at 19:45, Grigory Trenin wrote: 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. Ok. I think your arguments are reasonable. If the help viewer code was better organized I'd probably have insisted on keeping the variable holding the start of the node. However, I think that currently 'startpoint' brings more confusion than benefit, so I've applied your patch. Thanks! ___ Mc-devel mailing list http://mail.gnome.org/mailman/listinfo/mc-devel
Re: [PATCH] Help Viewer - incorrect behaviour of up arrrow key
Hello Grigory, On Mon, 11 Dec 2006, Grigory Trenin wrote: Pavel Tsekov wrote: I am not convinced that this patch is better than your first one. Its basically the same and removes a valid check - it doesn't fix the issue. It does fix the _original_ issue. And that one you are talking about is quite a different thing (see below). It isn't related to the original issue. Anyway, saving/restoring 'startpoint' in history won't fix it either. Try the following: COLS = 126 LINES = 60 1) F1 (Help) 2) Select contents and hit Enter 3) Press Page Down You should end up on Executing operating systems commands 4) Press Up You should notice that the marker doesn't select the previous link as it should. Why do you expect it? My mistake. Sorry. I'll review the patch more carefully. You are not annoying me - I just want to make sure that a given fix doesn't introduce new issues. In this case I was fooled. ___ Mc-devel mailing list http://mail.gnome.org/mailman/listinfo/mc-devel
Re: [PATCH] Help Viewer - incorrect behaviour of up arrrow key
Pavel Tsekov wrote: I am not convinced that this patch is better than your first one. Its basically the same and removes a valid check - it doesn't fix the issue. It does fix the _original_ issue. And that one you are talking about is quite a different thing (see below). It isn't related to the original issue. Anyway, saving/restoring 'startpoint' in history won't fix it either. Try the following: COLS = 126 LINES = 60 1) F1 (Help) 2) Select contents and hit Enter 3) Press Page Down You should end up on Executing operating systems commands 4) Press Up You should notice that the marker doesn't select the previous link as it should. Why do you expect it? Why pressing Up should scroll _two_ lines up? It is documented that Up key scrolls only _one_ line up. From MC's help: down Move to the next item or scroll a line down. upMove to the previous item or scroll a line up. You need to press Up one more time. That's right. Because the previous link is two lines up, you have to press Up two times to scroll two lines up. By the way, Down key works the same way. So Up/Down keys work the way they are documented: - If prev/next link is visible, jump to it. - Otherwise, scroll one line up or down. If we modify it to always jump to the prev/next link even if the link is not visible, it would be easier to navigate in the contents. But it may bring confusion while navigating inside the node. IMHO, scrolling _always_ one line up is more natural and predictable behaviour. But let me return to the original issue and the patch. Of course I am not saying that it is the best, but I thoroughly analyzed the source and came to conclusion that the check and 'startpoint' are not needed. Really. Waiting for your decision. P.S. Sorry for the late reply but I have some problems which I need to take care of. If I am annoying you with such a simple issue, just say it :) Regards, Grigory ___ Mc-devel mailing list http://mail.gnome.org/mailman/listinfo/mc-devel
Re: [PATCH] Help Viewer - incorrect behaviour of up arrrow key
Hello Grigory, On Fri, 1 Dec 2006, Grigory Trenin wrote: Pavel Tsekov wrote: I won't apply this patch yet. I want to investigate further. Oh, now I see that I had hurried over to make that patch. I'll check everything and make another one. Without haste. The problem is that the structure which stores the history of the links followed stores only the pointer to the start of the node and doesn't store a pointer to the start of the currently displayed area of a node. When following a link MC does this: history_ptr = (history_ptr+1) % HISTORY_SIZE; history [history_ptr].page = currentpoint; history [history_ptr].link = selected_item; currentpoint = startpoint = help_follow_link (...); The start of the node in the history is erronously set to 'currentpoint'. Then when going back to the previous node via the left key the following code is executed via prev_node_cmd(): currentpoint = startpoint = history [history_ptr].page; selected_item = history [history_ptr].link; This leads to incorrectly setting 'startpoint' to the previously recorded value of 'currentpoint' which is wrong. I think the correct solution would be to keep both 'startpoint' and 'currentpoint' in the history but I am open to suggestions. ___ Mc-devel mailing list http://mail.gnome.org/mailman/listinfo/mc-devel
Re: [PATCH] Help Viewer - incorrect behaviour of up arrrow key
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.0 +0400
+++ help.c 2006-12-02 16:35:18.0 +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
Re: [PATCH] Help Viewer - incorrect behaviour of up arrrow key
Pavel Tsekov wrote: I won't apply this patch yet. I want to investigate further. Oh, now I see that I had hurried over to make that patch. I'll check everything and make another one. Without haste. Regards, Grigory ___ Mc-devel mailing list http://mail.gnome.org/mailman/listinfo/mc-devel
Re: [PATCH] Help Viewer - incorrect behaviour of up arrrow key
Hello Grigory,
On Wed, 29 Nov 2006, Grigory Trenin wrote:
Here is a detailed description how to reproduce it:
1) Open the Help Contents (press F1, Tab, Enter)
2) Navigate some lines forward (press End or PageDown several times)
3) Enter the selected topic (press Enter)
4) Return back (press right arrow)
5) Move to the top of Contents (press Home)
6) Now try navigating the Contents using the up and down arrow keys.
(for example, press down arrow for 5 times, then press up arrow).
You will notice that when you press up arrow key the selection
jumps to the top of window.
I can confirm the problem.
The problem is in the help_handle_key() function:
case KEY_UP:
case ALT ('\t'):
/* select previous link */
new_item = select_prev_link (startpoint, selected_item);
The 'startpoint' variable should be a pointer to the first byte
displayed in the Help window. But here it has a wrong value - that's
the problem. That's why select_prev_link() cannot find the link and
returns NULL, and the selection moves to the first link in the window.
I tried to find out what's wrong with the 'startpoint' variable.
I came to the conclusion that 'startpoint' is used here erroneously
instead of 'currentpoint' variable.
I am not convinced of that yet - see below. It's more likely
that the navigation code messes the value of 'startpoint'.
'currentpoint' always contains a pointer to the first byte displayed,
and it should be used here. And by the way, 'startpoint' variable
seems to be totally useless.
This is not true - 'currentpoint' points to the first byte of
the currently disaplyed help contents, while 'startpoint'
points to the start of the current link/topic. 'startpoint'
gets messed after one returns back from following a link.
So in my patch I replaced 'startpoint' with 'currentpoint' and
removed the 'startpoint' variable completely.
I won't apply this patch yet. I want to investigate further.
___
Mc-devel mailing list
http://mail.gnome.org/mailman/listinfo/mc-devel
[PATCH] Help Viewer - incorrect behaviour of up arrrow key
Hello all,
While browsing MC's help, I've noticed a strange behaviour of up arrow
key. After I visit some topic and return back (using right arrow key),
the behaviour of up arrow key weirdly changes: instead of selecting
the previous link, it jumps to the top of the window!
Here is a detailed description how to reproduce it:
1) Open the Help Contents (press F1, Tab, Enter)
2) Navigate some lines forward (press End or PageDown several times)
3) Enter the selected topic (press Enter)
4) Return back (press right arrow)
5) Move to the top of Contents (press Home)
6) Now try navigating the Contents using the up and down arrow keys.
(for example, press down arrow for 5 times, then press up arrow).
You will notice that when you press up arrow key the selection
jumps to the top of window.
The problem is in the help_handle_key() function:
case KEY_UP:
case ALT ('\t'):
/* select previous link */
new_item = select_prev_link (startpoint, selected_item);
The 'startpoint' variable should be a pointer to the first byte
displayed in the Help window. But here it has a wrong value - that's
the problem. That's why select_prev_link() cannot find the link and
returns NULL, and the selection moves to the first link in the window.
I tried to find out what's wrong with the 'startpoint' variable.
I came to the conclusion that 'startpoint' is used here erroneously
instead of 'currentpoint' variable.
'currentpoint' always contains a pointer to the first byte displayed,
and it should be used here. And by the way, 'startpoint' variable
seems to be totally useless.
So in my patch I replaced 'startpoint' with 'currentpoint' and
removed the 'startpoint' variable completely.
Regards,
Grigory Trenin
--- help.c.orig 2006-11-29 18:06:27.0 +0300
+++ help.c 2006-11-29 18:09:20.0 +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 */
@@ -489,7 +489,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 +527,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 +561,7 @@
if (p == NULL)
return;
-currentpoint = startpoint = p + 1;
+currentpoint = p + 1;
selected_item = NULL;
help_callback (h, DLG_DRAW, 0);
}
@@ -582,7 +582,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 +595,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 +672,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,7 +704,7 @@
case KEY_UP:
case ALT ('\t'):
/* select previous link */
- new_item = select_prev_link (startpoint, selected_item);
+ new_item = select_prev_link (currentpoint, selected_item);
selected_item = new_item;
if (selected_item currentpoint || selected_item = last_shown){
if (c == KEY_UP)
@@ -835,7 +835,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
Re: [PATCH] Help Viewer - incorrect behaviour of up arrrow key
s/right arrow/left arrow/g Of course I meant left arrow key when I was talking about going back to the previous topic. Sorry for that. -- Grigory. ___ Mc-devel mailing list http://mail.gnome.org/mailman/listinfo/mc-devel
