Simulate a FileOk handler when choosing an image file #1335
Conversation
StephenMcConnel
force-pushed
the
BL-13552-EnforceFileFiltering
branch
from
cc4c4f8
to
5f048f8
Compare
There was a problem hiding this comment.
Thinking through the implications of semantic versioning here....
in one sense, this could be considered a breaking change if any client were expecting to allow the loophole of pasting whatever file you want. But given that the library won't do anything sensible (I assume) with other file types, I think we can probably rule that out.
I think I'm ok with considering this a bug fix.
This is a good compromise, btw. Good thinking!
Reviewed 2 of 2 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed (commit messages unreviewed), 2 unresolved discussions (waiting on @StephenMcConnel)
CHANGELOG.md line 27 at r2 (raw file):
### Fixed - [SIL.Window.Forms] When choosing a file in the ImageToolbox.AcquireImageControl, a FileOk handler is simulated that verifies the selected file passes the given filter. Users can defeat the filter mechanism by pasting or typing the file name. While the returned filename does not pass the filter, the dialog is reopened until the user either chooses a proper filename or cancels the dialog. The native FileOk handler can prevent the dialog from closing: we can't achieve that. (See BL-13552.)
I think this has to go into a new "Fixed" section under "Unreleased" instead of 14.1.1.
SIL.Windows.Forms/ImageToolbox/AcquireImageControl.cs line 110 at r1 (raw file):
if (DialogResult.OK == dlg.ShowDialog(this.ParentForm)) { // Simulate having a FileOk event handler that can cancel the OK button click.
Probably worth calling out that we would prefer to use FileOk but that it is tied up in DialogAdapters so not worth changing at this time.
This enforces the image file filter even when users type or paste in a file path that defeats the filter. This fix is motivated by https://issues.bloomlibrary.org/youtrack/issue/BL-13552. Using the DialogAdapters interface prevents having an actual FileOk handler.
StephenMcConnel
force-pushed
the
BL-13552-EnforceFileFiltering
branch
from
5f048f8
to
de1a607
Compare
There was a problem hiding this comment.
Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @andrew-polk)
CHANGELOG.md line 27 at r2 (raw file):
Previously, andrew-polk wrote…
I think this has to go into a new "Fixed" section under "Unreleased" instead of 14.1.1.
Good catch. My eyes saw "Fixed" but didn't see the version number above it.
SIL.Windows.Forms/ImageToolbox/AcquireImageControl.cs line 110 at r1 (raw file):
Previously, andrew-polk wrote…
Probably worth calling out that we would prefer to use FileOk but that it is tied up in DialogAdapters so not worth changing at this time.
Done.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @StephenMcConnel)
| // We would prefer to actually use the event handler since that can prevent the | ||
| // dialog from closing, but we can't do that because of using DialogAdapters for | ||
| // 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)) |
There was a problem hiding this comment.
Do we still need DialogAdapters given that we are no longer shipping Bloom (or other software) on Mono Linux? It would seem like a good idea to just remove the "cross-platform" code since it's not actually used and go with the simpler native implementation of FileOK for Windows.Forms. Am I missing something?
There was a problem hiding this comment.
We thought Flex was still using the mono code in libpalaso. Is that incorrect?
There was a problem hiding this comment.
Can you fact-check the following for me, @jasonleenaylor ?
FieldWorks won't be shipping a new version on mono linux, so we're safe to remove mono cross-platform code from libpalaso master and reduce complexity for everyone. I gather this PR would have been simpler without the mono requirement. We don't even have build infrastructure anymore to build FW mono on Linux.
WinForms in libpalaso should be assumed to be Windows-only runtime.
There was a problem hiding this comment.
Yes @megahirt you are correct. We have stopped packaging for linux and are focusing on rearchitecting with the hope handle future cross-platform versions without mono. Sorry for any extra effort that went in because that wasn't widely known. When I saw this comment I assumed Bloom was still shipping for linux with mono.
This enforces the image file filter even when users type or paste in a file path that defeats the filter. This fix is motivated by https://issues.bloomlibrary.org/youtrack/issue/BL-13552. Using the DialogAdapters interface prevents having an actual FileOk handler.
This change is