Conversation
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
andrew-polk
left a comment
There was a problem hiding this comment.
Approving based solely on the diff link Kevin posted.
imnasnainaec
left a comment
There was a problem hiding this comment.
Reviewable status: 0 of 29 files reviewed, 2 unresolved discussions (waiting on @josephmyers)
a discussion (no related file):
CHANGELOG probably needs updating for this change.
SIL.Windows.Forms/ImageToolbox/AcquireImageControl.cs line 115 at r2 (raw file):
// cross-platform compatibility. It's not worth messing with DialogAdapters at // this point just to get this feature, which might not be cross-platform. if (!DoubleCheckFileFilter(dlg.Filter, dlg.FileName))
The comment above this line (and perhaps the logic it describes) needs to be updated now that DialogAdapters aren't being used.
|
@imnasnainaec done |
imnasnainaec
left a comment
There was a problem hiding this comment.
Reviewable status: 0 of 29 files reviewed, 2 unresolved discussions (waiting on @hahn-kev and @josephmyers)
a discussion (no related file):
Previously, imnasnainaec (D. Ror.) wrote…
CHANGELOG probably needs updating for this change.
Oh, #1358 had no mention in the CHANGELOG of the ImageToolbox removal, so I'm not sure how/whether to mention its restoration. Perhaps just a CHANGELOG entry for what was was changed from the prior ImageToolbox?
SIL.Windows.Forms/ImageToolbox/ImageToolboxButtons.Designer.cs line 86 at r3 (raw file):
/// Looks up a localized resource of type System.Drawing.Bitmap. /// </summary> internal static System.Drawing.Bitmap camera64x64 {
It looks like the three bitmap images camera64x64, credits, and scanner64x64 aren't used.
Previously, imnasnainaec (D. Ror.) wrote…
If no release version went out since the change (guessing not), then I think we only need to mention what changed. If there was an intervening release version, then the changelog should probably be retroactively modified to show both the removal and re-addition. |
|
You can see the releases here. Unfortunately, instead of giving an exact date, it says version 15.0 was released 3 months ago. I looked at the repo to see the actual date associated with the v15.0.0 tag. Since that was on January 7, the thing you're fixing never went out as a stable release. |
|
Alright, I've added a changelog entry recording that we removed linux support |
|
If there's someone who is actually acquainted with this code or works on a program affected by it, they would probably be best suited to doing the review. Otherwise, I can look at it. I guess I'd just do a diff with the previous versions of these files from early January to see what changed. |
@tombogle |
Also, you can hover over the "3 months ago" to get the exact datetime. v15.0 says Jan 7th, 18:43 Z |
hahn-kev
left a comment
There was a problem hiding this comment.
Dismissed @tombogle from a discussion.
Reviewable status: 0 of 30 files reviewed, all discussions resolved (waiting on @imnasnainaec and @josephmyers)
fixes problems in downstream dependents which expected this to be used. This is fixing an issue caused by #1358
Some changes from the original version:
if you just want to view the changes I made to the code as it was before Joseph removed it you can see this diff: https://github.com/sillsdev/libpalaso/pull/1421/files/baa8e3f335db0b70a96cd3b4154840596d8db430..7e0a2f65c3159c518bad8447da6d40dbfd7ba73c
This change is