Issue 72394

Created attachment 41248 [details]
testcase

MRU->FME: see attached document; there's hidden text at the end, thus Ctrl-A
won't work - it will just transfer the cursor to begin of document.

lijian->fme:
It seems that this issue is regarding cursor and SwPam. I am interested in 
this issue. What do you think about this ? Is this issue OK for me ?
I need your suggestion.

assign to lijian

lijian->fme:
I found this issue related to the options in tools.
Tools-Options-Writer-Formatting Aids, if we select "Hidden Text",
this issue will not exist.
So, we should get the value of checkbox "Hidden Text" to see if we have to 
move the cursor skipping hidden text, I think. Though I havn't found a way to 
get this value. 
What do think about my thought ？

fme->lijian: Did you already find the code that skips the hidden paragraph? It's
located inside SwCrsr::IsSelOvr(). There's some code that checks if the point of
the current cursor is located inside a hidden frame (which in this case is
recognized by checking if the frame height = 0). If so, the point is moved to
the next paragraph. In our case there is no next paragraph, so the cursor
position is considered invalid. Maybe we should also try to move to the previous
paragraph...

Created attachment 49783 [details]
patch file

lijian->fme:
I wonder what pSavePos is used for and where its value 
gets from in SwCursor::IsSelOvr.
In my patch, I just changed the value of bGoNxt. Then 
all seems to work fine. I just take this as a test.
Actually, I think it should be like this :
if bGoNxt == 1 and pFrm is hidden, we could see if 
paragraph pFrm represents is the last paragraph. If so,
we need to move forward.
It's similar to bGoNxt == 0.
What's your opinion ?

fme->lijian: Thank you for your patch. Please find my comments below.

[...] I wonder what pSavePos is used for and where its value gets from in
SwCursor::IsSelOvr. [...]

pSavePos contains the last known 'save' position the cursor was set to. Usually
it works like this: before trying to move the cursor, an SwCrsrSaveState object
is created with the current cursor. Since ths current cursor is assumed to be on
a valid position, the SwCrsrSaveState object stored the last known valid cursor
position. Then the cursor is moved to a new position. After this, some checks
are executed, if the cursor is now 'valid', e.g., in IsSelOvr(). If the cursor
is found to be on an invalid position and the cursor either should not be
corrected (!SELOVR_CHANGEPOS) or could not be corrected (like in our case), the
SwCrsrSaveState object can be used to 'undo' the cursor move that resulted in
this 'invalid' cursor position.

[...] In my patch, I just changed the value of bGoNxt. Then all seems to work
fine. I just take this as a test. Actually, I think it should be like this :

if bGoNxt == 1 and pFrm is hidden, we could see if paragraph pFrm represents is
the last paragraph. If so, we need to move forward. It's similar to bGoNxt == 0.
What's your opinion ? [...]

Yes, I fully agree with you. bGoNxt == true indicates, that the cursor has been
moved forward, i.e., the last 'save' position is < than the position after the
cursor move. In this case the code tries to search forward for a suitable cursor
position. If bGoNxt == false, the cursor has been moved backward, i.e., the last
'save' position > thant the current position. In this case the code searches
backward for a suitable cursor position. We should keep this logic, and maybe
should check, if after the while loop pFrm == 0. This means that for bGoNxt ==
true (resp. false), we reached the end (resp. start) of the text without finding
a suitable paragraph. In this case we should restart the search with bGoNxt =
!bGoNxt. Hope this helps.

Created attachment 49803 [details]
patch file

lijian->fme:
I have made a new patch according to your helpful explanations which are 
really very clear and detailed. Thank you very much.
Please review this patch.

Created attachment 49809 [details]
new patch file

lijian->fme:
Maybe this patch is OK .

fme->lijian: Some more comments:

1. I just noticed that your while loop does not work if there are more than one
hidden paragraphs at the end. You should work with pFrm inside the new while
loop instead of pNd. 

2. Let's have a look at bGoNxt. A couple of lines below, bGoNxt is also used.
The meaning of bGoNxt in this context is that if the node postion of the cursor
has been corrected by going forward, the content member of the position is set
to the beginning of the new paragraph. If the cursor has been corrected by going
backward, the content member of the position is set to the end of the new
paragraph. So what should happen if we add our new code? Let's have a look at
this example:

Paragraph1
Paragraph2
hidden paragraph3
hidden paragraph4
Paragraph5

Let's assume that the cursor is moved from paragraph2 to paragraph3. In this
case bGoNxt=true. So we try to find a suitable paragraph after paragraph3 to set
the cursor to. The while loop sets pFrm to paragraph4 and finds that it is not
suitable. So the while loop advances to paragraph5 and breaks. The following
code sets the content member to the cursor to position 0 because the cursor
position was corrected to a position after the original position.

So here's an other example:

Paragraph1
Paragraph2
hidden paragraph3
hidden paragraph4

Let's assume that the cursor is moved from paragraph1 to paragraph3. In this
case bGoNxt=true. Since paragraph3 is hidden, we try to find a suitable
paragraph after paragraph3 to set the cursor to. The while loop sets pFrm to
paragraph4 and finds that it is not suitable. But paragraph4 is the last
paragraph, so the while loop breaks. Here we have to add our new code. We see
that pFrm == 0. We start over with pFrm = pNd->GetFrm(). Now we want to search
backward. We need have a while loop that does the same like the first while
loop, the only difference is that it has to search backward if the first loop
searched forward and vice versa. So our new code sets pFrm to paragraph3 and
searched backward. Because paragraph2 is fine, the new while loop breaks. What
do we have to do with the nContent member to the cursor? Since the cursor
position has been corrected by going backward, we have to set nContent to
pCnd->GetLen(). So we see that it is correct to set bGoNxt = !bGoNxt in case the
first while loop did not succeed.

Created attachment 49890 [details]
patch file

lijian->fme:
regarding 1: I have submitted a new patch. Please review it.

regarding 2: Thank you for your so clear explanations. I think 
I have understood this very clearly.  

fme->lijian: Cool, that's it, works fine. I'll add your fix to cws sw8u10bf02. 

fme: Fixed by lijian in swcrsr.cxx rev. 1.54.94.1

.

Ready for QA.

Reopening.

MRU->FME: the implementation implies a new crash. The non-printing characters
must be deactivated. Insert dummy text, select all, format as "hidden", open
format.character dialog again -> crash

fme->mru: You are right. We forgot to check this case. Now corrected.

.

Verified in CWS sw8u10bf02.

Checked in OO 2.4 RC6 and DEV300m3.