Re: [comment] fdo#65014 Fix backwards/forwards search change of direction.

Hi,

Thanks for the input -- I think I made a mess of that and still don't
entirely understand all the implications of what I'm doing yet.

I now have a much simpler 2 line fix where the cursor is normalised
based on search direction (which however only works for a single
selected region: for some reason the cursor Ring retrieved always points
to a single point rather than a region when multiple regions are
selected -- and there always seem to be two cursors in the ring no
matter how many regions are selected -- I'm still looking into how to
determine all the selected regions), however for the moment I'll abandon
the change and come back to it in 1.5 weeks when I've got time to fully
test it again.

(I'll probably add a check to make sure this is only used for normal
back/forward searching and is ignored for any special searches e.g.
within the selection, which should hopefully deal with any side-effects,
but I'll need to do some further testing to make sure.)

I'm not even certain it's worth doing anything for multiple selected
regions -- the main issue I wanted to fix was the bug when changing
direction of search (fdo#65014) in which case there is only a single
selected region -- with multiple selected regions it's arguably unclear
where the search should start from. Funnily enough I've just noticed
"Search in Selection" (in the search dialog) seems to be disabled when
there are multiple regions selected -- could possibly be related to the
cursor pointing to a single point in that case (on the other hand I've
seen that the method that counts the number of characters selected --
SwEditShell::CountWords, also uses the cursor ring and works correctly
-- still looking at how all that works).

On that topic: if I abandon a change on gerrit can I reactivate it using
logerrit submit again?

Cheers,

    Andrzej

On 28/05/13 12:50, Michael Stahl (via Code Review) wrote:
> Michael Stahl has posted comments on this change.
>
> Change subject: fdo#65014 Fix backwards/forwards search change of direction.
> ......................................................................
>
>
> Patch Set 2: This need some tweaks before it is merged
>
> (6 inline comments)
>
> haven't actually tried it out but i have a few complaints :)
>
> ....................................................
> File sw/source/ui/uiview/viewsrch.cxx
> Line 452:     SwPosition* pStart =( *pCrsr->GetPoint() < *pCrsr->GetMark() ) ?
> there is SwPaM::Start() method already for this
>
>
> Line 454:     SwPosition* pEnd = ( *pCrsr->GetPoint() < *pCrsr->GetMark() ) ?
> there is SwPaM::End() method already for this
>
>
> Line 456:     while ( ( pCrsr = ( SwPaM* ) pCrsr->GetNext() ) != pStartCrsr );
> please use C++ casts (static_cast/const_cast) instead of C casts
>
>
> Line 461:         pEnd = ( *pCrsr->GetMark() > *pEnd ) ? pCrsr->GetMark() : pEnd;
> what does this do?
> on the first iteration it will set pStart to pStart...
> on subsequent iterations it will set pStart to the earliest point/mark on any cursor in the 
> ring.... it's not obvous to me what good this does?
>
>
> ah wait /me reads commit message... it's intentional... but iirc there is an option to search in 
> the "selected" text, and i guess in that case changing the position in this way will give wrong 
> results.
>
>
> Line 463:     *pStartCrsr->GetPoint() = m_pSrchItem->GetBackward() ? *pStart : *pEnd;
> depending on whether GetMark() is pEnd or not this will result in having a selection or not... is 
> the problem the selection as such or would it be sufficient to re-order point and mark?
> there is a SwPaM::Normalize(bool) that will re-order and put either point or mark first depending 
> on parameter - perhaps that's sufficient to fix the problem?
>
>
> Line 464: 
> also i'm not entirely sure if it's a good idea to modify the shell cursor directly from outside - 
> there also is a SwCrsrShell::NormalizePam method, i wonder if calling that would be sufficient 
> here?  it wraps some weird SwCallLink around it, not sure what that does...