-
-
Notifications
You must be signed in to change notification settings - Fork 50
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
Renamed project to SIL.Windows.Forms.Archiving #1317
Conversation
To align with all other WinForms projects
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.
Reviewed 1 of 68 files at r1, all commit messages.
Reviewable status: 1 of 68 files reviewed, 1 unresolved discussion (waiting on @josephmyers)
Palaso.sln
line 30 at r1 (raw file):
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "SIL.Windows.Forms.Archiving", "SIL.Windows.Forms.Archiving\SIL.Windows.Forms.Archiving.csproj", "{BCE1F124-5479-4B23-90B1-B7A4EBE44FA3}" EndProject Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "SIL.Windows.Forms.Archiving.Tests", "SIL.Windows.Forms.Archiving.Tests\SIL.Windows.Forms.Archiving.Tests.csproj", "{892C7F20-FBBB-4AB3-BAC2-E40A135567B6}"
Now I'm wondering if we should actually be trying to split this stuff into Forms vs. non-Forms DLLs. None of the unit tests depend on WinForms, so all the classes they are testing could be in a non-Forms dependent DLL. I'm not sure if they do anything useful enough on their own to warrant this though.
Previously, tombogle (Tom Bogle) wrote…
Looking at the code, the ArchivingDlg classes (abstract base and two concrete implementations) are the only things that really need to depend on WinForms. The models do, but only minimally and in a way they really shouldn't. If we were to fix those design issues, then those models could be used to back a non WinForms implementation. |
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.
The changelog should not be included in the find/replace and there should also be a line for this rename in the changelog. Thanks!
+semver:major
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.
Looks good, thanks @josephmyers
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.
Reviewed 67 of 68 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @josephmyers)
@tombogle, any chance we'll see progress here this week or next? |
Not sure when I'll be able to get to this. |
I discovered today that SIL.Media is a WinForms project. So a bit more to come on this |
@josephmyers we can rename SIL.Media in a separate PR. |
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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @josephmyers)
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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @josephmyers and @tombogle)
Palaso.sln
line 30 at r1 (raw file):
Previously, tombogle (Tom Bogle) wrote…
Looking at the code, the ArchivingDlg classes (abstract base and two concrete implementations) are the only things that really need to depend on WinForms. The models do, but only minimally and in a way they really shouldn't. If we were to fix those design issues, then those models could be used to back a non WinForms implementation.
@tombogle how about we just leave this PR as a simple rename and if you'd like to circle back on a refactor that you can do that in a separate PR? I'd like to get this merged.
|
I'm doing some fixing in this area currently, so this might actually be a good time for me to try to break it into two parts. Give me a couple days, and if I can't, then we'll go ahead and merge as is. |
This helps clarify some unexpected behavior in SayMore tests.
# Conflicts: # CHANGELOG.md
…SIL.Core Added optional parameter to OlacSystem.GetRoles to allow caller to provide its own XML
155148d
to
b3b018a
Compare
…ndows.Forms.Archiving Included significant refactoring and replacing clunky cancellation of background process (which required calls to Application.DoEvents) with a proper async approach.
# Conflicts: # SIL.Media.Tests/SIL.Media.Tests.csproj
LibPalaso Tests 17 files ±0 17 suites ±0 9m 34s ⏱️ - 2m 50s Results for commit 67b1ecd. ± Comparison against base commit d85edaa. This pull request removes 54 and adds 60 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
…ves. Improved documentation and did code cleanup for some of the IMDI stuff. Closed some loopholes to make it easier for clients to create valid packages
I have completed my work on splitting this into two DLLs. Ready for review. |
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.
there's some issues with how async works at the moment, but it looks good otherwise.
Previously, megahirt (Christopher Hirt) wrote…
No longer relevant |
What sort of issues? |
Never mind. I see there are some github comments that Reviewable knows nothing about. |
… TargetFrameworks property
Changed OlacSystem.GetRoles to report a non-fatal exception if the custom XML file cannot be loaded. Removed ArchivingPackage and AddSession from ArchivingDlgViewModel and RampArchivingDlgViewModel
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.
Looks good, I agree _imdiData.CreateIMDIPackage()
in IMDI view model should also be awaited, nice catch.
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.
Looks good to me. I'll leave it up to you @tombogle how you handle the conversation on IMDIArchivingDlgViewModel.cs
995b464
to
8874e89
Compare
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.
Reviewed 2 of 73 files at r1, 102 of 119 files at r3, 7 of 22 files at r4, 5 of 6 files at r5, 4 of 8 files at r6, 1 of 1 files at r7, 2 of 2 files at r8, 4 of 4 files at r11, 2 of 2 files at r12, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @josephmyers and @megahirt)
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.
Reviewed all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @hahn-kev, @josephmyers, and @megahirt)
SIL.Archiving/IMDI/IMDIArchivingDlgViewModel.cs
line 189 at r7 (raw file):
Previously, tombogle (Tom Bogle) wrote…
In reality, most of this stuff is pretty fast on modern hardware, so unless you have a lot of files or some really big files, it's probably going to finish before you've had much of a chance to think about canceling.
I went ahead and made CreateIMDIPackage take a cancellation token and return Task, so now it checks the cancellation state before each IO call. Technically it's still synchronous
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.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @hahn-kev and @josephmyers)
This change is