Skip to content

Renamed SIL.Media to SIL.Windows.Forms.Media#1325

Draft
josephmyers wants to merge 2 commits intomasterfrom
sil.windows.forms.media
Draft

Renamed SIL.Media to SIL.Windows.Forms.Media#1325
josephmyers wants to merge 2 commits intomasterfrom
sil.windows.forms.media

Conversation

@josephmyers
Copy link
Collaborator

@josephmyers josephmyers commented Jun 4, 2024

This change is Reviewable

@josephmyers josephmyers linked an issue Jun 4, 2024 that may be closed by this pull request
8 tasks
@tombogle
Copy link
Contributor

tombogle commented Aug 1, 2024

I wonder if it might be best to split SIL.Media into a utility DLL and a Forms DLL, as we have with the other things. There are only a small handful of WinForms-specific things here.

@github-actions
Copy link

github-actions bot commented Aug 1, 2024

LibPalaso Tests

   17 files  ±0     17 suites  ±0   8m 33s ⏱️ +22s
4 883 tests ±0  4 652 ✅ ±0  231 💤 ±0  0 ❌ ±0 
4 902 runs  ±0  4 658 ✅ ±0  244 💤 ±0  0 ❌ ±0 

Results for commit 0b7a9af. ± Comparison against base commit db9df03.

@josephmyers
Copy link
Collaborator Author

If it's a small handful, would there be any value in absorbing it into an existing library? Is the benefit of a separate dll enough to justify its cost?

@tombogle
Copy link
Contributor

tombogle commented Aug 2, 2024

I don't see any logical fit anywhere else. The only possible place would be Core, but everything uses thats, so I think it would be less than ideal to put it there. Eventually, HearThis will probably become a non-WinForms app (or get a non-WinForms cousin), so breaking the functionality up really does make sense. I don't know enough about the architecture of Bloom, but it would be another possible candidate for using stuff in SIL.Media if it were not WinFoms dependent.

@josephmyers josephmyers removed a link to an issue Aug 15, 2024
8 tasks
@josephmyers
Copy link
Collaborator Author

I'm also fine leaving it as is. The only thing this PR does is make the name less misleading.

@andrew-polk
Copy link
Contributor

Bloom will need to have this teased apart eventually, so that is going to be helpful whenever it happens.
However, I think this rename is useful as a standalone change.
It needs a +semver:major in the commit message.
Also, unfortunately, this has languished long enough to have many merge conflicts.

@josephmyers
Copy link
Collaborator Author

Yes, and I've changed roles as well, so this unfortunately now falls outside my purview. It may be easier to start from scratch.

@imnasnainaec imnasnainaec marked this pull request as draft February 2, 2026 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants