Re: Proposal: Adding a class to automatically manage the wait cursor and OK button enabled status

OK I get that we can't use the helper class in methods that run on a separate thread, because only the thread creating the window and controls can be used to effectuate any change. 

I'm attaching a patch file that shows some questions I have about possible violations of the above rule.  Please don't commit this - its just a means of communicating possible problems.  I'm not sure that Win32 allows EnableWindow() on a different thread.  Setting Focus, etc is probably not allowed, because it causes a redraw, and the thing they are trying to avoid is two threads using the same GDI shared resource.  Messing with the cursor cross thread *probably* works because of the ref counting, but I don't know for sure. Creating a brand new window (dialog) and doing stuff in the thread is OK, since that does not violate the rule.  Making the new window a child of the window created on the other thread is also probably OK, since this works on a higher level handle.   It might make sense to leave some of the comments that indicate which methods are possibly invoked from a separate thread - since those methods are then not allowed any main window
 changes.

My experience has been with C# and WPF - there the rules are very clear and you get an exception right-away if you don't play by the rules.  With Win32, I think things are less clear, and things sometimes work, sometimes not.

One way around this is to pass the STA (creator thread) id to the worker thread.  Requesting changes to the UI could be done through a PostThreadMessage() [RegisterMessage], and the actual work would be done in the handler of the message running in the original thread.

If the AsyncScheduler had a feature / way to run an action (lambda) in the context of the original thread after the worker thread completes, this would solve some problems.  You could then disable UI in the original tread - do the action on the other thread and have the original thread re-enable the UI.

We could, of course also just not bother with all of this business, and remove the class I introduced and revert the code to what it was...

Stefan, I would be interested in your input on this.

F.

________________________________
 From: Stefan Küng <tortoisesvn_at_gmail.com>
To: dev_at_tortoisesvn.tigris.org
Sent: Friday, May 31, 2013 11:42:54 AM
Subject: Re: Proposal: Adding a class to automatically manage the wait cursor and OK button enabled status
 

On 31.05.2013 05:09, Friedrich Brunzema wrote:
> I have committed the class, and will now remove the duplication.
> Stefan, could you do a review?  I have some comments on possible code
> removals, but don't know if they are needed for some other reason.  I
> know some of the code is process re-entrant, don't know how it
> behaves in that case.
>
> If this class gets used in a nested way, I think the hourglass cursor
> will still stay (as its reference counted), after the inner object
> goes out of scope - it then also re-enables OK.  As the out object
> goes out of scope, the hourglass go away, and the Ok which is already
> enabled gets re-enabled, which is fine.

There's a problem now with this implementation:
You're enabling the OK button when this helper class goes out of scope.
That seems ok on first sight, but it doesn't work correctly when the
command is executed on another thread.

Right now, you're creating the helper class before the commands are
executed. It goes out of scope and reenables the OK button as soon as
the command is done. But the commands that start another thread will be
'done' as soon as the thread is created, not when the actual command is
finished (it runs on a different thread and takes much much longer to
finish). So while the real command is still running, the OK button is
already enabled again.

You've noticed that there's something wrong already: you've marked those
places with the comment 'necessary?  (running in a separate thread)'.
Yes: that's necessary. But in these situations you must create the
helper class on that thread or not use it at all. The way it is now is
wrong.

Stefan

-- 
        ___
   oo  // \\      "De Chelonian Mobile"
  (_,\/ \_/ \     TortoiseSVN
    \ \_/_\_/>    The coolest interface to (Sub)version control
    /_/   \_\    http://tortoisesvn.net
------------------------------------------------------
http://tortoisesvn.tigris.org/ds/viewMessage.do?dsForumId=757&dsMessageId=3056808
To unsubscribe from this discussion, e-mail: [dev-unsubscribe_at_tortoisesvn.tigris.org].
------------------------------------------------------
http://tortoisesvn.tigris.org/ds/viewMessage.do?dsForumId=757&dsMessageId=3056816
To unsubscribe from this discussion, e-mail: [dev-unsubscribe_at_tortoisesvn.tigris.org].