NAppUpdate icon indicating copy to clipboard operation
NAppUpdate copied to clipboard

Redo feed builder fix

Open vbjay opened this issue 7 years ago • 12 comments

  • [x] upgrade framework version for feedbuilder
  • [x] make feedbuilder responsive
  • [x] load icons from os
  • [x] get human readable file sizes
  • [x] select listview items by typing regex pattern
  • [x] functionality and ui improvements Feed builder rework because of of merge conflicts.

vbjay avatar May 26 '17 22:05 vbjay

Ready to merge

vbjay avatar May 31 '17 05:05 vbjay

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:

  • 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.

robinwassen avatar May 31 '17 07:05 robinwassen

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 [email protected] 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 https://github.com/synhershko/NAppUpdate/pull/99#issuecomment-305106407, or mute the thread https://github.com/notifications/unsubscribe-auth/ADdhsX4D_m6nM5ayqDBq_vG9eKubrgE3ks5r_RRIgaJpZM4NoGaq .

vbjay avatar May 31 '17 10:05 vbjay

Let me know if there is anything else.

vbjay avatar Jun 01 '17 01:06 vbjay

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?

vbjay avatar Jun 06 '17 14:06 vbjay

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, Robin

robinwassen avatar Jun 07 '17 05:06 robinwassen

I have reviewed each commit up to d2a4fc97a4664aa3ae73159f6eb08fcb49dfcd8b this far.

robinwassen avatar Jun 07 '17 06:06 robinwassen

@vbjay Do not worry about the conflict.

Some questions I have:

  • What is the reason that you upgraded to .NET 4.6.2?
  • What is the purpose of including the RX .NET package?

robinwassen avatar Jun 07 '17 07:06 robinwassen

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 avatar Jun 07 '17 13:06 vbjay

@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.

robinwassen avatar Jun 07 '17 19:06 robinwassen

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: https://github.com/synhershko/NAppUpdate/pull/99/commits/af11eac3f7f2222295bc78b442a2911b655ae1b1?w=1

My modified: https://github.com/synhershko/NAppUpdate/commit/641e4f229d3e0dd3a89184ab2d14f26a8dca7a04?w=1

This can take a while, but I will get through it :)

robinwassen avatar Jun 07 '17 20:06 robinwassen

@robinwassen I appreciate the work you are doing. It means a lot.

vbjay avatar Jun 08 '17 00:06 vbjay