Automatically refresh used tab when log file changes #1042
Conversation
Right now, this ClientLogFileWatcher both listens for file changes and reads the file on a tight schedule, in order to minimize the amount of time between the file being updated and Procurement seeing it. Using a FileSystemWatcher is, unfortunately, not sufficient, since this generally misses the updates PoE makes to the file. Currently the only log message that is searched for is for when the user changes areas. The file watcher then triggers an event, which can be listened for by other parts of Procurement.
When the ClientLogFileWatcher detects the log file has been updated and triggers an event, the StashViewModel will now listen for that event and trigger updating all used tabs.
Check that the config value "EnableTabRefreshOnLocationChanged" is true before adding the event listener for the config file changes, which performs the automatic tab refresh.
|
|
||
| namespace Procurement.Utility | ||
| { | ||
| public class ClientLogFileEventArgs : EventArgs |
There was a problem hiding this comment.
Move this class into a seperate file
| } | ||
| } | ||
|
|
||
| protected static System.IO.FileSystemWatcher FileWatcher |
There was a problem hiding this comment.
System.IO is in the includes and the fully qualified path shouldn't be specified, this goes for the rest of the file.
There was a problem hiding this comment.
Done. I have a bad habit of doing this.
|
|
||
| protected void Initialize() | ||
| { | ||
| if (!Settings.UserSettings.Keys.Contains("ClientLogFileLocation")) |
There was a problem hiding this comment.
Move this magic string into a constant and update usage online 84
|
|
||
| internal void Start() | ||
| { | ||
| if (!Settings.UserSettings.Keys.Contains("EnableClientLogFileMonitoring")) |
There was a problem hiding this comment.
Same comment, move key to a constant
|
|
||
| PollingTimer = new System.Timers.Timer(); | ||
| PollingTimer.Elapsed += (s, e) => { ReadClientLogFile(); }; | ||
| PollingTimer.Interval = 30000; // 30 seconds |
There was a problem hiding this comment.
Magic int. Constant needed.
| if (string.IsNullOrWhiteSpace(fullFilePath)) | ||
| return; | ||
|
|
||
| FileWatcher = new System.IO.FileSystemWatcher(); |
There was a problem hiding this comment.
I appreciate that all this code runs "Once" but just incase, null check the setup of both the FileWatcher here and the polling timer. If it does manage to hit the initialize method again it will leak the event handlers you are assigning.
| PollingTimer.Start(); | ||
| } | ||
|
|
||
| internal void Stop() |
There was a problem hiding this comment.
Unsubscribe your event handlers to prevent memory leaks,
There was a problem hiding this comment.
For which event? PollingTimer.Elapsed? That object's lifetime is the same as the ConfigLogFileWatcher, so, as I understand it, they will both be destructed together, avoiding any memory leaks.
| FileWatcher.Changed += OnFileChanged; | ||
|
|
||
| PollingTimer = new System.Timers.Timer(); | ||
| PollingTimer.Elapsed += (s, e) => { ReadClientLogFile(); }; |
There was a problem hiding this comment.
Having an anonymous signature like this means you will be unable to unsubscribe from the event later on.
There was a problem hiding this comment.
As per my other comment, I think this is ok, since the subscriber and publisher's lifetimes are the same, and does not need to be unsubscribed. Please correct me if I am wrong.
| @@ -185,6 +185,8 @@ public void Login(bool offline) | |||
|
|
|||
| ApplicationState.SetDefaults(); | |||
|
|
|||
| ClientLogFileWatcher.Instance.Start(); | |||
There was a problem hiding this comment.
You have called start, but I can't see where you are calling stop. Is there an appropriate place to call stop?
There was a problem hiding this comment.
Since this is being called when logging in, and we don't have a concept of logging out (that would be nice), I don't know of a good place to call Stop.
| @@ -153,6 +179,16 @@ public StashViewModel(StashView stashView) | |||
| currencyDistributionUsesCount = true; | |||
| else | |||
| configuredOrbType = (OrbType)Enum.Parse(typeof(OrbType), currencyDistributionMetric); | |||
|
|
|||
| if (Settings.UserSettings.Keys.Contains("EnableTabRefreshOnLocationChanged")) | |||
thailyn
force-pushed
the
log-file-monitor
branch
from
8355e09 to
c942d6f
Compare
This PR accomplishes two things:
Both of these behaviors are disabled by default. The former is disabled when the config value
ClientLogFileLocation, which should point to the log file, is missing or empty; or if the config valueEnableClientLogFileMonitoringis missing or false. The latter is disabled when the config valueEnableTabRefreshOnLocationChangedis missing or false.