From: Martin Sevior (msevior@physics.unimelb.edu.au)
Date: Sun Aug 04 2002 - 08:34:10 EDT
On Sun, 2002-08-04 at 13:01, Dom Lachowicz wrote:
> Martin,
>
> Sorry to say, but this argument is BS. We already do have poor-man's
threads implemented via timers and idles, and all without the protection that
things like mutexes and conditionals provide. I'll be impressed if anyone can
tell me any singificant difference between having a thread that runs once
every 1/2 second and a timer which fires every 1/2 second.
The difference is that the in the timer-idle loop situation your 100%
gaurenteed to be able to finish your piece of code before returning to
the main loop. This is a huge difference.
Threads actually interupt the flow of code. Say we have a blinking
cursor, that fires while we're doing a recalculation of the document
length in the piece table, or we're handling a scroll event and in the
middle of blitting graphics to the screen, or we're doing a redraw
update. We can't redraw the cursor during any of these processes because
either document position is changing or the screen is changing
underneath you and used to be black might white or vice-versa.
Everything that is remotely useful has to be allowed to be finished. The
only time it's safe to actually run a thread based timer is if we're
idling in the main-loop. And we already have a mechanism to do that.
In both cases you're
running asynchronous bits of code which has the possibility to wreak havoc in
our codebase. The major difference is that Pat's and my implementation actually
protected certain bits of important data with a mutex. Sure, you do have some
*non-atomic* booleans in there, but well, they're not atomic and not used
consistently enough to protect all of our vital innards. The only real
consideration I see here might be how OS drawing primitives play with threads,
but even those can be easily circumvented since all we're doing is an XOR or
Drawline call.
As I said above, the docposition could be changing or the screen could
change at the location of the cursor.
>
> Further, these threads aren't poking around the text/ or fmt/ directories.
All they do is manage blinking events. They don't touch the fmt or text
directories. The code in the fmt directory simply tells a *non-threaded* object
to queue a blink request. That's all. The threaded worker class simply loops
waiting to service a blink request and goes back to sleep. To do this, it tells
the view to draw or erase the insertion point. We're doing this from within a
timer context anyway already.
>
> The current cursor stuff is downright nasty. There are hundreds of
mismatched enable and disable calls to the cursor and clearly something has
to be done about it. It's responsible for lots of inconsistencies in the code
(mostly regarding text selections) and cursor dirt. Our only salvation right
now is the fact that there are more disable calls than enable calls, so dirt
goes away via the benefit of whiting-out an already whited-out region. Having
an object whose sole responsibility is to manage things like blinking is a
good idea. Whether it gets implemented via a thread or a timer isn't a
valid argument as we could easily have it implemented in terms of either with
equivalent beneficial and detrimental results.
Not quite. Requiring threads in AbiWord is a significant extra
requirement on the hosting OS and a potential portability issue. We
don't even have a threads implementation for Windows. As I noted above
we gain nothing for a threads implementation of a blink.
>
> Sorry,
Don't be sorry Dom. It's a useful discussion. My main objection is the
use of threads for a case that doesn't need them. I think we don't need
to do anything as complex as Mike's full cursor class if we invent a
class that manages to do the right thing when hit with a clearCursor()
method. If the cursor class *always* knows the state of the cursor and
we make sure we do a clearCursor() method before any screen manipulation
I think we can come up with a better solution than the one we have now.
So I imagine we just do a clearCursor() upon entry to a screen
manipulation method, and no restore upon exit. The cursor class then
does the right thing.
I'm by no means wedded to my old code, I just want to make sure everyone
fully understands what they're getting into before they set out to redo
the cursor blinking code.
> Dom
>
> On Saturday, August 3, 2002, at 10:50 PM, Martin Sevior wrote:
>
> > On Sun, 2002-08-04 at 06:43, Patrick Lam wrote:
> >> On Sat, Aug 03, 2002 at 11:46:29AM +0100, Tomas Frydrych wrote:
> >>>
> >>> Martins doubts about my suggestion to move the cursor timer into a
> >>> separate thread made me go back and study the spell checking
> >>> problem bit more, and it turned out that the problem was not with the
> >>> timer per se being blocked, but elsewhere in
> >>> fl_BlockLayout::_doCheckWord, which I have now fixed.
> >>>
> >>> Enjoy orderly cursor blinking.
> >>
> >> That's cool. But I still think the cursor code really sucks, because
> >> it's really ad-hoc and not abstracted. I think it should be fixed in
> >> a more general way with threads.
> >>
> >
> > I'm strongly not in favour of threads unless it can directly
> > demonstrated that only threads can solve the problem at hand and even
> > then I'd be worried. I'm by no means convinced that threads will help
> > the cursor blinking problem, even with the nicely abstracted code from
> > Mike's cursor class. Looking at this code it is clear that this really
> > affects a lot of the code. I'd be really wary of implementing it unless
> > the person doing the job was fully committed to following through on it
> > and had no other major job with AbiWord.
> >
> > IMHO we'd be much better off fixing revision marks, hidden text,
> > Footnotes, tables and import/exporting thereof etc before tackling this.
> >
> > Pat, you know how hard it is debug abi code, imagine if you had to deal
> > with threads within the text/* directories.
> >
> > I think you could make a case for threads for the has downloaded as abi
> > is blocked until the dictionary is downloaded. In addition it a rather
> > isolated piece of code that does not affect the rest of the application.
> >
> > Cheers
> >
> > Martin
> >
> >
>
This archive was generated by hypermail 2.1.4 : Sun Aug 04 2002 - 20:48:57 EDT