-
Notifications
You must be signed in to change notification settings - Fork 163
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Redo feed builder fix #99
base: master
Are you sure you want to change the base?
Conversation
a080ce6
to
e9e54ae
Compare
Ready to merge |
Hi @vbjay! I appreciate the changes you have done, but it seems like you managed to revert some of the later fixes to the project. For example:
I will go through the changes and see how we can perform the merge gracefully. |
I'll look. I tried really hard not to do that. I'll go through the diff
again.
…On Wed, May 31, 2017, 3:18 AM Robin Andersson ***@***.***> wrote:
Hi @vbjay <https://github.com/vbjay>!
I appreciate the changes you have done, but it seems like you managed to
revert some of the later fixes to the project.
*For example:*
- Removing FileLockWait(); in FileUpdateTask.cs and adding the
commented wait logic
- Removing FileSystem.CopyAccessControl(...) in FileUpdateTask.cs
I will go through the changes and see how we can perform the merge
gracefully.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#99 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADdhsX4D_m6nM5ayqDBq_vG9eKubrgE3ks5r_RRIgaJpZM4NoGaq>
.
|
Let me know if there is anything else. |
I understand other things taking your time. I had to redo this work because it was ignored the first time. Am i wasting my time with trying to give back to this project? |
Hi @vbjay, It is my ambition to merge this - but it will take some time since I am usually very thorough when reviewing changes and want to understand each line that is changed to determine the impact of the change. Which makes it take some time when the change is on 2500 LOC. Regards, |
I have reviewed each commit up to d2a4fc9 this far. |
FeedBuilder/frmMain.cs
Outdated
@@ -81,18 +83,9 @@ private void frmMain_Load(Object sender, EventArgs e) | |||
if (!_argParser.ShowGui) Close(); | |||
} | |||
|
|||
private void InitializeFormSettings() | |||
private async Task InitializeFormSettings() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Marking the method as async does not make it run async per default.
All places that calls InitializeFormSettings() also need to be changed.
Example:
public class AsyncTestClass
{
// Output:
// Initialize() done
// Run() done
public void Run()
{
Initialize();
Console.WriteLine("Run() done");
}
// Output:
// Run() done
// Initialize() done
public void RunAsync()
{
Task.Run(Initialize);
Console.WriteLine("Run() done");
}
private async Task Initialize()
{
Thread.Sleep(2000);
Console.WriteLine("Initialize() done");
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that InitializeFormSettings() is called with the await keyword in a later commit, don't understand why it is made async if it is only called sync
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll look again. I knew it was sync in the load of the program. But notice it's different when you load a file. I knew that in load we want it to be sync.
@vbjay Do not worry about the conflict. Some questions I have:
|
Look at the next commit for why I added Rx. I upgraded to 4.62 because I wanted framework 4.0 stuff. It also runs on windows 7sp1 and higher. Windows XP support should not be a factor. Microsoft dumped their support for xp in 2014. We should too. O |
@vbjay Sorry, my mistake about the upgrade. Thought it was from 4.5 -> 4.6.2 which left me puzzling, but 3.5 -> 4.x makes more sense! Thanks! Everything is a bit more clear now, so I can keep on working with the review and merge. |
Most of the refactors and smaller changes has been merged now. I noticed some more collateral damage in the commits, so I am going through commit per commit and cherry-pick them to the branch feedbuilder-improvements and review all changes in detail. One comparision is the original commit: af11eac?w=1 My modified: 641e4f2?w=1 This can take a while, but I will get through it :) |
@robinwassen I appreciate the work you are doing. It means a lot. |
Feed builder rework because of of merge conflicts.