refactor: migrate first batch of Forms to FormBase#4690
refactor: migrate first batch of Forms to FormBase#4690mvanhorn wants to merge 2 commits intoTASEmulators:masterfrom
Conversation
Changed base class from Form to FormBase for 7 forms: - ArchiveChooser - FFmpegWriterForm - GifWriterForm - JmdForm - SynclessRecordingTools - AutofireConfig - ControllerConfig Part of TASEmulators#4005. The remaining forms can follow in subsequent PRs.
YoshiRulz
added
App: EmuHawk
YoshiRulz
left a comment
YoshiRulz
left a comment
There was a problem hiding this comment.
Thanks for stepping up, but just changing the supertype isn't enough, you at least need to override WindowTitleStatic (which is not abstract for technical reasons). These will all throw when opened because of that, so clearly you haven't done any testing either. The main thing to test would be the Alt+key "mnemonics".
Address @YoshiRulz's review on TASEmulators#4690. Changing the supertype alone was not enough: FormBase's WindowTitleStatic throws NotImplementedException by default (it is virtual instead of abstract only because the Designer does not tolerate abstract), and FormBase's Text setter throws InvalidOperationException when hit outside DesignMode. Without the override, every one of these forms would throw on OnLoad before being shown. For each of the seven forms in this batch, add protected override string WindowTitleStatic => "<title>"; using the title that was previously set in the Designer, and remove the `this.Text = "<title>";` line from the Designer (it would throw at runtime now that the supertype's Text setter is guarded). Fixed forms: - ArchiveChooser: "Choose File From Archive" - FFmpegWriterForm: "Choose Video Format" - GifWriterForm: "GIF Writer Options" - JmdForm: "JMD Compression Options" - SynclessRecordingTools: "Syncless Recording" - AutofireConfig: "Autofire Configuration" - ControllerConfig: "Controller Config" Honest note on testing: BizHawk is a Windows host and I am on macOS without a working .NET build, so I cannot run these forms interactively or exercise the Alt+key mnemonics end-to-end from here. The pattern follows existing FormBase-derived forms (CDL, HexEditor, GBAGPUView, etc.) that already override WindowTitleStatic and do not set `this.Text` in their designers.
|
@YoshiRulz you're right, and thank you for the direct feedback. The supertype change alone would have thrown on every one of these forms - I missed that Pushed 59a6fcc which for each of the seven forms:
Followed the pattern from existing FormBase-derived forms (CDL, HexEditor, GBAGPUView). Honest note on testing: I'm on macOS without a working .NET build, so I cannot open these forms or drive the Alt+key mnemonics from here. If the pattern checks out for you and you'd like, I'm happy to leave this PR paused until I can set up a Windows env to verify, or close it and resubmit from there. Totally understand if you'd rather not merge something I can't demonstrate running. |
2 participants
Migrated 7 forms from
FormtoFormBaseas tracked in #4005:ArchiveChooserFFmpegWriterFormGifWriterFormJmdFormSynclessRecordingToolsAutofireConfigControllerConfigEach change is a one-line base class swap. The remaining forms from the checklist can follow in subsequent PRs.
Partial fix for #4005