Reputation: 1602
By specification i build an application that monitors certain resource. Now i have a main window that when an event occurs throws an alert that specification must be a big window with bright colors. I gave solution to the problem as follows
private void StatusChanges(Alarm m, EventArgs e)
{
//Compares two 32-bit signed integers for equality and, if they are equal, replaces one of the values.
if (Interlocked.CompareExchange(ref running, 1, 0) == 0)
{
newWindowThread = new Thread(new ThreadStart(() =>
{
try
{
// Create and show the Window
Alert tempWindow = new Alert();
tempWindow.Show();
//if (cancelRun)
// tempWindow.Close();
// Start the Dispatcher Processing
System.Windows.Threading.Dispatcher.Run();
}
catch (Exception)
{
throw;
}
finally
{
running = 0;
}
}));
// Set the apartment state
newWindowThread.SetApartmentState(ApartmentState.STA);
// Make the thread a background thread
newWindowThread.IsBackground = true;
newWindowThread.Start();
}
}
else
{
if (Interlocked.CompareExchange(ref running, 0, 1) == 1)
{
try
{
System.Windows.Threading.Dispatcher.FromThread(newWindowThread).InvokeShutdown();
Alert tempWindow = new Alert();
tempWindow.Close();
}
catch (Exception)
{
//throw;
MessageBox.Show("Todo esta ok");
}
}
//running = 0;
}
}
This method is executed from at interval by a Timer
my goal was when the alarm will end the process of removing the alert window, I used Thread.Abort (); did not work, and sometimes the program threw an exception and showed no visual studio in aplition happening outside, try using threa.Join ();
but it didn't work, until
System.Windows.Threading.Dispatcher.FromThread use (newWindowThread) InvokeShutdown ().;
this at least stopped the showing up of the window, but when I create the
Alert tempWindow = new Alert();
in the initialize constructor
SoundPlayer player = new Sound ("../../Sounds/BOMB_SIREN-BOMB_SIREN-247265934.wav");
player.PlayLooping();
and when I close the window
player.Stop();
Now if not but I do the next
Alert tempWindow = new Alert();
tempWindow.Close();
the sound continues to play
I know that's not the best solution, but it works. I would like to give me your opinion, and give me suggestions on how to improve the code.
Upvotes: 2
Views: 6033
Reputation: 17402
Taking a quick look at your code, what you're doing is closing the temp. window incorrectly. You are creating a brand new Window when you want to close the existing one. This should be updated so that the existing temp Window is saved so that that the same instance can be closed later.
I've also updated the Closing of the temp Window so that it's thread shutdown's after it has closed.
I updated the code as follows:
Alert _tempWindow;
private void StatusChanges(Alarm m, EventArgs e)
{
//Compares two 32-bit signed integers for equality and, if they are equal, replaces one of the values.
if (Interlocked.CompareExchange(ref running, 1, 0) == 0)
{
newWindowThread = new Thread(new ThreadStart(() =>
{
try
{
// Create and show the Window
_tempWindow = new Alert();
_tempWindow.Close += OnTempClosed;
_tempWindow.Show();
System.Windows.Threading.Dispatcher.Run();
}
catch (Exception)
{
throw;
}
finally
{
running = 0;
}
}));
// Set the apartment state
newWindowThread.SetApartmentState(ApartmentState.STA);
// Make the thread a background thread
newWindowThread.IsBackground = true;
newWindowThread.Start();
}
}
else
{
if (Interlocked.CompareExchange(ref running, 0, 1) == 1)
{
try
{
_tempWindow.Dispatcher.BeginInvoke((Action)_tempWindow.Close);
}
catch (Exception)
{
//throw;
MessageBox.Show("Todo esta ok");
}
}
//running = 0;
}
}
private void OnTempClosed(object sender, EventArgs e)
{
System.Windows.Threading.Dispatcher.FromThread(newWindowThread).InvokeShutdown();
}
Upvotes: 3
Reputation: 63772
The idea is to work with the UI just from the one thread. So rather than creating a whole new dispatcher, why not simply dispatch the new window logic to the existing UI thread (after all, doesn't the timer already end-up on the UI thread all by itself?).
Why are you starting the new window in a new thread? What are you hoping to accomplish by that?
Your close code doesn't make sense:
Dispatcher.FromThread(newWindowThread).InvokeShutdown();
Alert tempWindow = new Alert();
tempWindow.Close();
Let's walk through this. First, you shutdown the dispatcher of the second UI thread. This means that all the messages that would flow to the window - don't.
Then, you create a new window, on the original UI thread, and you close that. What's that supposed to accomplish? You want to close the window that you've opened before, not a new window!
In practice, you need to keep a reference to the original window you've opened - the reference to it's thread isn't really important. Now, if you really want to keep using your two-UI-threads approach, you'd do this instead:
alertWindow.Dispatcher.Invoke(alertWindow.Close);
This will tell the message loop on the (already opened) alert window to close the window, as soon as it can.
With that out of the way, there's just too many things wrong with your code. For one, you're using way too low level operations (Interlocked.CompareExchange
to maintain alert / no alert logic?). Two, your comments are completely useless - you're just saying what the next line does, literally - that's not helpful. Instead of
// Compares two 32-bit signed integers for equality and,
// if they are equal, replaces one of the values.
You want to use something descriptive of what you're trying to achieve, not how you're doing it:
// If we're the first ones to attempt to change the running flag,
// we'll create a new alert window.
Note how that comment also quite nicely shows that you're doing something very weird.
Three, you're needlessly creating threads (and even more so, UI threads - there should really only be one except for very specific circumstances). There's no reason whatsoever to create the new window in a new thread. Why are you doing that?
Four, your exception handling isn't very helpful either. For one, you can omit the catch
clause if all you're doing is rethrowing the exception anyway - you can just use try ... finally
alone. More importantly, the exception is going to be propagated to that new thread, not your old one - this will most likely cause your application to crash completely.
Upvotes: 2