Visual Studio support #4
Conversation
|
Just tested this and that solves the issue with Visual Studio for me too |
|
Thanks for testing @Mrxx99 ! |
|
I apologize for the slight delay in my response. Unfortunately, hardcoding any file extensions is definitely not the route I would like to take here. First of all, file extensions don't matter; they are merely a hint for the user, not a hard rule for the compiler. Thus, they are always subject to change. For example, not too long ago, Avalonia's convention used just a regular ".xaml" extension before proposing a more distinct (once again, only from the user's perspective) "Avalonia XAML" (".axaml") file extension for newer projects. So, this PR as it stands breaks compatibility with older projects that still use the ".xaml" convention. Secondly, I'm very open to implementing support for assets hot reloading. And assets can have any file extension in the world (even something like ".mzo~") or no file extension at all. All in all, what I want to emphasize is that relying on file extensions in the development world is not the best idea, especially in a project like this one.
So, this is the pattern we need to detect, and this is the only way to make it right. I'm going to rework this PR a bit during this weekend. Thank you for bringing the issue to my attention and making a fair attempt to fix it yourself; it's always very much appreciated! :) |
|
Wouldn't an easy improvement be to instead of filter in by file extension, to filter out files that match ".*~\w+.TMP"? |
|
Hi @Kir-Antipov , I like your package, but I have 2 questions
|
Yes and no. Once again, this may cause problems down the line when support for asset reloading is introduced, as I don't see a reason why somebody shouldn't be able to name their file This is not really a matter of how "difficult" it is to implement. It's a matter of me finally getting a chill day with my laptop all to myself. As you can probably tell by my git anti-streak, the last month or so has been pretty rough on me. |
Emulators are a completely different beast. You may want to open a separate issue to discuss solutions on that matter. In short, HotAvalonia extends your app with the ability to self-update its controls on certain filesystem events. This means that HotAvalonia is tooling-independent, so it can be used in any environment (well, at least when I fix this issue), but this comes at the price of the app needing to have access to the same filesystem that contains your project files. And as you may guess, there are some problems with that with remote debugging (debugging an app via an emulator is essentially a special case of remote debugging). However, I believe this can be overcome by mounting the project you are working with as a remote filesystem on a target device where the app is being debugged. So, for example, this project lives at this.EnableHotReload("/some/other/path/HotAvalonia/samples/HotReloadDemo/App.axaml.cs");If nothing else breaks, this should already work. So, give it a try ;) |
|
Yay, I'm finally back! Since I don't have access to Visual Studio, I used this shell script to simulate the process described by @toomasz: #!/bin/sh
# toggle-width.sh
SRC_NAME="MainWindow.axaml"
MZO_NAME="$(echo "${RANDOM}" | md5sum | head -c 8).mzo~"
TMP_NAME="${SRC_NAME}~RF$(echo "${RANDOM}" | md5sum | head -c 8).TMP"
# + mgwudjxu.mzo~
cp "${SRC_NAME}" "${MZO_NAME}"
# ! mgwudjxu.mzo~
if grep -qF "Width=\"300\"" "${MZO_NAME}"; then
sed -i"" "s/Width=\"300\"/Width=\"600\"/" "${MZO_NAME}"
else
sed -i"" "s/Width=\"600\"/Width=\"300\"/" "${MZO_NAME}"
fi
# MainWindow.axaml -> MainWindow.axaml~RF44d4e140.TMP
mv "${SRC_NAME}" "${TMP_NAME}"
# mgwudjxu.mzo~ -> MainWindow.axaml
mv "${MZO_NAME}" "${SRC_NAME}"
# - MainWindow.axaml~RF44d4e140.TMP
rm "${TMP_NAME}"The implementation capable of handling this monstrosity ended up being relatively straightforward. As this entire endeavor is designed to ensure your file won't be corrupted during the write operation (for example, in the event of a power outage), this means that the original file is guaranteed to be deleted only after its edited copy has successfully replaced it. Therefore, when we see a file being deleted, we check if it had been recently moved, and if so, we verify if something now exists at its original location. If this is the case, instead of issuing a "delete" event, we generate a synthetic "move" event. This lets our event subscribers know they should refer back to the file's original location. We then emit an additional "change" event at the original site. Everything seems to work seamlessly. Please let me know if the current implementation is now compatible with Visual Studio! :) |
Kir-Antipov
changed the title
|
@Kir-Antipov this fix sadly doesn't work for me, when I change file in VS, it still does nothing |
|
@zeryk24, thank you for testing! Welp, that's annoying. Could you please try to debug and see what happens there in your case? Here's a list of possible problems:
Currently, all events are processed via this method: HotAvalonia/src/HotAvalonia/IO/FileWatcher.cs Line 182 in b36ec66 So, you can just slap a Debug.WriteLine($"[${DateTime.Now}] {args.ChangeType}: {args.FullPath} {(args is MovedEventArgs moved ? $"(from {moved.OldFullPath})" : "")}"); |
|
@Kir-Antipov I debuged it and the problem was the check for duplicate event in OnFileSystemEvent method. When the deleted event was called, that early return from that method caused that TryProcessComplexEvent wasn't called so the changes weren't propagated. I can't push changes here so I forked this repo and here is my fix: fix . With my change everything works but, you can let me know if I changed the code correctly. |
|
@zeryk24, thank you so much for debugging the issue. This helps and means a ton! Unfortunately, your fix effectively completely disables a check for duplicate events - its primary purpose was to prevent the processing of duplicates and subsequently passing them down the pipeline to the event subscribers. This is particularly important for Windows users targeted by this PR. On Windows, a single file operation may trigger up to 3+ duplicate change events due to certain quirks of NTFS and how the OS handles such matters internally. Consequently, each time someone saves a file, a corresponding control may be reloaded multiple times. While I'm not overly concerned about a performance hit, I am worried about the impact on memory. Every time we reload a control, Avalonia compiles a new method to populate its contents, while the previous one remains in memory. Therefore, there's already a memory leak. While it may not be noticeable during a short debugging session, accelerating that leak could become a problem much sooner. I'm glad that the core logic of my suggested fix works. Now, we just need to determine what's wrong with the duplicate event detection. It either wrongly identifies something as a duplicate, or the delete event is somehow fired twice - once before the substitution for the edited file is ready and once after. Could you please try one of these alternatives instead? - if (IsDuplicateEvent(args))
+ if (IsDuplicateEvent(args) && args.ChangeType != WatcherChangeTypes.Deleted)
return;or - if (IsDuplicateEvent(args))
+ if (IsDuplicateEvent(args) && args.ChangeType != WatcherChangeTypes.Renamed)
return;While these changes don't qualify as a proper fix, they can help us identify which event is not being handled correctly. |
I.e., disabled duplicate detection for events other than `Changed`.
|
@zeryk24, actually, I thought about this for a bit and decided to disable duplicate event detection for all events other than |
|
@Kir-Antipov Yes that works as well. The problem is that first delete event on that .TMP file is added to cache in (!IsWatchingFile(path)) check and early returned, so second time its already in the cache, but you need to trigger TryProcessComplexEvent, so you cant early return in IsDuplicateEvent check. Btw. just for information: I am not sure if that call to RemoveStale method needs to be in Count method, but it makes the code hard to debug, I had to comments this one out every time. |
Oh, right! I overlooked this event sequence in toomasz's report: And later, I completely forgot that this is indeed a thing! I suppose VS checks if a file with the random name it generated already exists by attempting to create it and then deleting it upon success?
Sadly, yes, it should be there because Once again, I immensely appreciate your help in debugging this issue! @toomasz, @Mrxx99, it would be nice to hear from you just to ensure that everything finally works as expected on your machines as well :) And, for my future self/everyone interested, here's the corrected version of the script I used earlier to simulate what VS does to apply changes to a file. With this, I could finally reproduce the issue on my laptop as well: #!/bin/sh
# samples/HotReloadDemo/Views/toggle-width.sh
# We need to simulate the following event sequence:
#
# File created: '\Views\mgwudjxu.mzo~'
# File changed: '\Views\mgwudjxu.mzo~'
# File changed: '\Views\mgwudjxu.mzo~'
# File created: '\Views\MainWindow.axaml~RF44d4e140.TMP'
# File deleted: '\Views\MainWindow.axaml~RF44d4e140.TMP'
# File moved: '\Views\MainWindow.axaml~RF44d4e140.TMP' -> '\Views\MainWindow.axaml~RF44d4e140.TMP'
# File moved: '\Views\MainWindow.axaml' -> '\Views\MainWindow.axaml~RF44d4e140.TMP'
# File moved: '\Views\mgwudjxu.mzo~' -> '\Views\MainWindow.axaml'
# File changed: '\Views\MainWindow.axaml~RF44d4e140.TMP'
# File changed: '\Views\MainWindow.axaml'
# File deleted: '\Views\MainWindow.axaml~RF44d4e140.TMP'
# File moved: '\Views\MainWindow.axaml~RF44d4e140.TMP' -> '\Views\MainWindow.axaml~RF44d4e140.TMP'
SRC_NAME="MainWindow.axaml"
MZO_NAME="$(echo "${RANDOM}" | md5sum | head -c 8).mzo~"
TMP_NAME="${SRC_NAME}~RF$(echo "${RANDOM}" | md5sum | head -c 8).TMP"
# Ensure that the '.mzo~' file exists, then
# copy the contents of '.axaml' to it, and finally,
# apply the changes to the '.mzo~' file.
touch "${MZO_NAME}"
cat "${SRC_NAME}" > "${MZO_NAME}"
if grep -qF "Width=\"300\"" "${MZO_NAME}"; then
sed -i"" "s/Width=\"300\"/Width=\"600\"/" "${MZO_NAME}"
else
sed -i"" "s/Width=\"600\"/Width=\"300\"/" "${MZO_NAME}"
fi
# Create and immediately delete the '.TMP' file.
# This step simulates a file existence check
# performed by Visual Studio, as far as I can tell.
touch "${TMP_NAME}"
rm "${TMP_NAME}"
# Move a deleted file? Huh?
# '.axaml' -> '.TMP'
# '.mzo~' -> '.axaml'
mv "${SRC_NAME}" "${TMP_NAME}"
mv "${MZO_NAME}" "${SRC_NAME}"
# Trigger 'Changed' events on '.TMP' and '.axaml'.
touch "${TMP_NAME}"
touch "${SRC_NAME}"
# Finally, delete the '.TMP'.
rm "${TMP_NAME}"
# Move a deleted file? Huh? |
|
@MinikPLayer, I see that you made a fork, where you mention that this PR "doesn't work right now." Have you actually tested 9b26cad? |
Yes, i've tried that and it doesn't work. My fork was for my personal project but i'm happy to help fix the issue here as well. I've tried to do some debugging. No logs show up when doing a change in ToDoListView.axaml, so i've added a new log at OnFileSystemEvent to log every IDE action. Here is my log: |
|
After some digging i've found something interesting. But i've also found a different, weird thing. Adding NotifyFilters.Attributes to the watcher Filters adds another ToDoListView.axaml change after it was renamed from the ToDoListView.axaml.tmp. (And because of that, it now works). FileWatcher.cs: So it looks like VS changes some attributes after saving the file. EDIT: Adding this flag in the master branch also works in VS. (at least for me) |
|
I just finally harassed my good friend, who dual-boots macOS and Windows, into giving me remote access to the latter and permission to temporarily waste 20 GB of his hard drive for a Visual Studio install. 9b26cad worked perfectly and reliably for me there. So, we need to understand what's going wrong in your case. The log you've posted depicts the exact sequence described by toomasz. The most important events are there: This should be enough to trigger
It shouldn't be. Could you please modify the block of code that searches for the previous path of the deleted file to something like this: + FileSystemEventArgs[] cachedArgs;
string? previousPath;
lock (_lock)
{
+ cachedArgs = _eventCache.ToArray();
previousPath = _eventCache
.OfType<MovedEventArgs>()
.FirstOrDefault(x => fileNameComparer.Equals(Path.GetFullPath(x.FullPath), path))?.OldFullPath;
}
+ LoggingHelper.Logger?.Log(this, "Cached event args: {CachedArgs}", "\n" + string.Join("\n", cachedArgs
+ .Select(x => $"{x.ChangeType}: {x.FullPath} {(x is MovedEventArgs moved ? $"(from {moved.OldFullPath})" : "")}")
+ ));
if (!File.Exists(previousPath))
return false;Then run the demo, change a file in Visual Studio, and post the log here? |
And it shouldn't be. We are not interested in this file.
Yes, this would work, but this is not a proper solution, because now the hot reload will be triggered whenever you access a control file (since simply reading from the file updates its last access date). I already described why I do not want to resort to such solutions somewhere above. |
|
@MinikPLayer, just a thought: perhaps it's somehow related to the fact that your project is located on the D: drive? I for the life of me cannot think of a single reason why this could be an issue, but I don't see anything else standing out in your event sequence. |
It is only run once (after ToDoListView.axaml deletion). Here is the log: |
That's genius, it looks like you're right! EDIT: |
|
Oh, wait, my bad. I don't know what happened to my attention span in the last few months, but I misread the first log you published. It's not the same as the one published by toomasz. In your case, the original file is not moved to |
Yeah, just asked that. Great, now we know why it doesn't work for you - the current solution relies on the event sequence generated by NTFS, usually used by Windows starting from the NT era. Give me a moment, I'll implement a solution for your case too :) |
There's no need to pass a `path` there, as it's already available via `args`.
|
@MinikPLayer, I believe it should be fixed now! I've used this script to replicate what happens on your ReFS drive: #!/bin/sh
# samples/HotReloadDemo/Views/toggle-width-refs.sh
# We need to simulate the following event sequence:
#
# File created: '\Views\mgwudjxu.mzo~'
# File changed: '\Views\mgwudjxu.mzo~'
# File created: '\Views\MainWindow.axaml~RF44d4e140.TMP'
# File deleted: '\Views\MainWindow.axaml'
# File changed: '\Views\MainWindow.axaml~RF44d4e140.TMP'
# File moved: '\Views\mgwudjxu.mzo~' -> '\Views\MainWindow.axaml'
# File deleted: '\Views\MainWindow.axaml~RF44d4e140.TMP'
SRC_NAME="MainWindow.axaml"
MZO_NAME="$(echo "${RANDOM}" | md5sum | head -c 8).mzo~"
TMP_NAME="${SRC_NAME}~RF$(echo "${RANDOM}" | md5sum | head -c 8).TMP"
# Ensure that the '.mzo~' file exists, then
# write the changes to the '.mzo~' file.
touch "${MZO_NAME}"
if grep -qF "Width=\"300\"" "${SRC_NAME}"; then
sed "s/Width=\"300\"/Width=\"600\"/" "${SRC_NAME}" > "${MZO_NAME}"
else
sed "s/Width=\"600\"/Width=\"300\"/" "${SRC_NAME}" > "${MZO_NAME}"
fi
# Create the '.TMP' file, delete the source file,
# write the original content to '.TMP'.
touch "${TMP_NAME}"
SRC_CONTENT="$(cat "${SRC_NAME}")"
rm "${SRC_NAME}"
echo "${SRC_CONTENT}" > "${TMP_NAME}"
# Move '.mzo~' to the original file's location.
mv "${MZO_NAME}" "${SRC_NAME}"
# Finally, delete the '.TMP'.
rm "${TMP_NAME}"You should now be able to use HotAvalonia regardless of which drive you choose to work on. Well, at least in theory. So, please, let me know if everything's fine (i.e., hot reload works on both your NTFS and ReFS drives) :) |
Awesome, really awesome work. Works like a charm now! |
That's great! Finally, I think we've covered all the most important edge cases, and the PR is ready to be merged.
Sadly, no, I don't have any means to accept donations at the moment. I've thought about setting up something like this because I maintain quite a few projects that have turned out to be interesting and/or useful for the communities I consider myself a part of, and somebody may be interested in supporting my work; but I haven't taken any action on it yet. However, the sentiment is very much appreciated! It's always so nice to hear that your work is important to somebody that much :) |
|
Many thanks to everyone involved! @zeryk24, @MinikPLayer, even though your commits haven't ended up as part of this PR, I've marked you as co-authors, because fixing the issue without your help and you debugging the code on your end would not have been possible! Hope that's fine with you :) |
|
@Kir-Antipov Thanks, but you didn't have to do that. I helped because I really like your package and I'll be using it a lot. I'm looking forward to new extensions (maybe an emulator?), where I might get more involved. Keep it up! |
Here is what happens when user saves file.axaml with some changes:
So the problem was that FileWatcher._files contained name like
\Views\MainWindow.axaml~RF44d4e140.TMPafter BTherefore I decided to watch only .axaml files. Not ideal solution but don't have better idea for now
Also added logging to let user know that change was observed.
Here is the full log of what happens without filtering files:
After my change it becomes:
19:31:57:885 [HotReload]File changed: '\Views\MainWindow.axaml', change type: 'Changed'(FileWatcher #45653674)Fixes #3