Re: [PATCH] fdo#38837: Timers must end eventually [Writer idle timer]

2012/12/5 Caolán McNamara:
> 1. In e.g. SetNeedGrammarCheck and SetFieldsDirty shouldn't
> StartBackgroundJobs only be called for the "true" case and not the
> "false" case ?

1. Yes, that's right.


>  if ( !GetUpdtFlds().IsInUpdateFlds() &&
>       !IsExpFldsLocked() )

2. I planned to negate. That should be:

if ( GetUpdtFlds().IsInUpdateFlds() ||
     IsExpFldsLocked() )


3. IDocumentTimerAccess's patched class-level documentation reads:
    It starts with a mode of 'stopped'
That's wrong, it should be:
    It starts with a mode of 'start'


--
pkoroau

2012/12/5 Caolán McNamara <caolanm@redhat.com>:
> On Wed, 2012-11-28 at 23:25 +0100, pkoroau pkoroau wrote:
> > Hello, I'm sending this patch to fix Writer's aIdleTimer.
>
> This looks promising. But a couple of things.
>
> 1. In e.g. SetNeedGrammarCheck and SetFieldsDirty shouldn't
> StartBackgroundJobs only be called for the "true" case and not the
> "false" case ?
>
> 2. the...
>
>  if ( !GetUpdtFlds().IsInUpdateFlds() &&
>       !IsExpFldsLocked() )
>  {
>      pTimer->Start();
>      return 0;
>  }
>
> condition to me looks that it will restart the timer if we not currently
> updating-fields but the updating-fields flag is set after that condition
> and the updating fields works done there. So it suggests that fields
> will never be actually updated and the timer run again and again.
>
> Maybe that should be...
>
> if (GetUpdtFlds().IsInUpdateFlds())
> {
>     //Already updating fields, try again later
>     pTimer->Start();
>     return 0;
> }
>
> ?
>
> C.